Last Comment Bug 777671 - [b2g-bluetooth] Need a way to get paired devices in nsIDOMBluetoothAdapter
: [b2g-bluetooth] Need a way to get paired devices in nsIDOMBluetoothAdapter
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Gina Yeh [:gyeh] [:ginayeh]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: b2g-bluetooth 782420
  Show dependency treegraph
 
Reported: 2012-07-26 03:41 PDT by Eric Chou [:ericchou] [:echou]
Modified: 2012-08-19 22:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
v1: add function getPairedDevices() in nsIDOMBluetoothAdapter (860 bytes, patch)
2012-08-08 04:42 PDT, Gina Yeh [:gyeh] [:ginayeh]
no flags Details | Diff | Splinter Review
v1: add function getPairedDevices() in nsIDOMBluetoothAdapter (17.71 KB, patch)
2012-08-08 05:03 PDT, Gina Yeh [:gyeh] [:ginayeh]
kyle: feedback-
Details | Diff | Splinter Review
v2: add function getPairedDevices() in nsIDOMBluetoothAdapter (9.89 KB, patch)
2012-08-09 05:12 PDT, Gina Yeh [:gyeh] [:ginayeh]
no flags Details | Diff | Splinter Review
v3: add function getPairedDevices() in nsIDOMBluetoothAdapter (11.05 KB, patch)
2012-08-13 05:32 PDT, Gina Yeh [:gyeh] [:ginayeh]
no flags Details | Diff | Splinter Review
v3: add function getPairedDevices() in nsIDOMBluetoothAdapter (11.05 KB, patch)
2012-08-13 05:35 PDT, Gina Yeh [:gyeh] [:ginayeh]
no flags Details | Diff | Splinter Review
v4: add function getPairedDevices() in nsIDOMBluetoothAdapter (16.35 KB, patch)
2012-08-13 21:03 PDT, Gina Yeh [:gyeh] [:ginayeh]
kyle: review-
Details | Diff | Splinter Review
v5: add function getPairedDevices() in nsIDOMBluetoothAdapter (17.11 KB, patch)
2012-08-14 02:09 PDT, Gina Yeh [:gyeh] [:ginayeh]
kyle: review+
Details | Diff | Splinter Review
Final version: add function getPairedDevices() in nsIDOMBluetoothAdapter, r=qDot (16.43 KB, patch)
2012-08-14 19:24 PDT, Gina Yeh [:gyeh] [:ginayeh]
mrbkap: review+
Details | Diff | Splinter Review
Final version: add function getPairedDevices() in nsIDOMBluetoothAdapter, r=qdot, sr=mrbkap (19.74 KB, patch)
2012-08-16 19:37 PDT, Gina Yeh [:gyeh] [:ginayeh]
no flags Details | Diff | Splinter Review

Description Eric Chou [:ericchou] [:echou] 2012-07-26 03:41:49 PDT
We need to implement getPairedDevices() in BluetoothAdapter to get paired devices.
Comment 1 Kyle Machulis [:qdot] 2012-07-26 12:15:32 PDT
A bit more background:

Right now, the only way to get paired devices is rather roundable. We expose device addresses from the adapter via the devices attribute, but there's no way to turn these addresses into BluetoothDevice objects. We need a function that returns a DOMRequest, and when it succeeds, a JSObject* array of BluetoothDevices, all of which should have had GetProperties called on them to fill out their variables. We should be able to just create an nsTArray<BluetoothDevice*> of them, then use the nsTArrayToJSArray function from Telephony (though this has implicit problems of its own, it'll work for now).

One of the problems will be that we create new BluetoothDevice objects every time we call this, instead of having a central store of them per-tab/app/process/whatever. This shouldn't be a huge deal since they'll all receive update signals and have their data backed via dbus, but in the future it might be nice to hand out the same objects every time (bent and I have talked about this, but it's probably a long-term/V2 thing).
Comment 2 Eric Chou [:ericchou] [:echou] 2012-07-27 03:59:31 PDT
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #1)
> A bit more background:
> 
> Right now, the only way to get paired devices is rather roundable. We expose
> device addresses from the adapter via the devices attribute, but there's no
> way to turn these addresses into BluetoothDevice objects. We need a function
> that returns a DOMRequest, and when it succeeds, a JSObject* array of
> BluetoothDevices, all of which should have had GetProperties called on them
> to fill out their variables. We should be able to just create an
> nsTArray<BluetoothDevice*> of them, then use the nsTArrayToJSArray function
> from Telephony (though this has implicit problems of its own, it'll work for
> now).
> 

Just curious, can we just use a "devices" attribute to get an array of BluetoothDevice? I've checked the whole process of creating a BluetoothDevice, and it seems not necessary to make it asynchronous via passing a DOMRequest?
Comment 3 Kyle Machulis [:qdot] 2012-07-30 22:33:52 PDT
(In reply to Eric Chou [:ericchou] [:echou] from comment #2)
> Just curious, can we just use a "devices" attribute to get an array of
> BluetoothDevice? I've checked the whole process of creating a
> BluetoothDevice, and it seems not necessary to make it asynchronous via
> passing a DOMRequest?

Nope, because:

- We can't guarentee runtime. Let's say we exposed members as an array of BluetoothDevices. Whenever we get that the devices member changed in the adapter, we'll have to run /something/ async to call GetProperties on the device. However, while that's happening, someone might query the devices member, which is now in a half finished state. If we change it to a function that hands back a DOMRequest, they'll have to wait and will be notified.
- Changing it to a DOMRequest also means we can be lazy. Just because devices has changed doesn't mean we care enough to run yet. We can wait until the developer calls it and do all of the calls then.

There's a small continuity issue with the API. We're going to get a propertychanged event with "devices" as the property, but instead of just finding out what "devices" is, the developer will have to call GetDevices(), which isn't the same as just accessing the member like it is for, say, "paired". However, I'm fine taking that continuity hit for the moment since it keeps a much larger portion of the API consistent.
Comment 4 Gina Yeh [:gyeh] [:ginayeh] 2012-08-08 04:42:19 PDT
Created attachment 650028 [details] [diff] [review]
v1: add function getPairedDevices() in nsIDOMBluetoothAdapter

This is the first version of getPairedDevices(), which is mainly based on dbus-blocking branch and some functions are still under review. I only put code changes of mine in the patch, so it may not work with the latest code base. However, you can still figure out the implementation details of getPairedDevices() with no effect. Please let me know if you have any feedback. Thanks.

Gina
Comment 5 Gina Yeh [:gyeh] [:ginayeh] 2012-08-08 05:03:23 PDT
Created attachment 650033 [details] [diff] [review]
v1: add function getPairedDevices() in nsIDOMBluetoothAdapter

The last one is incorrect and I created a new one. Please check this one. Thanks.
Comment 6 Kyle Machulis [:qdot] 2012-08-08 09:10:16 PDT
Remember to bump your patch version in your description whenever you upload a new version (this is why my final patches are usually something like v15 :) )
Comment 7 Kyle Machulis [:qdot] 2012-08-08 11:28:27 PDT
Comment on attachment 650033 [details] [diff] [review]
v1: add function getPairedDevices() in nsIDOMBluetoothAdapter

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

Ok. The good news is, most of your code is at least correct. The bad news is, it's in the wrong place/order, for reasons that are pretty opaque, so don't feel bad.

So, the idea is that we want to end up with an array of BluetoothDevices. That's what happens here, which is good. The order you're doing it in currently is:

- Create a bunch of BluetoothDevices that just have their path
- Make a DOMRequest that passes along with the array of BluetoothDevices to the BluetoothDBusService
- On each of those, call GetProperties
- Return runnable

The problems happen in the first 2 steps. One is just a nit, the other is a blocker. 

First off, I was trying to avoid the variant constructor on BluetoothDevices. Having the BluetoothValue be either a string /or/ one of our weird dict types is pretty bad design, especially since it leaves the type checking to us since it's a variant struct. I was just trying to get /something/ working at the time I did that, and really don't think it's the right idea. It'd be nice if BluetoothDevice only ever took dicts.

As for passing BluetoothDevice down to BluetoothDBusService, this is where things get weird and bad and completely non-obvious. Notice that no other BluetoothDBusService function takes a DOM object of any kind. They only talk in DOMRequests and BluetoothValue structures. That's because all of this has to be process seperated. Once we get to the e10s bug, we'll be switching out the DOMRequests with some IPDL structure I'm not sure about yet that is basically a cross process callback, and the BluetoothValue which is already an IPDL type. We don't have an IPDL equivilent to BluetoothDevice, nor do we want to try to come up with one. We'd like to keep BluetoothDevice in the content process, meaning we'll build it after we get back a BluetoothValue dict. This is kinda what I was saying in my email about this, but I have a really hard time conveying these things clearly since I'm pretty new to this myself. :)

So, here's what I'm thinking things should look like:

- We get a device address array from BluetoothAdapter via GetProperties (already done as of 768306, which just landed).
- We pass that array of addresses down to BluetoothDBusService, along with a runnable.
- The GetPairedDeviceProperties Runnable for BluetoothDBusService is fired off to our DBus thread (also landed in bug 768306) and blocking runs GetProperties for each of these addresses, forming a big BluetoothValue dict based on BluetoothNamedValue types.
- We attach the BluetoothValue dict to the Runnable sent to us by BluetoothAdapter, runnable sent back to main thread
- On the main thread in the runnable, we unpack the BluetoothValue dict, create BluetoothDevice objects from the values in it, fill the mJsDevices array, return that.

So, yeah, it's really roundabout, but it means we have the seperation in place once e10s rolls around (Which we're already horribly behind on). With this setup, we have firm seperation points in place between BluetoothAdapter and BluetoothDBusService via the IPDL structs.

What you've got here is pretty close to this, just needs to be refactored a bit and I think we should be good. Feel free to ask for clarification anywhere on this, it can get pretty confusing, especially since this is one of your first tasks inside WebAPI/DOM. :)

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +63,5 @@
>  NS_IMPL_RELEASE_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper)
>  
> +template <class T>
> +inline nsresult
> +nsTArrayToJSArray(JSContext* aCx, JSObject* aGlobal,

For clarity sake, throw this over in BluetoothUtil, with the other *ToJSArray function. Also, call it something like XPCOMArrayToJSArray, as we're apparently not supposed to prefix functions with ns.

@@ +83,5 @@
> +  if (aSourceArray.IsEmpty()) {
> +    arrayObj = JS_NewArrayObject(aCx, 0, nsnull);
> +  } else {
> +    nsTArray<jsval> valArray;
> +    valArray.SetLength(aSourceArray.Length());

This is using the old non-rooted array version from Telephony. Check out the new version I landed for telephony a few days ago that roots the array before filling it, otherwise we're liable to lose objects in gc runs while we're building the array.

@@ +500,5 @@
> +  if (!bs) {
> +    NS_WARNING("BluetoothService not available!");
> +    return NS_ERROR_FAILURE;
> +  }
> +

We should check to make sure there's been a change to the devices array first before we go about reconstructing everything, otherwise we can pass back our cached version by firing the DOMRequest here.

::: dom/bluetooth/BluetoothService.h
@@ +113,5 @@
> +   * specific method.
> +   *
> +   * @return properties on success, NULL otherwise
> +   */
> +  virtual nsresult GetDevicePropertiesInternal(BluetoothReplyRunnable* aRunnable,

Let's just call this GetPairedDevicesPropertiesInternal, since it'll take a certain array of devices instead of just arbitrary ones.

@@ +114,5 @@
> +   *
> +   * @return properties on success, NULL otherwise
> +   */
> +  virtual nsresult GetDevicePropertiesInternal(BluetoothReplyRunnable* aRunnable,
> +                                               nsTArray<nsRefPtr<BluetoothDevice> >& aDeviceArray) = 0;

We can't pass XPCOM objects here if we want simple e10s seperation. See feedback description. For now, this could probably be a nsTArray<nsString>, though we could also add a nsString[] to BluetoothValue if you're feeling adventerous.
Comment 8 Gina Yeh [:gyeh] [:ginayeh] 2012-08-09 05:12:10 PDT
Created attachment 650511 [details] [diff] [review]
v2: add function getPairedDevices() in nsIDOMBluetoothAdapter

I've fixed the function getPairedDevices() as the attaching patck. Please let me know if there's anything should be changed. Thanks.
Comment 9 Kyle Machulis [:qdot] 2012-08-09 14:26:44 PDT
BTW, you can mark old patches "obsolete" when you put new ones in, since the v2 updates the v1.
Comment 10 Kyle Machulis [:qdot] 2012-08-09 14:46:37 PDT
Comment on attachment 650511 [details] [diff] [review]
v2: add function getPairedDevices() in nsIDOMBluetoothAdapter

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

Ok, mostly right idea here, commented on what I could, but it looks like the patch got seriously mangled? There's a lot of weird tab differences, the diff in BluetoothUtils shows the new TArray function as cutting the StringArray function in half, etc... Did something get screwed up when you were exporting the patch maybe?

::: dom/bluetooth/BluetoothPropertyContainer.cpp
@@ +89,1 @@
>    }

Right idea, wrong place. This should happen in the Runnable callback that's passed in BluetoothAdapter::GetPairedDevices. We don't want the Property base class to have to know about its children. Honestly, I expect BluetoothPropertyContainer::GetProperties to disappear, as I don't think we'll ever need to call GetProperties outside of object creation, where it'll happen as part of the blocking call in BluetoothDBusService? (I wrote it to try and unify some of the repeated properties code, without actually updating it once I moved some of the properties calls around :| )
Comment 11 Eric Chou [:ericchou] [:echou] 2012-08-09 20:09:14 PDT
@Gina: FYI, BluetoothPropertyContainer::GetProperties will be removed after bug 781692 lands.
Comment 12 Gina Yeh [:gyeh] [:ginayeh] 2012-08-13 05:32:39 PDT
Created attachment 651335 [details] [diff] [review]
v3: add function getPairedDevices() in nsIDOMBluetoothAdapter

I add a callback function for GetPropertiesTask and create BluetoothDevice in the callback function. Please see the attached patch for details.
Comment 13 Gina Yeh [:gyeh] [:ginayeh] 2012-08-13 05:35:53 PDT
Created attachment 651336 [details] [diff] [review]
v3: add function getPairedDevices() in nsIDOMBluetoothAdapter
Comment 14 Kyle Machulis [:qdot] 2012-08-13 11:59:40 PDT
Comment on attachment 651336 [details] [diff] [review]
v3: add function getPairedDevices() in nsIDOMBluetoothAdapter

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

First off, you can really just submit these are reviews. That way when they pass we don't have to go through the formality of marking it r?, and if they fail it's no big deal. :)

Getting close to done! :)

::: dom/bluetooth/BluetoothPropertyContainer.cpp
@@ +55,5 @@
>      NS_WARNING("Not a BluetoothNamedValue array!");
>      return false;
>    }
> +
> +	const InfallibleTArray<BluetoothNamedValue>& reply = 

Nit: tabs are off here.

::: dom/bluetooth/BluetoothPropertyContainer.h
@@ +47,1 @@
>    class GetPropertiesTask : public BluetoothReplyRunnable

Just move this to BluetoothAdapter and call it GetDevicePropertiesTask.

@@ +47,4 @@
>    class GetPropertiesTask : public BluetoothReplyRunnable
>    {
>    public:
> +    typedef nsresult (*GetPropertiesCallback)(const InfallibleTArray<BluetoothNamedValue>& aReply, 

We shouldn't need a callback here once we move this over to BluetoothAdapter, since we'll only be doing this for device arrays (unless there's some use case this needs to be generalized for that I'm not seeing?). Should be able to just move this code into the Run() of the task.

@@ +48,5 @@
>    {
>    public:
> +    typedef nsresult (*GetPropertiesCallback)(const InfallibleTArray<BluetoothNamedValue>& aReply, 
> +                                              jsval* aValue, 
> +                                              nsPIDOMWindow* aOwner, 

Once this is moved to BluetoothAdapter, pass a this pointer into it, from that you can extract the Owner and ScriptContext (see GetAdapterTask in BluetoothManager for an example)

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +844,5 @@
> +public:
> +  BluetoothPairedDevicePropertiesRunnable(BluetoothReplyRunnable* aRunnable, 
> +																		nsTArray<nsString> aDeviceAddresses)
> +    : mRunnable(dont_AddRef(aRunnable)),
> +			mDeviceAddresses(aDeviceAddresses)

Nit: Tabs are off here.

@@ +857,5 @@
> +    dbus_error_init(&err);
> +   
> +    nsString replyError;
> +    DBusMessage* msg;
> +		BluetoothValue values = InfallibleTArray<BluetoothNamedValue>();

Nit: Tabs are off here.

@@ +858,5 @@
> +   
> +    nsString replyError;
> +    DBusMessage* msg;
> +		BluetoothValue values = InfallibleTArray<BluetoothNamedValue>();
> +		const char* interface = sBluetoothDBusIfaces[2];

Don't need this, just use DBUS_DEVICE_IFACE

@@ +881,5 @@
> +			v.get_ArrayOfBluetoothNamedValue().AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Path"), mDeviceAddresses[i]));
> +
> +			// Only paired devices will be return back to main thread
> +			for (uint32_t p = 0; p < v.get_ArrayOfBluetoothNamedValue().Length(); ++p) {
> +				if (v.get_ArrayOfBluetoothNamedValue()[p].name().EqualsLiteral("Paired")) {

nit: Make a temp ref out of v.get_ArrayOfBluetoothNamedValue() and v.get_ArrayOfBluetoothNamedValue()[p]. Just for brevity.

@@ +884,5 @@
> +			for (uint32_t p = 0; p < v.get_ArrayOfBluetoothNamedValue().Length(); ++p) {
> +				if (v.get_ArrayOfBluetoothNamedValue()[p].name().EqualsLiteral("Paired")) {
> +					bool paired = v.get_ArrayOfBluetoothNamedValue()[p].value();
> +					if (paired) {
> +						BluetoothValue properties = v.get_ArrayOfBluetoothNamedValue();

Nit: Make this a reference, no reason to copy.
Comment 15 Kyle Machulis [:qdot] 2012-08-13 12:42:30 PDT
> Just move this to BluetoothAdapter and call it GetDevicePropertiesTask.

Er, sorry, not GetDevicePropertiesTask, GetPairedDevicesTask
Comment 16 Kyle Machulis [:qdot] 2012-08-13 12:55:57 PDT
Comment on attachment 651336 [details] [diff] [review]
v3: add function getPairedDevices() in nsIDOMBluetoothAdapter

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +938,4 @@
>  }
>  
>  nsresult
> +BluetoothDBusService::GetPairedDevicePropertiesInternal(BluetoothReplyRunnable* aRunnable, 

You're missing member declarations for this in BluetoothDBusService and BluetoothService.
Comment 17 Kyle Machulis [:qdot] 2012-08-13 13:45:03 PDT
Ooooooooooook I think I get what's going on with the diffs now, which is why the patches seem so weird. It looks like you're only submitting diffs against your last patch. Make sure you're diffing against the head of your root branch, not against whatever your last patch was. Each patch you submit here should be /everything/ that is needed to build this off the head of m-c, or the head of whatever branch you're on, either one. Most reviewers won't actually try and compile your patches, but since this is directly in line with stuff I need right now, I've been trying to integrate this under socket stuff, hence the questions about why things don't even compile in the attachments. Makes more sense now. :)
Comment 18 Gina Yeh [:gyeh] [:ginayeh] 2012-08-13 21:03:51 PDT
Created attachment 651623 [details] [diff] [review]
v4: add function getPairedDevices() in nsIDOMBluetoothAdapter

code cleanup and move GetPairedDevicesTask into class BluetoothAdapter
Comment 19 Kyle Machulis [:qdot] 2012-08-13 21:39:49 PDT
Comment on attachment 651623 [details] [diff] [review]
v4: add function getPairedDevices() in nsIDOMBluetoothAdapter

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

Ok. Patch definitely looks like the full thing now, and tabs look way better. We're gettin' there. :)

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +420,5 @@
> +  return NS_OK;
> +}
> +
> +bool
> +GetPairedDevicesTask::ParseSuccessfulReply(jsval* aValue)

Just move this into the class definition when you move it to file scope. See GetAdapterTask in BluetoothManager.

::: dom/bluetooth/BluetoothAdapter.h
@@ +22,5 @@
>  class BluetoothNamedValue;
>  class BluetoothValue;
> +class BluetoothAdapter;
> +
> +class GetPairedDevicesTask : public BluetoothReplyRunnable

This doesn't need to be in the header. No one else will ever be using it. Just declare it in file scope.

@@ +30,5 @@
> +                       nsIDOMDOMRequest* aReq) :
> +    mAdapterPtr(aAdapterPtr),
> +    BluetoothReplyRunnable(aReq)
> +  {
> +    MOZ_ASSERT(aReq);

Might want to assert the AdapterPtr too.

@@ +38,5 @@
> +
> +  void
> +  ReleaseMembers()
> +  {
> +    BluetoothReplyRunnable::ReleaseMembers();

You forgot to release mAdapterPtr.

::: dom/bluetooth/BluetoothPropertyContainer.cpp
@@ +13,3 @@
>  USING_BLUETOOTH_NAMESPACE
>  
>  nsresult

I landed this in bug 781692, but that's not to m-c yet. Just a heads up if you gets conflicts on it.

::: dom/bluetooth/BluetoothService.h
@@ +107,5 @@
>     */
>    virtual nsresult GetDefaultAdapterPathInternal(BluetoothReplyRunnable* aRunnable) = 0;
>  
> +  /**
> +   * Returns the properties of pared devices, implemented via a platform

Nit: paired. But yay adding comments! :)

@@ +110,5 @@
> +  /**
> +   * Returns the properties of pared devices, implemented via a platform
> +   * specific method.
> +   *
> +   * @return Default adapter path/name on success, NULL otherwise

This returns an nsresult, so it's either NS_OK or NS_ERROR_FAILURE

@@ +113,5 @@
> +   *
> +   * @return Default adapter path/name on success, NULL otherwise
> +   */
> +  virtual nsresult GetPairedDevicePropertiesInternal(BluetoothReplyRunnable* aRunnable,
> +                                                     nsTArray<nsString> aDeviceAddresses) = 0;

Nit: Flip the order of the arguments, for consistency with other functions.

More than a nit: const nsTArray<nsString>&

::: dom/bluetooth/BluetoothUtils.cpp
@@ +80,5 @@
> +
> +  if (aSourceArray.IsEmpty()) {
> +    arrayObj = JS_NewArrayObject(aCx, 0, nsnull);
> +  } else {
> +    nsTArray<jsval> valArray;

As I said in earlier reviews, this isn't the right version of this method, it's not going to root jsobjects correctly. Check out what's in Telephony now.

::: dom/bluetooth/BluetoothUtils.h
@@ +21,5 @@
>                       const nsTArray<nsString>& aSourceArray,
>                       JSObject** aResultArray);
>  
> +nsresult
> +TArrayToJSArray(JSContext* aCx, JSObject* aGlobal,

Just realized this isn't actually a TArraytoJSArray, it's a BluetoothDeviceArrayToJSArray. So just change the function name for now. If we need to serialize more object arrays, we'll figure out something about templatizing.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +880,5 @@
> +      v.get_ArrayOfBluetoothNamedValue().AppendElement(
> +        BluetoothNamedValue(NS_LITERAL_STRING("Path"), mDeviceAddresses[i])
> +      );
> +
> +      InfallibleTArray<BluetoothNamedValue>& deviceProperties = v.get_ArrayOfBluetoothNamedValue();      

I still don't think we need this block? Like I said in the last review, everything in this array should be paired, otherwise it wouldn't be in the array? Check with echou, I may be wrong here.

::: dom/bluetooth/linux/BluetoothDBusService.h
@@ +27,5 @@
>    virtual nsresult StartInternal();
>    virtual nsresult StopInternal();
>    virtual nsresult GetDefaultAdapterPathInternal(BluetoothReplyRunnable* aRunnable);
> +  virtual nsresult GetPairedDevicePropertiesInternal(BluetoothReplyRunnable* aRunnable,
> +                                                     nsTArray<nsString> aDeviceAddresses);

flip argument order, const nsTArray<>&

::: dom/bluetooth/nsIDOMBluetoothAdapter.idl
@@ +33,4 @@
>    nsIDOMDOMRequest setDiscoverableTimeout(in unsigned long timeout);
>    nsIDOMDOMRequest startDiscovery();
>    nsIDOMDOMRequest stopDiscovery();
> +  nsIDOMDOMRequest getPairedDevices();

I think you'll need to rev the uuid on this IDL since you're changing the vtable. (Just give it a new UUID)
Comment 20 Gina Yeh [:gyeh] [:ginayeh] 2012-08-14 02:09:40 PDT
Created attachment 651668 [details] [diff] [review]
v5: add function getPairedDevices() in nsIDOMBluetoothAdapter

This patch is modified as following:
- move GetPairedDevicesTask to file scope
- exchange the order of the arguments of GetDefaultAdapterPathInternal()
- update function from Telephony and rename as BluetoothDeviceArrayToJSArray,
- generate a new UUID for nsIDOMBluetoothAdapter.idl

As for checking the paired value in BluetoothDBusService.cpp, I've discussed with Eric and he will give a comment here later.
Comment 21 Eric Chou [:ericchou] [:echou] 2012-08-14 02:24:27 PDT
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +880,5 @@
> > +      v.get_ArrayOfBluetoothNamedValue().AppendElement(
> > +        BluetoothNamedValue(NS_LITERAL_STRING("Path"), mDeviceAddresses[i])
> > +      );
> > +
> > +      InfallibleTArray<BluetoothNamedValue>& deviceProperties = v.get_ArrayOfBluetoothNamedValue();      
> 
> I still don't think we need this block? Like I said in the last review,
> everything in this array should be paired, otherwise it wouldn't be in the
> array? Check with echou, I may be wrong here.
> 

To my knowledge, there are three cases could happen when we tend to create a device via CreateDevice & CreatePairedDevice:

(1) CreateDevice - The device would be created, and the 'paired' property would be 'false'.
(2) CreatePairedDevice - Pairing failed (e.g. timed out, wrong PIN, user cancelled...), the device would be created, but the 'paired' property would be 'false'.
(3) CreatePairedDevice - Pairing succeeded, the device would be created, and the 'paired' property would be 'true'.

So I think checking 'paired' property is necessary.
Comment 22 Kyle Machulis [:qdot] 2012-08-14 11:20:30 PDT
Comment on attachment 651668 [details] [diff] [review]
v5: add function getPairedDevices() in nsIDOMBluetoothAdapter

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

r=me with nits picked. :D

Since this requires a DOM change, we'll need to get this sr'd too. I'll talk to mrbkap.

::: dom/bluetooth/BluetoothAdapter.h
@@ +21,4 @@
>  class BluetoothSignal;
>  class BluetoothNamedValue;
>  class BluetoothValue;
> +class BluetoothAdapter;

Nit: remove

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +842,5 @@
> +class BluetoothPairedDevicePropertiesRunnable : public nsRunnable
> +{
> +public:
> +  BluetoothPairedDevicePropertiesRunnable(BluetoothReplyRunnable* aRunnable,
> +                                          nsTArray<nsString> aDeviceAddresses)

nit: const nsTArray<>&

@@ +946,5 @@
> +  if (!mConnection || !gThreadConnection) {
> +    NS_ERROR("Bluetooth service not started yet!");
> +    return NS_ERROR_FAILURE;
> +  }
> +  NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!");

nit: MOZ_ASSERT this, we want death, not warning. :)
Comment 23 Gina Yeh [:gyeh] [:ginayeh] 2012-08-14 19:24:07 PDT
Created attachment 651972 [details] [diff] [review]
Final version: add function getPairedDevices() in nsIDOMBluetoothAdapter, r=qDot
Comment 24 Kyle Machulis [:qdot] 2012-08-14 19:32:07 PDT
Comment on attachment 651972 [details] [diff] [review]
Final version: add function getPairedDevices() in nsIDOMBluetoothAdapter, r=qDot

Flagging mrbkap for review due to DOM change.
Comment 25 Blake Kaplan (:mrbkap) 2012-08-16 14:52:06 PDT
Comment on attachment 651972 [details] [diff] [review]
Final version: add function getPairedDevices() in nsIDOMBluetoothAdapter, r=qDot

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

One major comment, but r+ assuming you fix it before checkin.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +106,5 @@
> +
> +    rv = BluetoothDeviceArrayToJSArray(sc->GetNativeContext(),
> +                                       sc->GetNativeGlobal(), devices, &JsDevices);
> +
> +    if (JsDevices) {;

Nit: extra ; here.

::: dom/bluetooth/BluetoothUtils.cpp
@@ +82,5 @@
> +    arrayObj = JS_NewArrayObject(aCx, 0, nullptr);
> +  } else {
> +    uint32_t valLength = aSourceArray.Length();
> +    mozilla::ScopedDeleteArray<jsval> valArray(new jsval[valLength]);
> +    JS::AutoArrayRooter tvr(aCx, valLength, valArray);

I think this was copied from elsewhere, but there's a small problem. valArray's elements are uninitialized, so if there's a GC during the loop, we'll mark a bunch of uninitialized values. So, either we need to memset valArray to 0 (JSVAL_NULL) or pass 0 for the length and use changeLength(index + 1) each time through the loop (see http://hg.mozilla.org/mozilla-central/file/57e59b2e017e/dom/base/nsDOMClassInfo.cpp#l5773 for an example).

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +869,5 @@
> +                                   DBUS_DEVICE_IFACE,
> +                                   "GetProperties",
> +                                   DBUS_TYPE_INVALID);
> +      UnpackDevicePropertiesMessage(msg, &err, v, replyError);
> +      if(!replyError.IsEmpty()) {

Nit: here and below: need a space after if and before the (.
Comment 26 Kyle Machulis [:qdot] 2012-08-16 17:33:06 PDT
> ::: dom/bluetooth/BluetoothUtils.cpp
> @@ +82,5 @@
> > +    arrayObj = JS_NewArrayObject(aCx, 0, nullptr);
> > +  } else {
> > +    uint32_t valLength = aSourceArray.Length();
> > +    mozilla::ScopedDeleteArray<jsval> valArray(new jsval[valLength]);
> > +    JS::AutoArrayRooter tvr(aCx, valLength, valArray);
> 
> I think this was copied from elsewhere, but there's a small problem.
> valArray's elements are uninitialized, so if there's a GC during the loop,
> we'll mark a bunch of uninitialized values. So, either we need to memset
> valArray to 0 (JSVAL_NULL) or pass 0 for the length and use
> changeLength(index + 1) each time through the loop (see
> http://hg.mozilla.org/mozilla-central/file/57e59b2e017e/dom/base/
> nsDOMClassInfo.cpp#l5773 for an example).

Followup for other places this happens at bug 783431
Comment 27 Gina Yeh [:gyeh] [:ginayeh] 2012-08-16 19:37:23 PDT
Created attachment 652655 [details] [diff] [review]
Final version: add function getPairedDevices() in nsIDOMBluetoothAdapter, r=qdot, sr=mrbkap

This patch doesn't fix the js array initialization in function BluetoothDeviceArrayToJSArray, and please see the followup in Bug783431.
Comment 28 Kyle Machulis [:qdot] 2012-08-16 19:50:14 PDT
Yeah, don't worry about it, I'll take care of it Bug 783431 ASAP. We can land what we've got here and I'll wrap the fixes into that.
Comment 29 Eric Chou [:ericchou] [:echou] 2012-08-16 19:56:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c47a3d1bca14
Comment 30 Ed Morley [:emorley] 2012-08-17 05:27:26 PDT
https://hg.mozilla.org/mozilla-central/rev/c47a3d1bca14
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-19 21:40:45 PDT
Gina, can you avoid adding diffstat output to your patch summaries? We generally don't do that, and it makes the hg pushlog output more difficult to read. Thanks!
Comment 32 Gina Yeh [:gyeh] [:ginayeh] 2012-08-19 22:59:10 PDT
Thank you for your reminding me of that, Robert. I'll do that from next commit. :)

Note You need to log in before you can comment on or make changes to this bug.