Closed Bug 777671 Opened 12 years ago Closed 12 years ago

[b2g-bluetooth] Need a way to get paired devices in nsIDOMBluetoothAdapter

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17
Project Flags:
blocking-basecamp +

People

(Reporter: echou, Assigned: gyeh)

References

Details

Attachments

(1 file, 8 obsolete files)

Final version: add function getPairedDevices() in nsIDOMBluetoothAdapter, r=qdot, sr=mrbkap
19.74 KB, patch
Details | Diff | Splinter Review 
We need to implement getPairedDevices() in BluetoothAdapter to get paired devices.
Assignee: nobody → echou
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).
(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?
(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.
blocking-basecamp: --- → ?
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
Assignee: echou → gyeh
Attachment #650028 - Flags: feedback?(kyle)
The last one is incorrect and I created a new one. Please check this one. Thanks.
Attachment #650028 - Attachment is obsolete: true
Attachment #650028 - Flags: feedback?(kyle)
Attachment #650033 - Flags: feedback?(kyle)
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 :) )
blocking-basecamp: ? → +
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.
Attachment #650033 - Flags: feedback?(kyle) → feedback-
I've fixed the function getPairedDevices() as the attaching patck. Please let me know if there's anything should be changed. Thanks.
Attachment #650511 - Flags: feedback?(kyle)
BTW, you can mark old patches "obsolete" when you put new ones in, since the v2 updates the v1.
Attachment #650033 - Attachment is obsolete: true
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 :| )
Attachment #650511 - Flags: feedback?(kyle)
@Gina: FYI, BluetoothPropertyContainer::GetProperties will be removed after bug 781692 lands.
I add a callback function for GetPropertiesTask and create BluetoothDevice in the callback function. Please see the attached patch for details.
Attachment #650511 - Attachment is obsolete: true
Attachment #651335 - Flags: feedback?(kyle)

        Attachment #651335 -
        Attachment is obsolete: true

        Attachment #651335 -
        Flags: feedback?(kyle)

        Attachment #651336 -
        Flags: feedback?(kyle)

    
    

    
  
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.

        Attachment #651336 -
        Flags: feedback?(kyle)

    
    

    
  
> Just move this to BluetoothAdapter and call it GetDevicePropertiesTask.

Er, sorry, not GetDevicePropertiesTask, GetPairedDevicesTask

    
    

    
  
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.

    
    

    
  
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. :)

    
    

    
  


  
code cleanup and move GetPairedDevicesTask into class BluetoothAdapter

        Attachment #651336 -
        Attachment is obsolete: true

        Attachment #651623 -
        Flags: review?(kyle)

    
    

    
  
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)

        Attachment #651623 -
        Flags: review?(kyle) → review-

    
    

    
  


  
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.

        Attachment #651623 -
        Attachment is obsolete: true

        Attachment #651668 -
        Flags: review?(kyle)

    
    

    
  
> ::: 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 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. :)

        Attachment #651668 -
        Flags: review?(kyle) → review+

    
    

    
  
Comment on attachment 651972 [details] [diff] [review]
Final version: add function getPairedDevices() in nsIDOMBluetoothAdapter, r=qDot

Flagging mrbkap for review due to DOM change.

        Attachment #651972 -
        Flags: review?(mrbkap)

    
    

    
  
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 (.

        Attachment #651972 -
        Flags: review?(mrbkap) → review+

    
    

    
  
> ::: 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

    
    

    
  


  
This patch doesn't fix the js array initialization in function BluetoothDeviceArrayToJSArray, and please see the followup in Bug783431.

        Attachment #651972 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c47a3d1bca14
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

    
    

    
  
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!

    
    

    
  
Thank you for your reminding me of that, Robert. I'll do that from next commit. :)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: