[b2g-bluetooth] BluetoothService Cleanup

RESOLVED FIXED in 1.1 QE3 (26jun)

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gyeh, Assigned: gyeh)

Tracking

unspecified
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
I'd like to re-arrange the order of static functions in BluetoothDBusService and also do some clean-up.
(Assignee)

Updated

6 years ago
No longer depends on: 878728
(Assignee)

Comment 1

6 years ago
Created attachment 757345 [details] [diff] [review]
Patch 1(v1): BluetoothService cleanup

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

6 years ago
Created attachment 757758 [details] [diff] [review]
Patch 1(v1): BluetoothDBusService cleanup

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
(Assignee)

Comment 4

6 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

6 years ago
Created attachment 759032 [details] [diff] [review]
Patch 1(v2): BluetoothDBusService cleanup
Attachment #757758 - Attachment is obsolete: true
Attachment #757758 - Flags: review?(echou)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 8

6 years ago
Created attachment 760264 [details] [diff] [review]
Final patch: BluetoothService cleanup, r=echou

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
https://hg.mozilla.org/mozilla-central/rev/1b54057a41cc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

6 years ago
Created attachment 760335 [details] [diff] [review]
Patch 2(v1): follow-up patch

return value type error
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 13

6 years ago
Created attachment 760340 [details] [diff] [review]
Followup patch, r=vyang
Attachment #760335 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8a05d7dc509f
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 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.