Closed Bug 878745 Opened 12 years ago Closed 12 years ago

[b2g-bluetooth] BluetoothService Cleanup

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.1 QE3 (26jun)

People

(Reporter: gyeh, Assigned: gyeh)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(2 files, 5 obsolete files)

I'd like to re-arrange the order of static functions in BluetoothDBusService and also do some clean-up.
No longer depends on: 878728
A new function is added in BluetoothService: DispatchToCommandThread. With this function, we can discard some class and make code clearer. Please see patches for more details. The patch seems large since the order of static functions is changed. All fixes should be trivial and safe :)
Assignee: nobody → gyeh
Attachment #757345 - Flags: review?(echou)
Update patch
Attachment #757345 - Attachment is obsolete: true
Attachment #757345 - Flags: review?(echou)
Attachment #757758 - Flags: review?(echou)
Comment on attachment 757758 [details] [diff] [review] Patch 1(v1): BluetoothDBusService cleanup Review of attachment 757758 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for cleaning up! Overall looks good. Please have me review again after revision. Thanks. ::: dom/bluetooth/BluetoothService.cpp @@ +822,5 @@ > } > + > +void > +BluetoothService::DispatchToCommandThread(nsRunnable* aRunnable) > +{ Some assertions may be neccesary: MOZ_ASSERT(NS_IsMainThread()); // Not sure if we need this, though MOZ_ASSERT(aRunnable); MOZ_ASSERT(mBluetoothCommandThread); ::: dom/bluetooth/BluetoothService.h @@ +282,5 @@ > void > RemoveObserverFromTable(const nsAString& key); > > + void > + DispatchToCommandThread(nsRunnable* aRunnable); nit: trailing whitespace ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +358,2 @@ > { > + uint32_t* handles = NULL; nit: nullptr @@ +361,5 @@ > > DBusError err; > dbus_error_init(&err); > > + if (dbus_message_get_args(aReply, &err, Please assist with correcting indentation inside this if-block. @@ +789,5 @@ > +MOZ_STATIC_ASSERT( > + sizeof(sBluetoothDBusPropCallbacks) == sizeof(sBluetoothDBusIfaces), > + "DBus Property callback array and DBus interface array must be same size"); > + > +void static would be better. @@ +814,5 @@ > + } > + aValue = props; > +} > + > +bool static would be better. @@ +1469,5 @@ > + ToLowerCase(newValue.get_ArrayOfBluetoothNamedValue()[0].name()); > + BluetoothSignal signal(NS_LITERAL_STRING("PairedStatusChanged"), > + NS_LITERAL_STRING(KEY_LOCAL_AGENT), > + newValue); > + handler = new DistributeBluetoothSignalTask(signal); I would recommend using NS_DispatchToMainThread(new DistributeBluetoothSignalTask(signal)); then we could save a variable. @@ +1480,5 @@ > LOG_AND_FREE_DBUS_ERROR_WITH_MSG(&err, aMsg); > errorStr.AssignLiteral("Cannot parse manager path!"); > } else { > v = NS_ConvertUTF8toUTF16(str); > + handler = new PrepareAdapterTask(v.get_nsString()); I would recommend using NS_DispatchToMainThread(new PrepareAdapterTask(v.get_nsString())); then we could save a variable. @@ +1767,5 @@ > BluetoothDBusService::SendDiscoveryMessage(const char* aMessageName, > BluetoothReplyRunnable* aRunnable) > { > + MOZ_ASSERT(NS_IsMainThread()); > + nit: extra line
Comment on attachment 757758 [details] [diff] [review] Patch 1(v1): BluetoothDBusService cleanup Review of attachment 757758 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for reviewing. Will update patch soon. ::: dom/bluetooth/BluetoothService.cpp @@ +822,5 @@ > } > + > +void > +BluetoothService::DispatchToCommandThread(nsRunnable* aRunnable) > +{ For current cases of using DispatchToCommandThread, it should be run on main thread. So, I think we can add the assertion for now, and remove it if necessarily. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +1469,5 @@ > + ToLowerCase(newValue.get_ArrayOfBluetoothNamedValue()[0].name()); > + BluetoothSignal signal(NS_LITERAL_STRING("PairedStatusChanged"), > + NS_LITERAL_STRING(KEY_LOCAL_AGENT), > + newValue); > + handler = new DistributeBluetoothSignalTask(signal); Agree. @@ +1480,5 @@ > LOG_AND_FREE_DBUS_ERROR_WITH_MSG(&err, aMsg); > errorStr.AssignLiteral("Cannot parse manager path!"); > } else { > v = NS_ConvertUTF8toUTF16(str); > + handler = new PrepareAdapterTask(v.get_nsString()); Agree. @@ +1501,5 @@ > } > > + if (handler) { > + NS_DispatchToMainThread(handler); > + } Removed
Attachment #757758 - Attachment is obsolete: true
Attachment #757758 - Flags: review?(echou)
Attachment #759032 - Flags: review?(echou)
Comment on attachment 759032 [details] [diff] [review] Patch 1(v2): BluetoothDBusService cleanup Review of attachment 759032 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #759032 - Flags: review?(echou) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch 2(v1): follow-up patch (obsolete) — Splinter Review
return value type error
Attachment #760335 - Flags: review?(vyang)
Comment on attachment 760335 [details] [diff] [review] Patch 2(v1): follow-up patch Review of attachment 760335 [details] [diff] [review]: ----------------------------------------------------------------- Oh YA!
Attachment #760335 - Flags: review?(vyang) → review+
Attachment #760335 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.1 QE3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: