Closed
Bug 1080883
Opened 10 years ago
Closed 10 years ago
Telephony.dial returning a Promise that resolves to a DOMRequest makes no sense
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S9 (21Nov)
People
(Reporter: ehsan.akhgari, Assigned: aknow)
References
Details
(Keywords: dev-doc-complete)
Attachments
(4 files, 9 obsolete files)
2.38 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
1.36 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
12.54 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
10.46 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
DOMRequest was our old version of promises, before they were added to DOM as a way to provide an asynchronous result. Therefore, resolving a promise to a DOMRequest makes no sense conceptually. Hsin-Yi, do you know why we did this? And can we just resolve the promise to the result of the DOMRequest in the MMI case?
Flags: needinfo?(htsai)
Comment 1•10 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #0) > Hsin-Yi, do you know why we did this? And can we just resolve the promise > to the result of the DOMRequest in the MMI case? The logic behind that choice was that we want a relatively quick response to let the dialer know if the specified number issued a regular call or an MMI message so that it can provide immediate feedback to the user. Once this is done we can further wait for the actual result of the MMI message which can take multiple seconds to come back from the network. To avoid the DOMRequest I had suggested doing a promise within a promise which might not be the most elegant thing ever but would have worked. I'm not sufficiently familiar with the implementation details to tell if the API could be designed without this two-steps asynchronous procedure.
Reporter | ||
Comment 2•10 years ago
|
||
Hmm, that's an interesting issue. What do we do for the case where the number dialed was not an MMI code? Is the TelephonyCall object passed to the caller through resolving the promise immdiately?
Comment 3•10 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2) > Hmm, that's an interesting issue. What do we do for the case where the > number dialed was not an MMI code? Is the TelephonyCall object passed to > the caller through resolving the promise immdiately? Yes, in the case the number is not an MMI code the promise immediately resolves to a TelephonyCall object.
Comment 4•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #1) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #0) > > Hsin-Yi, do you know why we did this? And can we just resolve the promise > > to the result of the DOMRequest in the MMI case? Thanks Ehsan for raising this! > > The logic behind that choice was that we want a relatively quick response to > let the dialer know if the specified number issued a regular call or an MMI > message so that it can provide immediate feedback to the user. Once this is > done we can further wait for the actual result of the MMI message which can > take multiple seconds to come back from the network. > The reason is as Gabriele explained. Thanks, Gabriele. This is the original request [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=889737#c29 Actually, our original idea is to introduce a new API MMIRequest to make the API itself clearer. However, as MMIRequest would simply inherit DOMRequest and do exactly the same as DOMRequest, we decided not to have a new one but take advantage of DOMRequest implementation. If we turn back to have |Promise<(TelephonyCall or MMIRequest)> dial(...)|, would it be better? Or you'd still suggest we should use Promise as much as possible for any async operation instead of creating a new request API? > To avoid the DOMRequest I had suggested doing a promise within a promise > which might not be the most elegant thing ever but would have worked. As Aknow has mentioned in [2], |Promise<(TelephonyCall or Promise<MMIResult>)> dial(...)| seems an option. Do you think this makes more sense? [2] https://groups.google.com/d/msg/mozilla.dev.webapi/q4GvtJ3JeDc/Ctc8t__eYb0J > I'm > not sufficiently familiar with the implementation details to tell if the API > could be designed without this two-steps asynchronous procedure. If we want to meet the UI request in [1], then it's inevitable.
Flags: needinfo?(htsai)
Reporter | ||
Comment 6•10 years ago
|
||
Why can't we resolve the promise to an object that gives you a way of accessing the MMI result? Something like: interface Telephony { Promise<TelephonyCall or MMICall> dial(); }; interface MMICall { Promise<MMIResult /* or whatever the result is */> getResult(); }; Also, is the unification of dial() and sendMMI a good idea in the first place?
Flags: needinfo?(ehsan.akhgari)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6) > Why can't we resolve the promise to an object that gives you a way of > accessing the MMI result? Something like: > > interface Telephony { > Promise<TelephonyCall or MMICall> dial(); > }; > interface MMICall { > Promise<MMIResult /* or whatever the result is */> getResult(); > }; > > Also, is the unification of dial() and sendMMI a good idea in the first > place? +1 for the proposal. It looks good and indeed covers our requirement? Gabriele, Do you have any comment from gaia side? If you are ok with it. I can work on this bug.
Comment 8•10 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6) > Why can't we resolve the promise to an object that gives you a way of > accessing the MMI result? Something like: > > interface Telephony { > Promise<TelephonyCall or MMICall> dial(); > }; > interface MMICall { > Promise<MMIResult /* or whatever the result is */> getResult(); > }; > Looks good to me as well. Thank you Ehsan. > Also, is the unification of dial() and sendMMI a good idea in the first > place? Yes!
Comment 9•10 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8) > > Also, is the unification of dial() and sendMMI a good idea in the first > > place? > > Yes! To provide more context, dial() and sendMMI() are being unified because the Gaia code doesn't have a way to figure out if a number is an MMI code or not before sending it. I'm not familiar with the low-level interface to the modem but I have the feeling that even there it's impossible to establish beforehand if a number is an MMI code or not because different carriers support different codes (and even custom ones). So unifying those functions is not necessarily a good idea but it's definitely a necessary evil.
Comment 10•10 years ago
|
||
(In reply to Szu-Yu Chen [:aknow] from comment #7) > +1 for the proposal. It looks good and indeed covers our requirement? > > Gabriele, Do you have any comment from gaia side? If you are ok with it. I > can work on this bug. This works for me. The changes required to my existing patch would be fairly small.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → szchen
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #9) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8) > > > Also, is the unification of dial() and sendMMI a good idea in the first > > > place? > > > > Yes! > > To provide more context, dial() and sendMMI() are being unified because the > Gaia code doesn't have a way to figure out if a number is an MMI code or not > before sending it. I'm not familiar with the low-level interface to the > modem but I have the feeling that even there it's impossible to establish > beforehand if a number is an MMI code or not because different carriers > support different codes (and even custom ones). So unifying those functions > is not necessarily a good idea but it's definitely a necessary evil. Makes perfect sense! FWIW I wasn't questioning the decision, just wanted to understand the reasoning. :-)
Assignee | ||
Comment 12•10 years ago
|
||
> interface Telephony {
> Promise<TelephonyCall or MMICall> dial();
> };
> interface MMICall {
> Promise<MMIResult /* or whatever the result is */> getResult();
> };
>
I got a question when implementing the above proposal.
For MMI code request, the result could be successful (MMIResult) or failed (DOMMMIError). What is a better way to handle the failed result through MMICall.getResult()? It may cause misunderstanding if we simply pass the failed result to promise.reject. To me, it looks like we got the problem for getResult() itself. However, what I want to show is that getResult() is successful but the result is failure.
We are thinking using one type (MMIResult) for both successful and failed result.
Do you have any idea?
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8515788 -
Flags: review?(htsai)
Assignee | ||
Comment 14•10 years ago
|
||
As I described in Comment 12, I'd like to have an augmented MMIResult that could also represent a failure. Do you have any suggestion for the structure?
Attachment #8515789 -
Flags: feedback?(htsai)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8515790 -
Flags: review?(htsai)
Assignee | ||
Comment 16•10 years ago
|
||
Just to make it works. I should have something different after Bug 1071469.
Attachment #8515791 -
Flags: feedback?(htsai)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8515793 -
Flags: review?(htsai)
Comment 18•10 years ago
|
||
Comment on attachment 8515789 [details] [diff] [review] Part 2: Augmented MozMMIResult for error result (webidl) Review of attachment 8515789 [details] [diff] [review]: ----------------------------------------------------------------- Agree that promise.reject() should mean getResult() fails rather than MMICall fails. We need a way to tell it's the result of a failed MMICall. This part looks good to me! Hi Gabriele, Aknow raised a question in comment 12. To make sure we are on the same page and all happy with the design, could you take a look? Thank you :)
Attachment #8515789 -
Flags: feedback?(htsai)
Attachment #8515789 -
Flags: feedback?(gsvelto)
Attachment #8515789 -
Flags: feedback+
Comment 19•10 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18) > Aknow raised a question in comment 12. To make sure we are on the same page > and all happy with the design, could you take a look? Thank you :) I agree that having the promise returned by getResult() to mean "sending the MMI code failed" is a bit confusing, one would expect that the getResult() call failed. Maybe it's just a matter of tweaking the names a bit, if instead of getResult() we'd use getPromise() it would be clear that if it rejects it's because the MMICall object encountered an error and not the method itself.
Comment 20•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #19) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #18) > > Aknow raised a question in comment 12. To make sure we are on the same page > > and all happy with the design, could you take a look? Thank you :) > > I agree that having the promise returned by getResult() to mean "sending the > MMI code failed" is a bit confusing, one would expect that the getResult() > call failed. Maybe it's just a matter of tweaking the names a bit, if > instead of getResult() we'd use getPromise() it would be clear that if it > rejects it's because the MMICall object encountered an error and not the > method itself. I feel puzzled about the name |getPromise()|. What is this *Promise* got from getPromise() supposed to mean? With this name, the meaning of this function becomes more obscure to me. I've tried to figure out another name but in vain. I also have a feeling this isn't really a naming problem. With the pattern |Promise<> getFoo()|, promise.reject implies the operation of getFoo() itself fails but not the result of MMICall. attachment 8515789 [details] [diff] [review] makes sense to me from the perspective of meaning and use cases, but not sure if that causes problems on gaia. May I know your concern/comment on that, Gabriele?
Comment 21•10 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #20) > attachment 8515789 [details] [diff] [review] makes sense to me from the > perspective of meaning and use cases, but not sure if that causes problems > on gaia. May I know your concern/comment on that, Gabriele? Yes, you're right, it does make sense and it will work in gaia.
Comment 22•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #21) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #20) > > attachment 8515789 [details] [diff] [review] makes sense to me from the > > perspective of meaning and use cases, but not sure if that causes problems > > on gaia. May I know your concern/comment on that, Gabriele? > > Yes, you're right, it does make sense and it will work in gaia. Good to hear that :)
Comment 23•10 years ago
|
||
Comment on attachment 8515788 [details] [diff] [review] Part 1: Add MMICall (webidl) Review of attachment 8515788 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed, thank you. ::: dom/webidl/MMICall.webidl @@ +7,5 @@ > +[Pref="dom.telephony.enabled", > + CheckPermissions="telephony", > + AvailableIn="CertifiedApps"] > +interface MMICall { > + [Throws] Please add attribute NewObject.
Attachment #8515788 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #23) > Comment on attachment 8515788 [details] [diff] [review] > Part 1: Add MMICall (webidl) > > Review of attachment 8515788 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comment addressed, thank you. > > ::: dom/webidl/MMICall.webidl > @@ +7,5 @@ > > +[Pref="dom.telephony.enabled", > > + CheckPermissions="telephony", > > + AvailableIn="CertifiedApps"] > > +interface MMICall { > > + [Throws] > > Please add attribute NewObject. It's not the new object. The users will get the same promise if they call getResult() multiple times. See part 3 implementation
Comment 25•10 years ago
|
||
> The users will get the same promise if they call getResult() multiple times.
Why not make it a readonly attribute in that case?
Assignee | ||
Comment 26•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #25) > > The users will get the same promise if they call getResult() multiple times. > > Why not make it a readonly attribute in that case? Did you mean a readonly attribute for the promise? or for the result? If it is for promise, could you show me an example? I don't know how to name that attribute so that the meaning is clear. What I am thinking for the original design is that: 1. getResult() is an async function because the result may not be ready at that time. 2. Once the result is ready (promise resolved), the next time user calls getResult(), we can just return the same promise which is already resolved. We don't have to create a new one.
Flags: needinfo?(bzbarsky)
Comment 27•10 years ago
|
||
I was thinking something like: [Throws] readonly attribute Promise<MozMMIResult> result; Assuming a given MMICall has only one MozMMIResult (which may not be available yet) this should work fine: just return the promise, which may or may not be resolved yet. In terms of the C++ it would look identical to what you have now, even.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 28•10 years ago
|
||
Address bz's comment
Attachment #8515788 -
Attachment is obsolete: true
Attachment #8517202 -
Flags: review?(htsai)
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8515793 -
Attachment is obsolete: true
Attachment #8515793 -
Flags: review?(htsai)
Attachment #8517204 -
Flags: review?(htsai)
Comment 30•10 years ago
|
||
Comment on attachment 8515789 [details] [diff] [review] Part 2: Augmented MozMMIResult for error result (webidl) I forgot to add my feedback here, sorry. As I said in comment 21 this is OK.
Attachment #8515789 -
Flags: feedback?(gsvelto) → feedback+
Comment 31•10 years ago
|
||
Comment on attachment 8517202 [details] [diff] [review] Part 1#2: Add MMICall (webidl) Review of attachment 8517202 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #8517202 -
Flags: review?(htsai) → review+
Comment 32•10 years ago
|
||
Comment on attachment 8515791 [details] [diff] [review] Part 4: Add success in MMIResult (ril) Review of attachment 8515791 [details] [diff] [review]: ----------------------------------------------------------------- I am really sorry for the late response. After Bug 1071469, there's no MMIResult in TelephonyService.js. Please rebase on that, thank you.
Attachment #8515791 -
Flags: feedback?(htsai)
Comment 33•10 years ago
|
||
Comment on attachment 8515790 [details] [diff] [review] Part 3: MMICall implementation (dom) Review of attachment 8515790 [details] [diff] [review]: ----------------------------------------------------------------- Since Bug 1071469, nsITelephonyService.idl changes a little bit. Please have the patch rebased, sorry about that :( ::: dom/telephony/MMICall.h @@ +9,5 @@ > + > +#include "mozilla/Attributes.h" > +#include "mozilla/ErrorResult.h" > +#include "mozilla/dom/BindingDeclarations.h" > +#include "mozilla/dom/DOMMMIError.h" We don't need this, right? @@ +16,5 @@ > +#include "nsAutoPtr.h" > +#include "nsCOMPtr.h" > +#include "nsCycleCollectionParticipant.h" > +#include "nsJSUtils.h" > +#include "nsPIDOMWindow.h" Forward declaration.
Attachment #8515790 -
Flags: review?(htsai)
Comment 34•10 years ago
|
||
Comment on attachment 8517204 [details] [diff] [review] Part 5#2: Update test case Review of attachment 8517204 [details] [diff] [review]: ----------------------------------------------------------------- Awesome! ::: dom/telephony/test/marionette/test_mmi.js @@ +6,5 @@ > > function testGettingIMEI() { > log("Test *#06# ..."); > > + return gSendMMI("*#06#").then(result => { nit: s/result/aResult/ here and below.
Attachment #8517204 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8515789 -
Attachment is obsolete: true
Attachment #8519691 -
Flags: review?(htsai)
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8515790 -
Attachment is obsolete: true
Attachment #8515791 -
Attachment is obsolete: true
Attachment #8519692 -
Flags: review?(htsai)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [ETA: 11/13]
Updated•10 years ago
|
Attachment #8519691 -
Flags: review?(htsai) → review+
Comment 37•10 years ago
|
||
Comment on attachment 8519692 [details] [diff] [review] Part 3#2: MMICall implementation (dom) Review of attachment 8519692 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thank you! ::: dom/telephony/MMICall.h @@ +16,5 @@ > +#include "nsCOMPtr.h" > +#include "nsCycleCollectionParticipant.h" > +#include "nsJSUtils.h" > +#include "nsWrapperCache.h" > +#include "mozilla/dom/MozMobileConnectionBinding.h" Let's remove this unnecessary header.
Attachment #8519692 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8517202 -
Attachment is obsolete: true
Attachment #8520480 -
Flags: review+
Assignee | ||
Comment 39•10 years ago
|
||
Attachment #8519691 -
Attachment is obsolete: true
Attachment #8520481 -
Flags: review+
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8519692 -
Attachment is obsolete: true
Attachment #8520482 -
Flags: review+
Assignee | ||
Comment 41•10 years ago
|
||
Attachment #8517204 -
Attachment is obsolete: true
Attachment #8520484 -
Flags: review+
Assignee | ||
Comment 42•10 years ago
|
||
try looks good https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dcf317ec2700
Assignee | ||
Comment 43•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/5759c3bccda2 https://hg.mozilla.org/integration/b2g-inbound/rev/958a917b1f09 https://hg.mozilla.org/integration/b2g-inbound/rev/96bd39475fb2 https://hg.mozilla.org/integration/b2g-inbound/rev/d2674be507df
Comment 44•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5759c3bccda2 https://hg.mozilla.org/mozilla-central/rev/958a917b1f09 https://hg.mozilla.org/mozilla-central/rev/96bd39475fb2 https://hg.mozilla.org/mozilla-central/rev/d2674be507df
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ETA: 11/13]
Target Milestone: --- → 2.1 S9 (21Nov)
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 45•9 years ago
|
||
This is now documented. I've updated the dial() information: https://developer.mozilla.org/en-US/docs/Web/API/Telephony/dial And add MMICall too, as it was missing: https://developer.mozilla.org/en-US/docs/Web/API/MMICall https://developer.mozilla.org/en-US/docs/Web/API/MMICall/result I've also added a note to the relevant release notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Releases/2.1#Web_API_changes
Keywords: dev-doc-needed → dev-doc-complete
Comment 46•9 years ago
|
||
Excellent stuff Chris, thanks for documenting these bits. They are particularly obscure and they tend to pose a significant barrier to entry to those who want to contribute to the dialer so having them documented is a big plus.
You need to log in
before you can comment on or make changes to this bug.
Description
•