1033898 - Implement BluetoothPairingEvent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

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

Comment 1

[Ben Tian (inactive)]

Jocelyn, please help on this bug. Thanks.

Comment 2

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

Attachment: [WIP] Bug 1033898 : Add BluetoothPairingEvent and BluetoothPairingHandle webidls and its class implementation

* Add BluetoothPairingEvent.webidl and BluetoothPairingHandle.webidl
* Implement both webidls, except for SetPasskey and SetPairingConfirmation methods

Comment 3

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

Attachment: [WIP] Bug 1033898 - Patch1(v1): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

* Revise BluetoothPairingEvent implementation
* Implement SetPasskey and SetPairingConfirmation
* Rebase

Comment 4

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

Attachment: [WIP] Bug 1033898 - Patch1(v1): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

I attached a wrong file, this is the latest one.

Comment 5

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

Attachment: [WIP] Bug 1033898 - Patch1(v2): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

* Reject |SetPasskey|/|SetPairingConfirmation| request if the pairing type is not enterpasskeyreq/pairingconfirmationreq.

Comment 6

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

Attachment: Bug 1033898 - Patch1(v1): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

Comment 7

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

Comment on attachment 8455212 [details] [diff] [review]
Bug 1033898 - Patch1(v1): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

Patch Summary:
* Move SetPasskey and SetPairingConfirmation from Adapter to PairingHandle
* Implement PairingEvent and PairingHandle
* Implement SetPasskeyInternal
* Add webidl of PairingEvent and PairingHandle

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8455212 [details] [diff] [review]
Bug 1033898 - Patch1(v1): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

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

Please 1) integrate setPinCode into setPasskey and 2) add BluetoothPairingEvent in [1] ().
[1] http://dxr.mozilla.org/mozilla-central/source/dom/events/test/test_all_synthetic_events.html#47

::: dom/bluetooth2/BluetoothAdapter.h
@@ -105,5 @@
>      Unpair(const nsAString& aDeviceAddress, ErrorResult& aRv);
>    already_AddRefed<DOMRequest>
>      GetPairedDevices(ErrorResult& aRv);
>    already_AddRefed<DOMRequest>
>      SetPinCode(const nsAString& aDeviceAddress, const nsAString& aPinCode,

Remove setPinCode as well. |BluetoothPairingHandle.setPasskey| already covers it.

@@ -118,1 @@
>      SetAuthorization(const nsAString& aDeviceAddress, bool aAllow,

Can we remove this method?

::: dom/bluetooth2/BluetoothPairingHandle.cpp
@@ +64,5 @@
> +    return nullptr;
> +  }
> +  nsRefPtr<Promise> promise = new Promise(global);
> +
> +  BT_ENSURE_TRUE_REJECT(mType.Equals(NS_LITERAL_STRING("enterpasskeyreq")),

Use EqualsLiteral.

@@ +76,5 @@
> +                                   promise,
> +                                   NS_LITERAL_STRING("SetPasskey"));
> +
> +  nsresult rv;
> +  int passkey = nsString(aPasskey).ToInteger(&rv);

We should divert to passkey or pincode here.

@@ +96,5 @@
> +  }
> +  nsRefPtr<Promise> promise = new Promise(global);
> +
> +  BT_ENSURE_TRUE_REJECT(
> +      mType.Equals(NS_LITERAL_STRING("pairingconfirmationreq")),

Ditto.

::: dom/webidl/BluetoothAdapter2.webidl
@@ -123,5 @@
>    DOMRequest getPairedDevices();
>    [NewObject, Throws]
>    DOMRequest getConnectedDevices(unsigned short serviceUuid);
>    [NewObject, Throws]
>    DOMRequest setPinCode(DOMString deviceAddress, DOMString pinCode);

Remove setPinCode.

Comment 9

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

Attachment: Bug 1033898 - Patch1(v2): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

1. Address review comments
2. Replace setPasskey API to setPinCode
3. Change request type name from enterpasskeyreq to enterpincodereq

During offline discussion, we found bluedroid didn't implement set passkey in ssp_reply.
Also, ssp_request with BT_SSP_VARIANT_PASSKEY_ENTRY paring type won't happened on devices with display functionality(including our firefox os mobile phone).
Therefore, we exposed setPinCode instead of setPasskey and made change2 and 3.

Will update our wikipedia after these changes are firmed.

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8459977 [details] [diff] [review]
Bug 1033898 - Patch1(v2): Add BluetoothPairingEvent and BluetoothPairingHandle
webidls and their class implementations

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

Please see my comment below. I think the setting of |BluetoothPairingHandle.mPasskey| is missing.

::: dom/bluetooth2/BluetoothPairingEvent.cpp
@@ +57,5 @@
> +  nsCOMPtr<nsPIDOMWindow> window = aTarget->GetOwner();
> +
> +  nsString addr;
> +  aDevice->GetAddress(addr);
> +  event->mHandle = BluetoothPairingHandle::Create(window,

Pass |aTarget->GetOwner| directly since |window| is not used elsewhere.

::: dom/bluetooth2/BluetoothPairingHandle.cpp
@@ +62,5 @@
> +  if (!global) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }
> +  nsRefPtr<Promise> promise = new Promise(global);

Please revise promise creation according to bug 1040263.

@@ +89,5 @@
> +  if (!global) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }
> +  nsRefPtr<Promise> promise = new Promise(global);

Ditto.

::: dom/bluetooth2/BluetoothPairingHandle.h
@@ +40,5 @@
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;
> +
> +  void GetPasskey(nsString& aPasskey) const
> +  {
> +    aPasskey = mPasskey;

How to set |mPasskey|?

Comment 11

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

Attachment: Bug 1033898 - Patch1(v3): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

* Add passkey parameters
* rebase and revise promise

Thanks for pointing the passkey thing out, Ben!

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8460117 [details] [diff] [review]
Bug 1033898 - Patch1(v3): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations

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

r=me with comment addressed. Please also request bz review webidl part. Thanks.

::: dom/bluetooth2/BluetoothPairingHandle.cpp
@@ +36,5 @@
> +{
> +  MOZ_ASSERT(aOwner && aDeviceAddress && aType);
> +
> +  if (aType.EqualsLiteral("displaypasskeyreq") ||
> +      aType.EqualsLiteral("pairingconfirmationreq")) {

Define "displaypasskeyreq", "pairingconfirmationreq", "enterpincodereq", and "pairingconsentreq" in BluetoothCommon.h

@@ +37,5 @@
> +  MOZ_ASSERT(aOwner && aDeviceAddress && aType);
> +
> +  if (aType.EqualsLiteral("displaypasskeyreq") ||
> +      aType.EqualsLiteral("pairingconfirmationreq")) {
> +    MOZ_ASSERT(aPasskey);

MOZ_ASSERT(!aPasskey.IsEmpty());

@@ +40,5 @@
> +      aType.EqualsLiteral("pairingconfirmationreq")) {
> +    MOZ_ASSERT(aPasskey);
> +    mPasskey = aPasskey;
> +  } else {
> +    mPasskey = nullptr;

Add MOZ_ASSERT(aPasskey.IsEmpty());

Comment 13

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

Attachment: Bug 1033898 - Patch1(v4): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations. r=btian

Thanks for your time, Ben.

Comment 14

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

Comment on attachment 8460182 [details] [diff] [review]
Bug 1033898 - Patch1(v4): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations. r=btian

Hi Boris,

This patch defines webidls of BluetoothPairingEvent[1], BluetoothPairingHandle[2] and implement them.
BluetoothPairingEvent will be used later in Bug 1036228 to notify Gaia for four kinds of pairing requests.
And the pairing handle is for Gaia to access necessary property and methods for respective pairing types.[3]

The implementation has been review+ by one of our bluetooth module peers.
Please also help to review the webidl and the DOM-facing part.

Thanks,
Jocelyn

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#BluetoothPairingEvent
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#BluetoothPairingHandle
[3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#handle

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460182 [details] [diff] [review]
Bug 1033898 - Patch1(v4): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations. r=btian

As long as you're moving BluetoothClassOfDevice in Bindings.conf, can you please put it in the right place in alphabetical order?  It needs to at least come before BluetoothDevice.

>+BluetoothPairingEvent::BluetoothPairingEvent(EventTarget* aTarget,
>+                                             nsPresContext* aPresContext,
>+                                             WidgetEvent* aEvent)

The second and third args are unused, right?  Why are they needed?

>+BluetoothPairingEvent::Constructor(DOMEventTargetHelper* aTarget,

This seems to be unused.  Why is it needed?  If it _is_ needed, why is it named like a WebIDL constructor, when it's nothing of the sort?

This event is a bit weird in that it doesn't actually store/return its constructor arguments as its own members.  That's not how most DOM events work...

>+  MOZ_ASSERT(aTarget && aDevice && aType);

I assume you never compiled this code in a debug build?  aType shouldn't be convertible to boolean.  Please fix this to test what it really means to be testing, if this code stays.

>+                         nullptr /* Bubbles */,
>+                         nullptr /* Cancelable */);

Why are we passing nullptr when we mean false?  Just pass false.  Again, if this code stays.

>+namespace mozilla {
>+  class DOMEventTargetHelper;
>+  class ErrorResult;
>+}

Please don't indent things inside a namespace block.

>+  MOZ_ASSERT(aOwner && aDeviceAddress && aType);

Again, please fix this to compile.

>+    mPasskey = nullptr;

Why?  It's already empty.

>+namespace mozilla {
>+  class ErrorResult;
>+namespace dom {
>+  class Promise;
>+}

Again, no indenting in namespace blocks.


>+nsresult
> BluetoothServiceBluedroid::SetPinCodeInternal(

Why nsresult if it always returns NS_OK?  Why not void?

>+nsresult
> BluetoothServiceBluedroid::SetPasskeyInternal(

Likewise.

>+nsresult
> BluetoothServiceBluedroid::SetPairingConfirmationInternal(

And here.

And same for BluetoothDBusService and BluetoothServiceChildProcess.

>+  readonly attribute DOMString? passkey;

Is this ever actually null?  I rather doubt it, given the C++....

I'd appreciate an interdiff, not just an updated patch.

Comment 16

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

Thank you, Boris.
Please see my comments below.
(In reply to Boris Zbarsky [:bz] from comment #15)
> Comment on attachment 8460182 [details] [diff] [review]
> Bug 1033898 - Patch1(v4): Add BluetoothPairingEvent and
> BluetoothPairingHandle webidls and their class implementations. r=btian
> 
> >+BluetoothPairingEvent::Constructor(DOMEventTargetHelper* aTarget,
> 
> This seems to be unused.  Why is it needed?  If it _is_ needed, why is it
> named like a WebIDL constructor, when it's nothing of the sort?
>
> This event is a bit weird in that it doesn't actually store/return its
> constructor arguments as its own members.  That's not how most DOM events
> work...
> 
This will be used by BluetoothPairingRequestListeningHandle(https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingRequestListeningHandle#Interface), which will be implemented by Bug 1036228.
We will use this handle to call this event constructor and dispatch the returned event.

In Constructor, it saves aDevice as its own member.
aPasskey and device address saved in aDevice are used for creating pairing handle, then saved it as its own member.
I left the handle creating part in |BluetoothPairingEvent::Constructor| to save some works from callers.

However, if it's weird based on our code base, how about I change the signature into

BluetoothPairingEvent::Constructor(DOMEventTargetHelper* aTarget, BluetoothDevice* aDevice, const nsAString& aType, BluetoothPairingHandle* aHandle)?

What do you think?

Besides, may I ask is it necessary to check the return value of |Event::InitEvent|?
You can see that I return this value to caller in this patch.
But I can see a lot of codes in our m-c that didn't check this at all.
Also, if I use event codegen for BluetoothPairingEvent, it also won't do the check.
Which way is better? Could I skip this check?

> >+    mPasskey = nullptr;
> 
> Why?  It's already empty.
>  
Because we originally designed as it will be nullptr for cases that doesn't need passkey.
(https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#passkey)
Just as our webidl, mPasskey is marked as DOMString?.

> 
> >+  readonly attribute DOMString? passkey;
> 
> Is this ever actually null?  I rather doubt it, given the C++....
> 
Sometimes, see the above comment.
So, would you suggest we just use empty string for cases that passkey wasn't needed?

> >+nsresult
> > BluetoothServiceBluedroid::SetPinCodeInternal(
> 
> Why nsresult if it always returns NS_OK?  Why not void?
> 
> >+nsresult
> > BluetoothServiceBluedroid::SetPasskeyInternal(
> 
> Likewise.
> 
> >+nsresult
> > BluetoothServiceBluedroid::SetPairingConfirmationInternal(
> 
> And here.
> 
> And same for BluetoothDBusService and BluetoothServiceChildProcess.
Just following the convention in our bluetooth module here.
If there's some logical check, we always make it a nsresult or bool function.
I admit it's weird that sometimes we just return NS_OK even there's some error(which we shouldn't), and that makes it looks like it's redundant to return nsresult/bool.
We plan to have a overall refinement on all of these *Internal functions, and will file a followup bug later for it.
For this bug, we can left them unchanged(using bool), or just using nsresult since we will make it as nsresult in the future.
Is any of these two ways make sense to you?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> This will be used by BluetoothPairingRequestListeningHandle

Then it probably makes the most sense to add it in the bug that adds 
This will be used by BluetoothPairingRequestListeningHandle, for what it's worth.

> aPasskey and device address saved in aDevice are used for creating pairing
> handle, then saved it as its own member.

Right.  That's not how most events work.

But all this is somewhat moot, because this is not a script-exposed Web IDL constructor.  Please just name it something other than "Constructor".  "Create" is a good candidate.

> Besides, may I ask is it necessary to check the return value of
> |Event::InitEvent|?

Looks like this always returns NS_OK.  Please file a followup DOM:Events bug to rejigger the APIs to make this clearer (e.g. by making the XPCOM method noscript and notxpcom, returning void, and having the Web IDL method not take an ErrorResult.

> Also, if I use event codegen for BluetoothPairingEvent

Which should generally be preferred, by the way.  Why aren't we doing it here?

> Because we originally designed as it will be nullptr

Assigning "nullptr" to an XPCOM string doesn't make it "null" in DOM terms.  It just sets it to the empty string.  If you actually _want_ it to be null in some cases, this is not the right way to do it.

This is why we should have tests...

> Is any of these two ways make sense to you?

I think it makes the most sense to leave the functions void until they can actually fail, _then_ to change the signatures.

Because otherwise, you'll have people who look at the function, see it always returns NS_OK, and write code that ignores the return value.  So when you change it to not always return NS_OK those callers will continue ignoring the return value.  We have lots of instances of this happening in the past, and we shouldn't set this code up to fail in the same way.

Comment 18

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

(In reply to Boris Zbarsky [:bz] from comment #17)
> > This will be used by BluetoothPairingRequestListeningHandle
> 
> Then it probably makes the most sense to add it in the bug that adds 
> This will be used by BluetoothPairingRequestListeningHandle, for what it's
> worth.
> 
Sorry, didn't quite get what you were saying here.
Move whole |Create| function part to the other bug or what?

> > aPasskey and device address saved in aDevice are used for creating pairing
> > handle, then saved it as its own member.
> 
> Right.  That's not how most events work.
> 
> But all this is somewhat moot, because this is not a script-exposed Web IDL
> constructor.  Please just name it something other than "Constructor". 
> "Create" is a good candidate.
> 
> > Besides, may I ask is it necessary to check the return value of
> > |Event::InitEvent|?
> 
> Looks like this always returns NS_OK.  Please file a followup DOM:Events bug
> to rejigger the APIs to make this clearer (e.g. by making the XPCOM method
> noscript and notxpcom, returning void, and having the Web IDL method not
> take an ErrorResult.
> 
> > Also, if I use event codegen for BluetoothPairingEvent
> 
> Which should generally be preferred, by the way.  Why aren't we doing it
> here?
> 
Because I did the handle creating part in |BluetoothPairingEvent::Create(Constructor)|.
I can't do this if I use event codegen, right?

> > Because we originally designed as it will be nullptr
> 
> Assigning "nullptr" to an XPCOM string doesn't make it "null" in DOM terms. 
> It just sets it to the empty string.  If you actually _want_ it to be null
> in some cases, this is not the right way to do it.
> 
> This is why we should have tests...
> 
Alright... I totally misunderstood this, sorry about that.
Then I think empty string is probably much easier. 
> > Is any of these two ways make sense to you?
> 
> I think it makes the most sense to leave the functions void until they can
> actually fail, _then_ to change the signatures.
> 
> Because otherwise, you'll have people who look at the function, see it
> always returns NS_OK, and write code that ignores the return value.  So when
> you change it to not always return NS_OK those callers will continue
> ignoring the return value.  We have lots of instances of this happening in
> the past, and we shouldn't set this code up to fail in the same way.
Got your point. Thanks for the detail explanation.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Move whole |Create| function part to the other bug or what?

That's what I would have done, yes.  That way you don't have unused code sitting around in the tree.

If the other bug is going to land pretty soon, might not be worth worrying about it.

> I can't do this if I use event codegen, right?

Well, yes.  On the other hand it also means you're introducing an event without a WeIDL constructor, which is not very desirable.  I'm not sure how much we care, since I expect this stuff will never be standardized...

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

We should definitely use event codegen for BluetoothPairingEvent and have constructor as 
every other event interface should have too.
(Some legacy event interface may still miss ctor)

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #20)
> We should definitely use event codegen for BluetoothPairingEvent and have
> constructor as 
> every other event interface should have too.
> (Some legacy event interface may still miss ctor)

Thanks for your prompt reply, and thanks Boris for bringing this up.
I will just leave the handle creating part for callers, and use event codegen for BluetoothPairingEvent since it's the right thing.

But it seems there are many events that doesn't use event codegen in our code base.
It's hard to find out what the real guideline is by looking around dxr.
Do we have to use event codegen no matter what?
If it doesn't, what's the rule here we should follow for implementing that?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

The "rule" is: use event codegen whenever possible. That makes it less likely to have errors in the
code and reviewing becomes easier, and also writing code is simpler.

We can't generate all sort of events, especially when there is UIEvent somewhere in the
interface hierarchy, or if attribute types are something unusual (which may also hint that the
design for the event interface is too complicated).

Comment 23

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

Attachment: Bug 1033898 - Patch1(v5): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations. r=btian

* Rearrange things in Bindings.conf
* Remove BluetoothPairingEvent.cpp/h and use event codegen instead
* Fix incorrect MOZ_ASSERT usages
* mPasskey will be an empty string instead of null for pairing requests that doesn't need to use passkey.
* void for |SetPinCodeInternal|, |SetPasskeyInternal|, and |SetPairingConfirmationInternal|

Comment 24

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

Attachment: interdiff between patch1(v4) and patch1(v5)

This is the hg diff output between patch1(v4) and patch1(v5) since bugzilla's interdiff on them doesn't work correctly.

Comment 25

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

Comment on attachment 8463463 [details] [diff] [review]
Bug 1033898 - Patch1(v5): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations. r=btian

Hi Boris,

I have addresses your review comments and upload attachment 8463463 [details] [diff] [review] as patch1(v5).
Interdiff is also provided in attachment 8463469 [details] [diff] [review], but one thing is missed in the interdiff.
In dom/webidl/BluetoothPairingHandle.webidl, This attribute is "an" empty string... (It's in patch1(v5) but missed in interdiff.)
Could you review it again?

Thanks,
Jocelyn

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8463463 [details] [diff] [review]
Bug 1033898 - Patch1(v5): Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations. r=btian

r=me.  Thank you for the updates, and especially the interdiff!

Comment 27

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

Attachment: [Final] Bug 1033898 - Patch1: Add BluetoothPairingEvent and BluetoothPairingHandle webidls and their class implementations. r=btian, r=bz

Thank you for your time, Boris.

Comment 28

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

no try server link since dom/bluetooth2 won't be build by try server.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b38e9ad6d3b3

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b38e9ad6d3b3