Closed
Bug 878745
Opened 12 years ago
Closed 12 years ago
[b2g-bluetooth] BluetoothService Cleanup
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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)
74.79 KB,
patch
|
Details | Diff | Splinter Review | |
1.61 KB,
patch
|
Details | Diff | Splinter Review |
I'd like to re-arrange the order of static functions in BluetoothDBusService and also do some clean-up.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
Update patch
Attachment #757345 -
Attachment is obsolete: true
Attachment #757345 -
Flags: review?(echou)
Attachment #757758 -
Flags: review?(echou)
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #757758 -
Attachment is obsolete: true
Attachment #757758 -
Flags: review?(echou)
Assignee | ||
Updated•12 years ago
|
Attachment #759032 -
Flags: review?(echou)
Comment 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
try:
https://tbpl.mozilla.org/?tree=Try&rev=3a44068ffd88
https://tbpl.mozilla.org/?tree=Try&rev=f28197710d6c
Attachment #759032 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
Note that this patch should be landed after bug872907.
try:
https://tbpl.mozilla.org/?tree=Try&rev=1883bc329d0a
https://tbpl.mozilla.org/?tree=Try&rev=f3a3bb177406
Attachment #759676 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
Whiteboard: [fixed-in-birch]
Comment 10•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•12 years ago
|
||
return value type error
Assignee | ||
Updated•12 years ago
|
Attachment #760335 -
Flags: review?(vyang)
Comment 12•12 years ago
|
||
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+
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #760335 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 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.
Description
•