Closed
Bug 1061481
Opened 10 years ago
Closed 10 years ago
Implement BluetoothDevice.type attribute
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ben.tian, Assigned: ben.tian)
References
Details
(Whiteboard: [webbt-api])
Attachments
(3 files, 4 obsolete files)
11.61 KB,
patch
|
echou
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
Details | Diff | Splinter Review | |
7.15 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
* https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#type Revise BluetoothDevice2.webidl and code under dom/bluetooth2.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → btian
Whiteboard: [webbt-api]
Assignee | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
Changes: - add attribute BluetoothDevice.type - add enum BluetoothDeviceType
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8482600 -
Flags: review?(echou)
Assignee | ||
Comment 4•10 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)
Comment 5•10 years ago
|
||
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?
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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•10 years ago
|
||
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 10•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 years ago
|
Attachment #8482599 -
Flags: review?(bzbarsky)
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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•10 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•10 years ago
|
||
Revise based on bz's comment.
Attachment #8482599 -
Attachment is obsolete: true
Comment 17•10 years ago
|
||
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+
Comment 18•10 years ago
|
||
> 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•10 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•10 years ago
|
||
Make members in |BluetoothPairingEventInit| "required."
Attachment #8482597 -
Attachment is obsolete: true
Attachment #8487076 -
Flags: review?(bzbarsky)
Comment 21•10 years ago
|
||
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•10 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•10 years ago
|
Attachment #8487076 -
Attachment description: Patch 1/3 (v2): Cleanup WebIDL files → [final] Patch 1/3: Cleanup WebIDL files, r=bz
Assignee | ||
Updated•10 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•10 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•10 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
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•