Closed Bug 969218 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

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+
Blocks: 881174
nominate it as it blocks 1.4+ bugs.
blocking-b2g: --- → 1.4?
Blocks: 969280
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.
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.