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 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.

nominate it as it blocks 1.4+ bugs.

Modify all test cases to use the new promise form.

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.

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()

(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 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 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 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 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 on attachment 8374740 [details] [diff] [review] Part 5#3: gonk provider Review of attachment 8374740 [details] [diff] [review]: ----------------------------------------------------------------- This part looks good.

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.

(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!

Modify a new test case test_outgoing_answer_radio_off.

Add "already dialing" check.

Comment on attachment 8376147 [details] [diff] [review] Part 3#3: internal interface Review of attachment 8376147 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

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 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 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+

(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

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.

(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 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?

(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).

Kyle, Thanks for the awesome promise helper.

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.

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.

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

(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.

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

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

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

(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

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