1019544 - Fire onattribute changed when BluetoothAdapter received PropertyChanged event

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jocelyn Liu [:jocelyn] [:joliu]]

Extracted from Bug1006308.
When BluetoothAdapter received PropertyChanged event from service, fire an onattributechanged event to notify gaia the type and value.

Comment 1

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1: Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event

handle propertychanged event from service and dispatch onattributechanged to gaia

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v2): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event

* Revise JS:Value related part in previous version.
* Revise State property handling

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v2): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event

Update comment

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8434716 [details] [diff] [review]
Patch1(v2): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event

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

Almost there. Please revise |HandlePropertyChanged| to make the logic clearer.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +770,5 @@
> +}
> +
> +bool
> +BluetoothAdapter::IsPropertyChanged(BluetoothAdapterAttribute aType,
> +                                   const BluetoothValue& aValue)

nit: one more space.

@@ +774,5 @@
> +                                   const BluetoothValue& aValue)
> +{
> +  switch(aType) {
> +    case BluetoothAdapterAttribute::State: {
> +      bool isEnabled = aValue.get_bool();

Add |MOZ_ASSERT| to ensure BluetoothValue's type.

@@ +775,5 @@
> +{
> +  switch(aType) {
> +    case BluetoothAdapterAttribute::State: {
> +      bool isEnabled = aValue.get_bool();
> +      if (isEnabled) {

Rewrite as in |SetPropertyByValue|:

  mState = isEnabled ? mState != BluetoothAdapterState::Enabled
                     : mState != BluetoothAdapterState::Disabled;

@@ +782,5 @@
> +        return mState != BluetoothAdapterState::Disabled;
> +      }
> +    }
> +    case BluetoothAdapterAttribute::Name:
> +      return !mName.Equals(aValue.get_nsString());

Ditto.

@@ +784,5 @@
> +    }
> +    case BluetoothAdapterAttribute::Name:
> +      return !mName.Equals(aValue.get_nsString());
> +    case BluetoothAdapterAttribute::Address:
> +      return !mAddress.Equals(aValue.get_nsString());

Ditto.

@@ +786,5 @@
> +      return !mName.Equals(aValue.get_nsString());
> +    case BluetoothAdapterAttribute::Address:
> +      return !mAddress.Equals(aValue.get_nsString());
> +    case BluetoothAdapterAttribute::Discoverable:
> +      return mDiscoverable != aValue.get_bool();

Ditto.

@@ +788,5 @@
> +      return !mAddress.Equals(aValue.get_nsString());
> +    case BluetoothAdapterAttribute::Discoverable:
> +      return mDiscoverable != aValue.get_bool();
> +    case BluetoothAdapterAttribute::Discovering:
> +      return mDiscovering != aValue.get_bool();

Ditto.

@@ +806,5 @@
> +  for (uint32_t i = 0, propCount = arr.Length(); i < propCount; ++i) {
> +    BluetoothAdapterAttribute type =
> +      ConvertStringToAdapterAttribute(arr[i].name());
> +
> +    if (IsPropertyChanged(type, arr[i].value())) {

Rewrite as following to filter out non-BluetoothAdapterAttribute at the beginning:
 
  // Non-BluetoothAdapterAttribute properties
  if (type == BluetoothAdapterAttribute::Unknown) {
    SetPropertyByValue(arr[i]);
    return;
  }

  // BluetoothAdapterAttribute properties
  if (IsAdapterAttributeChanged(type, arr[i].value())) {
    SetPropertyByValue(arr[i]);
    DispatchAttributeEvent(type);
  }

Additional changes: 1) |IsPropertyChanged| is renamed to |IsAdapterAttributeChanged|, 2) |IsAdapterAttributeChanged| returns false by default, and 3) |DispatchAttributeEvent| prints BT_WARNING if it gets unknown attribute type instead of return directly.

@@ +832,5 @@
> +      value.setInt32(int32_t(mState));
> +      break;
> +    case BluetoothAdapterAttribute::Address: {
> +      JSString* jsData = JS_NewStringCopyN(cx,
> +                                           NS_ConvertUTF16toUTF8(mAddress).get(),

nit: this line is too long (> 80 characters).

::: dom/bluetooth2/BluetoothAdapter.h
@@ +186,5 @@
>    already_AddRefed<mozilla::dom::DOMRequest>
>      PairUnpair(bool aPair, const nsAString& aDeviceAddress, ErrorResult& aRv);
> +  bool
> +    IsPropertyChanged(BluetoothAdapterAttribute aType,
> +                      const BluetoothValue& aValue);

nit: wrap it in 2 lines.

@@ +188,5 @@
> +  bool
> +    IsPropertyChanged(BluetoothAdapterAttribute aType,
> +                      const BluetoothValue& aValue);
> +  void
> +    HandlePropertyChanged(const BluetoothValue& aValue);

Ditto.

@@ +190,5 @@
> +                      const BluetoothValue& aValue);
> +  void
> +    HandlePropertyChanged(const BluetoothValue& aValue);
> +  void
> +    DispatchAttributeChanged(BluetoothAdapterAttribute aType);

Rename to |DispatchAttributeEvent| to conform with BluetoothManager::DispatchAttributeEvent.

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v3): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event

The patch had been revised according to the review comments.
Thanks for the review!

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v3): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event

Sorry, I attached a wrong file.

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8435483 [details] [diff] [review]
Patch1(v3): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event

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

r=me with comment addressed. Please request Boris review JS related implementation. Thanks.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +760,5 @@
> +    case BluetoothAdapterAttribute::Discovering:
> +      MOZ_ASSERT(aValue.type() == BluetoothValue::Tbool);
> +      return mDiscovering != aValue.get_bool();
> +    default:
> +      return false;

Add |BT_WARNING| to print |aType| here.

@@ +779,5 @@
> +
> +    // Non-BluetoothAdapterAttribute properties
> +    if (type == BluetoothAdapterAttribute::Unknown) {
> +      SetPropertyByValue(arr[i]);
> +      return;

Should be |continue|.

@@ +832,5 @@
> +    case BluetoothAdapterAttribute::Discovering:
> +      value.setBoolean(mDiscovering);
> +      break;
> +    default:
> +      BT_WARNING("onattributechanged is not fired for unknown attribute type");

Print |aType| in warning message.

::: dom/bluetooth2/BluetoothAdapter.h
@@ +184,5 @@
>    already_AddRefed<mozilla::dom::DOMRequest>
>      StartStopDiscovery(bool aStart, ErrorResult& aRv);
>    already_AddRefed<mozilla::dom::DOMRequest>
>      PairUnpair(bool aPair, const nsAString& aDeviceAddress, ErrorResult& aRv);
> +  bool IsAdapterAttributeChanged(BluetoothAdapterAttribute aType,

nit: newline here to group new attribute-related functions.

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1019544 - Patch1: Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event. r=btian

Hi Boris,

Could you help to review JS related implementation for this patch?

|DispatchAttributeEvent|: Wrap and fire a BluetoothAttributeEvent with attribute type and value to notify gaia about adapter's attribute change.

Thanks,
Jocelyn

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8436635 [details] [diff] [review]
Bug 1019544 - Patch1: Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event. r=btian

>+BluetoothAdapter::ConvertStringToAdapterAttribute(const nsAString& aString)
>+    if (aString.LowerCaseEqualsASCII(strings[index].value)) {

Pass in the length too?

>+BluetoothAdapter::HandlePropertyChanged(const BluetoothValue& aValue)
>+      DispatchAttributeEvent(type);

Can this code handle being reentered from that event?  This function seems to me like it can, to me, but what about the code more generally?

If this code can't handle reentry, you may want to use a scriptrunner for the event dispatch.

>+BluetoothAdapter::DispatchAttributeEvent(BluetoothAdapterAttribute aType)
>+  AutoJSContext cx;

You should use AutoJSAPI instead.

>+  JS::Rooted<JS::Value> value(cx, JS::NullValue());

This is annoying.  Do we have other places that put a sequence of strings in the event?  The IDL comments suggest we do, but I'm not seeing it here.

>+      JSString* jsData =
>+        JS_NewStringCopyN(cx,
>+                          NS_ConvertUTF16toUTF8(mAddress).get(),
>+                          mAddress.Length());

This is wrong in several ways: makes extra copies, lengths won't match unless mAddress is ASCII.

More importantly, it should just be:

  if (!ToJSValue(cx, mAddress, &value))

and the right thing will happen.

>+      NS_ENSURE_TRUE_VOID(jsData);

Need to report or squelch the pending exception on cx.

Same comments apply to the name case.

>+      BT_WARNING("onattributechanged is not fired for type %d",

It's fired, just with null as the value, right?

>+++ b/dom/bluetooth2/BluetoothAdapter.h
>+#include "mozilla/dom/BluetoothAttributeEvent.h"

Why do you need this in the header?

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8436635 [details] [diff] [review]
> Bug 1019544 - Patch1: Fire onattributechanged to gaia while BluetoothAdapter
> received PropertyChanged event. r=btian
> 
> >+BluetoothAdapter::HandlePropertyChanged(const BluetoothValue& aValue)
> >+      DispatchAttributeEvent(type);
> 
> Can this code handle being reentered from that event?  This function seems
> to me like it can, to me, but what about the code more generally?
> 
> If this code can't handle reentry, you may want to use a scriptrunner for
> the event dispatch.
> 
Sorry, Boris, I didn't quite understand this comment.
We call this each time we received PropertyChange from BluetoothService.
What do you mean about being reentered from that event? Which event?
> >+BluetoothAdapter::DispatchAttributeEvent(BluetoothAdapterAttribute aType)
> >+  AutoJSContext cx;
> 
> You should use AutoJSAPI instead.
> 
Is it OK to just substitude AutoJSContext to AutoSafeJSContext?
> >+  JS::Rooted<JS::Value> value(cx, JS::NullValue());
> 
> This is annoying.  Do we have other places that put a sequence of strings in
> the event?  The IDL comments suggest we do, but I'm not seeing it here.
> 
In BluetoothAdapter, no.
But BluetoothManager which also uses BluetoothAttributeEvent will.
> >+      BT_WARNING("onattributechanged is not fired for type %d",
> 
> It's fired, just with null as the value, right?
> 
It will return directly,
BluetoothAttributeEvent is not constructed and fired under this circumstance.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> What do you mean about being reentered from that event? Which event?

DispatchAttributeEvent will run some sort of event handlers, right?  Which can modify the state of this object and whatnot, while we're going through this loop.  Is that OK?

> Is it OK to just substitude AutoJSContext to AutoSafeJSContext?

No.  We're trying to get rid of AutoJSContext (and I believe AutoSafeJSContext).  You should be using AutoJSAPI, which is the replacement for those.

> It will return directly,

Ah, good point.  OK.

Comment 12

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Boris Zbarsky [:bz] from comment #11)
> > What do you mean about being reentered from that event? Which event?
> 
> DispatchAttributeEvent will run some sort of event handlers, right?  Which
> can modify the state of this object and whatnot, while we're going through
> this loop.  Is that OK?
> 
Just wanna double check if we're on the same page.
Take mState as an example, it might be changed again while I am still processing and firing event for the previous change. 
Hence, I will lost the previous change and notify two times for the latest change.
Is that what you're saying here?

I don't quite understand the scriptrunner way.
But if I understand the problem correctly, I might pass the value directly into DispatchAttributeEvent() and construct JSValue based on the type of aValue(bool, int, string).
Does this make sense to you?

Comment 13

[Jocelyn Liu [:jocelyn] [:joliu]]

To make it more clear, it will be DispatchAttributeEvent(type, arr[i].value()).

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Take mState as an example, it might be changed again while I am still processing and
> firing event for the previous change. 

Yes.  Note that if it does it will presumably fire an event of its own on that change.  So a script observing those change events might effectively see state being X, then an event saying the state changed and see the new value as being X (because we put mState in the event, not the thing from the incoming BluetoothValue, then nothing.

The fact that what the observer sees might make no sense is a fundamental problem with event dispatch systems that have multiple listeners.  You don't need multiple events for that, even; just two listeners for the same event is enough.  Not much you can do about that.  

The only real worry is whether it can make our internal state in the C++ inconsistent in any way.

> I don't quite understand the scriptrunner way.

All it would do is first figure out what all the events we need to fire are, then fire them all.  Again, that may not be what we actually want; I just want to make sure that the setup in the patch right now doesn't get us into internally inconsistent states.

> I might pass the value directly into DispatchAttributeEvent() and construct JSValue
> based on the type of aValue(bool, int, string).
> Does this make sense to you?

I think that actually makes things worse, actually, in terms of what the script sees.  But again, I don't think that's the problem I'm worried about.

Comment 15

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> > Take mState as an example, it might be changed again while I am still processing and
> > firing event for the previous change. 
> 
> Yes.  Note that if it does it will presumably fire an event of its own on
> that change.  So a script observing those change events might effectively
> see state being X, then an event saying the state changed and see the new
> value as being X (because we put mState in the event, not the thing from the
> incoming BluetoothValue, then nothing.
> 
> The fact that what the observer sees might make no sense is a fundamental
> problem with event dispatch systems that have multiple listeners.  You don't
> need multiple events for that, even; just two listeners for the same event
> is enough.  Not much you can do about that.  
> 
> The only real worry is whether it can make our internal state in the C++
> inconsistent in any way.

When we want to notify BluetoothAdapter about PropertyChanged, we will dispatch a task to main thread and BluetoothAdapter::Notify() will then be runned on mainthread to update BluetoothAdapter object and fire BluetoothAttributeEvents.

In our new webidl[1], a script can request to change adapter properties through setName and setDiscoverable.
(Directly assigned is not allowed since they're readonly attributes.)
We will first ask bluetooth stack to change the property then expected AdapterPropertiesCallback[2] being called if success. When it's been called, it will do the things as above paragraph described.

Since BluetoothAdapter::Notify() is the only entry for HandlePropertyChanged() and both Notify() and brocasting PropertyChanged notifications are happened in the main thread, I think it won't be reentered if a script trying to change the property.

Does this make sense to you?

[1]: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#BluetoothAdapter
[2]: http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#335

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [WIP] Bug 1019544 - Patch1: Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event. r=btian

Addressed Boris' review comments.
Marked as WIP since the reentered thing is still under discussion.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I think it won't be reentered if a script trying to change the property.

I don't see why not.  Consider a script that calls setDiscoverable(), say, and then spins the event loop for a while before returning.

Comment 18

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Boris Zbarsky [:bz] from comment #17)
> > I think it won't be reentered if a script trying to change the property.
> 
> I don't see why not.  Consider a script that calls setDiscoverable(), say,
> and then spins the event loop for a while before returning.

Hi Boris,

It seems to me that HandlePropertyChanged() will not be reentered while it's already running on the same thread in same process.

Here's the scenario we expect:
1) [main thread]
   |HandlePropertyChanged| is running for prior |setDiscoverable| method, so is |DispatchAttributeEvent|.
2) [main/non-main thread]
   Application calls |setDiscoverable| again on either main or non-main thread.
3) [non-main thread]
   AdapterPropertiesCallback will be called on bluedroid thread
4) [non-main thread] 
   Dispatch a task to main thread to notify observers
5) [main thread]
   Notify observers about "PropertyChanged"
6) [main thread]
   BluetoothAdapter::Notify(), then HandlePropertyChanged() is called

From my understanding, your concern is that |HandlePropertyChanged| will be reentered in 6) before 1) finishes.
But as they both run on main thread, 6) shouldn't run until 1) finishes, so |HandlePropertyChanged| wouldn't be reentered in 6).

Please correct us if what we expect is wrong, and let us know in which step do you think the function will be reentered and causes inconsistent internal state.

Thanks,
Jocelyn

Comment 19

[Boris Zbarsky [:bzbarsky]]

> But as they both run on main thread, 6) shouldn't run until 1) finishes

This is not the case in Gecko right now if the script spins the event loop after calling setDiscoverable.

The control flow will go like this:

[Main Thread]
1.  Call into JS handler for the attribute event.
2.  JS calls setDiscoverable, which posts a runnable to the non-main thread.
3.  JS spins the event loop (via showModalDialog or sync XHR) until a point of its
    choosing.
4.  JS returns back into the C++ that called HandlePropertyChanged.

[Non-main Thread]
I.  Receive runnable from the setDiscoverable call.
II.  Do whatever work it does.
III.  Post a runnable back to the main thread.

Since the JS is spinning the event loop, all of I,II,III happen while step 3 is happening on the main thread, and the runnable that III posts is processed before we get to step 4.

Comment 20

[Ben Tian (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #19)
> [Main Thread]
> 1.  Call into JS handler for the attribute event.
> 2.  JS calls setDiscoverable, which posts a runnable to the non-main thread.
> 3.  JS spins the event loop (via showModalDialog or sync XHR) until a point
> of its
>     choosing.
> 4.  JS returns back into the C++ that called HandlePropertyChanged.
> 
> [Non-main Thread]
> I.  Receive runnable from the setDiscoverable call.
> II.  Do whatever work it does.
> III.  Post a runnable back to the main thread.
> 
> Since the JS is spinning the event loop, all of I,II,III happen while step 3
> is happening on the main thread, 

Agree until here.

> and the runnable that III posts is processed before we get to step 4.

Why isn't the runnable that III posts processed after step 4 finishes? Doesn't the order of being processed on main thread match the order of being posted (step 1-4 and then III)?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Why isn't the runnable that III posts processed after step 4 finishes?

Because step 3 us spinning the event loop.  As in, it's explicitly processing runnables.

> Doesn't the order of being processed on main thread match the order of being posted

Order of start of processing matches order of being posted.  But since a runnable can explicitly process other runnables, processing can nest.  So runnable A can start before B starts and finish after B finishes.

Comment 22

[Ben Tian (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #21)
> > Doesn't the order of being processed on main thread match the order of being posted
> 
> Order of start of processing matches order of being posted.  But since a
> runnable can explicitly process other runnables, processing can nest.  So
> runnable A can start before B starts and finish after B finishes.

So in this nested case main thread switches between runnable A (that runs step 1-4) and runnable B (that is posted in III) instead of starting B after A finishes, right? If so, does a MonitorAutoLock help here?

--
[Main Thread]
1.  Call into JS handler for the attribute event.
2.  JS calls setDiscoverable, which posts a runnable to the non-main thread.
3.  JS spins the event loop (via showModalDialog or sync XHR) until a point of its
    choosing.
4.  JS returns back into the C++ that called HandlePropertyChanged.

[Non-main Thread]
I.  Receive runnable from the setDiscoverable call.
II.  Do whatever work it does.
III.  Post a runnable back to the main thread.

Comment 23

[Boris Zbarsky [:bzbarsky]]

The main thread doesn't _switch_ between runnables.  It's in the middle of running runnable A, and some of the script that calls invokes APIs that process runnables.  So at that point it's still in the middle of running runnable A, but it's also running runnable B.  Which could in turn spin the event loop and start processing of runnable C, etc.

> does a MonitorAutoLock help here?

How would it help, exactly?

Comment 24

[Ben Tian (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #23)
> The main thread doesn't _switch_ between runnables.  It's in the middle of
> running runnable A, and some of the script that calls invokes APIs that
> process runnables.  So at that point it's still in the middle of running
> runnable A, but it's also running runnable B.  Which could in turn spin the
> event loop and start processing of runnable C, etc.

That's confusing. As there's only one thread - the main thread, how can it run runnable A and also B without switching between them? 

> > does a MonitorAutoLock help here?
> 
> How would it help, exactly?

My idea is a lock held by runnable A to keep subsequent calls to |HandlePropertChanged| blocked before A is done.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ben, the main thread is fundamentally something that looks like this, in JS-like pseudocode

  var queue = [];
  function processQueue() {
    while (queue.length != 0) {
      processRunnable();
    }
    waitForRunnables()
  }
  function processRunnable() {
    var runnable = queue.shift();
    if (runnable) {
      runnable();
    }
  }
  function addRunnable(runnable) {
    queue.push(runnable);
  }

In Gecko, processQueue is actually called NS_ProcessPendingEvents, processRunnable is called NS_ProcessNextEvent, and addRunnable is NS_DispatchToCurrentThread/MainThread.

There are things JS can do that will explicitly call NS_ProcessNextEvent.  In the pseudocode above, that would correspond to doing this:

  var done = false;
  addRunnable(function() {
    while (!done) {
      processRunnable();
    }
  });

Does that help any?

> My idea is a lock held by runnable A to keep subsequent calls to |HandlePropertChanged|
> blocked before A is done.

Which threads would take the lock and when?  And for how long?

Comment 26

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Boris,

We now get that why will runnableB be finished before runnableA is, thank you.
We plan to revise |HandlePropertyChanged| to fix this problem as follows.

1) Go through a loop to set values in Adapter object from the array of changed properties.
2) Dispatch a single BluetoothAttributeEvent for all changes with a array of changed attribute types 
   after 1) is done.
The webidl of BluetoothAttributeEvent is also revised as
https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAttributeEvent#Interface.
For using the attribute(name for example), JS just use adapter.name to get the latest value.

How's this sound to you? 
Please let us know if you still have concerns.

Thanks,
Jocelyn

Comment 27

[Boris Zbarsky [:bzbarsky]]

From an implementation standpoint, that sounds fine.

From an API standpoint, what do the unsigned shorts in the event attribute mean?  The description in the wiki says "The enumeration values are either BluetoothManagerAttribute, BluetoothAdapterAttribute, or BluetoothDeviceAttribute" but all of those are string values, not unsigned shorts, right?

Comment 28

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1019544 - Patch1: Revise BluetoothAttributeEvent to fire a single event for multiple attribute changes

Revised BluetoothAttributeEvent to bring a array of changed property names only.

Comment 29

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1019544 - Patch2(v4): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event.

Revised HandlePropertyChanged
* Modify Adapter object before firing onattributechanged

Comment 30

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1019544 - Patch1: Revise BluetoothAttributeEvent to fire a single event for multiple attribute changes

* Revise BluetoothAttributeEvent.webidl
* Revise current BluetoothManager to addressed event change

Comment 31

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1019544 - Patch2(v4): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event. r=btian

* Addressed Boris's review comments

Comment 32

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Boris Zbarsky [:bz] from comment #27)
> From an implementation standpoint, that sounds fine.
> 
> From an API standpoint, what do the unsigned shorts in the event attribute
> mean?  The description in the wiki says "The enumeration values are either
> BluetoothManagerAttribute, BluetoothAdapterAttribute, or
> BluetoothDeviceAttribute" but all of those are string values, not unsigned
> shorts, right?
We have revised the webidl to use DOMStrings instead of unsigned shorts.
However, it seems event generator doesn't support sequence yet, so we use any for now.
The wiki page has also been updated.
https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAttributeEvent#Interface
Thanks.

Comment 33

[Boris Zbarsky [:bzbarsky]]

How about we just fix the event generator to support sequence?  That should be a really easy change, especially on top of bug 1026080.

Comment 34

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Boris Zbarsky [:bz] from comment #33)
> How about we just fix the event generator to support sequence?  That should
> be a really easy change, especially on top of bug 1026080.

Not sure how to fix it right now since we're not quite familiar with event codegen.
We're also interested in fixing it if we have time.
But could we wrap this bug first then do the fix in Bug 1023762 later on?

Comment 35

[Boris Zbarsky [:bzbarsky]]

In case that wasn't clear, I'm offering to fix it if that would avoid you having to use "any".  ;)

Comment 36

[Boris Zbarsky [:bzbarsky]]

Hmm.  Actually... what sort of sequence members will you need in this case?

Comment 37

[Jocelyn Liu [:jocelyn] [:joliu]]

In this case, sequence<DOMString>.
Thanks!

Comment 38

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Boris,

Could we revise to sequence later in bug1015796 after bug 1026080 and bug 1023762 landed?
Sorry to bring up this again, but we have some schedule concern to land this bug first(this week hopefully, still depends on the review result, of course.)
So we can start to involve gaia work.
Is that OK to you?

Thanks,
Jocelyn

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8441881 [details] [diff] [review]
Bug 1019544 - Patch1: Revise BluetoothAttributeEvent to fire a single event for multiple attribute changes

r=me, but please file a bug on changing this IDL, depending on bug 1023762, and reference it in the comments here?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8441884 [details] [diff] [review]
Bug 1019544 - Patch2(v4): Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event. r=btian

>+  JS::Rooted<JS::Value> value(cx, JS::NullValue());

You don't need the JS::NullValue bit here.

r=me

Comment 41

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1019544 - Patch1: Revise BluetoothAttributeEvent to fire a single event for multiple attribute changes. r=bz

Comment 42

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1019544 - Patch2: Fire onattributechanged to gaia while BluetoothAdapter received PropertyChanged event. r=btian, r=bz

Comment 43

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/f01c30be85be
https://hg.mozilla.org/integration/b2g-inbound/rev/618f84dcf244

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f01c30be85be
https://hg.mozilla.org/mozilla-central/rev/618f84dcf244