Closed Bug 1061481 Opened 10 years ago Closed 10 years ago

Implement BluetoothDevice.type attribute

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

(Whiteboard: [webbt-api])

Attachments

(3 files, 4 obsolete files)

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

Revise BluetoothDevice2.webidl and code under dom/bluetooth2.
Assignee: nobody → btian
Whiteboard: [webbt-api]
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
Changes:
- add attribute BluetoothDevice.type
- add enum BluetoothDeviceType
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)
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+
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+
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)
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)
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+
(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)
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)
(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!
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+
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.
Attachment #8487076 - Attachment description: Patch 1/3 (v2): Cleanup WebIDL files → [final] Patch 1/3: Cleanup WebIDL files, r=bz
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: