Closed Bug 969218 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S2 (28feb)
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: aknow, Assigned: aknow)

References

Details

Attachments

(7 files, 23 obsolete files)

3.34 KB, patch
Details | Diff | Splinter Review
1.25 KB, patch
aknow
: review+
Details | Diff | Splinter Review
47.45 KB, patch
aknow
: review+
Details | Diff | Splinter Review
2.57 KB, patch
aknow
: review+
Details | Diff | Splinter Review
11.86 KB, patch
aknow
: review+
Details | Diff | Splinter Review
2.68 KB, patch
aknow
: review+
Details | Diff | Splinter Review
19.01 KB, patch
aknow
: review+
Details | Diff | Splinter Review
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.
Attachment #8372020 - Flags: superreview?(htsai)
Attached file Ref: sample code (obsolete) —
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.
Attachment #8372020 - Flags: superreview?(htsai) → review+
nominate it as it blocks 1.4+ bugs.
blocking-b2g: --- → 1.4?
Attachment #8372020 - Attachment is obsolete: true
Attachment #8372151 - Flags: review+
Attached patch Part 2: Modify tests (obsolete) — Splinter Review
Modify all test cases to use the new promise form.
Attachment #8372152 - Flags: review?(htsai)
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?
(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.
Depends on: 968713
Attached patch Part 3: internal interface (obsolete) — Splinter Review
Attachment #8373273 - Flags: review?(htsai)
Attached patch Part 4: dom (obsolete) — Splinter Review
Attachment #8373274 - Flags: review?(htsai)
Attached patch Part 5: gonk provider (obsolete) — Splinter Review
Attachment #8373275 - Flags: review?(htsai)
Attached patch Part 6: ipc (obsolete) — Splinter Review
Attachment #8373276 - Flags: review?(htsai)
Attached patch Part 4#2: dom (obsolete) — Splinter Review
Attachment #8373274 - Attachment is obsolete: true
Attachment #8373274 - Flags: review?(htsai)
Attachment #8373836 - Flags: review?(htsai)
Attached patch Part 6#2: ipc (obsolete) — Splinter Review
Attachment #8373276 - Attachment is obsolete: true
Attachment #8373276 - Flags: review?(htsai)
Attachment #8373837 - Flags: review?(htsai)
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()
Attachment #8373273 - Flags: review?(htsai)
(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.
Blocks: 951607
Attached patch Part 5#2: gonk provider (obsolete) — Splinter Review
Attachment #8373275 - Attachment is obsolete: true
Attachment #8373275 - Flags: review?(htsai)
Attachment #8374702 - Flags: review?(htsai)
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!
Attachment #8372152 - Flags: review?(htsai) → review+
Attached patch Part 3#2: internal interface (obsolete) — Splinter Review
Attachment #8373273 - Attachment is obsolete: true
Attachment #8374737 - Flags: review?(htsai)
Attached patch Part 4#3: dom (obsolete) — Splinter Review
Attachment #8373836 - Attachment is obsolete: true
Attachment #8373836 - Flags: review?(htsai)
Attachment #8374738 - Flags: review?(htsai)
Attached patch Part 5#3: gonk provider (obsolete) — Splinter Review
Attachment #8374702 - Attachment is obsolete: true
Attachment #8374702 - Flags: review?(htsai)
Attachment #8374740 - Flags: review?(htsai)
Attached patch Part 6#3: ipc (obsolete) — Splinter Review
Attachment #8373837 - Attachment is obsolete: true
Attachment #8373837 - Flags: review?(htsai)
Attachment #8374742 - Flags: review?(htsai)
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.
Attachment #8374737 - Flags: review?(htsai) → review+
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.
Attachment #8374737 - Flags: review+
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
Attachment #8374738 - Flags: review?(htsai)
Comment on attachment 8374740 [details] [diff] [review]
Part 5#3: gonk provider

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

This part looks good.
Attachment #8374740 - Flags: review?(htsai) → review+
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.
Attachment #8374742 - Flags: review?(htsai)
(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.
(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!
Attached patch Part 2#2: Modify tests (obsolete) — Splinter Review
Modify a new test case test_outgoing_answer_radio_off.
Attachment #8372152 - Attachment is obsolete: true
Attachment #8376144 - Flags: review?(htsai)
Attached patch Part 3#3: internal interface (obsolete) — Splinter Review
Attachment #8374737 - Attachment is obsolete: true
Attachment #8376147 - Flags: review?(htsai)
Attached patch Part 4#4: dom (obsolete) — Splinter Review
Attachment #8374738 - Attachment is obsolete: true
Attachment #8376148 - Flags: feedback?(htsai)
Attached patch Part 5#4: gonk provider (obsolete) — Splinter Review
Add "already dialing" check.
Attachment #8374740 - Attachment is obsolete: true
Attachment #8376150 - Flags: review?(htsai)
Attached patch Part 6#4: ipc (obsolete) — Splinter Review
Attachment #8374742 - Attachment is obsolete: true
Attachment #8376151 - Flags: review?(htsai)
Comment on attachment 8376147 [details] [diff] [review]
Part 3#3: internal interface

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

Looks good.
Attachment #8376147 - Flags: review?(htsai) → review+
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.
Attachment #8376148 - Flags: feedback?(htsai) → feedback+
Attachment #8376150 - Flags: review?(htsai) → review+
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
Attachment #8376151 - Flags: review?(htsai)
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+
Attachment #8376144 - Flags: review?(htsai) → review+
(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
Attached patch Part 4#5: dom (obsolete) — Splinter Review
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.
Attachment #8376148 - Attachment is obsolete: true
Attachment #8376950 - Flags: review?(khuey)
Attached patch Part 6#5: ipc (obsolete) — Splinter Review
Attachment #8376151 - Attachment is obsolete: true
Attachment #8376951 - Flags: review?(khuey)
(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.
No longer blocks: 969280
Depends on: 969280
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 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.
(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 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?
Attachment #8376950 - Flags: review?(khuey) → review-
Depends on: 974120
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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).
Attached patch Part 4#6: dom (obsolete) — Splinter Review
Kyle,
Thanks for the awesome promise helper.
Attachment #8376950 - Attachment is obsolete: true
Attachment #8378118 - Flags: review?(khuey)
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.
Attachment #8378118 - Flags: review?(khuey) → review+
I will try to finish up and land my patch in bug 974120 tomorrow.
Setting the milestone to Feb. 28 because this blocks bug 881174.
Target Milestone: --- → 1.4 S2 (28feb)
Blocks: 958733
Looks like we got r+ for all parts. Can we land the codes?
Flags: needinfo?(szchen)
(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.
Bug 974120 is on inbound, but it looks like bug 969280 is not done.
Sorry. I haven't attach the final version of patches here.
Attachment #8376144 - Attachment is obsolete: true
Attachment #8382727 - Flags: review+
Attachment #8382726 - Attachment description: [final#2] Part 1: Use promise for dial and dialEmergency - webidl. r=hsinyi → [final #2] Part 1: Use promise for dial and dialEmergency - webidl. r=hsinyi
Attachment #8372021 - Attachment is obsolete: true
blocking-b2g: 1.4? → 1.4+
Depends on: 978031
Depends on: 995630
You need to log in before you can comment on or make changes to this bug.