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)

x86
macOS
defect
Not set
normal

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)
See Also: → 839838
(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.
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?
(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.
(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)
NI :Ehsan for comment 4.
Flags: needinfo?(ehsan.akhgari)
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)
(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.
(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!
(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.
(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: nobody → szchen
Blocks: 1058398
(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.  :-)
>   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?
Attached patch Part 1: Add MMICall (webidl) (obsolete) — Splinter Review
Attachment #8515788 - Flags: review?(htsai)
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)
Attachment #8515790 - Flags: review?(htsai)
Just to make it works. I should have something different after Bug 1071469.
Attachment #8515791 - Flags: feedback?(htsai)
Attached patch Part 5: Update test case (obsolete) — Splinter Review
Attachment #8515793 - Flags: review?(htsai)
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+
(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.
(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?
(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.
(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 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+
(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
> The users will get the same promise if they call getResult() multiple times.

Why not make it a readonly attribute in that case?
(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)
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)
Attached patch Part 1#2: Add MMICall (webidl) (obsolete) — Splinter Review
Address bz's comment
Attachment #8515788 - Attachment is obsolete: true
Attachment #8517202 - Flags: review?(htsai)
Attached patch Part 5#2: Update test case (obsolete) — Splinter Review
Attachment #8515793 - Attachment is obsolete: true
Attachment #8515793 - Flags: review?(htsai)
Attachment #8517204 - Flags: review?(htsai)
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 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 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 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 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+
Attachment #8515789 - Attachment is obsolete: true
Attachment #8519691 - Flags: review?(htsai)
Attachment #8515790 - Attachment is obsolete: true
Attachment #8515791 - Attachment is obsolete: true
Attachment #8519692 - Flags: review?(htsai)
Whiteboard: [ETA: 11/13]
Attachment #8519691 - Flags: review?(htsai) → review+
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+
Attachment #8517202 - Attachment is obsolete: true
Attachment #8520480 - Flags: review+
Attachment #8517204 - Attachment is obsolete: true
Attachment #8520484 - Flags: review+
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)
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.

Attachment

General

Created:
Updated:
Size: