Closed
Bug 730990
Opened 13 years ago
Closed 13 years ago
Device object boilerplate for DOM Bluetooth
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: qdot, Assigned: echou)
References
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
qdot
:
review+
qdot
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
We need objects created per adapter (Bug 730970) that represent objects available to the adapter to bond and use.
Reporter | ||
Updated•13 years ago
|
Blocks: b2g-bluetooth
Depends on: 730970
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → echou
Assignee | ||
Comment 1•13 years ago
|
||
A BluetoothDevice object represents a remote device which :
(1) has been discovered/paired/connected by BluetoothAdapter
(2) is created by an object factory (like xxx.CreateRemoteDevice([BD Address]))
It's an entity to keep remote device information, and let BluetoothAdapter could use it to decide the connecting strategy.
Attachment #604823 -
Flags: review?(kyle)
Comment 2•13 years ago
|
||
Comment on attachment 604823 [details] [diff] [review]
Patch 1: The 1st version of DOM BluetoothDevice object, r=qDot
Review of attachment 604823 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bluetooth/nsIDOMBluetoothDevice.idl
@@ +8,5 @@
> + readonly attribute DOMString name;
> + readonly attribute boolean legacyPairing;
> +
> + // PAIRED, PAIRING, NONE
> + readonly attribute unsigned long state;
It would be nicer to have a string here.
Reporter | ||
Comment 3•13 years ago
|
||
Comment on attachment 604823 [details] [diff] [review]
Patch 1: The 1st version of DOM BluetoothDevice object, r=qDot
Review of attachment 604823 [details] [diff] [review]:
-----------------------------------------------------------------
r-'ing not because something is specifically wrong, but because I think we can go a little farther with this on this bug. I know I usually say to crib off my old BluetoothAdapter commit for setting up DOM objects, but I wasn't exactly aware of all the boilerplate needed for event handling when I did that, and there's more framework needed for event handling objects that we might as well do at this point too.
::: dom/base/nsDOMClassInfo.cpp
@@ +1621,5 @@
> #ifdef MOZ_B2G_BT
> NS_DEFINE_CLASSINFO_DATA(BluetoothAdapter, nsEventTargetSH,
> EVENTTARGET_SCRIPTABLE_FLAGS)
> + NS_DEFINE_CLASSINFO_DATA(BluetoothDevice, nsDOMGenericSH,
> + DOM_DEFAULT_SCRIPTABLE_FLAGS)
Devices will be event targets just like Adapters, since they'll handle property change and node create/delete events themselves. So we should have them mirror the eventtarget properties the adapter has.
::: dom/bluetooth/BluetoothDevice.cpp
@@ +1,1 @@
> +#include "BluetoothDevice.h"
Missing license header.
::: dom/bluetooth/BluetoothDevice.h
@@ +1,1 @@
> +#ifndef mozilla_dom_bluetooth_bluetoothdevice_h__
Missing license header
::: dom/bluetooth/nsIDOMBluetoothDevice.idl
@@ +1,1 @@
> +#include "nsISupports.idl"
Missing license header.
@@ +8,5 @@
> + readonly attribute DOMString name;
> + readonly attribute boolean legacyPairing;
> +
> + // PAIRED, PAIRING, NONE
> + readonly attribute unsigned long state;
Paired is already covered in the dbus Paired property. Don't know if we need an intermediate state while pairing?
@@ +9,5 @@
> + readonly attribute boolean legacyPairing;
> +
> + // PAIRED, PAIRING, NONE
> + readonly attribute unsigned long state;
> +};
The IDL seems to be missing a ton of possible attributes (http://maemo.org/api_refs/5.0/beta/bluez/device.html), and should probably start as an event handler since we know it'll need events.
Attachment #604823 -
Flags: review?(kyle) → review-
Assignee | ||
Comment 4•13 years ago
|
||
> The IDL seems to be missing a ton of possible attributes
> (http://maemo.org/api_refs/5.0/beta/bluez/device.html), and should probably
> start as an event handler since we know it'll need events.
It seems like we're going to implement an interface which is very similar to BlueZ D-Bus Device API, and I'm not sure if that's actually we want. I think the BlueZ API is not suitable for web page developers because it has not provided easy-use profile connection interface. The way I think How Bluetooth API should be looked like is more like Android Bluetooth API, which is using the singleton BluetoothAdapter represents local bt firmware and use it for connect/discover/pair, and BluetoothDevice for just an entity object to keep information/state of remote devices.
By the way, I should use 'feedback' instead of 'review' because I was just looking for your advice, sorry. :)
Reporter | ||
Comment 5•13 years ago
|
||
Ah, ok, if this was feedback, then yeah, you're going in the right direction. :)
The only reason I've been replicating DBus in JS is because I'm not sure what interaction we want happening in XPCOM and what we want to expose out to JS, so I figured we'd expose pretty much everything then peel back as needed. Obviously I'm not really tied to that approach, since I'm still not super familiar with bluetooth as a whole. I'm /hoping/ the dbus util functions I'm working on mean that the xpcom stuff is really just a pretty simple shim layer on top of dbus, so we won't have much code to add/remove to change stuff.
I've been contemplating posting more about this to the mailing list, I might do that tomorrow just to see what happens.
Assignee | ||
Comment 6•13 years ago
|
||
I took Kyle's advice, simply implemented a similar interface to BlueZ BluetoothDevice API. We can take advantage of it because we can regard this class as a wrapper, which wraps DBus communications, then we write 'cleaner' code in other classes without those DBus things.
One thing to say, I remove all properties/methods related to 'Node' in BlueZ API just because I'm not quite sure what it means.
Attachment #604823 -
Attachment is obsolete: true
Attachment #606132 -
Flags: feedback?(kyle)
Assignee | ||
Comment 7•13 years ago
|
||
sorry using a wrong patch, the feedback request will be canceled later.
Assignee | ||
Comment 8•13 years ago
|
||
I took Kyle's advice, simply implemented a similar interface to BlueZ BluetoothDevice API. We can take advantage of it because we can regard this class as a wrapper, which wraps DBus communications, then we write 'cleaner' code in other classes without those DBus things.
One thing to say, I remove all properties/methods related to 'Node' in BlueZ API just because I'm not quite sure what it means.
Attachment #606132 -
Attachment is obsolete: true
Attachment #606133 -
Flags: feedback?(kyle)
Attachment #606132 -
Flags: feedback?(kyle)
Reporter | ||
Comment 9•13 years ago
|
||
Comment on attachment 606133 [details] [diff] [review]
Patch 1: interface of BluetoothDevice
Review of attachment 606133 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bluetooth/nsIDOMBluetoothDevice.idl
@@ +22,5 @@
> + attribute nsIDOMEventListener onpropertychanged;
> + attribute nsIDOMEventListener ondisconnectrequested;
> +
> + void disconnect();
> + jsval getProperties();
Ok, maybe I spoke a little too soon about completely replicating DBus. :)
For get/setproperties, we may just rely on the underlying js/xpcom implementation to deal with that. i.e. when a Device object is created, we assume getproperties is called to fill in the variables, and when an object member is set, we call setProperties for the user. This is how I was doing things in the gtk version of the code.
However, for now, this is fine, we can take care of property set/get in another bug if you just want to get this landed. :)
Attachment #606133 -
Flags: feedback?(kyle) → feedback+
Assignee | ||
Comment 10•13 years ago
|
||
Get properties when Device object is created then notify property changed via event is a good idea. I can modify the patch.
Assignee | ||
Comment 11•13 years ago
|
||
In this patch, we only call GetProperties() once in constructor, so web page developers will be notified whenever any property has been changed via event 'propertychanged', and that also means I can remove definitions of GetProperties & SetProperties from idl.
Since it's a prototype of BluetoothDevice, we have at least two things need to be done. One is Implemention of all TODO items in the patch, most of them are about communication with BlueZ via DBus. Another one is to re-design exposed interface of BluetoothDevice and BluetoothAdapter (and other class we haven't defined yet).
Attachment #606133 -
Attachment is obsolete: true
Attachment #607895 -
Flags: feedback?(kyle)
Assignee | ||
Comment 12•13 years ago
|
||
By the way, I don't know if this issue should be changed to "resolved" after this patch is checked in, because "Device object for DOM Bluetooth" is a big topic. Nevertheless, I still think that it would be better if we can get this prototype patch landed first.
Reporter | ||
Updated•13 years ago
|
Summary: Device object for DOM Bluetooth → Device object boilerplate for DOM Bluetooth
Reporter | ||
Comment 13•13 years ago
|
||
(In reply to Eric Chou [:ericchou] [:echou] from comment #12)
> By the way, I don't know if this issue should be changed to "resolved" after
> this patch is checked in, because "Device object for DOM Bluetooth" is a big
> topic. Nevertheless, I still think that it would be better if we can get
> this prototype patch landed first.
When in doubt, we just change the bug title. :)
Reporter | ||
Comment 14•13 years ago
|
||
Comment on attachment 607895 [details] [diff] [review]
Patch 1: prototype of BluetoothDevice
Review of attachment 607895 [details] [diff] [review]:
-----------------------------------------------------------------
This all looks good, should be landable with the bug title change, since we're just expecting boilerplate here. It'll be changing a bit once the utility classes come in anyways, we can make a new bug for that.
Attachment #607895 -
Flags: feedback?(kyle) → feedback+
Assignee | ||
Updated•13 years ago
|
Attachment #607895 -
Flags: review?(kyle)
Reporter | ||
Updated•13 years ago
|
Attachment #607895 -
Flags: review?(kyle) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 15•13 years ago
|
||
Keywords: checkin-needed
Comment 16•13 years ago
|
||
Please can you set the target milestone when landing on inbound, along the lines of http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing :-)
https://hg.mozilla.org/mozilla-central/rev/4a9ebf972d27
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment 17•13 years ago
|
||
Kyle and Eric, this patch should never have been pushed to mozilla-central without a review from a DOM peer!
You should have a look at the wiki page about the review process:
https://developer.mozilla.org/en/Code_Review_FAQ
And please, make sure to always ask a review to an appropriate peer/module owner before landing anything in mozilla-central.
Reporter | ||
Comment 18•13 years ago
|
||
Yeah, we actually figured out this should be chrome anyways. It's being undone as of bug 744705, and we'll have the real mozBluetooth implementation reviewed via a DOM peer.
Comment 19•13 years ago
|
||
The feature being forbidden to regular content doesn't make the review process optional.
I also forgot to point that this patch should have been super-reviewed because it is adding a new API to Gecko.
Comment 20•13 years ago
|
||
Given that this was never an API we want to expose to the web (and yes, this landed w/o the appropriate reviews) and it's being removed in bug 744705 we need to back this out of aurora on when we branch on Tuesday so this doesn't actually ship to Firefox users.
tracking-firefox14:
--- → +
Comment 21•13 years ago
|
||
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #20)
> Given that this was never an API we want to expose to the web (and yes, this
> landed w/o the appropriate reviews) and it's being removed in bug 744705 we
> need to back this out of aurora on when we branch on Tuesday so this doesn't
> actually ship to Firefox users.
Eric/Johnny - can we perform that backout now?
Reporter | ||
Comment 22•13 years ago
|
||
Yup, go ahead and back it out. We'll deal with the redo in bug 744705.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 23•13 years ago
|
||
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #22)
> Yup, go ahead and back it out. We'll deal with the redo in bug 744705.
I do not want to sound disrespectful but maybe you (or Eric) could fix your mistake yourself instead of asking others to do it for you.
Assignee | ||
Comment 24•13 years ago
|
||
Backout due to this DOM object should not be checked-in without superreview
Assignee | ||
Comment 25•13 years ago
|
||
Updated comment.
Attachment #620644 -
Attachment is obsolete: true
Comment 26•13 years ago
|
||
Comment 27•13 years ago
|
||
Comment on attachment 620648 [details] [diff] [review]
Backout changeset:4a9ebf972d27
We need this to be backouted on aurora as well. Requesting approval.
[Approval Request Comment]
User impact if declined: We'll expose APIs we never meant to expose to webpages
Risk to taking this patch (and alternatives if risky): backout, very low risk.
String changes made by this patch: none
Attachment #620648 -
Flags: approval-mozilla-aurora?
Comment 28•13 years ago
|
||
Not saying we should not backout on aurora, but this is guarded by MOZ_B2G_BT which is not defined for fx desktop or fennec.
Comment 29•13 years ago
|
||
Comment on attachment 620648 [details] [diff] [review]
Backout changeset:4a9ebf972d27
Hmm, in that case I don't think I care... If someone disagrees and feels this needs to be backed out, then feel free to renominate! Thanks!
Attachment #620648 -
Flags: approval-mozilla-aurora?
Comment 30•13 years ago
|
||
Try run for 8e58cbc77775 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=8e58cbc77775
Results (out of 290 total builds):
success: 244
warnings: 33
failure: 7
other: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-8e58cbc77775
Timed out after 12 hours without completing.
Comment 31•13 years ago
|
||
Try run for 8e58cbc77775 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=8e58cbc77775
Results (out of 290 total builds):
success: 245
warnings: 33
failure: 7
other: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-8e58cbc77775
Timed out after 12 hours without completing.
Comment 32•13 years ago
|
||
Try run for 8e58cbc77775 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=8e58cbc77775
Results (out of 290 total builds):
success: 246
warnings: 33
failure: 7
other: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-8e58cbc77775
Timed out after 12 hours without completing.
Comment 33•13 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #26)
> Backouted
>
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9b9e948dca
https://hg.mozilla.org/mozilla-central/rev/3b9b9e948dca
Assignee | ||
Updated•13 years ago
|
Target Milestone: mozilla14 → ---
Assignee | ||
Comment 34•13 years ago
|
||
Move BluetoothDevice discussion to Bug 758556, close this issue since it's been backed out.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → INVALID
Comment 35•13 years ago
|
||
[Triage Comment]
Removing tracking since this issue is backed out and moved.
tracking-firefox14:
+ → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•