If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement BluetoothDevice.type attribute

RESOLVED FIXED

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: btian, Assigned: btian)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webbt-api])

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

3 years ago
* https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#type

Revise BluetoothDevice2.webidl and code under dom/bluetooth2.
(Assignee)

Updated

3 years ago
Assignee: nobody → btian
Whiteboard: [webbt-api]
(Assignee)

Comment 1

3 years ago
Created attachment 8482597 [details] [diff] [review]
Patch 1/3 (v1): Cleanup WebIDL files

Changes:
- make BluetoothPairingEvent.device and .handle non-nullable according to [1].
- re-order declaration
- add comments

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#BluetoothPairingEvent
(Assignee)

Comment 2

3 years ago
Created attachment 8482599 [details] [diff] [review]
Patch 2/3 (v1): WebIDL change of BluetoothDevice.type attribute

Changes:
- add attribute BluetoothDevice.type
- add enum BluetoothDeviceType
(Assignee)

Comment 3

3 years ago
Created attachment 8482600 [details] [diff] [review]
Patch 3/3 (v1): Implementation of BluetoothDevice.type attribute
Attachment #8482600 - Flags: review?(echou)
(Assignee)

Comment 4

3 years ago
Comment on attachment 8482600 [details] [diff] [review]
Patch 3/3 (v1): Implementation of BluetoothDevice.type attribute

Hi Thomas,

I rename |BluetoothDeviceType| in BluetoothCommon.h since [1] also has enum with identical name. Let me know if you have any suggestion.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#BluetoothDeviceType

--
Changes:
- handle device type change
- rename BluetoothDeviceType in BluetoothCommon.h to BluetoothTypeOfDevice
- add DEVICE_TYPE_UNKNOWN to conform the value of enum BluetoothDeviceType in webidl and BluetoothTypeOfDevice.
Attachment #8482600 - Flags: feedback?(tzimmermann)
(Assignee)

Updated

3 years ago
Depends on: 1060245
Comment on attachment 8482600 [details] [diff] [review]
Patch 3/3 (v1): Implementation of BluetoothDevice.type attribute

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

Hi Ben,

'TypeOfDevice' is not a good name. And 'DeviceType' in general is a bad name, because it could refer to anything on the device. I just picked it because Android uses it. I'd rather like to see something like 'ProtocolStack', or 'ProtocolSupport', because that's what these values represent.

Actually, I'd encourage you to rename the WebIDL enum to this name and leave the low-level enum as it is. Otherwise please use the new name for the low-level enum. If you go for the latter option, please also rename the enum constants and PROPERTY_TYPE_OF_DEVICE accordingly. Thanks.
Attachment #8482600 - Flags: feedback?(tzimmermann)
Hi Ben,
Add my two cent here.

Device type is actually the term used in Generic Access Profile, which reflects to types of bluetooth products (the bluetooth chipset). This profile defines three device types based on the supported Core Configurations as defined in [Vol. 0], Part B Section 3.1. The device types are shown in
Table 1.1:
Device Type Description
BR/EDR Devices that support the “Basic Rate” Core Configuration (see
[Vol. 0], Part B Section 4.1)
LE only Devices that support the “Low Energy” Core Configuration (see
[Vol. 0], Part B Section 4.4)
BR/EDR/LE Devices that support the “Basic Rate and Low Energy Combined”
Core Configuration (see [Vol. 0], Part B Section 4.5)
Table 1.1: Device Types

Since it reflects to supported Core Configurations as defined in [Vol. 0], Part B Section 3.1.
Maybe we can also call it ProductType instead of DeviceType. Due to he overall Bluetooth spec uses "Device Type" term all around, shall we change the most common term?
Do you think ProductConfiguration is better?
Hi Shawn,

I see, thanks for the info. Then let's keep 'TypeOfDevice,' as it is at least close to the spec and there is also already a Bluedroid property with that name.

Ben, please also rename the enumerator's DEVICE_TYPE_ prefixes to 'TYPE_OF_DEVICE_'.
Comment on attachment 8482600 [details] [diff] [review]
Patch 3/3 (v1): Implementation of BluetoothDevice.type attribute

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

f+ with nits

::: dom/bluetooth2/BluetoothCommon.h
@@ +205,4 @@
>    DEVICE_TYPE_BREDR,
>    DEVICE_TYPE_BLE,
>    DEVICE_TYPE_DUAL
>  };

Please rename the enum's prefixes according to its name. And please remove 'UNKNOWN'. This doesn't exist in the low-level code.
Attachment #8482600 - Flags: feedback+
(Assignee)

Comment 9

3 years ago
Created attachment 8483377 [details] [diff] [review]
Patch 3/3 (v2): Implementation of BluetoothDevice.type attribute

Revise based on Thomas' feedback. Note in BluetoothDevice.cpp I have to +1 to backend device type value since it doesn't has "unknown" device type.

Set fb? Thomas again to confirm.
Attachment #8482600 - Attachment is obsolete: true
Attachment #8482600 - Flags: review?(echou)
Attachment #8483377 - Flags: feedback?(tzimmermann)
Comment on attachment 8483377 [details] [diff] [review]
Patch 3/3 (v2): Implementation of BluetoothDevice.type attribute

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

I have a general question: I thought that all WebIDL constants are supposed to become strings in the long term. Isn't that the case for Bluetooth? The device type is an enum.

::: dom/bluetooth2/BluetoothDevice.cpp
@@ +131,5 @@
>      mUuids = value.get_ArrayOfnsString();
>      BluetoothDeviceBinding::ClearCachedUuidsValue(this);
> +  } else if (name.EqualsLiteral("Type")) {
> +    // +1 since backend device type value ranges [0, 2] without "unknown"
> +    mType = static_cast<BluetoothDeviceType>(value.get_uint32_t() + 1);

I'd encourage you to write a small function that does the conversion using a lookup table. The casting and adding seems fragile.
Attachment #8483377 - Flags: feedback?(tzimmermann) → feedback+
(Assignee)

Comment 11

3 years ago
Created attachment 8484809 [details] [diff] [review]
[final] Patch 3/3: Implementation of BluetoothDevice.type attribute, f=tzimmermann, r=echou

Revise based on Thomas' comment.

Changes:
- add a function to convert uint32_t to BluetoothDeviceType in BluetoothDevice.
- move macro |CONVERT| definition from BluetoothInterface.cpp to BluetoothCommon.h
Attachment #8483377 - Attachment is obsolete: true
Attachment #8484809 - Flags: review?(echou)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8482597 [details] [diff] [review]
Patch 1/3 (v1): Cleanup WebIDL files

Hi bz,

Can you review the following two patches of WebBluetooth IDL change?

The 1st patch cleans up prior WebIDL files with changes:
- make BluetoothPairingEvent.device and .handle non-nullable according to [1].
- re-order declaration
- add comments
[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#BluetoothPairingEvent

The 2nd patch adds an attribute BluetoothDevice.type to indicate the remote device' type (comment 6):
- add attribute BluetoothDevice.type [2]
- add enum BluetoothDeviceType [3]
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#type
[3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#BluetoothDeviceType
Attachment #8482597 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Attachment #8482599 - Flags: review?(bzbarsky)
Comment on attachment 8482597 [details] [diff] [review]
Patch 1/3 (v1): Cleanup WebIDL files

>+  Promise<sequence<DOMString> >             fetchUuids();

Why the added space?  There's no reason to do that.

>+++ b/dom/webidl/BluetoothPairingEvent.webidl
>-  readonly attribute BluetoothDevice?        device;
>-  readonly attribute BluetoothPairingHandle? handle;
>+  readonly attribute BluetoothDevice        device;
>+  readonly attribute BluetoothPairingHandle handle;

That change looks wrong to me, since the constructor takes a dictionary that can pass null for those values.
Attachment #8482597 - Flags: review?(bzbarsky) → review-
Comment on attachment 8482599 [details] [diff] [review]
Patch 2/3 (v1): WebIDL change of BluetoothDevice.type attribute

>+ * Note "address" are "type" are excluded since they never change once

s/are/and/

r=me
Attachment #8482599 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #13)
> >+  Promise<sequence<DOMString> >             fetchUuids();
> 
> Why the added space?  There's no reason to do that.

Just want to make them distinctive as in .cpp files [1]. I'll remove the space as it doesn't seem a convention in .webidl files.
[1] http://dxr.mozilla.org/mozilla-central/source/accessible/base/EventQueue.cpp#489

> >+++ b/dom/webidl/BluetoothPairingEvent.webidl
> >-  readonly attribute BluetoothDevice?        device;
> >-  readonly attribute BluetoothPairingHandle? handle;
> >+  readonly attribute BluetoothDevice        device;
> >+  readonly attribute BluetoothPairingHandle handle;
> 
> That change looks wrong to me, since the constructor takes a dictionary that
> can pass null for those values.

Do objects in an event have to be nullable if their default values are null in the EventInit dictionary? If so, do you suggest other default values except null for EventInit dictionary?

The original design of BluetoothPairingEvent doesn't expect |device| and |handle| to be null but the EventInit dictionary requires default values, so we assign null for them. I wonder if there's better way to keep original design instead of making objects in the event nullable.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 16

3 years ago
Created attachment 8486243 [details] [diff] [review]
[final] Patch 2/3: WebIDL change of BluetoothDevice.type attribute, r=bz

Revise based on bz's comment.
Attachment #8482599 - Attachment is obsolete: true
Comment on attachment 8484809 [details] [diff] [review]
[final] Patch 3/3: Implementation of BluetoothDevice.type attribute, f=tzimmermann, r=echou

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

LGTM
Attachment #8484809 - Flags: review?(echou) → review+
> Just want to make them distinctive as in .cpp files

In .cpp files it was a workaround for a bug in an older version of the C++ standard and hence in some older compilers (which couldn't decide whether >> was the end of the template or a shift operator).  C++ code written today would not have the space there, for what it's worth.

> Do objects in an event have to be nullable if their default values are null in the
> EventInit dictionary?

Well... yes.  I mean, say you do |new BluetoothPairingEvent().device| (passing no dictionary, so picking up all the default values).  Now the "device" member gets set to null in the constructor, because that's the value in the dictionary.  Then you do the .device get, the C++ returns null to the bindings, the bindings know they won't get null (because you said so in the IDL), so they will unconditionally dereference the pointer and crash.  You should be able to observe this crash with attachment 8482597 [details] [diff] [review] and the above code.

> If so, do you suggest other default values except null for EventInit dictionary?

It depends on the type, no?

> The original design of BluetoothPairingEvent doesn't expect |device| and |handle| to be
> null 

OK, so what does the original design expect |new BluetoothPairingEvent().device| to return?

> I wonder if there's better way to keep original design

That relies on you being able to provide useful values for all properties of your event when it's constructed with an empty dictionary.  Can you do that?

One plausible option here, now that we have required dictionary members, is to require the relevant members in the dictionary.  So something like:

  dictionary BluetoothPairingEventInit : EventInit
  {
    required BluetoothDevice device;
    required BluetoothPairingHandle handle;
  };

This will make |new BluetoothPairingEvent()| throw, since the dictionary provided has no .device/.handle.

At heart, though, this is an API design decision: what do you want to happen for |new BluetoothPairingEvent()| with no argument?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 19

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #18)
> OK, so what does the original design expect |new
> BluetoothPairingEvent().device| to return?

I see. This is a case I didn't consider. The original design expects applications to obtain the event from only certain event handlers.

> One plausible option here, now that we have required dictionary members, is
> to require the relevant members in the dictionary.  So something like:
> 
>   dictionary BluetoothPairingEventInit : EventInit
>   {
>     required BluetoothDevice device;
>     required BluetoothPairingHandle handle;
>   };
> 
> This will make |new BluetoothPairingEvent()| throw, since the dictionary
> provided has no .device/.handle.
> 
> At heart, though, this is an API design decision: what do you want to happen
> for |new BluetoothPairingEvent()| with no argument?

Required dictionary members seem suitable since it prevents applications from |new BluetoothPairingEvent()| with no argument. I'll prepare a patch with required dictionary members for review.

Thanks for your explanation and insightful suggestion bz!
(Assignee)

Comment 20

3 years ago
Created attachment 8487076 [details] [diff] [review]
[final] Patch 1/3: Cleanup WebIDL files, r=bz

Make members in |BluetoothPairingEventInit| "required."
Attachment #8482597 - Attachment is obsolete: true
Attachment #8487076 - Flags: review?(bzbarsky)
Comment on attachment 8487076 [details] [diff] [review]
[final] Patch 1/3: Cleanup WebIDL files, r=bz

r=me.  Thanks!

Might be worth adding some tests that make sure the constructor throws on those members being missing.
Attachment #8487076 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

3 years ago
Sure. We'll test this in marionette test for pairing. Opened bug 1065823 to track.

(In reply to Boris Zbarsky [:bz] from comment #21)
> Comment on attachment 8487076 [details] [diff] [review]
> Patch 1/3 (v2): Cleanup WebIDL files
> 
> r=me.  Thanks!
> 
> Might be worth adding some tests that make sure the constructor throws on
> those members being missing.
(Assignee)

Updated

3 years ago
Attachment #8487076 - Attachment description: Patch 1/3 (v2): Cleanup WebIDL files → [final] Patch 1/3: Cleanup WebIDL files, r=bz
(Assignee)

Updated

3 years ago
Attachment #8484809 - Attachment description: Patch 3/3 (v3): Implementation of BluetoothDevice.type attribute, f=tzimmermann → [final] Patch 3/3: Implementation of BluetoothDevice.type attribute, f=tzimmermann, r=echou
(Assignee)

Comment 23

3 years ago
No try since the changed folder is not built by default.

https://hg.mozilla.org/integration/b2g-inbound/rev/efee139d45db
https://hg.mozilla.org/integration/b2g-inbound/rev/02f7498edae3
https://hg.mozilla.org/integration/b2g-inbound/rev/abc3a737f800
(Assignee)

Comment 24

3 years ago
http://hg.mozilla.org/mozilla-central/rev/efee139d45db
http://hg.mozilla.org/mozilla-central/rev/02f7498edae3
http://hg.mozilla.org/mozilla-central/rev/abc3a737f800
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.