1058397 - [B2G][Telephony] add UssdSession API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

+++ This bug was initially created as a clone of Bug 889737 comment 99 +++

Bug 889737 is a huge patch, so we are going to split it into smaller pieces in order for the attempt on moving each part (development, review, QA testing, ect) forward in parallel.

This bug is going to cover the "ussd-received" part, based on Bug 889737 comment 99.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Gabriele,

Let's keep discussing ussd session here, thank you :)

[quote from bug 889737 comment 105]
(In reply to Gabriele Svelto [:gsvelto] from comment #105)
> OK, thanks Anshul, Hsin-Yi. Then putting replyMMI() inside mozTelephony
> doesn't feel right since it can be used only within an active USSD session.
> As discussed before we could have an UssdSession object returned via the
> ussd-received system message; I wouldn't mind if the method was within the
> ussd-received event object, either works for me. 

Does that mean Dialer doesn't need to rely on system message to be waken up in ussd-received case? 
Is the concern (bug 889737 comment 97) below valid?
"However, I believe that current gaia relies on system message to wake up the app. If I put the ussdSession in the event, the app might miss that event because it doesn't have enough time to register the event listener."

> The point is, let's make
> sure replyMMI() is in a place where calling it makes sense.

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Does that mean Dialer doesn't need to rely on system message to be waken up
> in ussd-received case? 

The dialer app is woken up by that message only when we receive an unsolicited USSD message (i.e. it's not a reply to a message we've sent ourselves).

> Is the concern (bug 889737 comment 97) below valid?
> "However, I believe that current gaia relies on system message to wake up
> the app. If I put the ussdSession in the event, the app might miss that
> event because it doesn't have enough time to register the event listener."

I don't think I understand the problem. The dialer can be woken up by a ussd-received system message, it does register the handler at startup like we do for practically all system messages:

https://github.com/mozilla-b2g/gaia/blob/637d724e26da0ec3ff9792ac9ad36e268e0fdbe8/apps/communications/dialer/js/dialer.js#L500

We also explicitly request for it in the manifest:

https://github.com/mozilla-b2g/gaia/blob/cd6ee1dc4439a4be8455d42b6a9b995574a7207d/apps/communications/manifest.webapp#L148

I don't see how putting the UssdSession object in the system message event might cause us to miss the message; but maybe I'm missing something.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > Does that mean Dialer doesn't need to rely on system message to be waken up
> > in ussd-received case? 
> 
> The dialer app is woken up by that message only when we receive an
> unsolicited USSD message (i.e. it's not a reply to a message we've sent
> ourselves).
> 
> > Is the concern (bug 889737 comment 97) below valid?
> > "However, I believe that current gaia relies on system message to wake up
> > the app. If I put the ussdSession in the event, the app might miss that
> > event because it doesn't have enough time to register the event listener."
> 
> I don't think I understand the problem. The dialer can be woken up by a
> ussd-received system message, it does register the handler at startup like
> we do for practically all system messages:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> 637d724e26da0ec3ff9792ac9ad36e268e0fdbe8/apps/communications/dialer/js/
> dialer.js#L500
> 
> We also explicitly request for it in the manifest:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> cd6ee1dc4439a4be8455d42b6a9b995574a7207d/apps/communications/manifest.
> webapp#L148
> 
> I don't see how putting the UssdSession object in the system message event
> might cause us to miss the message; but maybe I'm missing something.

Actually, the original question was about whether putting the UssdSession object in DOM event really helps if Dialer counts on 'system message' to wake up. But anyway I've got the answer, i.e. let's put the UssdSession object in ussd-received system message. Thanks.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Our discussion was scattered all over the bugzilla, and not easy to track. Let me collect what we have, mainly based on the discussion with Aknow and Gabriele, and hopefully helps us know if everybody is on the same page.

=== Proposal ===
partial interface Telephony
{
  attribute EventHandler onussdreceived;
};

interface USSDReceivedEvent : Event
{
   readonly attribute unsingned long serviceId;
   readonly attribute DOMString? message;
   readonly attribute MozUssdSession? session;
};
USSDReceivedEvent::SessionEnded is removed because in the new API user could tell if the session is terminated from the nullable session. If session is null, that means it's ended.

interface MozUssdSession
{
  DOMRequest send(DOMString ussd);
};
With the new design, "MobileConnection::Onussdreceived" is removed as well as MobileConnection::CancelMMI().
CancelMMI was designed for cancelling any existing ussd session when we init the dialer appto make sure we're not accidentally sending the data on an existing session.

Comment 5

[Gabriele Svelto [:gsvelto]]

+1

This should work very nicely. I like specifically the fact that we're getting rid of all the bits in MobileConnection which and especially MobileConnection::Onussdreceived which made the whole implementation not very orthogonal. I'll update the patch in bug 1058398 accordingly.

Comment 6

[Gabriele Svelto [:gsvelto]]

I meant bug 1031175 obviously.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> Our discussion was scattered all over the bugzilla, and not easy to track.
> Let me collect what we have, mainly based on the discussion with Aknow and
> Gabriele, and hopefully helps us know if everybody is on the same page.
> 
> === Proposal ===
> partial interface Telephony
> {
>   attribute EventHandler onussdreceived;
> };
> 
> interface USSDReceivedEvent : Event
> {
>    readonly attribute unsingned long serviceId;
>    readonly attribute DOMString? message;
>    readonly attribute MozUssdSession? session;
> };
> USSDReceivedEvent::SessionEnded is removed because in the new API user could
> tell if the session is terminated from the nullable session. If session is
> null, that means it's ended.
> 
> interface MozUssdSession
> {
>   DOMRequest send(DOMString ussd);
> };

Oh, sorry, here should be Promise rather than DOMRequest.


> With the new design, "MobileConnection::Onussdreceived" is removed as well
> as MobileConnection::CancelMMI().
> CancelMMI was designed for cancelling any existing ussd session when we init
> the dialer appto make sure we're not accidentally sending the data on an
> existing session.

Comment 8

[Gabriele Svelto [:gsvelto]]

While working on bug 1031175 I've noticed a case where we were using MobileConnection.cancelMMI() outside of the initialization path: we did so when the user explicitly dismissed the interface during an interactive MMI section.

I was wondering how to deal with that case. Maybe we should add back that bit of functionality into MozUssdSession object? Alternatively we could just dismiss the interface without explicitly canceling the session as long as:

- We're sure that we won't get any new messages for it. I would say this is a safe assumption as it's our turn at replying and we would never reply.
- Calling Telephony.dial() will automatically cancel the old session for us.

Hsin-Yi, is this the case already?

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Gabriele Svelto [:gsvelto] from comment #8)
> While working on bug 1031175 I've noticed a case where we were using
> MobileConnection.cancelMMI() outside of the initialization path: we did so
> when the user explicitly dismissed the interface during an interactive MMI
> section.
> 
> I was wondering how to deal with that case. Maybe we should add back that
> bit of functionality into MozUssdSession object? Alternatively we could just
> dismiss the interface without explicitly canceling the session as long as:
> 
> - We're sure that we won't get any new messages for it. I would say this is
> a safe assumption as it's our turn at replying and we would never reply.
> - Calling Telephony.dial() will automatically cancel the old session for us.
> 
> Hsin-Yi, is this the case already?

Sorry for the late reply. Just got back from my PTO :)

And the answer to your question is YES!  (Aknow, please correct me if I got something wrong.)

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3: USSDSession interface

Comment 11

[Gabriele Svelto [:gsvelto]]

Excellent, thanks!

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8487659 [details] [diff] [review]
Part 3: USSDSession interface

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

::: dom/bindings/Bindings.conf
@@ +1382,5 @@
>  }],
>  
> +'USSDSession': {
> +    'nativeType': 'mozilla::dom::USSDSession',
> +    'headerFile': 'USSDSession.h'

These are default values. Wondering if we do need them.

::: dom/webidl/USSDReceivedEvent.webidl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  
>  [Pref="dom.mobileconnection.enabled",

We shall change this to "dom.telephony.enabled" after the followup bug for removing backward compatibility. Please file that bug and comment the TODO bug number here.

@@ +12,3 @@
>    readonly attribute DOMString? message;
> +  readonly attribute USSDSession? session;  // null if session is ended.
> +  readonly attribute boolean sessionEnded;  // deprecated

Please comment the TODO bug number here.

::: dom/webidl/USSDSession.webidl
@@ +6,5 @@
> +
> +[Pref="dom.telephony.enabled",
> + Constructor(unsigned long serviceId)]
> +interface USSDSession {
> +  [Throws, CheckPermissions="telephony"]

Why not have the whole interface, instead of a single function, being guarded by |CheckPermissions="telephony"|?

Comment 13

[Szu-Yu Chen [:aknow] (inactive)]

Hi Gabriele, Hsinyi

Do we need an ussdreceived event on Telephony?

In current design, we send ussd received info by two ways: (1) mobileConnection event [1], (2) system message. However, only (2) is used in gaia [2]. Since we are going to phase out all sendMMI/ussd related api of MozMobileConnection after migrating them to Telephony. Do we still want to have this kind of DOM event on telephony?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileConnection.webidl#529
[2] https://github.com/mozilla-b2g/gaia/blob/2f5a1aff6b39f6e8e03e3a69c05d57d5b0987409/apps/communications/dialer/js/dialer.js#L509

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #13)
> Hi Gabriele, Hsinyi
> 
> Do we need an ussdreceived event on Telephony?
> 
> In current design, we send ussd received info by two ways: (1)
> mobileConnection event [1], (2) system message. However, only (2) is used in
> gaia [2]. Since we are going to phase out all sendMMI/ussd related api of
> MozMobileConnection after migrating them to Telephony. Do we still want to
> have this kind of DOM event on telephony?


It looks fine to not have telephony.onussdreceived as gaia doesn't need that though I don't have strong opinion.


> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileConnection.
> webidl#529
> [2]
> https://github.com/mozilla-b2g/gaia/blob/
> 2f5a1aff6b39f6e8e03e3a69c05d57d5b0987409/apps/communications/dialer/js/
> dialer.js#L509

Comment 15

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> It looks fine to not have telephony.onussdreceived as gaia doesn't need that
> though I don't have strong opinion.

Agreed, it's redundant since we have to use the system message anyway to be able to launch the app.

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Separate TelephonyCallback (only file rename)

I rename the original "TelephonyCallback" class to "TelephonyDialCallback" and introduce a new class called "TelephonyCallback" as its base class.

TelephonyCallback will be used in sendUSSD.

I separate the modification into two parts so that the diff is easier to compare.
- Part 1: This one, only file rename. All the content are the same.
- Part 2: All the remaining modification.

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Separate TelephonyCallback (cont.)

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3#2: USSDSession interface

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4: USSDSession implement

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5: wrap system message

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#2: wrap system message

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8492881 [details] [diff] [review]
Part 1: Separate TelephonyCallback (only file rename)

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

Thank you.

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8492882 [details] [diff] [review]
Part 2: Separate TelephonyCallback (cont.)

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

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +29,1 @@
>    Components.ID("{6e1af17e-37f3-11e4-aed3-60a44c237d2b}");

Learning from https://bugzilla.mozilla.org/show_bug.cgi?id=1063304#c38, we probably need to be more careful here. Please change the ID accordingly, thank you.

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +4,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "TelephonyChild.h"
>  
>  #include "mozilla/dom/telephony/TelephonyCallback.h"

Keeping line 9 is enough.

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8492884 [details] [diff] [review]
Part 3#2: USSDSession interface

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

Overall it is great. With some webidl attributes added, then it's ready to go :)

::: dom/webidl/USSDReceivedEvent.webidl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  
> +[Pref="dom.telephony.enabled",

Let's also have "CheckPermissions="telephony mobileconnection,", AvailableIn="CertifiedApps"]" thank you.

::: dom/webidl/USSDSession.webidl
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Pref="dom.telephony.enabled",
> + CheckPermissions="telephony",

Add "AvailableIn="CertifiedApps" as well.

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8492882 [details] [diff] [review]
Part 2: Separate TelephonyCallback (cont.)

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

::: dom/telephony/TelephonyCallback.h
@@ +5,5 @@
> +#ifndef mozilla_dom_TelephonyCallback_h
> +#define mozilla_dom_TelephonyCallback_h
> +
> +#include "nsCOMPtr.h"
> +#include "nsITelephonyService.h"

Sorry for missing this. Please remember to remove it.

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8492886 [details] [diff] [review]
Part 4: USSDSession implement

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

Please see my comments, thank you :)

::: dom/system/gonk/ril_worker.js
@@ +2663,5 @@
>     *
>     * @param ussd
>     *        String containing the USSD code.
> +   * @param checkSession
> +   *        True if an existing session should be there

nit: period at the end of the line

::: dom/telephony/USSDSession.cpp
@@ +54,5 @@
> +USSDSession::Constructor(const GlobalObject& aGlobal, uint32_t aServiceId,
> +                         ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
> +  nsRefPtr<USSDSession> session = new USSDSession(window, aServiceId);

What if line# 28 in constructor happens? Could we have a graceful implementation, maybe introducing a Create member?

::: dom/telephony/USSDSession.h
@@ +8,5 @@
> +#define mozilla_dom_USSDSession_h
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/BindingDeclarations.h"

Doesn't seem we need this .h.

@@ +9,5 @@
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "nsCycleCollectionParticipant.h"

We could replace #10, #11 and #13 with "TelephonyCommon.h"

@@ +24,5 @@
> +
> +class USSDSession MOZ_FINAL : public nsISupports,
> +                              public nsWrapperCache
> +{
> +public:

Please put the public sections together.

@@ +32,5 @@
> +public:
> +  USSDSession(nsPIDOMWindow* aWindow, uint32_t aServiceId);
> +
> +protected:
> +  ~USSDSession();

Since USSDSession is defined as MOZ_FINAL, seems no much sense to have "protected" destructor here? private should be fine.

@@ +35,5 @@
> +protected:
> +  ~USSDSession();
> +
> +public:
> +  nsPIDOMWindow* GetParentObject() const;

Align nsPIDOMWindow and GetParentObject ~

@@ +37,5 @@
> +
> +public:
> +  nsPIDOMWindow* GetParentObject() const;
> +
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;

ditto.

::: dom/telephony/gonk/TelephonyService.js
@@ +673,5 @@
>    },
>  
>    /**
>     * @param aMmi
>     *        Parsed MMI structure.

Don't forget to update the comments :)

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8495120 [details] [diff] [review]
Part 5#2: wrap system message

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

Awesome :D
Thanks for this. I am also excited about learning SystemMessageWrapper!

r=me with comment addressed.

::: dom/telephony/gonk/TelephonyService.js
@@ +1340,5 @@
> +  // nsISystemMessagesWrapper implementation.
> +  wrapMessage: function(aMessage, aWindow) {
> +    if (DEBUG) debug("wrapMessage: " + JSON.stringify(aMessage));
> +
> +    let session = aMessage.sessionEnd ? null :

Should be sessionEnded?

@@ +1346,5 @@
> +
> +    let event = new aWindow.USSDReceivedEvent("ussdreceived", {
> +      serviceId: aMessage.serviceId,
> +      message: aMessage.message,
> +      sessionEnd: aMessage.sessionEnd,

ditto.

Comment 28

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> Comment on attachment 8492882 [details] [diff] [review]
> Part 2: Separate TelephonyCallback (cont.)
> 
> Review of attachment 8492882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/TelephonyCallback.h
> @@ +5,5 @@
> > +#ifndef mozilla_dom_TelephonyCallback_h
> > +#define mozilla_dom_TelephonyCallback_h
> > +
> > +#include "nsCOMPtr.h"
> > +#include "nsITelephonyService.h"
> 
> Sorry for missing this. Please remember to remove it.

Did you mean "#include "nsITelephonyService.h"?
The header is needed for nsITelephonyCallback

  class TelephonyCallback : public nsITelephonyCallback

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #28)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> > Comment on attachment 8492882 [details] [diff] [review]
> > Part 2: Separate TelephonyCallback (cont.)
> > 
> > Review of attachment 8492882 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/telephony/TelephonyCallback.h
> > @@ +5,5 @@
> > > +#ifndef mozilla_dom_TelephonyCallback_h
> > > +#define mozilla_dom_TelephonyCallback_h
> > > +
> > > +#include "nsCOMPtr.h"
> > > +#include "nsITelephonyService.h"
> > 
> > Sorry for missing this. Please remember to remove it.
> 
> Did you mean "#include "nsITelephonyService.h"?
> The header is needed for nsITelephonyCallback
> 
>   class TelephonyCallback : public nsITelephonyCallback

You are right. My bad, please ignore it orz

Comment 30

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#3: Wrap system message. r=hsinyi

Comment 31

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3#3: USSDSession interface

Comment 32

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#2: USSDSession implement

Comment 33

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8499394 [details] [diff] [review]
Part 4#2: USSDSession implement

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

Thank you :)

::: dom/telephony/USSDSession.cpp
@@ +53,5 @@
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsITelephonyService> ril =
> +      do_GetService(TELEPHONY_SERVICE_CONTRACTID);

nit: indention

Comment 34

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 1: Separate TelephonyCallback. r=hsinyi

Comment 35

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 2: USSDSession interface. r=hsinyi

Comment 36

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 3: USSDSession implement. r=hsinyi

Comment 37

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 4: Wrap system message. r=hsinyi

Comment 38

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=84669ce7841b

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/87cd44cd57b7
https://hg.mozilla.org/integration/b2g-inbound/rev/c7d7762d5e27
https://hg.mozilla.org/integration/b2g-inbound/rev/774f695d8c0a
https://hg.mozilla.org/integration/b2g-inbound/rev/3ad7043d2116

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for non-unified bustage.
https://hg.mozilla.org/integration/b2g-inbound/rev/104587650717

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=612816&repo=b2g-inbound

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Also B2G mochitest failures.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=613372&repo=b2g-inbound

Comment 42

[Ryan VanderMeulen [:RyanVM]]

And https://treeherder.mozilla.org/ui/logviewer.html#?job_id=613376&repo=b2g-inbound

Comment 43

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5: Fix test: USSDReceivedEvent is not exposed

Hi bz,

May I have your review?

In part 2 patch, we added the permission check and certifiedApp check. Therefore, the interface USSDReceivedEvent is not exposed and I removed the check in test case. Is it correct? or how could I address the check with the condition of permission/certifiedApp

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8504003 [details] [diff] [review]
Part 5: Fix test: USSDReceivedEvent is not exposed

r=me

Comment 45

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final#2] Part 1: Separate TelephonyCallback. r=hsinyi

Comment 46

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final#2] Part 2: USSDSession interface. r=hsinyi

Comment 47

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final#2] Part 3: USSDSession implement. r=hsinyi

Comment 48

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final#2] Part 4: Wrap system message. r=hsinyi

Comment 49

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final#2] Part 5: Fix test: USSDReceivedEvent is not exposed. r=bzbarsky

Comment 50

[Szu-Yu Chen [:aknow] (inactive)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c08b6259e788

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1a2f098c634d
https://hg.mozilla.org/integration/b2g-inbound/rev/8ec09ed512d3
https://hg.mozilla.org/integration/b2g-inbound/rev/b93c5e78c526
https://hg.mozilla.org/integration/b2g-inbound/rev/1b5f60a8cbac
https://hg.mozilla.org/integration/b2g-inbound/rev/2c9e31c7739d

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a2f098c634d
https://hg.mozilla.org/mozilla-central/rev/8ec09ed512d3
https://hg.mozilla.org/mozilla-central/rev/b93c5e78c526
https://hg.mozilla.org/mozilla-central/rev/1b5f60a8cbac
https://hg.mozilla.org/mozilla-central/rev/2c9e31c7739d

Comment 53

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

setting feature-b2g:2.2+ for already landed.

Comment 54

[Michael Schwartz [:m4]]

Just to be clear, if cancel is removed then how will a user cancel/dismiss a USSD session?

eg.
# User dials *331#
# Network request for input
# User cancels

Comment 55

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Michael Schwartz [:m4] from comment #54)
> Just to be clear, if cancel is removed then how will a user cancel/dismiss a
> USSD session?
> 
> eg.
> # User dials *331#
> # Network request for input
> # User cancels

Just ignore it.
When next time gaia use telphony.send(MMI_CODE), gecko will guarantee to send the MMI_CODE on a new session by sending cancel request first if any session exists.

Comment 56

[Michael Schwartz [:m4]]

(In reply to Szu-Yu Chen [:aknow] from comment #55)
> Just ignore it.

Seems like we're being a bad citizen by leaving the network to timeout but I can't find a GCF case (doesn't mean there isn't one) for that so perhaps there's no issue.  3GPP 51.010 section 31.9.2 "Network initiated unstructured supplementary service operations" is where I looked.

Comment 57

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Michael Schwartz [:m4] from comment #56)
> (In reply to Szu-Yu Chen [:aknow] from comment #55)
> > Just ignore it.
> 
> Seems like we're being a bad citizen by leaving the network to timeout but I
> can't find a GCF case (doesn't mean there isn't one) for that so perhaps
> there's no issue.  3GPP 51.010 section 31.9.2 "Network initiated
> unstructured supplementary service operations" is where I looked.

Yes, it may be a point we need to concern about.

In comment 4
"""
CancelMMI was designed for cancelling any existing ussd session when we init the dialer app to make sure we're not accidentally sending the data on an existing session.
"""

That's the reason why we design the new API as this way because it indeed cover our original use case.

To address your comment, we can introduce a "cancel" method in USSDSession.