1016196 - Add Promise support in BluetoothReplyRunnable

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Need to revise BluetoothReplyRunnable because we use Promise instead of DOM request in WebBluetoothAPIv2.

Comment 1

[Ben Tian (inactive)]

Attachment: WIP patch (v1)

Comment 2

[Ben Tian (inactive)]

Attachment: Dev patch (v1)

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1/2 (v1): Add Promise support in BluetoothReplyRunnable

Comment 4

[Ben Tian (inactive)]

Attachment: Patch 2/2(v1): Add logs for Promise in BluetoothReplyRunnable

Comment 5

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

According to Bug1016560, Promise::MaybeReject() only accept nsresult and do not accept nsString anymore.

Comment 6

[Ben Tian (inactive)]

Attachment: Patch 1/2 (v2): Add Promise support in BluetoothReplyRunnable

Rebase on Bug 1016560 to reject with NS_ERROR_DOM_OPERATION_ERR always.

Comment 7

[Ben Tian (inactive)]

Attachment: Patch 2/2 (v2): Add logs for Promise in BluetoothReplyRunnabl

Comment 8

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

Comment on attachment 8440549 [details] [diff] [review]
Patch 1/2 (v2): Add Promise support in BluetoothReplyRunnable

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

r=me with questions answered.

::: dom/bluetooth2/BluetoothReplyRunnable.cpp
@@ +110,5 @@
>  
>    ReleaseMembers();
>    MOZ_ASSERT(!mDOMRequest,
> +             "mDOMRequest is still alive! Deriving class should call "
> +             "BluetoothReplyRunnable::ReleaseMembers()!");

I don't really get this design. In fact we can just set mDOMRequest / mPromise to nullptr right before calling ReleaseMembers() instead of placing assertions here. Do I miss something?

Comment 9

[Ben Tian (inactive)]

(In reply to Eric Chou [:ericchou] [:echou] (GSMA MAE @ Shanghai, 6/10 ~ 6/13) from comment #8)
> ::: dom/bluetooth2/BluetoothReplyRunnable.cpp
> @@ +110,5 @@
> >  
> >    ReleaseMembers();
> >    MOZ_ASSERT(!mDOMRequest,
> > +             "mDOMRequest is still alive! Deriving class should call "
> > +             "BluetoothReplyRunnable::ReleaseMembers()!");
> 
> I don't really get this design. In fact we can just set mDOMRequest /
> mPromise to nullptr right before calling ReleaseMembers() instead of placing
> assertions here. Do I miss something?
[1] still requires |ReleaseMembers| to revoke |BluetoothRequestParent::ReplyRunnable|. Let me check if we can bypass it.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/ipc/BluetoothParent.cpp#62

Comment 10

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #9)
> http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/ipc/
> BluetoothParent.cpp#62
|ReleaseMember| is required as |Revoke| needs a function to reset member variables w/o destructing it. I'll keep the patch unchanged.

Comment 11

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/1b7b1652de28
https://hg.mozilla.org/integration/b2g-inbound/rev/7a428e1bb7c1

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b7b1652de28
https://hg.mozilla.org/mozilla-central/rev/7a428e1bb7c1