969218 - B2G RIL: using promise for telephony.dial() and dialEmergency()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

In 889737, we are going to unify telephony.dial() and mobileconnection.sendMMI(). The unified function will return a promise object and it could be further resolved as a telephonyCall or MMI code result.

The purpose of this bug is for the first step -- dial() interface change. Instead of returning a telephonyCall directly (current design), dial() and dialEmergency() will return a promise. Later it could be resolved as a telephonyCall or be rejected with a error cause.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Use promise for dial and dialEmergency - webidl

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Ref: sample code

Comment 3

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8372020 [details] [diff] [review]
Part 1: Use promise for dial and dialEmergency - webidl

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

Looks good. Thank you!

::: dom/webidl/Telephony.webidl
@@ +15,5 @@
>     * simply the index of a service. Get number of services by acquiring
>     * |navigator.mozMobileConnections.length|.
>     */
>  
>    [Throws]

Please add a comment: // Promise<TelephonyCall>

@@ +21,3 @@
>  
>    [Throws]
> +  Promise dialEmergency(DOMString number, optional unsigned long serviceId);

ditto.

Comment 4

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

nominate it as it blocks 1.4+ bugs.

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 1: Use promise for dial and dialEmergency - webidl

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Modify tests

Modify all test cases to use the new promise form.

Comment 7

[Michael Schwartz [:m4]]

Comment on attachment 8372151 [details] [diff] [review]
[final] Part 1: Use promise for dial and dialEmergency - webidl

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

::: dom/webidl/Telephony.webidl
@@ +15,5 @@
>     * simply the index of a service. Get number of services by acquiring
>     * |navigator.mozMobileConnections.length|.
>     */
>  
> +  // Promise<TelephonyCall>

Can't this return a Promise to a call or supplementary service?

Comment 8

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(In reply to Michael Schwartz [:m4] from comment #7)
> Comment on attachment 8372151 [details] [diff] [review]
> [final] Part 1: Use promise for dial and dialEmergency - webidl
> 
> Review of attachment 8372151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Telephony.webidl
> @@ +15,5 @@
> >     * simply the index of a service. Get number of services by acquiring
> >     * |navigator.mozMobileConnections.length|.
> >     */
> >  
> > +  // Promise<TelephonyCall>
> 
> Can't this return a Promise to a call or supplementary service?

This bug is only the first step, returning a Call object only. Then in bug 881174 we will have mmi support. At that time it returns supplementary service as well.

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3: internal interface

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4: dom

Comment 11

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5: gonk provider

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 6: ipc

Comment 13

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#2: dom

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 6#2: ipc

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Ref: testing gaia

Comment 16

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8373273 [details] [diff] [review]
Part 3: internal interface

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

::: dom/telephony/nsIGonkTelephonyProvider.idl
@@ +9,5 @@
>  #define GONK_TELEPHONY_PROVIDER_CONTRACTID \
>          "@mozilla.org/telephony/gonktelephonyprovider;1"
>  %}
>  
> +[scriptable, uuid(703d81c4-e352-4727-be70-5416c05e49e8)]

Anything missed @@?

::: dom/telephony/nsITelephonyProvider.idl
@@ +151,5 @@
> +  /**
> +   * Called when nsITelephonyProvider completed a call request.
> +   */
> +  void notifyDialCallComplete();
> +};

Sorry that I don't get the reason of having notifyDialCallStart(). Could you explain it?

Looks both notifyDialError() and notifyDialCallComplete() imply a call request is completed. The difference is for one request succeeded and for the other request failed.

How about we name them as below to make the API clearer:
notifyDial() and notifyDialFailed()

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> Comment on attachment 8373273 [details] [diff] [review]
> Part 3: internal interface
> 
> Review of attachment 8373273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/nsIGonkTelephonyProvider.idl
> @@ +9,5 @@
> >  #define GONK_TELEPHONY_PROVIDER_CONTRACTID \
> >          "@mozilla.org/telephony/gonktelephonyprovider;1"
> >  %}
> >  
> > +[scriptable, uuid(703d81c4-e352-4727-be70-5416c05e49e8)]
> 
> Anything missed @@?

Do we have to change the uuid of a interface that is inherited from a modified interface?
nsITelephonyProvider is modified and nsIGonkTelephonyProvider is inherited from nsITelephonyProvider.

> ::: dom/telephony/nsITelephonyProvider.idl
> @@ +151,5 @@
> > +  /**
> > +   * Called when nsITelephonyProvider completed a call request.
> > +   */
> > +  void notifyDialCallComplete();
> > +};
> 
> Sorry that I don't get the reason of having notifyDialCallStart(). Could you
> explain it?
> 
> Looks both notifyDialError() and notifyDialCallComplete() imply a call
> request is completed. The difference is for one request succeeded and for
> the other request failed.
> 
> How about we name them as below to make the API clearer:
> notifyDial() and notifyDialFailed()

NotifyXXXStart() will be called before send the request to modem.
NotifyXXXComplete() will be called after modem completed the request.

Furthermore, I plan to extend them to notifyDial'Call' and notifyDial'MMI'. So, there will be
notifyDialError, ntifyDialCallStart, notifyDialCallComplte, notifyDialMMIStart, notifyDialMMIComplte.

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#2: gonk provider

Comment 19

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8372152 [details] [diff] [review]
Part 2: Modify tests

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

::: dom/telephony/test/marionette/test_emergency.js
@@ +25,5 @@
> +    outgoing.onalerting = function onalerting(event) {
> +      log("Received 'onalerting' call event.");
> +      is(outgoing, event.call);
> +      is(outgoing.state, "alerting");
> +

Let's add one more check:

is(outgoing.emergency, true);

::: dom/telephony/test/marionette/test_outgoing_badNumber.js
@@ +12,5 @@
>    log("Make an outgoing call to an invalid number.");
>  
> +  // Note: The number is valid from the view of phone and the call could be
> +  // dialed out successfully. However, it will later receive the BadNumberError
> +  // from network side.

Nice!

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3#2: internal interface

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#3: dom

Comment 22

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#3: gonk provider

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 6#3: ipc

Comment 24

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8374737 [details] [diff] [review]
Part 3#2: internal interface

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

Looks good!

::: dom/telephony/nsITelephonyProvider.idl
@@ +135,5 @@
> +[scriptable, uuid(d4698fe9-5cbe-4e7e-b7a1-876091f4479f)]
> +interface nsITelephonyCallback : nsISupports
> +{
> +  /**
> +   * Called when dial error.

Called when a dial request fails.

@@ +143,5 @@
> +   */
> +  void notifyDialError(in AString error);
> +
> +  /**
> +   * Called when dial success.

Called when a dial request succeeds.

Comment 25

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8374737 [details] [diff] [review]
Part 3#2: internal interface

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

::: dom/telephony/nsITelephonyProvider.idl
@@ +140,5 @@
> +   *
> +   * @param error
> +   *        Error from RIL.
> +   */
> +  void notifyDialError(in AString error);

Since we need 'jscontext' in .cpp, we need

[implicit_jscontext]
void notifyDialError()

@@ +145,5 @@
> +
> +  /**
> +   * Called when dial success.
> +   */
> +  void notifyDialSuccess();

ditto.

Comment 26

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8374738 [details] [diff] [review]
Part 4#3: dom

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

We need a DOM peer's review for this part though I am very happy to provide feedbacks.

::: dom/telephony/Telephony.cpp
@@ +64,5 @@
>  
> +class Telephony::Callback : public nsITelephonyCallback
> +{
> +  Telephony* mTelephony;
> +  JSContext* mCx;

With [implicit_jscontext] on nsITelephonyCallback, we don't need mCx.

@@ +70,5 @@
> +  uint32_t mServiceId;
> +  nsString mNumber;
> +
> +public:
> +  NS_DECL_ISUPPORTS

Use the macro NS_DECL_NSITELEPHONYCALLBACK

@@ +91,5 @@
> +
> +  NS_IMETHODIMP
> +  NotifyDialSuccess() {
> +    nsRefPtr<TelephonyCall> call =
> +        mTelephony->CreateNewDialingCall(mServiceId, mNumber);

nit: indention

@@ +98,5 @@
> +    for (uint32_t i = 0; i < gTelephonyList->Length(); i++) {
> +      Telephony*& telephony = gTelephonyList->ElementAt(i);
> +      if (telephony != mTelephony) {
> +        nsRefPtr<Telephony> kungFuDeathGrip = telephony;
> +        telephony->NoteDialedCallFromOtherInstance(mServiceId, mNumber);

Do we still need 'NoteDialedCallFromOtherInstance' once we switch to Promise? Can't other instances within the same process listen to 'telephonyListener::callStateChanged' as those in different processes?

@@ +285,5 @@
>            mActiveCall->ServiceId() == aCall->ServiceId());
>  }
>  
> +void
> +Telephony::RejectPromiseByString(JSContext* aCx, Promise* aPromise, const nsAString& aString) {

Do we really need the argument aCx?

@@ +315,2 @@
>    // We only support one outgoing call at a time.
>    if (HasDialingCall()) {

Considering OOP and multi-instances cases, I am thinking we should move HasDialingCall() to TelephonyProvider.js. How do you think?

::: dom/telephony/Telephony.h
@@ +8,5 @@
>  #define mozilla_dom_telephony_telephony_h__
>  
>  #include "mozilla/dom/BindingDeclarations.h"
>  #include "mozilla/dom/telephony/TelephonyCommon.h"
> +#include "mozilla/dom/Promise.h"

Arrange in alphabetical order，please

Comment 27

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8374740 [details] [diff] [review]
Part 5#3: gonk provider

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

This part looks good.

Comment 28

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8374742 [details] [diff] [review]
Part 6#3: ipc

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

::: dom/telephony/ipc/PTelephonyRequest.ipdl
@@ +27,2 @@
>     */
>    __delete__();

I am thinking we need one more argument to specify which request the response is coming for, so that no matter which request is completed we are able to _delete_() correctly.

Taking sms as an example,
http://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/ipc/PSmsRequest.ipdl?from=PSmsRequest.ipdl#19

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +128,5 @@
> +  if (mListener) {
> +    // EnumerateCallsRequest
> +    mListener->EnumerateCallStateComplete();
> +  } else {
> +    // DialRequest

Seems not done yet?

::: dom/telephony/ipc/TelephonyParent.cpp
@@ +503,5 @@
> +TelephonyRequestParent::NotifyDialError(const nsAString& aError)
> +{
> +  NS_ENSURE_TRUE(!mActorDestroyed, NS_ERROR_FAILURE);
> +
> +  return SendNotifyDialError(nsString(aError)) ? NS_OK : NS_ERROR_FAILURE;

We should Send_delete_ once a request is completed.

@@ +511,5 @@
> +TelephonyRequestParent::NotifyDialSuccess()
> +{
> +  NS_ENSURE_TRUE(!mActorDestroyed, NS_ERROR_FAILURE);
> +
> +  return SendNotifyDialSuccess() ? NS_OK : NS_ERROR_FAILURE;

ditto.

Comment 29

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #26)

> @@ +285,5 @@
> >            mActiveCall->ServiceId() == aCall->ServiceId());
> >  }
> >  
> > +void
> > +Telephony::RejectPromiseByString(JSContext* aCx, Promise* aPromise, const nsAString& aString) {
> 
> Do we really need the argument aCx?
> 

Need context to build JS::Value and handle the promise.

> @@ +315,2 @@
> >    // We only support one outgoing call at a time.
> >    if (HasDialingCall()) {
> 
> Considering OOP and multi-instances cases, I am thinking we should move
> HasDialingCall() to TelephonyProvider.js. How do you think?
> 

I think that we could keep the check here. This is for checking whether there is already a dialing call for this telephony instance.

Then, we could have a further check in TelephonyProvider. It could return the error when it found that another dialing call exists.

Comment 30

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #29)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #26)
> 
> > @@ +285,5 @@
> > >            mActiveCall->ServiceId() == aCall->ServiceId());
> > >  }
> > >  
> > > +void
> > > +Telephony::RejectPromiseByString(JSContext* aCx, Promise* aPromise, const nsAString& aString) {
> > 
> > Do we really need the argument aCx?
> > 
> 
> Need context to build JS::Value and handle the promise.
> 

But we can use "JSContext *cx = nsContentUtils::GetCurrentJSContext();" so that we don't need that argument.

> > @@ +315,2 @@
> > >    // We only support one outgoing call at a time.
> > >    if (HasDialingCall()) {
> > 
> > Considering OOP and multi-instances cases, I am thinking we should move
> > HasDialingCall() to TelephonyProvider.js. How do you think?
> > 
> 
> I think that we could keep the check here. This is for checking whether
> there is already a dialing call for this telephony instance.
> 
> Then, we could have a further check in TelephonyProvider. It could return
> the error when it found that another dialing call exists.
That sounds good!

Comment 31

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Modify tests

Modify a new test case test_outgoing_answer_radio_off.

Comment 32

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3#3: internal interface

Comment 33

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#4: dom

Comment 34

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#4: gonk provider

Add "already dialing" check.

Comment 35

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 6#4: ipc

Comment 36

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8376147 [details] [diff] [review]
Part 3#3: internal interface

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

Looks good.

Comment 37

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8376148 [details] [diff] [review]
Part 4#4: dom

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

::: dom/telephony/Telephony.cpp
@@ +63,5 @@
>  };
>  
> +class Telephony::Callback : public nsITelephonyCallback
> +{
> +  Telephony* mTelephony;

The Telephony object could theoretically go away between the time that callback is dispatched and the time it runs. So, I am thinking we should make this a nsRefPtr. That way we know the Telephony won't die.

::: dom/telephony/Telephony.h
@@ +181,5 @@
>    already_AddRefed<TelephonyCall>
>    CreateNewDialingCall(uint32_t aServiceId, const nsAString& aNumber);
>  
>    void
>    NoteDialedCallFromOtherInstance(uint32_t aServiceId,

Remove it as it's not referenced.

Comment 38

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8376151 [details] [diff] [review]
Part 6#4: ipc

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

I am not a DOM/IPC peer so cannot do a formal review, though I am happy to provide feedbacks.

::: dom/telephony/ipc/PTelephonyRequest.ipdl
@@ +41,2 @@
>    /**
> +   * Sent when the asynchronous request has completed.

It looks __delete__ covers "NotifyDialError" and "NotifyDialSuccess". Do we really need these two in addition to __delete__()?

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +167,5 @@
> +
> +bool
> +TelephonyRequestChild::RecvNotifyDialSuccess()
> +{
> +  MOZ_ASSERT(mCallback);

nit: add a new line here

Comment 39

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8376144 [details] [diff] [review]
Part 2#2: Modify tests

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

Assuming test_outgoing_answer_radio_off.js is the only change compared with #1, r+

Comment 40

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #38)
> Comment on attachment 8376151 [details] [diff] [review]
> Part 6#4: ipc
> 
> Review of attachment 8376151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not a DOM/IPC peer so cannot do a formal review, though I am happy to
> provide feedbacks.
> 
> ::: dom/telephony/ipc/PTelephonyRequest.ipdl
> @@ +41,2 @@
> >    /**
> > +   * Sent when the asynchronous request has completed.
> 
> It looks __delete__ covers "NotifyDialError" and "NotifyDialSuccess". Do we
> really need these two in addition to __delete__()?

Yes, we could carry all the params in __delete__(). However, in the future, we might have something like NotifyStartMMI(). We will only send the notification but not the destructor. In this case, we might still need to introduce a new NotifyXXX() function which could not be achieved by __delete__().

> ::: dom/telephony/ipc/TelephonyChild.cpp
> @@ +167,5 @@
> > +
> > +bool
> > +TelephonyRequestChild::RecvNotifyDialSuccess()
> > +{
> > +  MOZ_ASSERT(mCallback);
> 
> nit: add a new line here

Comment 41

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#5: dom

Hi Kyle,

Could you help review the modification in telephony dom and IPC part? Hsinyi had already given some feedback for the patch.

1. We are now returning a Promise for telephony.dial(). Related interface change is in Part 1 patch.
2. dial() in telephony provider supports a callback. We could get the successful/error result of the current dial request.

Comment 42

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 6#5: ipc

Comment 43

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Szu-Yu Chen [:aknow] from comment #41)
> Created attachment 8376950 [details] [diff] [review]
> Part 4#5: dom
> 
> Hi Kyle,
> 
> Could you help review the modification in telephony dom and IPC part? Hsinyi
> had already given some feedback for the patch.
> 
> 1. We are now returning a Promise for telephony.dial(). Related interface
> change is in Part 1 patch.
> 2. dial() in telephony provider supports a callback. We could get the
> successful/error result of the current dial request.

And the related interface change for item 2 is in part 3 patch.

Comment 44

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 8376950 [details] [diff] [review]
Part 4#5: dom

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

We shouldn't have |notifyError| being called with aCallIndex == -1 anymore. Please remember to remove corresponding code in Telephony.cpp.

Comment 45

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8376950 [details] [diff] [review]
Part 4#5: dom

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

Drive by comments:
Telephony::callback should be cycle collected since it holds a strong reference to Promise, unless I'm missing something (it's odd that you are seeing the members to null in the destructor).

Iirc already_Addrefed can be used with forward declared classes.

Comment 46

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #45)
> Drive by comments:
> Telephony::callback should be cycle collected since it holds a strong
> reference to Promise, unless I'm missing something (it's odd that you are
> seeing the members to null in the destructor).

It's not obvious to me that it needs to be CCd.  In particular nsITelephonyCallback is held by the IPDL actor, which has a well-defined lifetime.

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8376950 [details] [diff] [review]
Part 4#5: dom

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

The JSAPI handling around calling MaybeResolve/Reject is wrong.  Rather than ask you to fix it I wrote some helpers in bug 947120 to handle your use cases.  Please rebase your patch on top of those.

::: dom/telephony/Telephony.cpp
@@ +80,5 @@
> +
> +  virtual ~Callback() {
> +    mTelephony = nullptr;
> +    mPromise = nullptr;
> +  }

There's no need to write this out explicitly.  The compiler generated destructor will call ~nsRefPtr on both which will do this.

@@ +100,5 @@
> +    }
> +
> +    JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx));
> +    JS::Rooted<JS::Value> result(cx, OBJECT_TO_JSVAL(call->WrapObject(cx, global)));
> +    mPromise->MaybeResolve(cx, result);

This can be replaced by a MaybeResolve(call); with the patch I mentioned.

@@ +280,5 @@
>            mActiveCall->ServiceId() == aCall->ServiceId());
>  }
>  
> +void
> +Telephony::RejectPromiseByString(Promise* aPromise, const nsAString& aString) {

And you could probably just get rid of this and call promise->MaybeReject(NS_LITERAL_STRING("foo")); directly

@@ +320,3 @@
>    if (NS_FAILED(rv)) {
> +    RejectPromiseByString(promise, NS_LITERAL_STRING("InvalidStateError"));
> +    return promise.forget();

You are dropping the information in rv here (by converting every type of error to InvalidStateError).  Is that ok?

Comment 48

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #47)

> @@ +320,3 @@
> >    if (NS_FAILED(rv)) {
> > +    RejectPromiseByString(promise, NS_LITERAL_STRING("InvalidStateError"));
> > +    return promise.forget();
> 
> You are dropping the information in rv here (by converting every type of
> error to InvalidStateError).  Is that ok?

I have checked the return value of mProvider->Dial(...). Looks like the current implementation always return NS_OK, so we don't drop any information in this situation. Moreover, I think we could either replace the whole 'if' block by an assertion because it should be successful or find a better name for the error string (but I can't find a name which seems better than InvalidStateError from DOMError).

Comment 49

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#6: dom

Kyle,
Thanks for the awesome promise helper.

Comment 50

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8378118 [details] [diff] [review]
Part 4#6: dom

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

r=me

::: dom/telephony/Telephony.cpp
@@ +72,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  Callback(Telephony* aTelephony, Promise* aPromise, uint32_t aServiceId, const nsAString& aNumber)
> +    : mTelephony(aTelephony), mPromise(aPromise), mServiceId(aServiceId), mNumber(aNumber)

80 character lines please.

Comment 51

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I will try to finish up and land my patch in bug 974120 tomorrow.

Comment 52

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Setting the milestone to Feb. 28 because this blocks bug 881174.

Comment 53

[Kai-Chih Hu [:khu]]

Looks like we got r+ for all parts. Can we land the codes?

Comment 54

[[:fabrice] Fabrice Desré]

(In reply to Kevin Hu [:khu] from comment #53)
> Looks like we got r+ for all parts. Can we land the codes?

We need to land the dependent bugs first.

Comment 55

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Bug 974120 is on inbound, but it looks like bug 969280 is not done.

Comment 56

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/e0fa4e0eee36
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/323c1329614b
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/b9f4ba525eec
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/75219ceb5175
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/46fa16a18c27
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/86356906ecf0

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/b2g-inbound/rev/677c0fa83472

https://tbpl.mozilla.org/php/getParsedLog.php?id=35294618&tree=B2g-Inbound

Comment 58

[Szu-Yu Chen [:aknow] (inactive)]

Sorry. I haven't attach the final version of patches here.

Comment 59

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final #2] Part 1: Use promise for dial and dialEmergency - webidl. r=hsinyi

Comment 60

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final #2] Part 2: Modify tests. r=hsinyi

Comment 61

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final #2] Part 3: Use promise for dial and dialEmergency - internal interface. r=hsinyi

Comment 62

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final #2] Part 4: Use promise for dial and dialEmergency - dom. r=khuey

Comment 63

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final #2] Part 5: Use promise for dial and dialEmergency - provider. r=hsinyi

Comment 64

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final #2] Part 6: Use promise for dial and dialEmergency - ipc. r=khuey

Comment 65

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=e21283d009c7

Comment 66

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #65)
> https://tbpl.mozilla.org/?tree=Try&rev=e21283d009c7

Try green, pushing...

   https://hg.mozilla.org/integration/b2g-inbound/rev/8c07c3fd9d6e
   https://hg.mozilla.org/integration/b2g-inbound/rev/f4d99adbd94d
   https://hg.mozilla.org/integration/b2g-inbound/rev/607545e9bb6c
   https://hg.mozilla.org/integration/b2g-inbound/rev/1492c73fa69a
   https://hg.mozilla.org/integration/b2g-inbound/rev/a381a07fffb4
   https://hg.mozilla.org/integration/b2g-inbound/rev/5838091659a7

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c07c3fd9d6e
https://hg.mozilla.org/mozilla-central/rev/f4d99adbd94d
https://hg.mozilla.org/mozilla-central/rev/607545e9bb6c
https://hg.mozilla.org/mozilla-central/rev/1492c73fa69a
https://hg.mozilla.org/mozilla-central/rev/a381a07fffb4
https://hg.mozilla.org/mozilla-central/rev/5838091659a7