Closed Bug 1172914 Opened 8 years ago Closed 8 years ago

Reduce duplicated code in BluetoothServiceBluedroid.{cpp,h}

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox41 fixed)

Status:
RESOLVED FIXED
Milestone:
FxOS-S1 (26Jun)
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(3 files, 4 obsolete files)

[01] Bug 1172914: Minimize Bluetooth v1/v2 duplication in BluetoothServiceBluedroid.h (v2)
16.40 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[02] Bug 1172914: Minimize Bluetooth v1/v2 duplication in BluetoothServiceBluedroid.cpp (v2)
37.55 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[03] Bug 1172914: Merge duplicated code in |BluetoothServiceBluedroid| for simple cases (v3)
13.75 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review 
There's still duplicated code between Bluetooth v1 and v2 in BluetoothServiceBluedroid.{cpp,h}. We should try to merge some of it.
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
Attachment #8617335 - Flags: review?(btian) → review+
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 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.
Attachment #8617337 - Flags: review?(btian) → review+
> @@ +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 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.
Attachment #8617338 - Flags: review?(btian)
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.
Attachment #8617338 - Attachment is obsolete: true
Attachment #8622455 - Flags: review?(btian)
(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
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 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.
Attachment #8617338 - Attachment is obsolete: false
Attachment #8617338 - Flags: review+
Attachment #8622455 - Flags: review?(btian)
Attachment #8622455 - Attachment is obsolete: true
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!
See Also: → 1175130
> > ::: 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.
Changes since v1:

  - rebased on latest m-c
Attachment #8617335 - Attachment is obsolete: true
Attachment #8623055 - Flags: review+
See Also: → 1173282
Changes since v1:

  - rebased on latest m-c
  - cleaned up 'Static functions' comments
  - '(' style fixes
Attachment #8617337 - Attachment is obsolete: true
Attachment #8623080 - Flags: review+
Changes since v2:

  - rebased v1 onto latest m-c
Attachment #8617338 - Attachment is obsolete: true
Attachment #8623084 - Flags: review+
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
Target Milestone: NGA S3 (26Jun) → FxOS-S1 (26Jun)
Depends on: 1176527
You need to log in before you can comment on or make changes to this bug.