Closed Bug 1220121 Opened 9 years ago Closed 9 years ago

[Bluetooth] Move conversion of DOM strings into client app

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

(firefox45 fixed)

Status:
RESOLVED FIXED
Milestone:
2.6 S1 - 11/20
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(7 files, 5 obsolete files)

[03] Bug 1220121: Convert IPDL of Bluetooth Core API to |BluetoothPinCode|
12.20 KB, patch
brsun
: review+
Details | Diff | Splinter Review
[04] Bug 1220121: Convert IPDL of Bluetooth OPP API to |BluetoothAddress|
16.65 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
[05] Bug 1220121: Convert IPDL of Bluetooth AVRCP API to |ControlPlayStatus|
11.42 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[02] Bug 1220121: Convert IPDL of Bluetooth Core API to |BluetoothAddress| (v2)
49.05 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[06] Bug 1220121: Convert IPDL of Bluetooth GATT API to |BluetoothAddress|
25.23 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[01] Bug 1220121: Prepare IPDL support for additional Bluetooth types (v3)
3.68 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[07] Bug 1220121: Convert IPDL of Bluetooth GATT API to |BluetoothUuid| (v2)
75.05 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review 
Gecko's parent process converts strings into internal data types. This code should be moved into the client process, for reasons of security, overhead, and general design.
Bug 1211769 implements a time-stamp string, which might be represented by a special-purpose type.
See Also: → 1211769
Comment on attachment 8681893 [details] [diff] [review]
[04] Bug 1220121: Convert IPDL of Bluetooth OPP API to |BluetoothAddress|

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

LGTM.
Attachment #8681893 - Flags: review?(btian) → review+
Comment on attachment 8681890 [details] [diff] [review]
[01] Bug 1220121: Prepare IPDL support for additional Bluetooth types

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

LGTM. r=me if the following suggestion would be addressed.

::: dom/bluetooth/ipc/BluetoothMessageUtils.h
@@ +272,5 @@
> +    if (!ReadParam(aMsg, aIter, &result)) {
> +      return false;
> +    }
> +    switch (result) {
> +      case mozilla::dom::bluetooth::ControlPlayStatus::PLAYSTATUS_STOPPED:

There would be implicit type conversions between |uint8_t| and |mozilla::dom::bluetooth::ControlPlayStatus| while comparing the values of |result| and the values which each |case| specified.

Suggest to use explicit type conversions by our own, otherwise we have to make sure the compilers on different platforms with different configurations are comfortable with this implicit conversion.
Attachment #8681890 - Flags: review?(brsun) → review+
Comment on attachment 8681891 [details] [diff] [review]
[02] Bug 1220121: Convert IPDL of Bluetooth Core API to |BluetoothAddress|

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

LGTM. r=me if the following suggestions will be addressed.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +2914,5 @@
> +  deviceAddresses.SetLength(aDeviceAddresses.Length());
> +
> +  for (size_t i = 0; i < aDeviceAddresses.Length(); ++i) {
> +    AddressToString(aDeviceAddresses[i], deviceAddresses[i]);
> +  }

Suggest put these |AddressToString(...)| conversion into the class |BluetoothArrayOfDevicePropertiesReplyHandler|.

@@ +3169,2 @@
>  {
> +  auto task = new CreatePairedDeviceInternalTask(aDeviceAddress,

|Task*| has been replaced with |auto| on this line, but not on the other places. It is absolutely OK to use |auto| here, but the modifications are a little bit not so synchronous for me. Do you prefer to use |auto| as well on other places which is going to be modified in this patch? Or leave it unchanged (i.e. |Task*|) of this line?

Both ways are OK for me. Just want to make sure the intention of this modification is sync with other places in the whole patch.

@@ +3735,4 @@
>      // I choose to use raw pointer here because this is going to be passed as an
>      // argument into SendWithReply() at once.
>      OnUpdateSdpRecordsRunnable* callbackRunnable =
> +      new OnUpdateSdpRecordsRunnable(mDeviceAddress, mBluetoothProfileManager);

Remove the local variable |objectPath| if it is not used in other places anymore.
Attachment #8681891 - Flags: review?(brsun) → review+
Comment on attachment 8681895 [details] [diff] [review]
[06] Bug 1220121: Convert IPDL of Bluetooth GATT API to |BluetoothAddress|

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

r=me with comments addressed.

::: dom/bluetooth/common/webapi/BluetoothGatt.cpp
@@ +123,5 @@
> +  BluetoothAddress deviceAddr;
> +  BT_ENSURE_TRUE_REJECT(
> +    NS_SUCCEEDED(StringToAddress(mDeviceAddr, deviceAddr)),
> +    promise,
> +    NS_ERROR_DOM_INVALID_STATE_ERR);

Let's move this section to before |if (mAppUuid.IsEmpty()) {|.

@@ +156,5 @@
> +  BluetoothAddress deviceAddr;
> +  BT_ENSURE_TRUE_REJECT(
> +    NS_SUCCEEDED(StringToAddress(mDeviceAddr, deviceAddr)),
> +    promise,
> +    NS_ERROR_DOM_INVALID_STATE_ERR);

In this file, addresses are reported by stack, how about we use NS_ERROR_DOM_OPERATION_ERR here and below?

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +322,5 @@
> +  BluetoothAddress address;
> +  BT_ENSURE_TRUE_REJECT(
> +    NS_SUCCEEDED(StringToAddress(aAddress, address)),
> +    promise,
> +    NS_ERROR_DOM_INVALID_STATE_ERR);

Please use NS_ERROR_INVALID_ARG for converting aAddress to |BluetoothAddress| type in this file.
Attachment #8681895 - Flags: review?(joliu) → review+
Comment on attachment 8681892 [details] [diff] [review]
[03] Bug 1220121: Convert IPDL of Bluetooth Core API to |BluetoothPinCode|

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

LGTM.
Attachment #8681892 - Flags: review?(brsun) → review+
Comment on attachment 8681890 [details] [diff] [review]
[01] Bug 1220121: Prepare IPDL support for additional Bluetooth types

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

::: dom/bluetooth/ipc/BluetoothMessageUtils.h
@@ +69,5 @@
> +        return false;
> +      }
> +    }
> +    for (uint8_t i = aResult->mLength;
> +                 i < MOZ_ARRAY_LENGTH(aResult->mPinCode); ++i) {

One more thing. The style of the indent seems not so sync with other parts of Gecko. How about put the leading |i| on the same column of |u| on the previous line?
Changes since v1:

  - fixed casting of |ControlPlayStatus|
  - style cleanups
Attachment #8681890 - Attachment is obsolete: true
Attachment #8682407 - Flags: review+
Changes since v1:

  - moved address conversion into |BluetoothArrayOfDevicePropertiesReplyHandler|
  - explicitly declare variable as |Task*|
Attachment #8681891 - Attachment is obsolete: true
Attachment #8682408 - Flags: review+
Changes since v1:

  - fixed error codes of address conversion
  - moved address conversion to new location
Attachment #8681895 - Attachment is obsolete: true
Attachment #8682409 - Flags: review+
(In reply to Bruce Sun [:brsun] from comment #11)
> @@ +3735,4 @@
> >      // I choose to use raw pointer here because this is going to be passed as an
> >      // argument into SendWithReply() at once.
> >      OnUpdateSdpRecordsRunnable* callbackRunnable =
> > +      new OnUpdateSdpRecordsRunnable(mDeviceAddress, mBluetoothProfileManager);
> 
> Remove the local variable |objectPath| if it is not used in other places
> anymore.

It's still required by |SendWithReply|.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #18)
> (In reply to Bruce Sun [:brsun] from comment #11)
> > @@ +3735,4 @@
> > >      // I choose to use raw pointer here because this is going to be passed as an
> > >      // argument into SendWithReply() at once.
> > >      OnUpdateSdpRecordsRunnable* callbackRunnable =
> > > +      new OnUpdateSdpRecordsRunnable(mDeviceAddress, mBluetoothProfileManager);
> > 
> > Remove the local variable |objectPath| if it is not used in other places
> > anymore.
> 
> It's still required by |SendWithReply|.

Oops, I missed it. Thanks for the clarification.
Comment on attachment 8681894 [details] [diff] [review]
[05] Bug 1220121: Convert IPDL of Bluetooth AVRCP API to |ControlPlayStatus|

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
@@ +246,4 @@
>    virtual void
>    SendPlayStatus(int64_t aDuration,
>                   int64_t aPosition,
> +                 ControlPlayStatus aPlayStatus,

Maybe "ControlPlayStatus& aPlayStatus"?
Attachment #8681894 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #20)
> Comment on attachment 8681894 [details] [diff] [review]
> [05] Bug 1220121: Convert IPDL of Bluetooth AVRCP API to |ControlPlayStatus|
> 
> Review of attachment 8681894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
> @@ +246,4 @@
> >    virtual void
> >    SendPlayStatus(int64_t aDuration,
> >                   int64_t aPosition,
> > +                 ControlPlayStatus aPlayStatus,
> 
> Maybe "ControlPlayStatus& aPlayStatus"?

It's just an enum, so no overhead here.
Hi Thomas,

Currently the promise got from |BluetoothAdapter.startLeScan|[1] will always be rejected.

In bug 1215525[2], the type of |BluetoothGattClient::mAppUuid| in |BluetoothGattManager| has been changed from |nsString| to |BluetoothUuid|. But due to the type mismatch between the sender[3] and the receiver[4] of the IPC channel, the promise would always be rejected.

The solution to this issue might not exactly match the purpose of this bug. But since we probably won't use |nsString| for the UUID under WebAPI interfaces after this bug has been solved, would you mind also handling the type conversion between |nsString| and |BluetoothUuid| inside |BluetoothDiscoveryHandle|[5] in this bug as well? Or should we handle this issue in another individual bug as a follow-up?

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#startLeScan.28sequence.3CDOMString.3E_aServiceUuids.29
[2] https://hg.mozilla.org/mozilla-central/diff/4b87c7349135/dom/bluetooth/bluedroid/BluetoothGattManager.cpp#l1.79
[3] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothGattManager.cpp#768
[4] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothAdapter.cpp#152
[5] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothDiscoveryHandle.h#59-74
Flags: needinfo?(tzimmermann)
Hi,

For cleanliness, I'd suggest to open up a new bug report, but either is fine for me.
Flags: needinfo?(tzimmermann)
BTW, I'm working on patches for transfering data in "the other direction" (from driver to app). I found this very fragile. When I change the sender, I never know if I catched all the receivers and vice versa.
Changes since v2:

  - fixed unpacking of |ControlPlayStatus|
Attachment #8682407 - Attachment is obsolete: true
Attachment #8684197 - Flags: review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #23)
> Hi,
> 
> For cleanliness, I'd suggest to open up a new bug report, but either is fine
> for me.

I'll open another bug for it and provide patches after tested.
See Also: → 1222956
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> BTW, I'm working on patches for transfering data in "the other direction"
> (from driver to app). I found this very fragile. When I change the sender, I
> never know if I catched all the receivers and vice versa.

What do you mean "from driver to app"?
Flags: needinfo?(tzimmermann)
(In reply to Shawn Huang [:shawnjohnjr] from comment #27)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> > BTW, I'm working on patches for transfering data in "the other direction"
> > (from driver to app). I found this very fragile. When I change the sender, I
> > never know if I catched all the receivers and vice versa.
> 
> What do you mean "from driver to app"?

I mean the Bluetooth values and signals that are generated when the Bluedroid driver invokes a notification handler. The notification handlers convert some the data structures to strings, as named values currently often use strings. It would be nice to use strong types in the named values and do the conversation in the client.

The signaling code in this direction is more complex, so I'm not sure in how far it can be converted completely.
Flags: needinfo?(tzimmermann)
Comment on attachment 8681896 [details] [diff] [review]
[07] Bug 1220121: Convert IPDL of Bluetooth GATT API to |BluetoothUuid|

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

r=me with comments addressed.

Sorry for taking it so long, I was on biz trip last week and on leave for two days. :(

::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ +369,2 @@
>      for (uint32_t i = 0; i < mLeScanHandleArray.Length(); ++i) {
> +      mLeScanHandleArray[i]->GetLeScanUuid(uuidStr);

Better to use BluetoothUuid in DiscoveryHandle also, I saw you would like to do it in a followup bug, I'm fine with that.

@@ +689,5 @@
> +  for (size_t i = 0; i < serviceUuids.Length(); ++i) {
> +    BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(aServiceUuids[i],
> +                                                    serviceUuids[i])),
> +                          promise,
> +                          NS_ERROR_DOM_INVALID_STATE_ERR);

Suggest to use NS_ERROR_DOM_OPERATION_ERR here and below.

@@ +737,5 @@
> +
> +  BluetoothUuid scanUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(scanUuidStr, scanUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

DITTO.

::: dom/bluetooth/common/webapi/BluetoothGatt.cpp
@@ +122,5 @@
>  
> +  BluetoothUuid appUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(mAppUuid, appUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

Suggest to use NS_ERROR_DOM_OPERATION_ERR.

String type is used for registering/unregistering signals, how about we have separate member values for BluetoothUuid and nsString?
Or convert back to string when registering/unregistering, it would be less than doing it in each API method.

::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.cpp
@@ +157,5 @@
> +  BluetoothUuid appUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(mService->GetAppUuid(),
> +                                                  appUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

Please refer to my comment in BluetoothGattDescriptor.cpp

@@ +190,5 @@
> +  BluetoothUuid appUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(mService->GetAppUuid(),
> +                                                  appUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

DITTO.

@@ +377,5 @@
> +  BluetoothUuid appUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(mService->GetAppUuid(),
> +                                                  appUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

DITTO.

@@ +426,5 @@
> +  BluetoothUuid appUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(mService->GetAppUuid(),
> +                                                  appUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

DITTO.

::: dom/bluetooth/common/webapi/BluetoothGattDescriptor.cpp
@@ +235,5 @@
> +  BluetoothUuid appUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(
> +                          mCharacteristic->Service()->GetAppUuid(), appUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

Suggest to use NS_ERROR_DOM_OPERATION_ERR.

I think it's better to get BluetoothUuid directly other than converting it in each use case.
Could you revise it or open a follow up bug for this, please?

@@ +272,5 @@
> +  BluetoothUuid appUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(
> +                          mCharacteristic->Service()->GetAppUuid(), appUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

DITTO.

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +321,5 @@
>  
> +  BluetoothUuid appUuid;
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(mAppUuid, appUuid)),
> +                        promise,
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);

Please refer to my comments in BluetoothGatt.cpp for this file.
Attachment #8681896 - Flags: review?(joliu) → review+
Hi

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #29)

> ::: dom/bluetooth/common/webapi/BluetoothGatt.cpp
> @@ +122,5 @@
> >  
> > +  BluetoothUuid appUuid;
> > +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(mAppUuid, appUuid)),
> > +                        promise,
> > +                        NS_ERROR_DOM_INVALID_STATE_ERR);
> 
> Suggest to use NS_ERROR_DOM_OPERATION_ERR.
> 
> String type is used for registering/unregistering signals, how about we have
> separate member values for BluetoothUuid and nsString?
> Or convert back to string when registering/unregistering, it would be less
> than doing it in each API method.

I was working on a follow-up patch for signals. This would add an API that takes UUIDs, addresses, etc and converts them internally to strings. Can I open another bug report for this?

> ::: dom/bluetooth/common/webapi/BluetoothGattDescriptor.cpp
> @@ +235,5 @@
> > +  BluetoothUuid appUuid;
> > +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(
> > +                          mCharacteristic->Service()->GetAppUuid(), appUuid)),
> > +                        promise,
> > +                        NS_ERROR_DOM_INVALID_STATE_ERR);
> 
> Suggest to use NS_ERROR_DOM_OPERATION_ERR.
> 
> I think it's better to get BluetoothUuid directly other than converting it
> in each use case.
> Could you revise it or open a follow up bug for this, please?

Do you mean that these structures should store UUIDs as |BluetoothUuid| in stead of |nsString|? I was thinking about this as well. I'd like to investigate this in a follow-up bug report.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> Hi
> 
> (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #29)
> 
> > ::: dom/bluetooth/common/webapi/BluetoothGatt.cpp
> > @@ +122,5 @@
> > >  
> > > +  BluetoothUuid appUuid;
> > > +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(mAppUuid, appUuid)),
> > > +                        promise,
> > > +                        NS_ERROR_DOM_INVALID_STATE_ERR);
> > 
> > Suggest to use NS_ERROR_DOM_OPERATION_ERR.
> > 
> > String type is used for registering/unregistering signals, how about we have
> > separate member values for BluetoothUuid and nsString?
> > Or convert back to string when registering/unregistering, it would be less
> > than doing it in each API method.
> 
> I was working on a follow-up patch for signals. This would add an API that
> takes UUIDs, addresses, etc and converts them internally to strings. Can I
> open another bug report for this?
> 

Sounds great, open a followup bug is fine.

> > ::: dom/bluetooth/common/webapi/BluetoothGattDescriptor.cpp
> > @@ +235,5 @@
> > > +  BluetoothUuid appUuid;
> > > +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(StringToUuid(
> > > +                          mCharacteristic->Service()->GetAppUuid(), appUuid)),
> > > +                        promise,
> > > +                        NS_ERROR_DOM_INVALID_STATE_ERR);
> > 
> > Suggest to use NS_ERROR_DOM_OPERATION_ERR.
> > 
> > I think it's better to get BluetoothUuid directly other than converting it
> > in each use case.
> > Could you revise it or open a follow up bug for this, please?
> 
> Do you mean that these structures should store UUIDs as |BluetoothUuid| in
> stead of |nsString|? I was thinking about this as well. I'd like to
> investigate this in a follow-up bug report.
Yes, especially when you're going to refine signal registeration/unregisteration. :)
Done in a followup bug is fine by me.
Thanks a lot for the effort.
Changes since v1:

  - replace NS_ERROR_INVALID_STATE_ERR by NS_ERROR_OPERATION_ERR
Attachment #8681896 - Attachment is obsolete: true
Attachment #8685926 - Flags: review+
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=3325231&repo=b2g-inbound
Flags: needinfo?(tzimmermann)
I fixed the build errors and the other problems in the try session seem unrelated.

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=74199d201199
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=3343337&repo=b2g-inbound
Flags: needinfo?(tzimmermann)
Depends on: 1224166
This try session adds a fix for linking BluetoothCommon.o. I guess the failed builds in the previous try session had the same error as the one on b2g-inbound, but didn't show a log.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea97abfe664
Flags: needinfo?(tzimmermann)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: