1172914 - Reduce duplicated code in BluetoothServiceBluedroid.{cpp,h}

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

There's still duplicated code between Bluetooth v1 and v2 in BluetoothServiceBluedroid.{cpp,h}. We should try to merge some of it.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1172914: Minimize Bluetooth v1/v2 duplication in BluetoothServiceBluedroid.h

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1172914: Minimize Bluetooth v1/v2 duplication in BluetoothServiceBluedroid.cpp

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1172914: Merge duplicated code in |BluetoothServiceBluedroid| for simple cases

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8617335 [details] [diff] [review]
[01] Bug 1172914: Minimize Bluetooth v1/v2 duplication in BluetoothServiceBluedroid.h

Review of attachment 8617335 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
@@ +161,5 @@
>    virtual bool
>    IsConnected(uint16_t aProfileId);
> +#else
> +  virtual void
> +  IsConnected(const uint16_t aServiceUuid,

v2 should adopt v1 version [1] since v1's is newer. Please help open a follow-up and cc Jamin & me.

[1] bug 929376 comment 37

@@ +362,5 @@
> +#ifdef MOZ_B2G_BT_API_V2
> +  // Missing in Bluetooth v2
> +#else
> +  virtual void EnergyInfoNotification(
> +    const BluetoothActivityEnergyInfo& aInfo) override;

v2 should adopt this function for L compatibility [1]. Please help open another follow-up bug and cc Bruce and me.

[2] https://hg.mozilla.org/mozilla-central/rev/55358ad9f833

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
> @@ +161,5 @@
> >    virtual bool
> >    IsConnected(uint16_t aProfileId);
> > +#else
> > +  virtual void
> > +  IsConnected(const uint16_t aServiceUuid,
> 
> v2 should adopt v1 version [1] since v1's is newer. Please help open a
> follow-up and cc Jamin & me.
> 
> [1] bug 929376 comment 37

OK. Bug 1172266.

> @@ +362,5 @@
> > +#ifdef MOZ_B2G_BT_API_V2
> > +  // Missing in Bluetooth v2
> > +#else
> > +  virtual void EnergyInfoNotification(
> > +    const BluetoothActivityEnergyInfo& aInfo) override;
> 
> v2 should adopt this function for L compatibility [1]. Please help open
> another follow-up bug and cc Bruce and me.
> 
> [2] https://hg.mozilla.org/mozilla-central/rev/55358ad9f833

This is already done in patch [03] :)

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8617337 [details] [diff] [review]
[02] Bug 1172914: Minimize Bluetooth v1/v2 duplication in BluetoothServiceBluedroid.cpp

Review of attachment 8617337 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +138,1 @@
>   *  Static callback functions

Correct to 'Static functions' as these are not callbacks.

@@ +164,2 @@
>   *  Static callback functions
>   */

Remove this duplicate comment since it appears before |PlayStatusStringToControlPlayStatus| already.

@@ +414,5 @@
> +#ifdef MOZ_B2G_BT_API_V2
> +    BluetoothService::AcknowledgeToggleBt(true);
> +#else
> +    // Always make progress; even on failures
> +    BluetoothService::AcknowledgeToggleBt(false);

Should v2 adopt the change of 1132229 as well? If so, please open a follow-up bug for it.

@@ +463,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  // aRunnable will be a nullptr while startup
> +  if(aRunnable) {

nit: space before '('.

@@ +472,5 @@
> +  if (NS_FAILED(ret)) {
> +    BluetoothService::AcknowledgeToggleBt(false);
> +
> +    // Reject Promise
> +    if(aRunnable) {

Ditto.

@@ +515,5 @@
> +    }
> +  }
> +
> +  // aRunnable will be a nullptr during starup and shutdown
> +  if(aRunnable) {

Ditto.

@@ +524,5 @@
> +  if (NS_FAILED(ret)) {
> +    BluetoothService::AcknowledgeToggleBt(true);
> +
> +    // Reject Promise
> +    if(aRunnable) {

Ditto.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> @@ +414,5 @@
> > +#ifdef MOZ_B2G_BT_API_V2
> > +    BluetoothService::AcknowledgeToggleBt(true);
> > +#else
> > +    // Always make progress; even on failures
> > +    BluetoothService::AcknowledgeToggleBt(false);
> 
> Should v2 adopt the change of 1132229 as well? If so, please open a
> follow-up bug for it.

I opened bug 1173282.

I was wondering about this difference as well. I don't know which one is correct, but I'd guess that it's v1 with the restart changes.

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8617338 [details] [diff] [review]
[03] Bug 1172914: Merge duplicated code in |BluetoothServiceBluedroid| for simple cases

Review of attachment 8617338 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my question below.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +883,4 @@
>          DispatchReplyError(sGetDeviceRunnableArray[0],
>            NS_LITERAL_STRING("GetRemoteDeviceProperties failed"));
> +#else
> +        DispatchReplySuccess(sGetDeviceRunnableArray[0], sRemoteDevicesPack);

Shouldn't we reply error here? The difference results from bug 1033961 comment 16, where I suggested to reply error as |GetRemoteDeviceProperties| fails.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1172914: Merge duplicated code in |BluetoothServiceBluedroid| for simple cases (v2)

Changes since v1

  - send error if GetRemoteDeviceProperties fails

I read the old review from bug 1033961. I vaguely remember that I found it strange that the v1 code signals success even when it failed, but I also vaguely remember that I took this semantics from the earlier versions. Anyway, now I picked 'failing' for both variants.

Comment 10

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> I read the old review from bug 1033961. I vaguely remember that I found it
> strange that the v1 code signals success even when it failed, but I also
> vaguely remember that I took this semantics from the earlier versions.
> Anyway, now I picked 'failing' for both variants.

I think the question is: should we return successful result even though some |GetRemoteDeviceProperties| operations meet error?

[1] and [2] query remote device properties for each paired/connected device. Say there are 5 devices in total and 3 of them fail to get remote properties. In v1, we reply success with result of 2 devices; whereas in v2, we reply error to inform the failure of incompletion.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#1170
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#1136

Comment 11

[Ben Tian (inactive)]

One option is to append address-only properties array like [1][2] (as if we receive empty |aProperties| from callback) and always reply success. The pro is that application always gets correct number and addresses of paired/connected devices.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#2398
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#2470

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8617338 [details] [diff] [review]
[03] Bug 1172914: Merge duplicated code in |BluetoothServiceBluedroid| for simple cases

Thomas, I decide to keep both v1 and v2 approaches for comparison. Please open a follow-up bug to further discuss (based on comment 10 & 11) and revise this section separately.

r=me on this old patch that keeps both approaches. Let me know for any concern.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi!

I agree that this is a problem and both replies make sense. I think the core problem is in the interface, in that we reach out to multiple devices at the same time.

I'd rather have a separate bug for discussing on how to solve this issue, because it might be a bigger thing. I just wanted to suggest to use the original patch, but now I saw that you already gave an r+. :) Thanks!

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> > ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
> > @@ +161,5 @@
> > >    virtual bool
> > >    IsConnected(uint16_t aProfileId);
> > > +#else
> > > +  virtual void
> > > +  IsConnected(const uint16_t aServiceUuid,
> > 
> > v2 should adopt v1 version [1] since v1's is newer. Please help open a
> > follow-up and cc Jamin & me.
> > 
> > [1] bug 929376 comment 37
> 
> OK. Bug 1172266.

Sorry, bug 1173266.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1172914: Minimize Bluetooth v1/v2 duplication in BluetoothServiceBluedroid.h (v2)

Changes since v1:

  - rebased on latest m-c

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1172914: Minimize Bluetooth v1/v2 duplication in BluetoothServiceBluedroid.cpp (v2)

Changes since v1:

  - rebased on latest m-c
  - cleaned up 'Static functions' comments
  - '(' style fixes

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1172914: Merge duplicated code in |BluetoothServiceBluedroid| for simple cases (v3)

Changes since v2:

  - rebased v1 onto latest m-c

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=72abbfb4475b

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/adc5287cc336
https://hg.mozilla.org/integration/b2g-inbound/rev/b44ee3748020

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://hg.mozilla.org/integration/b2g-inbound/rev/72abbfb4475b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/adc5287cc336
https://hg.mozilla.org/mozilla-central/rev/b44ee3748020
https://hg.mozilla.org/mozilla-central/rev/72abbfb4475b

Comment 22

[Maria Angeles Oteo (:oteo)]

As NGA Program Manager suggested, let's replace the NGA-Sx milestones with FxOS-Sx ones (more generic ones), once Bug 1174794 has already landed