1006309 - (webbt-api-manager) Implement BluetoothManager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

* BluetoothManager: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothManager

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1/4 (v1): BluetoothAdapterEvent.webidl and BluetoothAttributeEvent.webidl

TODO: open followup bug for sequence<T> in union type.

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 2/4 (v1): Revise BluetoothManager2.webidl according to refined WebBluetooth API

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 3/4 (v1): BluetoothManager implementation of refined WebBluetooth API

Comment 4

[Ben Tian (inactive)]

Attachment: Patch 4/4 (v1): Add logs specific for refined WebBluetooth API

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8426112 [details] [diff] [review]
Patch 1/4 (v1): BluetoothAdapterEvent.webidl and BluetoothAttributeEvent.webidl

Move this patch to bug 1015090.

Comment 8

[Ben Tian (inactive)]

Attachment: Patch 1/4 (v2): Revise BluetoothManager2.webidl according to refined WebBluetooth API

Changes in [1]:
- attribute defaultAdapter becomes optional since it may be null if no adapter exists.
- add "unknown" in enum BluetoothManagerAttribute as default value of BluetoothAttributeEvent.attr [2]

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothManager#BluetoothManager
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAttributeEvent#attr

Note this patch is based on content/base/src/nsGkAtomList.h change in bug 1006316 comment 7.

Comment 9

[Ben Tian (inactive)]

Attachment: Patch 2/4 (v2): BluetoothManager implementation

Comment 10

[Ben Tian (inactive)]

Attachment: Patch 3/4 (v1): Query adapters list from bluetooth backend

Comment 11

[Ben Tian (inactive)]

Attachment: Patch 4/4 (v2): Add logs specific for refined WebBluetooth API

Comment 12

[Ben Tian (inactive)]

Attachment: Patch 3/4 (v2): Query adapters list from bluetooth backend

Revise BluetoothDBusService to avoid build break on blueZ.

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8428987 [details] [diff] [review]
Patch 1/4 (v2): Revise BluetoothManager2.webidl according to refined WebBluetooth API

Boris, can you help review BluetoothManager IDL change for WebBluetooth refinement?

Two minor changes are made in [1] since last discussion:
- attribute defaultAdapter becomes optional because it's nullptr if no adapter exists.
- add "unknown" in enum BluetoothManagerAttribute as default value of BluetoothAttributeEvent.attr [2]

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothManager#BluetoothManager
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAttributeEvent#attr

Note this patch is based on content/base/src/nsGkAtomList.h change of bug 1006316 comment 7.

Comment 14

[Ben Tian (inactive)]

Attachment: Patch 4/4 (v3): Add logs specific for refined WebBluetooth API

Add log in BluetoothManager constructor.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8428987 [details] [diff] [review]
Patch 1/4 (v2): Revise BluetoothManager2.webidl according to refined WebBluetooth API

r=me.  Long-term we could use Object.observe to see when defaultAdapter changes, but that infrastructure is nowhere close to ready yet.

Comment 16

[Ben Tian (inactive)]

Attachment: [final] Patch 1/4: Revise BluetoothManager2.webidl according to refined WebBluetooth API, r=bz

Rebase on bug 952486 change: replace [Func="Navigator::HasBluetoothSupport"] with [CheckPermissions="bluetooth"].

Comment 17

[Ben Tian (inactive)]

Attachment: Patch 2/4 (v3): BluetoothManager implementation

Rebase on bug 952486 change.

Comment 18

[Ben Tian (inactive)]

Attachment: Patch 3/4 (v3): Query adapters list from bluetooth backend

Rebase on bug 952486 change.

Comment 19

[Ben Tian (inactive)]

Attachment: Patch 4/4 (v4): Add logs specific for refined WebBluetooth API

Rebase on bug 952486 change.

Comment 20

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8429894 [details] [diff] [review]
Patch 2/4 (v3): BluetoothManager implementation

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

Hi Ben,

Overall looks good to me, r=me with nits picked and questions answered. Having said that, I didn't look into JS related implementation in this patch too much since I'm not super familiar with that, so please ask for a dom peer's review/feedback to make sure we didn't do anything wrong. Thanks.

::: dom/bluetooth2/BluetoothManager.cpp
@@ +91,5 @@
> +  // Set this adapter as default adapter if no adapter exists
> +  if (!DefaultAdapterExists()) {
> +    MOZ_ASSERT(mAdapters.Length() == 1);
> +    ReselectDefaultAdapter();
> +   }

super-nit: indentation

@@ +95,5 @@
> +   }
> +}
> +
> +void
> +BluetoothManager::GetAdapters(nsTArray<nsRefPtr<BluetoothAdapter> >& aAdapters)

From the function name "GetAdapters", I would think that the original data contained in aAdapters would be replaced with current adapters(mAdapters). In other words, I think it may be more reasonable to do aAdapters.TruncateLength(0) at the beginning.

@@ +100,5 @@
> +{
> +  uint32_t numAdapters = mAdapters.Length();
> +  for (uint32_t i = 0; i < numAdapters; i++) {
> +    aAdapters.AppendElement(mAdapters[i]);
> +  }

You can use aAdapters.AppendElements(mAdapters) instead of a for loop.

@@ +115,5 @@
>  }
>  
>  void
> +BluetoothManager::HandleAdapterAdded(const BluetoothValue& aValue)
> +{

MOZ_ASSERT(aValue.type() == ....)

@@ +121,5 @@
> +  nsRefPtr<BluetoothAdapter> adapterAdded = mAdapters[mAdapters.Length() - 1];
> +
> +  // Notify gaia of added adapter
> +  BluetoothAdapterEventInit init;
> +  init.mAdapter = adapterAdded;

Can we just do something like |init.mAdapter = mAdapters[mAdapters.Length() - 1]| so that adapterAdded won't be needed?

@@ +143,5 @@
> +      mAdapters.RemoveElementAt(i);
> +
> +      if (defaultAdapterIndex == (int)i) {
> +        ReselectDefaultAdapter();
> +      }

Is it reasonable to escape the loop here?

::: dom/bluetooth2/BluetoothManager.h
@@ +49,5 @@
> +   * and append it into adapters array.
> +   *
> +   * @param aValue [in] Properties array to create BluetoothAdapter object
> +   */
> +  void AppendAdapter(const BluetoothValue& aValue);

AppendAdapter() seems only to be called inside BluetoothManager. Move to private?

@@ +124,5 @@
> +   ***************************************************************************/
> +  /**
> +   * The index of default adapter in the adapters array.
> +   */
> +  int defaultAdapterIndex;

nit: mDefaultAdapterIndex

Comment 21

[Ben Tian (inactive)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #20)
> Comment on attachment 8429894 [details] [diff] [review]
> Patch 2/4 (v3): BluetoothManager implementation
> 
> Review of attachment 8429894 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/bluetooth2/BluetoothManager.h
> @@ +49,5 @@
> > +   * and append it into adapters array.
> > +   *
> > +   * @param aValue [in] Properties array to create BluetoothAdapter object
> > +   */
> > +  void AppendAdapter(const BluetoothValue& aValue);
> 
> AppendAdapter() seems only to be called inside BluetoothManager. Move to
> private?
A runnable in patch 3 calls it outside BluetoothManager so I leave it public here.

Comment 22

[Ben Tian (inactive)]

Attachment: Patch 2/4 (v4): BluetoothManager implementation, r=echou

Boris, can you review JS related implementation in patch 2? Patch 2 implements BluetoothManager based on https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothManager#BluetoothManager

Here are JS related BluetoothManager functions:
- |GetDefaultAdapter|: is called when applications access attribute BluetoothManager.defaultAdapter.
- |GetAdapters|: is called when applications call BluetoothManager.getAdapters to acquire all adapters.
- |DispatchAttributeEvent|: fires attributechanged event to notify application of default adapter change.
- |DispatchAdapterEvent|: fires adapteradded/adapterremoved event to notify application of an added/removed adapter.

Comment 23

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8429895 [details] [diff] [review]
Patch 3/4 (v3): Query adapters list from bluetooth backend

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

LGTM, except that everybody should keep in mind that a DOMRequest will not be a must-have for BluetoothReplyRunnable. You may want to slightly explain in BluetoothReplyRunnable.h.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +849,2 @@
>  
> +  DispatchBluetoothReply(aRunnable, adaptersProperties, errorStr);

You can use EmptyString() for the 3rd parameter so you don't need a new variable.

Comment 24

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8429896 [details] [diff] [review]
Patch 4/4 (v4): Add logs specific for refined WebBluetooth API

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

::: dom/bluetooth2/BluetoothManager.cpp
@@ +228,5 @@
>  BluetoothManager::ReselectDefaultAdapter()
>  {
>    // Select the first of existing/remaining adapters as default adapter
>    defaultAdapterIndex = (mAdapters.Length()) ? 0 : -1;
> +  BT_API2_LOGR("NEW defaultAdapterIndex: %d", defaultAdapterIndex);

Let's print out mAdapters.Length() as well to have more information.

Comment 25

[Ben Tian (inactive)]

Attachment: [final] Patch 3/4: Query adapters list from bluetooth backend, r=echou

Revise based on reviewer's comment.

Comment 26

[Ben Tian (inactive)]

Attachment: [final] Patch 4/4: Add logs specific for refined WebBluetooth API, r=echou

Revise based on reviewer's comment.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8430503 [details] [diff] [review]
Patch 2/4 (v4): BluetoothManager implementation, r=echou

>+++ b/dom/bluetooth2/BluetoothManager.cpp
>+already_AddRefed<BluetoothAdapter>
>+BluetoothManager::GetDefaultAdapter()

This could just return BluetoothAdapter*.

>+  nsRefPtr<BluetoothAdapter> adapter = mAdapters[mDefaultAdapterIndex];
>+  return adapter.forget();

And here you'd do:

  return mAdapters[mDefaultAdapterIndex];

>+BluetoothManager::GetAdapters(nsTArray<nsRefPtr<BluetoothAdapter> >& aAdapters)
>+{
>+  aAdapters.TruncateLength(0);
>+  aAdapters.AppendElements(mAdapters);

Why not replace those two lines with:

  aAdapters = mAdapters;

?

>+BluetoothManager::HandleAdapterAdded(const BluetoothValue& aValue)
>+  init.mAdapter = mAdapters[mAdapters.Length() - 1];

  init.mAdapter = mAdapters.LastElement();

>+BluetoothManager::HandleAdapterRemoved(const BluetoothValue& aValue)
>+      mAdapters.RemoveElementAt(i);

Can there be multiple adapters with the same address in the list?  If not, why are we not able to break even in the not-default case?  If there can, we need a --i here or something so we don't skip the next adapter in the list. 

>+BluetoothManager::ReselectDefaultAdapter()
>+  mDefaultAdapterIndex = (mAdapters.Length()) ? 0 : -1;

Probably clearer as mAdapters.IsEmpty() ? -1 : 0.

>+BluetoothManager::DispatchAdapterEvent(const nsAString& aType,
>+                                       BluetoothAdapterEventInit *aInit)

Why does this take a pointer to the dictionary instead of a |const BluetoothAdapterEventInit& aInit|?

>+  aInit->mBubbles = false;
>+  aInit->mCancelable = false;

Those are the default values, so already set.

>+BluetoothManager::DispatchAttributeEvent()
>+  // Wrap default adapter
>+  nsIScriptContext* sc = GetContextForEventHandlers(&rv);

Let's not do that, especially because this is hiding the thing that _really_ needs to happen here, which is entering the right compartment.  And the right compartment is the compartment of the adapter's GetOwner(), if it has one.  So something like this, assuming this code is mainthread-only:

  AutoJSContext cx;
  JS::Rooted<JS::Value> value(cx, JS::NullValue());
  if (DefaultAdapterExists()) {
    BluetoothAdapter* adapter = mAdapters[mDefaultAdapterIndex];
    nsIGlobalObject* global = adapter->GetParentObject();
    NS_ENSURE_TRUE_VOID(global);
    JS::Rooted<JSObject*> scope = global->GetGlobalJSObject();
    NS_ENSURE_TRUE_VOID(scope);
    JSAutoCompartment ac(cx, scope);
    if (!ToJSValue(cx, adapter, &value)) {
      JS_ClearPendingException(cx);
      return;
    }
  }

>+  BluetoothAttributeEventInit init;

This needs to be a RootedDictionary<BluetoothAttributeEventInit>, since it contains a value that needs tracing.

>+  init.mBubbles = false;
>+  init.mCancelable = false;

Those are already set by default.

>+++ b/dom/bluetooth2/BluetoothManager.h
>+   * Start/Stop listening bluetooth signal.

"listening to"

r=me with the above fixed.

Comment 28

[Ben Tian (inactive)]

Attachment: [final] Patch 2/4: BluetoothManager implementation, r=echou, r=bz

Revise based on bz's comment.

Comment 29

[Ben Tian (inactive)]

Attachment: [final] Patch 3/4: Query adapters list from bluetooth backend, r=echou

Rebase on patch 2 revision.

Comment 30

[Ben Tian (inactive)]

Attachment: [final] Patch 4/4: Add logs specific for refined WebBluetooth API, r=echou

Rebase on patch 2 change.

Comment 31

[Ben Tian (inactive)]

Boris, thanks for you review. Still I have some questions below about choosing correct compartment to enter and the condition to use RootedDictionary<T>.

> >+BluetoothManager::DispatchAttributeEvent()
> >+  // Wrap default adapter
> >+  nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> 
> Let's not do that, especially because this is hiding the thing that _really_
> needs to happen here, which is entering the right compartment.  And the
> right compartment is the compartment of the adapter's GetOwner(), if it has
> one.  So something like this, assuming this code is mainthread-only:
> 
>   AutoJSContext cx;
>   JS::Rooted<JS::Value> value(cx, JS::NullValue());
>   if (DefaultAdapterExists()) {
>     BluetoothAdapter* adapter = mAdapters[mDefaultAdapterIndex];
>     nsIGlobalObject* global = adapter->GetParentObject();
>     NS_ENSURE_TRUE_VOID(global);
>     JS::Rooted<JSObject*> scope = global->GetGlobalJSObject();
>     NS_ENSURE_TRUE_VOID(scope);
>     JSAutoCompartment ac(cx, scope);
>     if (!ToJSValue(cx, adapter, &value)) {
>       JS_ClearPendingException(cx);
>       return;
>     }
>   }

How to choose the correct compartment to enter? Are an object itself and all its attributes in the compartment of the object's GetOwner()? For this example, if BluetoothAdapter.name is wrapped to JSValue, is the compartment to enter also that of adapter's GetOwner()?

> >+  BluetoothAttributeEventInit init;
> 
> This needs to be a RootedDictionary<BluetoothAttributeEventInit>, since it
> contains a value that needs tracing.

So any struct containing a value that needs tracing has to be also rooted, right? Do all EventInit dictionaries containing a type 'any' value to trace have to be rooted?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Good questions!

> How to choose the correct compartment to enter?

In general in Gecko, if an object returns something non-null from GetParentObject() then its actual wrapper will always be created in the compartment determined by that return value.  So in a generic correctness sense the exact compartment you enter for wrapping such an object is not important.  That said, entering the compartment determined by the GetParentObject() before wrapping will allow you to avoid creating an extra cross-compartment wrapper that then just gets thrown away.

For objects that don't have a non-null GetParentObject() (not relevant here, of course, since all event targets do), the compartment you enter determines which global's built-in prototype is used for the wrapper.  Typically this will be decided based on whatever spec is involved.  Not that specs are particularly good about specifying this stuff right now.

> Are an object itself and all its attributes in the compartment of the object's
> GetOwner()?

Typically, but not always.  For new objects this is very likely to be true, though; the exceptions I know are things like iframe.contentDocument, which clearly have to cross compartments.

> For this example, if BluetoothAdapter.name is wrapped to JSValue, is the compartment to
> enter also that of adapter's GetOwner()?

For that case, since strings don't have a built-in concept of which global they belong in (and will just get copied as needed) it's theoretically best to enter the compartment of the event the string will hang off of, which will be the compartment of the initial target the event is created with, so of the BluetoothManager.

The only penalty for getting it wrong in that case is an extra string copy, and I expect in practice the BluetoothManager and BluetoothAdapter are in the same compartment anyway, so you can really use whichever is most convenient.

> So any struct containing a value that needs tracing has to be also rooted, right?

Yes.  Note that we have a static analysis (for non-b2g; about to be turned on for b2g) that enforces this on tbpl, so messing this up will cause test orange, thankfully.

> Do all EventInit dictionaries containing a type 'any' value to trace have to be rooted?

Yes.

Comment 33

[Ben Tian (inactive)]

Thanks for the detailed answers! I'll follow this guideline in later API implementation.

Comment 34

[Ben Tian (inactive)]

Set 'checkin-needed' for patch 1~4.

Note no try is needed as code change of these patches are not built yet.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/14a71f33287c
https://hg.mozilla.org/integration/b2g-inbound/rev/81da11356628
https://hg.mozilla.org/integration/b2g-inbound/rev/cda3f53079c3
https://hg.mozilla.org/integration/b2g-inbound/rev/5183b3c511ef

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14a71f33287c
https://hg.mozilla.org/mozilla-central/rev/81da11356628
https://hg.mozilla.org/mozilla-central/rev/cda3f53079c3
https://hg.mozilla.org/mozilla-central/rev/5183b3c511ef

Comment 37

[Ben Tian (inactive)]

Attachment: Dev patch: Modify gaia for testing BluetoothManager