Closed
Bug 903422
Opened 11 years ago
Closed 11 years ago
[Bluetooth] Non-blocking AppendDeviceNameRunnable
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: tzimmermann, Assigned: tzimmermann)
References
Details
Attachments
(3 files, 5 obsolete files)
3.12 KB,
patch
|
Details | Diff | Splinter Review | |
7.78 KB,
patch
|
echou
:
review+
gyeh
:
review+
|
Details | Diff | Splinter Review |
4.25 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
Part of bug 902396
Assignee | ||
Updated•11 years ago
|
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #788132 -
Flags: review?(gyeh)
Attachment #788132 -
Flags: review?(echou)
Assignee | ||
Comment 2•11 years ago
|
||
Eric, Gina, AppendDeviceNameRunnable gets dispatched from within the DBus thread. The only reason why AppendDeviceNameRunnable still exists in the patch is because it calls BluetoothService::Get(), which needs to run from within the main thread. But the result of this call is never used. Do you think we can delete BluetoothService::Get(), and replace the runnable it by a simple function? Best regards Thomas
Attachment #788143 -
Flags: review?(gyeh)
Attachment #788143 -
Flags: review?(echou)
Comment 3•11 years ago
|
||
Comment on attachment 788143 [details] [diff] [review] [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable Review of attachment 788143 [details] [diff] [review]: ----------------------------------------------------------------- The overall is good. One question to be answered and a few nits to be fixed. Note that |NS_WARNING| is used in many places and we'd like to replace them with |BT_WARNING|. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +842,5 @@ > + } > + > +private: > + nsCString mIface; > +}; Question: Can we remove class GetPropertiesReplyHandler and maintain the data member mIface in AppendDeviceNameReplyHandler directly? @@ +875,5 @@ > + > + bool success = UnpackPropertiesMessage(aReply, &err, deviceProperties, > + GetInterface().get()); > + if (!success) { > + NS_WARNING("Failed to get device properties"); nit: Please use BT_WARNING as well. @@ +880,5 @@ > + return; > + } > + > + // Replace object path with device address and... > + nit: The comment here seems incomplete. @@ +909,4 @@ > } > > private: > + nsString mDevicePath; nit: Please remove multiple white-spaces here. we usually don't do that. @@ +929,5 @@ > > BluetoothValue v = mSignal.value(); > if (v.type() != BluetoothValue::TArrayOfBluetoothNamedValue || > v.get_ArrayOfBluetoothNamedValue().Length() == 0) { > + NS_WARNING("Invalid argument type for AppendDeviceNameRunnable"); nit: Please use BT_WARNING as well. @@ +938,5 @@ > > // Device object path should be put in the first element > if (!arr[0].name().EqualsLiteral("path") || > arr[0].value().type() != BluetoothValue::TnsString) { > + NS_WARNING("Invalid object path for AppendDeviceNameRunnable"); nit: Please use BT_WARNING as well. @@ +946,5 @@ > + nsRefPtr<RawDBusConnection> threadConnection = gThreadConnection; > + > + if (!threadConnection.get()) { > + BT_WARNING("%s: DBus connection has been closed.", __FUNCTION__); > + return; return NS_ERROR_FAILURE;
Assignee | ||
Comment 4•11 years ago
|
||
Thanks for the review. See below for my comments. (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #3) > Comment on attachment 788143 [details] [diff] [review] > [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable > > Review of attachment 788143 [details] [diff] [review]: > ----------------------------------------------------------------- > > The overall is good. One question to be answered and a few nits to be fixed. > > Note that |NS_WARNING| is used in many places and we'd like to replace them > with |BT_WARNING|. Oh, I didn't know. I'll update the patch. > > ::: dom/bluetooth/linux/BluetoothDBusService.cpp > @@ +842,5 @@ > > + } > > + > > +private: > > + nsCString mIface; > > +}; > > Question: Can we remove class GetPropertiesReplyHandler and maintain the > data member mIface in AppendDeviceNameReplyHandler directly? We can, but I have patches for other operations that use 'GetProperties'. Introducing this base class here will make sense when the other patches arrive. Are you OK with that? > @@ +875,5 @@ > > + > > + bool success = UnpackPropertiesMessage(aReply, &err, deviceProperties, > > + GetInterface().get()); > > + if (!success) { > > + NS_WARNING("Failed to get device properties"); > > nit: Please use BT_WARNING as well. Ok. > @@ +880,5 @@ > > + return; > > + } > > + > > + // Replace object path with device address and... > > + > > nit: The comment here seems incomplete. It continues in the next comment below. I'll update the commenting here to make it more readable. > @@ +909,4 @@ > > } > > > > private: > > + nsString mDevicePath; > > nit: Please remove multiple white-spaces here. we usually don't do that. Ok. > @@ +929,5 @@ > > > > BluetoothValue v = mSignal.value(); > > if (v.type() != BluetoothValue::TArrayOfBluetoothNamedValue || > > v.get_ArrayOfBluetoothNamedValue().Length() == 0) { > > + NS_WARNING("Invalid argument type for AppendDeviceNameRunnable"); > > nit: Please use BT_WARNING as well. Ok. > @@ +938,5 @@ > > > > // Device object path should be put in the first element > > if (!arr[0].name().EqualsLiteral("path") || > > arr[0].value().type() != BluetoothValue::TnsString) { > > + NS_WARNING("Invalid object path for AppendDeviceNameRunnable"); > > nit: Please use BT_WARNING as well. Ok. > @@ +946,5 @@ > > + nsRefPtr<RawDBusConnection> threadConnection = gThreadConnection; > > + > > + if (!threadConnection.get()) { > > + BT_WARNING("%s: DBus connection has been closed.", __FUNCTION__); > > + return; > > return NS_ERROR_FAILURE; Strange. I don't remember getting a compiler warning here. I must have missed this.
Comment 5•11 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4) > > > > The overall is good. One question to be answered and a few nits to be fixed. > > > > Note that |NS_WARNING| is used in many places and we'd like to replace them > > with |BT_WARNING|. > > Oh, I didn't know. I'll update the patch. Thanks for your help. :) > > ::: dom/bluetooth/linux/BluetoothDBusService.cpp > > @@ +842,5 @@ > > > + } > > > + > > > +private: > > > + nsCString mIface; > > > +}; > > > > Question: Can we remove class GetPropertiesReplyHandler and maintain the > > data member mIface in AppendDeviceNameReplyHandler directly? > > We can, but I have patches for other operations that use 'GetProperties'. > Introducing this base class here will make sense when the other patches > arrive. Are you OK with that? Got it. Then it would be fine for me.
Assignee | ||
Comment 6•11 years ago
|
||
Updated according to review.
Attachment #788143 -
Attachment is obsolete: true
Attachment #788143 -
Flags: review?(gyeh)
Attachment #788143 -
Flags: review?(echou)
Attachment #788972 -
Flags: review?(gyeh)
Attachment #788972 -
Flags: review?(echou)
Comment 7•11 years ago
|
||
Comment on attachment 788972 [details] [diff] [review] [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable (v2) Review of attachment 788972 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +971,5 @@ > + handler.get(), > + NS_ConvertUTF16toUTF8(devicePath).get(), > + DBUS_DEVICE_IFACE, "GetProperties", > + DBUS_TYPE_INVALID); > + NS_ENSURE_TRUE(success, false); NS_ENSURE_TRUE(success, NS_ERROR_FAILURE);
Comment 8•11 years ago
|
||
Comment on attachment 788132 [details] [diff] [review] [01] Bug 903422: Cleanup GetPropertiesInternal Review of attachment 788132 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +736,5 @@ > + > + return replyError.IsEmpty(); > +} > + > + super-nit: extra newline
Attachment #788132 -
Flags: review?(echou) → review+
Comment 9•11 years ago
|
||
Hi Thomas, (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2) > Created attachment 788143 [details] [diff] [review] > [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable > > Eric, Gina, > > AppendDeviceNameRunnable gets dispatched from within the DBus thread. The > only reason why AppendDeviceNameRunnable still exists in the patch is > because it calls BluetoothService::Get(), which needs to run from within the > main thread. But the result of this call is never used. Do you think we can > delete BluetoothService::Get(), and replace the runnable it by a simple > function? > Yes, you're right. We no longer need to get BluetoothService instance in this case. Please update the patch and I would be happy to review once it's done.
Assignee | ||
Comment 10•11 years ago
|
||
Fixed the extra newline and converted to BT_WARNING.
Attachment #788132 -
Attachment is obsolete: true
Attachment #788132 -
Flags: review?(gyeh)
Attachment #789478 -
Flags: review?(gyeh)
Assignee | ||
Comment 11•11 years ago
|
||
Removed BluetoothService::Get and converted runnable to simple function call.
Attachment #788972 -
Attachment is obsolete: true
Attachment #788972 -
Flags: review?(gyeh)
Attachment #788972 -
Flags: review?(echou)
Attachment #789479 -
Flags: review?(gyeh)
Attachment #789479 -
Flags: review?(echou)
Assignee | ||
Updated•11 years ago
|
Attachment #789479 -
Attachment is obsolete: true
Attachment #789479 -
Flags: review?(gyeh)
Attachment #789479 -
Flags: review?(echou)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #789481 -
Flags: review?(gyeh)
Attachment #789481 -
Flags: review?(echou)
Comment 13•11 years ago
|
||
Comment on attachment 789481 [details] [diff] [review] [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable (v3) Review of attachment 789481 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. The only reason I r- is that, after these two patches being merged, the call flow related to function UnpackPropertiesMessage() would become a little complicated since there would be 2 UnpackPropertiesMessage(). For example, in AppendDeviceNameReplyHandler::Handle(), it calls UnpackPropertiesMessage() to unpack the DBusMessage, then Unpack[Device|Adapter|Manager]PropertiesMessage would be called depending on the variable aIface, and then the other UnpackPropertiesMessage() would be called to really parse the message. Thomas, I think we could simplify it by integrating UnpackPropertiesMessage() and Unpack[Device|Adapter|Manager]PropertiesMessage() into one function. Either modifying the second patch or adding another interdiff as the third patch would be fine. Thanks!
Attachment #789481 -
Flags: review?(echou) → review-
Assignee | ||
Comment 14•11 years ago
|
||
Maybe this was a bit hard to describe on irc. Separating the selection of the properties array from the actual parsing gives more readable code IMHO. I also added a boolean return value to UnpackPropertiesMessage to clearly signal when the function failed. The whole handling of error strings is still a bit sloppy, but I didn't want to introduce any extra changes here.
Attachment #790708 -
Flags: review?(echou)
Comment 15•11 years ago
|
||
Comment on attachment 790708 [details] [diff] [review] [03] Bug 903422: Cleanup UnpackPropertiesMessage Review of attachment 790708 [details] [diff] [review]: ----------------------------------------------------------------- Still thinking that we could merge UnpackPropertiesMessageFromInterface and UnpackPropertiesMessage, not want to be stuck on this, though. ;) It's much cleaner than before, r=me with the nit addressed. Thanks! ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +758,5 @@ > } else { > return false; > } > > + nsAutoString errorStr; nit: let's save a variable and just print warnings inside UnpackPropertiesMessage().
Attachment #790708 -
Flags: review?(echou) → review+
Comment 16•11 years ago
|
||
> Maybe this was a bit hard to describe on irc. Separating the selection of
> the properties array from the actual parsing gives more readable code IMHO.
> I also added a boolean return value to UnpackPropertiesMessage to clearly
> signal when the function failed.
Actually I did know what you were trying to say on IRC, just not sure which way (integrate or not) would be better.
Comment 17•11 years ago
|
||
Comment on attachment 789481 [details] [diff] [review] [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable (v3) Review of attachment 789481 [details] [diff] [review]: ----------------------------------------------------------------- Change to r+ since we've got patch 3!
Attachment #789481 -
Flags: review- → review+
Assignee | ||
Comment 18•11 years ago
|
||
Eric, I merged the two functions. We cannot remove errorStr, because it's needed by IsDBusMessageError and ParseProperties.
Attachment #790708 -
Attachment is obsolete: true
Attachment #790731 -
Flags: review+
Comment 19•11 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #18) > Created attachment 790731 [details] [diff] [review] > [03] Bug 903422: Cleanup UnpackPropertiesMessage (v2) > > Eric, > > I merged the two functions. We cannot remove errorStr, because it's needed > by IsDBusMessageError and ParseProperties. Thanks!
Assignee | ||
Comment 20•11 years ago
|
||
Gina, Don't forget about these patches here. :) Thanks!
Comment 21•11 years ago
|
||
Comment on attachment 789478 [details] [diff] [review] [01] Bug 903422: Cleanup GetPropertiesInternal (v2) Review of attachment 789478 [details] [diff] [review]: ----------------------------------------------------------------- Since this patch has been updated as v3, I think the review request should be cancelled, right?
Attachment #789478 -
Flags: review?(gyeh)
Assignee | ||
Comment 22•11 years ago
|
||
There is no v3 of this patch. I just updated the patch once and thought you might want to take another look at it.
Comment 23•11 years ago
|
||
Comment on attachment 789481 [details] [diff] [review] [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable (v3) Review of attachment 789481 [details] [diff] [review]: ----------------------------------------------------------------- Please see my following comments. Thanks. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +1154,5 @@ > // Increase ref count here because we need this message later. > // It'll be unrefed when set*Internal() is called. > dbus_message_ref(msg); > > + AppendDeviceName(signal); We probably should handle the return value of function AppendDeviceName. Otherwise, we can just make the return value to void. @@ +1159,3 @@ > } else { > handler = new DistributeBluetoothSignalTask(signal); > + NS_DispatchToMainThread(handler); We don't need variable |handler|. Please remove it and use the following line: NS_DispatchToMainThread(new DistributeBluetoothSignalTask(signal));
Attachment #789481 -
Flags: review?(gyeh) → review+
Comment 24•11 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #22) > There is no v3 of this patch. I just updated the patch once and thought you > might want to take another look at it. I'm a bit confused :| Are we going to apply patch 2 AND patch 3? Since some functions are removed in patch 3 (e.g. UnpackAdapterPropertiesMessage), I think the build would be broken, right?
Assignee | ||
Comment 25•11 years ago
|
||
I'm going to apply all 3 patches. Patch 3 merges all Unpack*PropertiesMessages into one. There should be just one user of UnpackAdapterPropertiesMessage et al. I'll check twice before committing this, but building and testing works for me.
Comment 26•11 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25) > I'm going to apply all 3 patches. Patch 3 merges all > Unpack*PropertiesMessages into one. There should be just one user of > UnpackAdapterPropertiesMessage et al. I'll check twice before committing > this, but building and testing works for me. Great! Then let's ship it.
Assignee | ||
Comment 27•11 years ago
|
||
Thanks a lot! I updated the patch set and pushed it to b2g-inbound. https://hg.mozilla.org/integration/b2g-inbound/rev/af1af7c82d0a https://hg.mozilla.org/integration/b2g-inbound/rev/13064db19b63 https://hg.mozilla.org/integration/b2g-inbound/rev/ffaa22db60ff
Assignee | ||
Comment 28•11 years ago
|
||
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=ffaa22db60ff
https://hg.mozilla.org/mozilla-central/rev/af1af7c82d0a https://hg.mozilla.org/mozilla-central/rev/13064db19b63 https://hg.mozilla.org/mozilla-central/rev/ffaa22db60ff
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•