Closed Bug 1006309 (webbt-api-manager) Opened 6 years ago Closed 6 years ago

Implement BluetoothManager

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S3 (6june)

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

(Whiteboard: [webbt-api])

Attachments

(5 files, 16 obsolete files)

2.34 KB, patch
Details | Diff | Splinter Review
16.10 KB, patch
Details | Diff | Splinter Review
20.01 KB, patch
Details | Diff | Splinter Review
5.96 KB, patch
Details | Diff | Splinter Review
14.88 KB, patch
Details | Diff | Splinter Review
Depends on: 1006319
Depends on: webbt-test-manager
Whiteboard: [webbt-api]
Alias: webbt-api-manager
TODO: open followup bug for sequence<T> in union type.
Assignee: nobody → btian
Duplicate of this bug: 1006306
Duplicate of this bug: 1006319
No longer depends on: 1006319
No longer depends on: webbt-test-manager
Depends on: 1015090
Comment on attachment 8426112 [details] [diff] [review]
Patch 1/4 (v1): BluetoothAdapterEvent.webidl and BluetoothAttributeEvent.webidl

Move this patch to bug 1015090.
Attachment #8426112 - Attachment is obsolete: true
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.
Attachment #8426113 - Attachment is obsolete: true
Attachment #8426114 - Attachment is obsolete: true
Attachment #8428988 - Flags: review?(echou)
Attachment #8428990 - Flags: review?(echou)
Attachment #8426115 - Attachment is obsolete: true
Attachment #8428991 - Flags: review?(echou)
Revise BluetoothDBusService to avoid build break on blueZ.
Attachment #8428990 - Attachment is obsolete: true
Attachment #8428990 - Flags: review?(echou)
Attachment #8429043 - Flags: review?(echou)
Blocks: 1016186
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.
Attachment #8428987 - Flags: review?(bzbarsky)
Add log in BluetoothManager constructor.
Attachment #8428991 - Attachment is obsolete: true
Attachment #8428991 - Flags: review?(echou)
Attachment #8429118 - Flags: review?(echou)
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.
Attachment #8428987 - Flags: review?(bzbarsky) → review+
Rebase on bug 952486 change: replace [Func="Navigator::HasBluetoothSupport"] with [CheckPermissions="bluetooth"].
Attachment #8428987 - Attachment is obsolete: true
Rebase on bug 952486 change.
Attachment #8428988 - Attachment is obsolete: true
Attachment #8428988 - Flags: review?(echou)
Attachment #8429894 - Flags: review?(echou)
Rebase on bug 952486 change.
Attachment #8429043 - Attachment is obsolete: true
Attachment #8429043 - Flags: review?(echou)
Attachment #8429895 - Flags: review?(echou)
Rebase on bug 952486 change.
Attachment #8429118 - Attachment is obsolete: true
Attachment #8429118 - Flags: review?(echou)
Attachment #8429896 - Flags: review?(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
Attachment #8429894 - Flags: review?(echou) → review+
(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.
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.
Attachment #8429894 - Attachment is obsolete: true
Attachment #8430503 - Flags: review?(bzbarsky)
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.
Attachment #8429895 - Flags: review?(echou) → review+
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.
Attachment #8429896 - Flags: review?(echou) → review+
Revise based on reviewer's comment.
Attachment #8429895 - Attachment is obsolete: true
Revise based on reviewer's comment.
Attachment #8429896 - Attachment is obsolete: true
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.
Attachment #8430503 - Flags: review?(bzbarsky) → review+
Revise based on bz's comment.
Attachment #8430503 - Attachment is obsolete: true
Rebase on patch 2 revision.
Attachment #8430618 - Attachment is obsolete: true
Rebase on patch 2 change.
Attachment #8430619 - Attachment is obsolete: true
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Thanks for the detailed answers! I'll follow this guideline in later API implementation.
Set 'checkin-needed' for patch 1~4.

Note no try is needed as code change of these patches are not built yet.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.