Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(99)

Issue 2159046: Add some comments about registry (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 6 months ago by Peng
Modified:
13 years, 6 months ago
Reviewers:
satorux, penghuang
Base URL:
git@github.com:ibus/ibus.git
Visibility:
Public.

Description

Add some comments about registry BUG=none TEST=built fine

Patch Set 1 #

Total comments: 17

Patch Set 2 : Refine comments #

Patch Set 3 : Update comments #

Patch Set 4 : Update #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -6 lines) Patch
M bus/ibusimpl.c View 1 1 chunk +1 line, -0 lines 0 comments Download
M bus/registry.c View 1 2 3 4 chunks +36 lines, -6 lines 0 comments Download

Messages

Total messages: 8
penghuang
13 years, 6 months ago (2010-09-10 03:23:58 UTC) #1
satorux
Thank you for adding comments! http://codereview.appspot.com/2159046/diff/1/bus/ibusimpl.c File bus/ibusimpl.c (right): http://codereview.appspot.com/2159046/diff/1/bus/ibusimpl.c#newcode614 bus/ibusimpl.c:614: /* start monitor of ...
13 years, 6 months ago (2010-09-10 05:51:46 UTC) #2
penghuang
Update comments.
13 years, 6 months ago (2010-09-10 08:01:39 UTC) #3
penghuang
http://codereview.appspot.com/2159046/diff/1/bus/ibusimpl.c File bus/ibusimpl.c (right): http://codereview.appspot.com/2159046/diff/1/bus/ibusimpl.c#newcode614 bus/ibusimpl.c:614: /* start monitor of changes of registry */ On ...
13 years, 6 months ago (2010-09-10 09:37:49 UTC) #4
satorux
LGTM, but see below. http://codereview.appspot.com/2159046/diff/1/bus/registry.c File bus/registry.c (right): http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode503 bus/registry.c:503: * ibus need a restart. ...
13 years, 6 months ago (2010-09-10 10:36:10 UTC) #5
satorux
Could you update the BUG and TEST lines, like: BUG=none TEST=built fine Or you can ...
13 years, 6 months ago (2010-09-10 10:37:33 UTC) #6
penghuang
On 2010/09/10 10:37:33, satorux wrote: > Could you update the BUG and TEST lines, like: ...
13 years, 6 months ago (2010-09-10 10:58:23 UTC) #7
satorux
13 years, 6 months ago (2010-09-10 11:30:22 UTC) #8
On 2010/09/10 10:58:23, penghuang wrote:
> On 2010/09/10 10:37:33, satorux wrote:
> > Could you update the BUG and TEST lines, like:
> > 
> > BUG=none
> > TEST=built fine
> > 
> > Or you can remove these by "Edit issue" button.
> > 
> > 
> > On 2010/09/10 10:36:10, satorux wrote:
> > > LGTM, but see below.
> > > 
> > > http://codereview.appspot.com/2159046/diff/1/bus/registry.c
> > > File bus/registry.c (right):
> > > 
> > > http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode503
> > > bus/registry.c:503: * ibus need a restart.
> > > On 2010/09/10 09:37:49, penghuang wrote:
> > > > On 2010/09/10 05:51:46, satorux wrote:
> > > > > Would be nice to explain why use g_cond_timed_wait rather than
sleep(),
> > > like:
> > > > > 
> > > > > 
> > > > > Note that we use g_cond_timed_wait() here rather than
> sleep()/nanosleep()
> > so
> > > > > that the loop can terminate immediately when the registry object is
> > > destroyed.
> > > > > See also comments in  bus_registry_destroy().
> > > > 
> > > > Done.
> > > 
> > > My comment about sleep()/nanosleep() was ignored, which is OK if you think
> > it's
> > > worthless. In this case, please say so, rather than hitting "Done"
button...
> 
> I added comments about sleep() but is in 514 line.

Oops, I missed. Sorry about that!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b