|
|
Created:
12 years, 6 months ago by Daiki Ueno Modified:
12 years, 6 months ago Reviewers:
shawn.p.huang, Peng Base URL:
git@github.com:ibus/ibus.git@master Visibility:
Public. |
DescriptionImplement org.freedesktop.DBus.StartServiceByName.
BUG=none
TEST=tested with https://github.com/ueno/ibus-gucharmap/tree/charmap-service
Patch Set 1 #
Total comments: 4
Patch Set 2 : don't connect to self signal #Patch Set 3 : remove BusDBusImpl::start-service signal #
Total comments: 8
Patch Set 4 : add timeout for starting component #
Total comments: 4
Patch Set 5 : define IBUS_BUS_START_REPLY_* in ibustypes.h #
Total comments: 20
Patch Set 6 : return D-Bus error rather than replying 0, improve start_service_calls traversal #Patch Set 7 : use GList instead of GSList for start_service_calls #Patch Set 8 : support StartServiceByName in python binding #
MessagesTotal messages: 16
It would be useful if certain components are started automatically when other components requests. My current use case is ibus-charmap, which provides an engine (ibus-engine-charmap) and a character map UI service implemented with GTK (ibus-ui-charmap-gtk): https://github.com/ueno/ibus-gucharmap/tree/charmap-service
Sign in to reply to this message.
https://codereview.appspot.com/4960060/diff/1/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/1/bus/dbusimpl.c#newcode1166 bus/dbusimpl.c:1166: &success); Why use signal to invoke start_service of register? Why not call it directlly? https://codereview.appspot.com/4960060/diff/1/bus/dbusimpl.c#newcode1171 bus/dbusimpl.c:1171: g_signal_connect (dbus, "name-acquired", It is weird to connect self's signal? You could add code in name-acquired function directly. And I can not find you disconnect the signal anywhere
Sign in to reply to this message.
https://codereview.appspot.com/4960060/diff/1/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/1/bus/dbusimpl.c#newcode1166 bus/dbusimpl.c:1166: &success); On 2011/09/08 12:24:27, Peng wrote: > Why use signal to invoke start_service of register? Why not call it directlly? Do you mean calling bus_registry_lookup_component_by_name() and bus_component_start() directly from bus_dbus_impl_start_service_by_name() ? There is currently no dependency from BusDBusImpl to objects like BusRegistry, BusIBusImpl, etc. so I was afraid to add one. I'll update the CL if you prefer. https://codereview.appspot.com/4960060/diff/1/bus/dbusimpl.c#newcode1171 bus/dbusimpl.c:1171: g_signal_connect (dbus, "name-acquired", On 2011/09/08 12:24:27, Peng wrote: > It is weird to connect self's signal? You could add code in name-acquired > function directly. > > And I can not find you disconnect the signal anywhere Done.
Sign in to reply to this message.
On 2011/09/09 07:27:43, Daiki Ueno wrote: > https://codereview.appspot.com/4960060/diff/1/bus/dbusimpl.c > File bus/dbusimpl.c (right): > > https://codereview.appspot.com/4960060/diff/1/bus/dbusimpl.c#newcode1166 > bus/dbusimpl.c:1166: &success); > On 2011/09/08 12:24:27, Peng wrote: > > Why use signal to invoke start_service of register? Why not call it directlly? > > Do you mean calling bus_registry_lookup_component_by_name() and > bus_component_start() directly from bus_dbus_impl_start_service_by_name() ? > > There is currently no dependency from BusDBusImpl to objects like BusRegistry, > BusIBusImpl, etc. so I was afraid to add one. I'll update the CL if you prefer. OK, I removed BusDBusImpl::start-service signal in CL 3.
Sign in to reply to this message.
Sorry for the late reply. https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1139 bus/dbusimpl.c:1139: guint32 flags; What´s the flags for? I do not it is used anywhere? https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1161 bus/dbusimpl.c:1161: retval = 2; /* DBUS_START_REPLY_ALREADY_RUNNING */ Could you define some macro in ibus? Instead of use those number directly. https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1172 bus/dbusimpl.c:1172: invocation); Maybe we should add a timeout here? The component may starts failed.
Sign in to reply to this message.
https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1139 bus/dbusimpl.c:1139: guint32 flags; On 2011/09/12 13:02:55, Peng wrote: > What´s the flags for? I do not it is used anywhere? Even though it is not used in dbusimpl.c, I think a dummy arg is necessary here to pass the variant type check. Otherwise it crashes if I simply omit flags arg from g_variant_get call, or gets assertion failure if I use "(&s)" instead of "(&su)": (ibus-daemon:26034): GLib-CRITICAL **: the GVariant format string `(&s)' has a type of `(s)' but the given value has a type of `(su)' https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1161 bus/dbusimpl.c:1161: retval = 2; /* DBUS_START_REPLY_ALREADY_RUNNING */ On 2011/09/12 13:02:55, Peng wrote: > Could you define some macro in ibus? Instead of use those number directly. Done. https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1172 bus/dbusimpl.c:1172: invocation); On 2011/09/12 13:02:55, Peng wrote: > Maybe we should add a timeout here? The component may starts failed. Done.
Sign in to reply to this message.
https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1139 bus/dbusimpl.c:1139: guint32 flags; On 2011/09/13 08:09:01, Daiki Ueno wrote: > On 2011/09/12 13:02:55, Peng wrote: > > What´s the flags for? I do not it is used anywhere? > > Even though it is not used in dbusimpl.c, I think a dummy arg is necessary here > to pass the variant type check. Otherwise it crashes if I simply omit flags arg > from g_variant_get call, or gets assertion failure if I use "(&s)" instead of > "(&su)": > > (ibus-daemon:26034): GLib-CRITICAL **: the GVariant format string `(&s)' has a > type of `(s)' but the given value has a type of `(su)' I did not mean omit it. I just want to ask what is flags for in dbus specification. If we do not support it right now, could you add some comment in here. like /* FIXME or TODO: xxxx */ https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c#newcode257 bus/dbusimpl.c:257: #define DBUS_START_REPLY_ALREADY_RUNNING 2 Please define them in header file (maybe ibustypes.h). I think libibus client applications may want to use it. BTW, it is better to rename them to IBUS_START_REPLY_SUCCESS and IBUS_XX. to avoid name conflict with dbus. https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c#newcode1209 bus/dbusimpl.c:1209: call); If ibus receives several start service requests, the the timeout will be override here. I think probably each request need have a separated timeout.
Sign in to reply to this message.
https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1139 bus/dbusimpl.c:1139: guint32 flags; On 2011/09/13 12:31:08, Peng wrote: > On 2011/09/13 08:09:01, Daiki Ueno wrote: > > On 2011/09/12 13:02:55, Peng wrote: > > > What´s the flags for? I do not it is used anywhere? > > > > Even though it is not used in dbusimpl.c, I think a dummy arg is necessary > here > > to pass the variant type check. Otherwise it crashes if I simply omit flags > arg > > from g_variant_get call, or gets assertion failure if I use "(&s)" instead of > > "(&su)": > > > > (ibus-daemon:26034): GLib-CRITICAL **: the GVariant format string `(&s)' has a > > type of `(s)' but the given value has a type of `(su)' > > I did not mean omit it. I just want to ask what is flags for in dbus > specification. If we do not support it right now, could you add some comment in > here. like /* FIXME or TODO: xxxx */ > flags is currently unused even in the spec :) I added a comment about that. https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c#newcode257 bus/dbusimpl.c:257: #define DBUS_START_REPLY_ALREADY_RUNNING 2 On 2011/09/13 12:31:08, Peng wrote: > Please define them in header file (maybe ibustypes.h). I think libibus client > applications may want to use it. BTW, it is better to rename them to > IBUS_START_REPLY_SUCCESS and IBUS_XX. to avoid name conflict with dbus. Done. https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c#newcode1209 bus/dbusimpl.c:1209: call); On 2011/09/13 12:31:08, Peng wrote: > If ibus receives several start service requests, the the timeout will be > override here. I think probably each request need have a separated timeout. Probably I don't understand what you mean correctly, but "call" is allocated per request and maintained in a list dbus->start_service_calls. So I think each request already has a seperated timeout. BTW, maybe better to have a lock when modifying the list. I'll upload a new try soon, but at the moment I uploaded no-lock version (CL 4) for a quick review.
Sign in to reply to this message.
https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1139 bus/dbusimpl.c:1139: g_variant_new ("(u)", 0)); When timeout happens, the method should return 0 or an error message? Please double check it in dbus. Thanks https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1142 bus/dbusimpl.c:1142: if (p != NULL) { Maybe it is better to assign timeout_id = 0 here, because this function returns FALSE, system will release the timeout for you. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1142 bus/dbusimpl.c:1142: if (p != NULL) { And I think p should not be NULL here, Probably it is better to use g_return_val_if_fail (p != NULL, False); SO when some wrong happens, we could get some warning in log. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1194 bus/dbusimpl.c:1194: g_hash_table_lookup (dbus->names, name) == NULL) { Is g_hash_table_loopup () necessary? https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1207 bus/dbusimpl.c:1207: retval = 0; What's 0? Could you double check dbus source code, if there is no service can provide the request name, what value should dbus return? Maybe we should and error instead of 0? https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1596 bus/dbusimpl.c:1596: if (g_strcmp0 (unique_name, _unique_name) == 0) { I think test call->connection == connection is better? https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1601 bus/dbusimpl.c:1601: break; break? I think several start_service_calls may from same connection, right?
Sign in to reply to this message.
https://codereview.appspot.com/4960060/diff/16001/bus/marshalers.list File bus/marshalers.list (right): https://codereview.appspot.com/4960060/diff/16001/bus/marshalers.list#newcode11 bus/marshalers.list:11: BOOL:STRING,UINT Is it still needed?
Sign in to reply to this message.
On 2011/09/14 06:47:04, Daiki Ueno wrote: > https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c > File bus/dbusimpl.c (right): > > https://codereview.appspot.com/4960060/diff/8/bus/dbusimpl.c#newcode1139 > bus/dbusimpl.c:1139: guint32 flags; > On 2011/09/13 12:31:08, Peng wrote: > > On 2011/09/13 08:09:01, Daiki Ueno wrote: > > > On 2011/09/12 13:02:55, Peng wrote: > > > > What´s the flags for? I do not it is used anywhere? > > > > > > Even though it is not used in dbusimpl.c, I think a dummy arg is necessary > > here > > > to pass the variant type check. Otherwise it crashes if I simply omit flags > > arg > > > from g_variant_get call, or gets assertion failure if I use "(&s)" instead > of > > > "(&su)": > > > > > > (ibus-daemon:26034): GLib-CRITICAL **: the GVariant format string `(&s)' has > a > > > type of `(s)' but the given value has a type of `(su)' > > > > I did not mean omit it. I just want to ask what is flags for in dbus > > specification. If we do not support it right now, could you add some comment > in > > here. like /* FIXME or TODO: xxxx */ > > > > flags is currently unused even in the spec :) I added a comment about that. > > https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c > File bus/dbusimpl.c (right): > > https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c#newcode257 > bus/dbusimpl.c:257: #define DBUS_START_REPLY_ALREADY_RUNNING 2 > On 2011/09/13 12:31:08, Peng wrote: > > Please define them in header file (maybe ibustypes.h). I think libibus client > > applications may want to use it. BTW, it is better to rename them to > > IBUS_START_REPLY_SUCCESS and IBUS_XX. to avoid name conflict with dbus. > > Done. > > https://codereview.appspot.com/4960060/diff/12001/bus/dbusimpl.c#newcode1209 > bus/dbusimpl.c:1209: call); > On 2011/09/13 12:31:08, Peng wrote: > > If ibus receives several start service requests, the the timeout will be > > override here. I think probably each request need have a separated timeout. > > Probably I don't understand what you mean correctly, but "call" is allocated per > request and maintained in a list dbus->start_service_calls. So I think each > request already has a seperated timeout. Sorry. It is my mistake. I thought them share one timeout. Please ignore my comment about it. > > BTW, maybe better to have a lock when modifying the list. I'll upload a new try > soon, but at the moment I uploaded no-lock version (CL 4) for a quick review. The lock is not necessary. Because those code are running in same thread.
Sign in to reply to this message.
https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1600 bus/dbusimpl.c:1600: g_slist_free_1 (p); g_slist_delete_link() could remove the node from line and free it. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1601 bus/dbusimpl.c:1601: break; BTW, if changing break to continue, it may have another problem. I think travel and modify a list at same time is not safe.
Sign in to reply to this message.
Thanks for the review and helpful advice. The issues should be addressed in CL 7. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c File bus/dbusimpl.c (right): https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1139 bus/dbusimpl.c:1139: g_variant_new ("(u)", 0)); On 2011/09/14 13:24:11, Peng wrote: > When timeout happens, the method should return 0 or an error message? Please > double check it in dbus. Thanks You are right, it should return error rather than 0. Fixed. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1142 bus/dbusimpl.c:1142: if (p != NULL) { On 2011/09/14 13:24:11, Peng wrote: > Maybe it is better to assign timeout_id = 0 here, because this function returns > FALSE, system will release the timeout for you. I think it is not necessary to clear call->timeout_id since it will be cleared as a result of "bus_method_call_free ((BusMethodCall *) p->data);" on the next line. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1142 bus/dbusimpl.c:1142: if (p != NULL) { On 2011/09/14 13:24:11, Peng wrote: > And I think p should not be NULL here, Probably it is better to use > g_return_val_if_fail (p != NULL, False); SO when some wrong happens, we could > get some warning in log. Done. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1194 bus/dbusimpl.c:1194: g_hash_table_lookup (dbus->names, name) == NULL) { On 2011/09/14 13:24:11, Peng wrote: > Is g_hash_table_loopup () necessary? Nope. Removed. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1207 bus/dbusimpl.c:1207: retval = 0; On 2011/09/14 13:24:11, Peng wrote: > What's 0? Could you double check dbus source code, if there is no service can > provide the request name, what value should dbus return? > Maybe we should and error instead of 0? Right, fixed. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1596 bus/dbusimpl.c:1596: if (g_strcmp0 (unique_name, _unique_name) == 0) { On 2011/09/14 13:24:11, Peng wrote: > I think test call->connection == connection is better? Done. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1600 bus/dbusimpl.c:1600: g_slist_free_1 (p); On 2011/09/14 14:21:18, Peng wrote: > g_slist_delete_link() could remove the node from line and free it. Done. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1601 bus/dbusimpl.c:1601: break; On 2011/09/14 13:24:11, Peng wrote: > break? I think several start_service_calls may from same connection, right? Done. https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1601 bus/dbusimpl.c:1601: break; On 2011/09/14 14:21:18, Peng wrote: > BTW, if changing break to continue, it may have another problem. I think travel > and modify a list at same time is not safe. I rewrote the loop so that it keeps a reference to correct p->next before modification. So I think it should be safe now. Also, I switched the type of start_service_calls from GSList to GList to avoid extra traversal on g_*list_delete_link. https://codereview.appspot.com/4960060/diff/16001/bus/marshalers.list File bus/marshalers.list (right): https://codereview.appspot.com/4960060/diff/16001/bus/marshalers.list#newcode11 bus/marshalers.list:11: BOOL:STRING,UINT On 2011/09/14 13:25:04, Peng wrote: > Is it still needed? Removed.
Sign in to reply to this message.
LGTM. Thanks. On 2011/09/15 07:46:05, Daiki Ueno wrote: > Thanks for the review and helpful advice. > The issues should be addressed in CL 7. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c > File bus/dbusimpl.c (right): > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1139 > bus/dbusimpl.c:1139: g_variant_new ("(u)", 0)); > On 2011/09/14 13:24:11, Peng wrote: > > When timeout happens, the method should return 0 or an error message? Please > > double check it in dbus. Thanks > > You are right, it should return error rather than 0. Fixed. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1142 > bus/dbusimpl.c:1142: if (p != NULL) { > On 2011/09/14 13:24:11, Peng wrote: > > Maybe it is better to assign timeout_id = 0 here, because this function > returns > > FALSE, system will release the timeout for you. > > I think it is not necessary to clear call->timeout_id since it will be cleared > as a result of "bus_method_call_free ((BusMethodCall *) p->data);" on the next > line. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1142 > bus/dbusimpl.c:1142: if (p != NULL) { > On 2011/09/14 13:24:11, Peng wrote: > > And I think p should not be NULL here, Probably it is better to use > > g_return_val_if_fail (p != NULL, False); SO when some wrong happens, we could > > get some warning in log. > > Done. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1194 > bus/dbusimpl.c:1194: g_hash_table_lookup (dbus->names, name) == NULL) { > On 2011/09/14 13:24:11, Peng wrote: > > Is g_hash_table_loopup () necessary? > > Nope. Removed. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1207 > bus/dbusimpl.c:1207: retval = 0; > On 2011/09/14 13:24:11, Peng wrote: > > What's 0? Could you double check dbus source code, if there is no service can > > provide the request name, what value should dbus return? > > Maybe we should and error instead of 0? > > Right, fixed. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1596 > bus/dbusimpl.c:1596: if (g_strcmp0 (unique_name, _unique_name) == 0) { > On 2011/09/14 13:24:11, Peng wrote: > > I think test call->connection == connection is better? > > Done. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1600 > bus/dbusimpl.c:1600: g_slist_free_1 (p); > On 2011/09/14 14:21:18, Peng wrote: > > g_slist_delete_link() could remove the node from line and free it. > > Done. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1601 > bus/dbusimpl.c:1601: break; > On 2011/09/14 13:24:11, Peng wrote: > > break? I think several start_service_calls may from same connection, right? > > Done. > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1601 > bus/dbusimpl.c:1601: break; > On 2011/09/14 14:21:18, Peng wrote: > > BTW, if changing break to continue, it may have another problem. I think > travel > > and modify a list at same time is not safe. > > I rewrote the loop so that it keeps a reference to correct p->next before > modification. So I think it should be safe now. > > Also, I switched the type of start_service_calls from GSList to GList to avoid > extra traversal on g_*list_delete_link. > > https://codereview.appspot.com/4960060/diff/16001/bus/marshalers.list > File bus/marshalers.list (right): > > https://codereview.appspot.com/4960060/diff/16001/bus/marshalers.list#newcode11 > bus/marshalers.list:11: BOOL:STRING,UINT > On 2011/09/14 13:25:04, Peng wrote: > > Is it still needed? > > Removed.
Sign in to reply to this message.
BTW, I am not sure if this API is supported in ibus python library. If not, how about add it in a new CL? On 2011/09/15 13:51:04, Peng wrote: > LGTM. Thanks. > > On 2011/09/15 07:46:05, Daiki Ueno wrote: > > Thanks for the review and helpful advice. > > The issues should be addressed in CL 7. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c > > File bus/dbusimpl.c (right): > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1139 > > bus/dbusimpl.c:1139: g_variant_new ("(u)", 0)); > > On 2011/09/14 13:24:11, Peng wrote: > > > When timeout happens, the method should return 0 or an error message? Please > > > double check it in dbus. Thanks > > > > You are right, it should return error rather than 0. Fixed. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1142 > > bus/dbusimpl.c:1142: if (p != NULL) { > > On 2011/09/14 13:24:11, Peng wrote: > > > Maybe it is better to assign timeout_id = 0 here, because this function > > returns > > > FALSE, system will release the timeout for you. > > > > I think it is not necessary to clear call->timeout_id since it will be cleared > > as a result of "bus_method_call_free ((BusMethodCall *) p->data);" on the next > > line. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1142 > > bus/dbusimpl.c:1142: if (p != NULL) { > > On 2011/09/14 13:24:11, Peng wrote: > > > And I think p should not be NULL here, Probably it is better to use > > > g_return_val_if_fail (p != NULL, False); SO when some wrong happens, we > could > > > get some warning in log. > > > > Done. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1194 > > bus/dbusimpl.c:1194: g_hash_table_lookup (dbus->names, name) == NULL) { > > On 2011/09/14 13:24:11, Peng wrote: > > > Is g_hash_table_loopup () necessary? > > > > Nope. Removed. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1207 > > bus/dbusimpl.c:1207: retval = 0; > > On 2011/09/14 13:24:11, Peng wrote: > > > What's 0? Could you double check dbus source code, if there is no service > can > > > provide the request name, what value should dbus return? > > > Maybe we should and error instead of 0? > > > > Right, fixed. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1596 > > bus/dbusimpl.c:1596: if (g_strcmp0 (unique_name, _unique_name) == 0) { > > On 2011/09/14 13:24:11, Peng wrote: > > > I think test call->connection == connection is better? > > > > Done. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1600 > > bus/dbusimpl.c:1600: g_slist_free_1 (p); > > On 2011/09/14 14:21:18, Peng wrote: > > > g_slist_delete_link() could remove the node from line and free it. > > > > Done. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1601 > > bus/dbusimpl.c:1601: break; > > On 2011/09/14 13:24:11, Peng wrote: > > > break? I think several start_service_calls may from same connection, right? > > > > Done. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/dbusimpl.c#newcode1601 > > bus/dbusimpl.c:1601: break; > > On 2011/09/14 14:21:18, Peng wrote: > > > BTW, if changing break to continue, it may have another problem. I think > > travel > > > and modify a list at same time is not safe. > > > > I rewrote the loop so that it keeps a reference to correct p->next before > > modification. So I think it should be safe now. > > > > Also, I switched the type of start_service_calls from GSList to GList to avoid > > extra traversal on g_*list_delete_link. > > > > https://codereview.appspot.com/4960060/diff/16001/bus/marshalers.list > > File bus/marshalers.list (right): > > > > > https://codereview.appspot.com/4960060/diff/16001/bus/marshalers.list#newcode11 > > bus/marshalers.list:11: BOOL:STRING,UINT > > On 2011/09/14 13:25:04, Peng wrote: > > > Is it still needed? > > > > Removed.
Sign in to reply to this message.
On 2011/09/15 13:52:17, Peng wrote: > BTW, I am not sure if this API is supported in ibus python library. If not, how > about add it in a new CL? I'm about to push CL 8 with that, thanks.
Sign in to reply to this message.
|