Last Comment Bug 778640 - [b2g-bluetooth] Firing devicedisappeared event
: [b2g-bluetooth] Firing devicedisappeared event
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]
:
Mentors:
Depends on:
Blocks: b2g-bluetooth
  Show dependency treegraph
 
Reported: 2012-07-30 00:59 PDT by Eric Chou [:ericchou] [:echou]
Modified: 2012-08-20 08:54 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: Firing devicedisappeared event (9.32 KB, patch)
2012-08-15 00:16 PDT, Gina Yeh [:gyeh] [:ginayeh]
kyle: review+
Details | Diff | Review
v2: Firing devicedisappeared event (4.57 KB, patch)
2012-08-16 02:32 PDT, Gina Yeh [:gyeh] [:ginayeh]
kyle: review+
bugs: review+
Details | Diff | Review
Final version: Firing devicedisappeared event, create nsIDOMBluetoothDeviceAddressEvent by event generator, r=qdot, r=smaug (4.50 KB, patch)
2012-08-16 20:54 PDT, Gina Yeh [:gyeh] [:ginayeh]
no flags Details | Diff | Review
v3: Firing devicedisappeared event, create nsIDOMBluetoothDeviceAddressEvent by event generator (3.27 KB, patch)
2012-08-17 05:06 PDT, Gina Yeh [:gyeh] [:ginayeh]
bugs: review+
kyle: review+
Details | Diff | Review
Final version: Firing devicedisappeared event, create nsIDOMBluetoothDeviceAddressEvent by event generator, r=qdot, r=smaug (4.43 KB, patch)
2012-08-19 20:39 PDT, Gina Yeh [:gyeh] [:ginayeh]
no flags Details | Diff | Review

Description Eric Chou [:ericchou] [:echou] 2012-07-30 00:59:09 PDT
We need to implement several basic events for notifying Gaia apps status change:

Generated on Discoverying: onDeviceFound, onDeviceDisappeared
Generated on Pairing: onDevicePaired, onDeviceUnpaired
Generated on Connecting: onDeviceConnected, onDeviceDisconnected
Comment 1 Eric Chou [:ericchou] [:echou] 2012-07-30 01:05:10 PDT
(Copied from Comment 40 of bug 730992)

If we want to ensure that all properties in BluetoothDevice objects passed to Gaia are valid, then we have only one conclusion - "Return a BluetoothDevice object only if the target device exists on the device list of BluetoothAdapter".

In that case, those events should be defined as the following:

  onDeviceFound(nsString address, nsString propertyStr) 
    - The only thing that can be confirmed we will receive through a discovering process is
      device address. Other properties are provided optionally from remote devices. 
      So, using a string to represent various number of properties is quite reasonable
      (e.g. "class=0x5a020c,name=i9100"). This leverages Android's Bluetooth implementation.
  onDeviceDisappear(nsString address) - Reasonable.

  onDeviceCreated(BluetoothDevice dev)
  onDeviceRemoved(nsString address) - Not return BluetoothDevice because it's been removed.

  onDeviceConnected(BluetoothDevice dev)
  onDeviceDisconnected(BluetoothDevice dev)

In addition, if we decided that not use a BluetoothDevice with only Address is valid, then we don't need the method "BluetoothAdapter.getRemoteDevice()" to create an empty BluetoothDevice.
Comment 2 [:fabrice] Fabrice Desré 2012-07-30 01:56:29 PDT
Ccing smaug to not miss an opportunity to use his event generator.
Comment 3 Olli Pettay [:smaug] 2012-07-30 02:07:33 PDT
(In reply to Eric Chou [:ericchou] [:echou] from comment #0)
> We need to implement several basic events for notifying Gaia apps status
> change:
> 
> Generated on Discoverying: onDeviceFound, onDeviceDisappeared
> Generated on Pairing: onDevicePaired, onDeviceUnpaired
> Generated on Connecting: onDeviceConnected, onDeviceDisconnected
Lowercase events, please, if you're not using Moz prefix
Comment 4 Olli Pettay [:smaug] 2012-07-30 02:16:51 PDT
(In reply to Eric Chou [:ericchou] [:echou] from comment #1)
> In that case, those events should be defined as the following:
> 
>   onDeviceFound(nsString address, nsString propertyStr) 
>     - The only thing that can be confirmed we will receive through a
> discovering process is
>       device address. Other properties are provided optionally from remote
> devices. 
>       So, using a string to represent various number of properties is quite
> reasonable
>       (e.g. "class=0x5a020c,name=i9100"). This leverages Android's Bluetooth
> implementation.
>   onDeviceDisappear(nsString address) - Reasonable.
> 
>   onDeviceCreated(BluetoothDevice dev)
>   onDeviceRemoved(nsString address) - Not return BluetoothDevice because
> it's been removed.
> 
>   onDeviceConnected(BluetoothDevice dev)
>   onDeviceDisconnected(BluetoothDevice dev)


I don't understand these at all. onfoo properties are event handlers, not events.
I guess you want something like DeviceStatusEvent interface which has properties address and property (whatever that means), and device.
Define that interface in its own .idl, add the .idl to the relevant Makefile.in and add it to event_impl_gen.conf.in
Comment 5 Eric Chou [:ericchou] [:echou] 2012-07-30 04:12:34 PDT
> I don't understand these at all. onfoo properties are event handlers, not
> events.

Sorry for the ambiguity. Changed title.

> I guess you want something like DeviceStatusEvent interface which has
> properties address and property (whatever that means), and device.
> Define that interface in its own .idl, add the .idl to the relevant
> Makefile.in and add it to event_impl_gen.conf.in

I'll check if this case is suitable for using event generator. Thanks. :)
Comment 6 Kyle Machulis [:kmachulis] [:qdot] 2012-07-30 13:20:26 PDT
(In reply to Eric Chou [:ericchou] [:echou] from comment #1)

> If we want to ensure that all properties in BluetoothDevice objects passed
> to Gaia are valid, then we have only one conclusion - "Return a
> BluetoothDevice object only if the target device exists on the device list
> of BluetoothAdapter".

I don't see why we have this conclusion. Just because a Device isn't paired doesn't mean it can't exist. We get a mostly full property map with the DeviceFound returns, we can easily create a BluetoothDevice out of that, that just happens to not be bonded to the adapter yet.

We're basically hitting that point we talked about months ago where we're gonna have to start switching from the original DBus mappings to our own more usable, jsapi-like structures. :)

> In that case, those events should be defined as the following:
> 
>   onDeviceFound(nsString address, nsString propertyStr) 
>     - The only thing that can be confirmed we will receive through a
> discovering process is device address. Other properties are provided 
> optionally from remote devices. 
>       So, using a string to represent various number of properties is quite
> reasonable (e.g. "class=0x5a020c,name=i9100"). This leverages Android's Bluetooth
> implementation.

I don't think this is the right interface, and I'm not sure where you're getting that this leverages Android's BT implementation. If you look at the Java APIs for android's bluetooth (which is what we want to concentrate on at this level), whenever a device is found, the code gets back an ACTION_FOUND intent with an ExtraDevice property that holds a created BluetoothDevice object. Passing back delimited strings isn't at all what a JS API user would expect, and that seems prone to error on a lot of levels.

I'm honestly fine with this the way it is. To address the new issue of having a device that's already paired but not connected (i.e. pair device with phone, reboot phone), we just need a way to call GetProperty on the device before returning it, but we don't necessarily care about the device being found.

>   onDeviceDisappear(nsString address) - Reasonable.

Ok, so this could be on BluetoothDevice. Basically, what you can do is create a device on DeviceFound, expect any JS to hold the device and register for devicedisappeared on the BluetoothDevice object, then when dbus gets the adapter message for device disappeared, fire to the /device/ object node instead of the adapter object node. That way you don't even need to expose the address, and we can just use a regular ol' DOMEvent with no arguments, since the event + the device IS the context.

Unlike some of the things following this, I think disappearance is ok to have an event for, since we won't actually get property changes any more at that point, that's basically a "this object is dead, start from scratch" signifier. 

There's some interesting questions about how we register these objects internally and that conflicting with the "dead" idea, i.e. do a discovery, get a device object, device disappears, rediscovery and find object again, but since we register on dbus object node path, the old and new object node paths will be exactly the same, meaning the old device will still get signals if it's not dead yet. I may bug this to unregister for signals on devicedisappeared, need to think about it some.

>   onDeviceCreated(BluetoothDevice dev)

Do we even need this? We have a paired property on BluetoothDevice that will change as needed. Not only that, Pair()/Unpair() are DOMRequests that can lambda their Success/Error events over the BluetoothDevice object that's being paired/unpaired.

>   onDeviceRemoved(nsString address) - Not return BluetoothDevice because
> it's been removed.

Same deal. If we unpair, the paired property will just go to false. We can just watch that.

>   onDeviceConnected(BluetoothDevice dev)
>   onDeviceDisconnected(BluetoothDevice dev)

These also aren't needed. They're relayed in the BluetoothDevice property change event.

> In addition, if we decided that not use a BluetoothDevice with only Address
> is valid, then we don't need the method "BluetoothAdapter.getRemoteDevice()"
> to create an empty BluetoothDevice.

Honestly, the JS API at the moment should never have to care about an address. We should just be able to pass whole BluetoothDevice objects around. 

So, here's an example of zero to paired device:

- Create BluetoothAdapter
- Call StartDiscovery on BluetoothAdapter
- BluetoothAdapter gets DeviceFound signals from DBus, uses properties that come with it to create BluetoothDevice objects to send to JS (up to now, this is the same way we do it now)
- After discovery is over, we have BluetoothDevice objects. To pair with one, we pass the BluetoothDevice object we want to pair with to BluetoothAdapter's pair() function
- Pairing events do their thing, we know whether we're paired or not by watching BluetoothDevice property change messages for whether or not paired/connected has changed.

And from device boot with an already paired device:

- Create BluetoothAdapter
- To get currently connected devices, BluetoothAdapter gets new GetPairedDevices() function that returns a DOMRequest. We async GetProperty calls to all devices in the adapter devices property (which is now not exposed to the user), and return an array of BluetoothDevice objects once all of those are done. Ignore my "keep a JSObject*" ramblings in bug 730992. Everytime this is called, we'll new up a new array of BluetoothDevices. They'll all receive the same signals anyways.
- After this, we can run functions as needed.

I've already discussed DeviceDisappeared above. Does this all make sense?
Comment 7 Eric Chou [:ericchou] [:echou] 2012-07-30 21:19:15 PDT
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #1)
> 
> > If we want to ensure that all properties in BluetoothDevice objects passed
> > to Gaia are valid, then we have only one conclusion - "Return a
> > BluetoothDevice object only if the target device exists on the device list
> > of BluetoothAdapter".
> 
> I don't see why we have this conclusion. Just because a Device isn't paired
> doesn't mean it can't exist. We get a mostly full property map with the
> DeviceFound returns, we can easily create a BluetoothDevice out of that,
> that just happens to not be bonded to the adapter yet.
> 

ok, the word 'mostly' is what I'm concerning here. In fact, I'm not pretty sure if we can get all properties of a device in DeviceFound event. Since you said we need to avoid "Handing back a partially filled out device object" in bug 730992 (yes I know that's because we only sent address to the creator) that made me think if we can have this part done better to ensure 'all' properties which can be accessed from BluetoothDevice DOM API correct. 

So, the point is, does DeviceFound event contain all needed information to let us create a BluetoothDevice object? I checked the source code of BlueZ and HCI spec, and the answer is Yes. DeviceFound event contains enough information to let us create a device object, although property 'name' might be empty. So, your idea is better.

> > In that case, those events should be defined as the following:
> > 
> >   onDeviceFound(nsString address, nsString propertyStr) 
> >     - The only thing that can be confirmed we will receive through a
> > discovering process is device address. Other properties are provided 
> > optionally from remote devices. 
> >       So, using a string to represent various number of properties is quite
> > reasonable (e.g. "class=0x5a020c,name=i9100"). This leverages Android's Bluetooth
> > implementation.
> 
> I don't think this is the right interface, and I'm not sure where you're
> getting that this leverages Android's BT implementation. If you look at the
> Java APIs for android's bluetooth (which is what we want to concentrate on
> at this level), whenever a device is found, the code gets back an
> ACTION_FOUND intent with an ExtraDevice property that holds a created
> BluetoothDevice object. Passing back delimited strings isn't at all what a
> JS API user would expect, and that seems prone to error on a lot of levels.
> 
> I'm honestly fine with this the way it is. To address the new issue of
> having a device that's already paired but not connected (i.e. pair device
> with phone, reboot phone), we just need a way to call GetProperty on the
> device before returning it, but we don't necessarily care about the device
> being found.
> 

Because we've clarified that if we can create a BluetoothDevice object with the 
information in a DeviceFound event, we don't need to change the interface of 
DeviceFound anymore.

> >   onDeviceDisappear(nsString address) - Reasonable.
> 
> Ok, so this could be on BluetoothDevice. Basically, what you can do is
> create a device on DeviceFound, expect any JS to hold the device and
> register for devicedisappeared on the BluetoothDevice object, then when dbus
> gets the adapter message for device disappeared, fire to the /device/ object
> node instead of the adapter object node. That way you don't even need to
> expose the address, and we can just use a regular ol' DOMEvent with no
> arguments, since the event + the device IS the context.
> 

I still think this event should be on BluetoothAdapter. These two events are the result from adapter.startDiscovery, why they would be fired to the different object? And another question is: In what kind of situation we'll need a BluetoothDevice object when it disappears (leaves the range of discovering or switch property 'discoverable' off)?

>>   onDeviceCreated(BluetoothDevice dev)
> Do we even need this? We have a paired property on BluetoothDevice that will change as 
> needed. Not only that, Pair()/Unpair() are DOMRequests that can lambda their Success/Error 
> events over the BluetoothDevice object that's being paired/unpaired.
>> onDeviceRemoved(nsString address) - Not return BluetoothDevice because it's been removed.
> Same deal. If we unpair, the paired property will just go to false. We can just watch that.

You're right. We don't even need to watch the paired property. Pair()/Unpair() will return a DOMRequest ,so just use onsuccess/onerror.

> >   onDeviceConnected(BluetoothDevice dev)
> >   onDeviceDisconnected(BluetoothDevice dev)
> 
> These also aren't needed. They're relayed in the BluetoothDevice property
> change event.
> 

I didn't explain very clear. The main idea I wanted to express here is: How about parsing the propertychanged event in Gecko and firing the corresponding event to Gaia? That way Gaia app could easily understand which property is changed without checking the parameter in propertychanged event. 

> > In addition, if we decided that not use a BluetoothDevice with only Address
> > is valid, then we don't need the method "BluetoothAdapter.getRemoteDevice()"
> > to create an empty BluetoothDevice.
> 
> Honestly, the JS API at the moment should never have to care about an
> address. We should just be able to pass whole BluetoothDevice objects
> around. 
> 

Then we need to revise the Bluetooth WebAPI document to remove this method since we do not allow creating a BluetoothDevice with only address but nothing else.

> So, here's an example of zero to paired device:
> 
> - Create BluetoothAdapter
> - Call StartDiscovery on BluetoothAdapter
> - BluetoothAdapter gets DeviceFound signals from DBus, uses properties that
> come with it to create BluetoothDevice objects to send to JS (up to now,
> this is the same way we do it now)
> - After discovery is over, we have BluetoothDevice objects. To pair with
> one, we pass the BluetoothDevice object we want to pair with to
> BluetoothAdapter's pair() function
> - Pairing events do their thing, we know whether we're paired or not by
> watching BluetoothDevice property change messages for whether or not
> paired/connected has changed.
> 

Good. One problem here was if we can create BluetoothDevice objects by information in DeviceFound events, which does not exist now. Another one is should we fire onpropertychanged or onconnected/ondisconnected?

> And from device boot with an already paired device:
> 
> - Create BluetoothAdapter
> - To get currently connected devices, BluetoothAdapter gets new
> GetPairedDevices() function that returns a DOMRequest. We async GetProperty
> calls to all devices in the adapter devices property (which is now not
> exposed to the user), and return an array of BluetoothDevice objects once
> all of those are done. Ignore my "keep a JSObject*" ramblings in bug 730992.
> Everytime this is called, we'll new up a new array of BluetoothDevices.
> They'll all receive the same signals anyways.
> - After this, we can run functions as needed.
> 
Sounds good. I've been working on GetPairedDevices() (bug 777671) with Gina.
Comment 8 Kyle Machulis [:kmachulis] [:qdot] 2012-07-30 22:18:51 PDT
(In reply to Eric Chou [:ericchou] [:echou] from comment #7)
> So, the point is, does DeviceFound event contain all needed information to
> let us create a BluetoothDevice object? I checked the source code of BlueZ
> and HCI spec, and the answer is Yes. DeviceFound event contains enough
> information to let us create a device object, although property 'name' might
> be empty. So, your idea is better.

Yeah, I was being rather loose with my "partially filled" comment, and trusting the DBus documentation when it said it was passing back properties. Glad you checked it out though, definitely good to have confirmation on that. :)

> Because we've clarified that if we can create a BluetoothDevice object with
> the 
> information in a DeviceFound event, we don't need to change the interface of 
> DeviceFound anymore.

Awesome. Yay less code changes!

> I still think this event should be on BluetoothAdapter. These two events are
> the result from adapter.startDiscovery, why they would be fired to the
> different object? And another question is: In what kind of situation we'll
> need a BluetoothDevice object when it disappears (leaves the range of
> discovering or switch property 'discoverable' off)?

So the reason I was thinking about moving this down to Device was because of the awkwardness of the parameter passing for the event. Having to do a lookup based on the address seems obtuse when you're handed back a BluetoothDevice object on DeviceFound(). At our implementation level, we certainly don't /need/ a BluetoothDevice when it disappears, but that lifetime in JS also isn't up to us and we don't have the ability to destroy it out from under the developer. That's why it seems like putting DeviceDisappeared /on/ the device might make that clearer. It requires the developer to pay attention to the device object they've gotten, and do something about it when they get DeviceDisappeared that's not going to require them to do a lookup, since they can just assign a callback to the event. Less code and management for the end dev seems like it'd mean the possibility of better cleanup tactics.

> You're right. We don't even need to watch the paired property.
> Pair()/Unpair() will return a DOMRequest ,so just use onsuccess/onerror.

Cool.

> I didn't explain very clear. The main idea I wanted to express here is: How
> about parsing the propertychanged event in Gecko and firing the
> corresponding event to Gaia? That way Gaia app could easily understand which
> property is changed without checking the parameter in propertychanged event. 

Excuse me while I somewhat contradict myself from two paragraphs ago here. :) 

I like the PropertyChanged approach because that's a lot more code on our side, and implementation matters on the C++ level are far, far less flexible than pushing the resposibility up to Gaia. Right now we're still happy saying that properties and DOM object layout are flexible in the DOM API because we don't know what end implementation is going to look like, and that may change drastically before we expose this to users down the line. Therefore, we can just leave what we've got with the propertychanged event currently.

I talked to Jonas about this earlier today and he agrees with me on the simplicity for the moment. Once we're a bit more sure about how we want the API to look in the end, then it might be a good idea to add specific events for properties. For right now though, propertychanged gives us a nice, default way to do this. And it's already done. :)

> Then we need to revise the Bluetooth WebAPI document to remove this method
> since we do not allow creating a BluetoothDevice with only address but
> nothing else.

Yeah, I think we can do that without a problem.

> Good. One problem here was if we can create BluetoothDevice objects by
> information in DeviceFound events, which does not exist now. 

I'm not sure I understand this? As far as I was aware, we already fire DeviceFound with a filled BluetoothDevice object. When I'm working with pairing in Gaia, I get a BluetoothDevice object, then when I click it, it passes the whole object to BluetoothAdapter.pair(). Which part doesn't exist?

> Another one is
> should we fire onpropertychanged or onconnected/ondisconnected?

From the explanation above, onpropertychanged should do it. 
 
> Sounds good. I've been working on GetPairedDevices() (bug 777671) with Gina.

Awesome! And oh crap you asked me a question over there I didn't notice. Will go answer that now. :D
Comment 9 Eric Chou [:ericchou] [:echou] 2012-07-30 22:47:49 PDT
> > Good. One problem here was if we can create BluetoothDevice objects by
> > information in DeviceFound events, which does not exist now. 
> 
> I'm not sure I understand this? As far as I was aware, we already fire
> DeviceFound with a filled BluetoothDevice object. When I'm working with
> pairing in Gaia, I get a BluetoothDevice object, then when I click it, it
> passes the whole object to BluetoothAdapter.pair(). Which part doesn't exist?
> 
Reorganized the sentence:

One problem, which does not exist now, was if we can create BluetoothDevice objects by
information in DeviceFound events.

Just wanted to make sure that we were on the same page. Damn, English's so hard.
Comment 10 Kyle Machulis [:kmachulis] [:qdot] 2012-07-31 00:12:55 PDT
Ah, ok, got it, yeah, we're on the same page.

And yeah, we get to use an overly complex natural language with lots of crazy rules to discuss issues with an overly complex programming language with lots of crazy rules used to implement another slightly less complex programming language but with even more crazy rules. \o/
Comment 11 Eric Chou [:ericchou] [:echou] 2012-08-07 04:19:36 PDT
> > I still think this event should be on BluetoothAdapter. These two events are
> > the result from adapter.startDiscovery, why they would be fired to the
> > different object? And another question is: In what kind of situation we'll
> > need a BluetoothDevice object when it disappears (leaves the range of
> > discovering or switch property 'discoverable' off)?
> 
> So the reason I was thinking about moving this down to Device was because of
> the awkwardness of the parameter passing for the event. Having to do a
> lookup based on the address seems obtuse when you're handed back a
> BluetoothDevice object on DeviceFound(). At our implementation level, we
> certainly don't /need/ a BluetoothDevice when it disappears, but that
> lifetime in JS also isn't up to us and we don't have the ability to destroy
> it out from under the developer. That's why it seems like putting
> DeviceDisappeared /on/ the device might make that clearer. It requires the
> developer to pay attention to the device object they've gotten, and do
> something about it when they get DeviceDisappeared that's not going to
> require them to do a lookup, since they can just assign a callback to the
> event. Less code and management for the end dev seems like it'd mean the
> possibility of better cleanup tactics.
> 

Faced a problem when tried to implement devicedisappeared. Based on current code base, when BluetoothDBusService.cpp got events sent from dbus/BlueZ, those events finally would be dispatched to BluetoothAdapter/Bluetooth device according to their path:

  BluetoothSignal signal(signalName, signalPath, v);

  nsRefPtr<DistributeBluetoothSignalTask> 
    t = new DistributeBluetoothSignalTask(signal);

Because event 'DeviceDisappeared' belongs to adapter interface, if we would like to redirect it to BluetoothDevice, we have to modify the 'signalPath' parameter of the above code. It's a little weird, isn't it? any better idea?

Again, we could pass this event to BluetoothAdapter with an address parameter. Yes, there would be an additional parameter than just send a notification on BluetoothDevice, nevertheless I still consider DeviceDisappeared and DeviceFound are two sides to one operation.
Comment 12 Kyle Machulis [:kmachulis] [:qdot] 2012-08-07 17:39:21 PDT
(In reply to Eric Chou [:ericchou] [:echou] from comment #11)
> > > I still think this event should be on BluetoothAdapter. These two events are
> > > the result from adapter.startDiscovery, why they would be fired to the
> > > different object? And another question is: In what kind of situation we'll
> > > need a BluetoothDevice object when it disappears (leaves the range of
> > > discovering or switch property 'discoverable' off)?
> > 
> > So the reason I was thinking about moving this down to Device was because of
> > the awkwardness of the parameter passing for the event. Having to do a
> > lookup based on the address seems obtuse when you're handed back a
> > BluetoothDevice object on DeviceFound(). At our implementation level, we
> > certainly don't /need/ a BluetoothDevice when it disappears, but that
> > lifetime in JS also isn't up to us and we don't have the ability to destroy
> > it out from under the developer. That's why it seems like putting
> > DeviceDisappeared /on/ the device might make that clearer. It requires the
> > developer to pay attention to the device object they've gotten, and do
> > something about it when they get DeviceDisappeared that's not going to
> > require them to do a lookup, since they can just assign a callback to the
> > event. Less code and management for the end dev seems like it'd mean the
> > possibility of better cleanup tactics.
> > 
> 
> Faced a problem when tried to implement devicedisappeared. Based on current
> code base, when BluetoothDBusService.cpp got events sent from dbus/BlueZ,
> those events finally would be dispatched to BluetoothAdapter/Bluetooth
> device according to their path:
> 
>   BluetoothSignal signal(signalName, signalPath, v);
> 
>   nsRefPtr<DistributeBluetoothSignalTask> 
>     t = new DistributeBluetoothSignalTask(signal);
> 
> Because event 'DeviceDisappeared' belongs to adapter interface, if we would
> like to redirect it to BluetoothDevice, we have to modify the 'signalPath'
> parameter of the above code. It's a little weird, isn't it? any better idea?

It's a bit of a hack, but it comes down to the original question of how much do we want to follow the DBus implementation. Looking at how other people do this...

Android hands back the address with both ACTION_FOUND and ACTION_DISAPPEARED, as EXTRA_DEVICE, at which point you have to call GetRemoteDevice() from the adapter to get it in the FOUND case.

Tizen hands back an object with DeviceFound, Address with DeviceDisappeared - 
https://developer.tizen.org/help/index.jsp?topic=%2Forg.tizen.help.web.api.device%2Ftizen%2Fbluetooth.html

So, looks like everyone expects you to hold your own store and compare against that store to find out who disappeared. 

I guess there's dueling assumptions that handing back DeviceDisappeared with a device address from BluetoothAdapter means you never lose data and assumes if you're interested in the device, you kept it around, versus my idea of if you own the device and care, you'll hold it somehow and register for the event, and if you haven't done that, you'll never see DeviceDisappeared. I don't really see either choice being better than the other, so if we want to go with what I guess is canon-via-other-apis, that's fine. I might poke one of the DOM gurus just to get some opinion on this.
Comment 13 Eric Chou [:ericchou] [:echou] 2012-08-09 03:09:33 PDT
> > 
> > Faced a problem when tried to implement devicedisappeared. Based on current
> > code base, when BluetoothDBusService.cpp got events sent from dbus/BlueZ,
> > those events finally would be dispatched to BluetoothAdapter/Bluetooth
> > device according to their path:
> > 
> >   BluetoothSignal signal(signalName, signalPath, v);
> > 
> >   nsRefPtr<DistributeBluetoothSignalTask> 
> >     t = new DistributeBluetoothSignalTask(signal);
> > 
> > Because event 'DeviceDisappeared' belongs to adapter interface, if we would
> > like to redirect it to BluetoothDevice, we have to modify the 'signalPath'
> > parameter of the above code. It's a little weird, isn't it? any better idea?
> 
> It's a bit of a hack, but it comes down to the original question of how much
> do we want to follow the DBus implementation. Looking at how other people do
> this...
> 
> Android hands back the address with both ACTION_FOUND and
> ACTION_DISAPPEARED, as EXTRA_DEVICE, at which point you have to call
> GetRemoteDevice() from the adapter to get it in the FOUND case.
> 
> Tizen hands back an object with DeviceFound, Address with DeviceDisappeared
> - 
> https://developer.tizen.org/help/index.jsp?topic=%2Forg.tizen.help.web.api.
> device%2Ftizen%2Fbluetooth.html
> 
> So, looks like everyone expects you to hold your own store and compare
> against that store to find out who disappeared. 
> 
> I guess there's dueling assumptions that handing back DeviceDisappeared with
> a device address from BluetoothAdapter means you never lose data and assumes
> if you're interested in the device, you kept it around, versus my idea of if
> you own the device and care, you'll hold it somehow and register for the
> event, and if you haven't done that, you'll never see DeviceDisappeared. I
> don't really see either choice being better than the other, so if we want to
> go with what I guess is canon-via-other-apis, that's fine. I might poke one
> of the DOM gurus just to get some opinion on this.

Talked with timdream earlier today. He said that adapter.ondevicedisappeared is more reasonable to him. Moreover, he also mentioned that it'd be better if we can provide adapter.ondeviceconnected & adapter.ondevicedisconnected. If they need to listen to devices' propertychanged event, "30+ lines of code will be needed (in Gaia)". (Tim commented about ondeviceconnected & ondevicedisconnected in https://github.com/mozilla-b2g/gaia/pull/3285/files)
Comment 14 Kyle Machulis [:kmachulis] [:qdot] 2012-08-09 14:09:42 PDT
Cool, I consider myself validly overruled, ship it! :)
Comment 15 Gina Yeh [:gyeh] [:ginayeh] 2012-08-15 00:16:30 PDT
Created attachment 652023 [details] [diff] [review]
v1: Firing devicedisappeared event

BluetoothDeviceAddressEvent is created and it is fired with the address of disappeared device.
Comment 16 Kyle Machulis [:kmachulis] [:qdot] 2012-08-15 10:38:43 PDT
Comment on attachment 652023 [details] [diff] [review]
v1: Firing devicedisappeared event

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

Ok. While this is r=me (will still need DOM peer review), I dunno if we can land this or not since it's a simple DOM Event implemented by hand. We've been getting warnings on other simple DOM event bugs about using the event generator.

Check out https://bugzilla.mozilla.org/show_bug.cgi?id=765163#c1 for more info. Yes, that's actually the only documentation I'm aware of on this. :|

Feel free to poke smaug on IRC about it too.

::: dom/bluetooth/BluetoothDeviceAddressEvent.cpp
@@ +1,2 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=40: */

Nit: Make tw=80 (that's text width not tab-width. I'm gonna make a quick bug to fix all these soon, it's a bad habit that got started late last year that keeps getting cut & pasted. :) )
Comment 17 Olli Pettay [:smaug] 2012-08-15 10:44:33 PDT
nsIDOMBluetoothDeviceAddressEvent is simple enough to be implemented using the code generator.

the interface should have [noscript] initBluetoothDeviceAddressEvent method. Add also dictionary for the constructor to the .idl. Add the event interface to http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/event_impl_gen.conf.in?force=1
Comment 18 Gina Yeh [:gyeh] [:ginayeh] 2012-08-16 02:32:03 PDT
Created attachment 652375 [details] [diff] [review]
v2: Firing devicedisappeared event

nsIDOMBluetoothDeviceAddressEvent is generated by event generator now.
Comment 19 Kyle Machulis [:kmachulis] [:qdot] 2012-08-16 13:38:04 PDT
Comment on attachment 652375 [details] [diff] [review]
v2: Firing devicedisappeared event

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

Wow this is WAY less code. Now looking forward to cleaning up the rest of our events. :D
Comment 20 Olli Pettay [:smaug] 2012-08-16 13:50:03 PDT
Comment on attachment 652375 [details] [diff] [review]
v2: Firing devicedisappeared event


>+    DispatchDOMEvent(nullptr, event, nullptr, nullptr);
I'd prefer DispatchEvent(event, &dummyBooleanVar)
Comment 21 Gina Yeh [:gyeh] [:ginayeh] 2012-08-16 20:54:54 PDT
Created attachment 652665 [details] [diff] [review]
Final version: Firing devicedisappeared event, create nsIDOMBluetoothDeviceAddressEvent by event generator, r=qdot, r=smaug

replace DispatchDOMEvent to DispatchEvent in this patch, and I'm going to modify the devicefound event in Bug783454.
Comment 22 Eric Chou [:ericchou] [:echou] 2012-08-16 21:05:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ae4450ab79
Comment 23 Chris Pearce (:cpearce) 2012-08-16 21:25:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe1c2d3d1f4

Sorry, I backed out you push since it broke the build on all platforms except ARM.

I recommend you try pushing to Try before pushing to inbound.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=26ae4450ab79
Comment 24 Eric Chou [:ericchou] [:echou] 2012-08-16 23:26:11 PDT
Thank you, Chris.
Comment 25 Gina Yeh [:gyeh] [:ginayeh] 2012-08-17 03:03:43 PDT
I checked the build result and found that it is caused by missing file nsIDOMBluetoothDeviceAddressEvent.idl. Since the bluetooth module is skipped during the building on some platforms, the event generator cannot find it and build failed.

One of the solutions is, as other events generated by event generator, we can just put nsIDOMBluetoothDeviceAddressEvent.idl at gecko/dom/interfaces/events/ . But it also means that nsDOMBluetoothDeviceAddressEvent will be generated on all platforms although the bluetooth module shouldn't be enabled on some platforms. Any idea for this?

Another problem I met whenever modifying devicefound event part. In this situation, we'd like to add a BluetoothDevice as a property to nsIDOMEvent. If we put nsIDOMBluetoothDeviceEvent.idl under the path above and use generator to automatically create nsIDOMBluetoothDeviceEvent, then nsIDOMBluetoothDevice.h will be included at GeneratedEvents.cpp and DictionaryHelpers.cpp, and, in fact, nsIDOMBluetoothDevice.h is not existed on some platforms due to the bluetooth module is skipped during the building.

One more thing, although we can automatically create devicedisappeared event (nsIDOMBluetoothDeviceAddressEvent), in order to keep consistency in our module, I'll suggest create both devicedisappeared event and devicefound event manually if we can't generate devicefound event (nsIDOMBluetoothDeviceEvent) by event generator.
Comment 26 Olli Pettay [:smaug] 2012-08-17 03:26:12 PDT
event_impl_gen.conf.in is preprocessed file. Just add some #ifdefs there.
(IIRC you many need to add DEFINES += ... to the Makefile.in in the directory where event_impl_gen.conf.in is.)

https://bugzilla.mozilla.org/show_bug.cgi?id=765947&mark=0#c0
Comment 27 Gina Yeh [:gyeh] [:ginayeh] 2012-08-17 05:06:29 PDT
Created attachment 652734 [details] [diff] [review]
v3: Firing devicedisappeared event, create nsIDOMBluetoothDeviceAddressEvent by event generator

create nsIDOMBluetoothDeviceAddressEvent by event generator and add flag MOZ_B2G_BT in event_impl_gen.conf.in.
Comment 28 Gina Yeh [:gyeh] [:ginayeh] 2012-08-17 05:14:42 PDT
Thanks, smaug. I add #ifdef in event_impl_gen.conf.in and the problem has been solved. :)
Comment 29 Olli Pettay [:smaug] 2012-08-17 05:18:15 PDT
Comment on attachment 652734 [details] [diff] [review]
v3: Firing devicedisappeared event, create nsIDOMBluetoothDeviceAddressEvent by event generator

the patch  misses the idl file, but r=me for the 
event_impl_gen.conf.in part.
(Hopefully you've even tested it. Would be great to push the patch to try, since
this is the first time #ifdef is used in event_impl_gen.conf.in)
Comment 30 Kyle Machulis [:kmachulis] [:qdot] 2012-08-17 13:02:09 PDT
Comment on attachment 652734 [details] [diff] [review]
v3: Firing devicedisappeared event, create nsIDOMBluetoothDeviceAddressEvent by event generator

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

Yeah, include IDL, and run thru try, but r=me too
Comment 31 Gina Yeh [:gyeh] [:ginayeh] 2012-08-19 20:39:34 PDT
Created attachment 653247 [details] [diff] [review]
Final version: Firing devicedisappeared event, create nsIDOMBluetoothDeviceAddressEvent by event generator, r=qdot, r=smaug

Wrap idl into patch as well and also push to try server. https://tbpl.mozilla.org/?tree=Try&rev=07d1dc965ada
Comment 32 Eric Chou [:ericchou] [:echou] 2012-08-19 21:24:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6913d1272cee
Comment 33 Ed Morley [:emorley] 2012-08-20 08:54:40 PDT
https://hg.mozilla.org/mozilla-central/rev/6913d1272cee

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