Closed Bug 730990 Opened 12 years ago Closed 12 years ago

Device object boilerplate for DOM Bluetooth

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: qdot, Assigned: echou)

References

Details

Attachments

(2 files, 4 obsolete files)

We need objects created per adapter (Bug 730970) that represent objects available to the adapter to bond and use.
Depends on: 730970
Assignee: nobody → echou
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 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.
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-
> 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. :)
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.
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)
sorry using a wrong patch, the feedback request will be canceled later.
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)
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+
Get properties when Device object is created then notify property changed via event is a good idea. I can modify the patch.
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)
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.
Summary: Device object for DOM Bluetooth → Device object boilerplate for DOM Bluetooth
(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. :)
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+
Attachment #607895 - Flags: review?(kyle)
Attachment #607895 - Flags: review?(kyle) → review+
Keywords: checkin-needed
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: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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.
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.
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.
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.
(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?
Yup, go ahead and back it out. We'll deal with the redo in bug 744705.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Attached patch Backout changeset:4a9ebf972d27 (obsolete) — Splinter Review
Backout due to this DOM object should not be checked-in without superreview
Updated comment.
Attachment #620644 - Attachment is obsolete: true
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?
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 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?
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.
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.
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.
Target Milestone: mozilla14 → ---
Move BluetoothDevice discussion to Bug 758556, close this issue since it's been backed out.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → INVALID
[Triage Comment]
Removing tracking since this issue is backed out and moved.
No longer blocks: 756299
You need to log in before you can comment on or make changes to this bug.