794011 - MMI Codes: Add 'sessionEnded' to nsIDOMUSSDReceivedEvent

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

As requested by the Gaia team, |nsIDOMUSSDReceivedEvent| should include information about the closure of the USSD session, so the UI can handle this situation properly (closing the USSD dialog or whatever)

Comment 1

[Beatriz Rodríguez [:brg]]

This functionality is important to have fully implemented USSD functionality in the platform.

Comment 2

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Patch

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 669592 [details] [diff] [review]
Patch

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

Very nice!
I would appreciate a few comments in the code though :)

::: dom/network/src/MobileConnection.cpp
@@ +196,5 @@
> +        str.init(cx, messageJS.toString());
> +        statusCode.Assign(str);
> +        if (statusCode.EqualsLiteral("2")) {
> +          sessionEnded = true;
> +        }

nit: |sessionEnded = statusCode.EqualsLiteral("2");| instead of the `if` statement.

Apart from that, there are two type codes sent by the RIL that notifies about an ended USSD session: 0 (USSD-Notify/no further action) and 2 (session terminated). You should check for both.

Also use defines for the literals, please.

@@ +206,3 @@
>      NS_ASSERTION(event, "This should never fail!");
>  
> +    rv =

nit: You don't need to break the line if is less than 80 chars

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 669592 [details] [diff] [review]
Patch

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

::: dom/system/gonk/ril_worker.js
@@ +4840,5 @@
>      return;
>    }
>    this.sendDOMMessage({rilMessageType: "USSDReceived",
> +                       message: message,
> +                       typeCode: typeCode});

Sorry Guillermo, I just realized that you should probably use the |RIL._ussdSession| flag instead of the |typeCode| here. 

That would change the logic that you have in MobileConnection.cpp.

Comment 5

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Patch v2

Also fixes the logic to the sessionEnded from a USSD session and progressing the message to the DOM even if it's empty.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 669981 [details] [diff] [review]
Patch v2

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

Nice! Only a few nits.

::: dom/network/src/MobileConnection.cpp
@@ +165,5 @@
> +      return NS_OK;
> +    }
> +
> +    nsString message;
> +    bool sessionEnded = false;

nit: declare this variables after decoding the JSON. It seems that you don't need them until then.

@@ +191,5 @@
> +    jsval sessionEndedJS;
> +    if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "sessionEnded", &sessionEndedJS)) {
> +      if (JSVAL_IS_BOOLEAN(sessionEndedJS)) {
> +        JSBool sessionEndedJSBool;
> +        sessionEndedJSBool = JSVAL_TO_BOOLEAN(sessionEndedJS);

Or |sessionEnded = JSVAL_TO_BOOLEAN(sessionEndedJS);|

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 669981 [details] [diff] [review]
Patch v2

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

This is good but need a few fixes.
What about waiting for bug 797806? It could make most of the work very trivial.

::: dom/network/interfaces/nsIDOMUSSDReceivedEvent.idl
@@ +8,4 @@
>  interface nsIDOMUSSDReceivedEvent : nsIDOMEvent
>  {
>    readonly attribute DOMString message;
> +  readonly attribute boolean sessionEnded;

Could you add [infallible] to this attribute, please?

::: dom/network/src/MobileConnection.cpp
@@ +160,5 @@
> +    nsString recvMessage;
> +    recvMessage.Assign(aData);
> +
> +    if (recvMessage.IsEmpty()) {
> +      NS_ERROR("Got a invalid 'ussd'!");

nit: "an invalid".

@@ +165,5 @@
> +      return NS_OK;
> +    }
> +
> +    nsString message;
> +    bool sessionEnded = false;

As Fernando said.

@@ +171,5 @@
> +    nsresult rv;
> +    nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> +    NS_ENSURE_STATE(sc);
> +    JSContext* cx = sc->GetNativeContext();
> +    NS_ASSERTION(cx, "Failed to get a context!");

Use NS_ENSURE_STATE(cx) here.

@@ +180,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    jsval messageJS;
> +    if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "message", &messageJS)) {
> +      if (JSVAL_IS_STRING(messageJS)) {

should be:
if (foo && bar) {
}
instead of:
if (foo) {
  if (bar) {
  }
}

Also, please use the C++ JS API, not the C one. See JS::Value in js/src/jsapi.h

JSVAL_TO_OBJECT => error->toObjectOrNull()
QSVAL_IS_STRING => messageJS->isString()

@@ +185,5 @@
> +        nsDependentJSString str;
> +        str.init(cx, messageJS.toString());
> +        message.Assign(str);
> +      }
> +    }

What do you do in case of failure?

@@ +189,5 @@
> +    }
> +
> +    jsval sessionEndedJS;
> +    if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "sessionEnded", &sessionEndedJS)) {
> +      if (JSVAL_IS_BOOLEAN(sessionEndedJS)) {

Ditto. In addition:
JSVAL_IS_BOOLEAN => sessionEndedJS.isBoolean()

@@ +191,5 @@
> +    jsval sessionEndedJS;
> +    if (JS_GetProperty(cx, JSVAL_TO_OBJECT(error), "sessionEnded", &sessionEndedJS)) {
> +      if (JSVAL_IS_BOOLEAN(sessionEndedJS)) {
> +        JSBool sessionEndedJSBool;
> +        sessionEndedJSBool = JSVAL_TO_BOOLEAN(sessionEndedJS);

Even better:
sessionEnded = sessionEndedJS->toBoolean()

@@ +196,5 @@
> +        if (sessionEndedJSBool) {
> +          sessionEnded = true;
> +        }
> +      }
> +    }

Same question for the failure.

::: dom/system/gonk/ril_worker.js
@@ -4837,5 @@
>  
> -  // Empty message should not be progressed to the DOM.
> -  if (!message || message == "") {
> -    return;
> -  }

Why did you remove that code?

Comment 8

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Patch v3, uses patch from bug 797806

This uses the patch from bug 797806 (and adds the content of such bug to re-review it).

Just tested and working.

Comment 9

[Guillermo López :willyaranda (probably SLOW response)]

(In reply to Mounir Lamouri (:mounir) from comment #7)
> Comment on attachment 669981 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 669981 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is good but need a few fixes.
> What about waiting for bug 797806? It could make most of the work very
> trivial.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ -4837,5 @@
> >  
> > -  // Empty message should not be progressed to the DOM.
> > -  if (!message || message == "") {
> > -    return;
> > -  }
> 
> Why did you remove that code?

Fernando told me that empty USSD should progress to the DOM

Comment 10

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Same as patch 3 but without bug 797806 parts

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 671546 [details] [diff] [review]
Patch v3, uses patch from bug 797806

>From 2c6986e9bc3fb31d643c6fbf8f1bb30e8893a6b9 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Guillermo=20Lo=CC=81pez?= <[e-mail]>
>Date: Mon, 15 Oct 2012 19:38:45 +0200
>Subject: [PATCH] Bug 794011. WIP
>
>---
> dom/bindings/BindingUtils.cpp                      | 21 +++++++++++++++++++++
> dom/bindings/BindingUtils.h                        |  9 +++++++++
> dom/bindings/Codegen.py                            | 16 ++++++++++++++++
> dom/network/interfaces/nsIDOMUSSDReceivedEvent.idl |  3 ++-
> dom/network/src/MobileConnection.cpp               | 12 +++++++-----
> dom/network/src/USSDReceivedEvent.cpp              | 10 +++++++++-
> dom/network/src/USSDReceivedEvent.h                |  3 ++-
> dom/system/gonk/RILContentHelper.js                |  5 +++--
> dom/system/gonk/ril_worker.js                      |  9 +++------
> dom/webidl/USSDReceivedEvent.webidl                | 11 +++++++++++
> dom/webidl/WebIDL.mk                               |  1 +
> 11 files changed, 84 insertions(+), 16 deletions(-)
> create mode 100644 dom/webidl/USSDReceivedEvent.webidl
>
>diff --git a/dom/bindings/BindingUtils.cpp b/dom/bindings/BindingUtils.cpp
>index 2f472a1..51c9cd3 100644
>--- a/dom/bindings/BindingUtils.cpp
>+++ b/dom/bindings/BindingUtils.cpp
>@@ -14,6 +14,8 @@
> #include "XPCQuickStubs.h"
> #include "nsIXPConnect.h"
> 
>+#include "nsContentUtils.h"
>+
> namespace mozilla {
> namespace dom {
> 
>@@ -739,5 +741,24 @@ SetXrayExpandoChain(JSObject* obj, JSObject* chain)
>   }
> }
> 
>+JSContext*
>+MainThreadDictionaryBase::ParseJSON(const nsAString& aJSON,
>+                                    mozilla::Maybe<JSAutoRequest>& aAr,
>+                                    mozilla::Maybe<JSAutoCompartment>& aAc,
>+                                    JS::Value& aVal)
>+{
>+  JSContext* cx = nsContentUtils::ThreadJSContextStack()->GetSafeJSContext();
>+  NS_ENSURE_TRUE(cx, nullptr);
>+  JSObject* global = JS_GetGlobalObject(cx);
>+  aAr.construct(cx);
>+  aAc.construct(cx, global);
>+  if (!JS_ParseJSON(cx,
>+                    static_cast<const jschar*>(PromiseFlatString(aJSON).get()),
>+                    aJSON.Length(), &aVal)) {
>+    return nullptr;
>+  }
>+  return cx;
>+}
>+
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/bindings/BindingUtils.h b/dom/bindings/BindingUtils.h
>index 3af15b9..980a67b 100644
>--- a/dom/bindings/BindingUtils.h
>+++ b/dom/bindings/BindingUtils.h
>@@ -1227,6 +1227,15 @@ MustInheritFromNonRefcountedDOMObject(NonRefcountedDOMObject*)
> JSObject* GetXrayExpandoChain(JSObject *obj);
> void SetXrayExpandoChain(JSObject *obj, JSObject *chain);
> 
>+struct MainThreadDictionaryBase
>+{
>+protected:
>+  JSContext* ParseJSON(const nsAString& aJSON,
>+                       mozilla::Maybe<JSAutoRequest>& aAr,
>+                       mozilla::Maybe<JSAutoCompartment>& aAc,
>+                       JS::Value& aVal);
>+};
>+
> } // namespace dom
> } // namespace mozilla
> 
>diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py
>index 5fcc6e6..ebefd01 100644
>--- a/dom/bindings/Codegen.py
>+++ b/dom/bindings/Codegen.py
>@@ -5390,6 +5390,9 @@ class CGDictionary(CGThing):
>         d = self.dictionary
>         if d.parent:
>             inheritance = ": public %s " % self.makeClassName(d.parent)
>+        elif not self.workers:
>+            inheritance = ": public MainThreadDictionaryBase "
>+
>         else:
>             inheritance = ""
>         memberDecls = ["  %s %s;" %
>@@ -5400,6 +5403,19 @@ class CGDictionary(CGThing):
>                 "struct ${selfName} ${inheritance}{\n"
>                 "  ${selfName}() {}\n"
>                 "  bool Init(JSContext* cx, const JS::Value& val);\n"
>+                "  \n" +
>+                ("  bool Init(const nsAString& aJSON)\n"
>+                "  {\n"
>+                "    if (aJSON.IsEmpty()) {\n"
>+                "      return true;\n"
>+                "    }\n"
>+                "    mozilla::Maybe<JSAutoRequest> ar;\n"
>+                "    mozilla::Maybe<JSAutoCompartment> ac;\n"
>+                "    jsval json = JSVAL_VOID;\n"
>+                "    JSContext* cx = ParseJSON(aJSON, ar, ac, json);\n"
>+                "    NS_ENSURE_TRUE(cx, false);\n"
>+                "    return Init(cx, json);\n"
>+                "  }\n" if not self.workers else "") +
>                 "\n" +
>                 "\n".join(memberDecls) + "\n"
>                 "private:\n"

Please don't include my patch here ;)


> 
>   if (!strcmp(aTopic, kUssdReceivedTopic)) {
>-    nsString ussd;
>-    ussd.Assign(aData);
>-    nsRefPtr<USSDReceivedEvent> event = USSDReceivedEvent::Create(ussd);
>+    mozilla::dom::USSDReceivedEventDict dict;
>+    dict.Init(nsDependentString(aData));
Please check the return value.
NS_ENSURE_TRUE(dict.Init(nsDependentString(aData)));
should work

>+    nsRefPtr<USSDReceivedEvent> event =
>+              USSDReceivedEvent::Create(dict.message, dict.sessionEnded);
Indentation is odd here.
      nsRefPtr<USSDReceivedEvent> event =
        USSDReceivedEvent::Create(dict.message, dict.sessionEnded);


>+++ b/dom/webidl/USSDReceivedEvent.webidl
Maybe USSDReceivedEventDict.webidl
>+++ b/dom/webidl/WebIDL.mk
>@@ -43,6 +43,7 @@ webidl_files = \
>   SVGTransformList.webidl \
>   TextDecoder.webidl \
>   TextEncoder.webidl \
>+  USSDReceivedEvent.webidl \
>   WebSocket.webidl \
>   XMLHttpRequest.webidl \
>   XMLHttpRequestEventTarget.webidl \

You should #ifdef your file so that it is compile only for B2G.

Comment 12

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Patch v4

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 671773 [details] [diff] [review]
Patch v4

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

It's way nicer on top of Olli's patch! :)

r=me with the things below fixed.

::: dom/network/src/MobileConnection.cpp
@@ +159,5 @@
>    }
>  
>    if (!strcmp(aTopic, kUssdReceivedTopic)) {
> +    mozilla::dom::USSDReceivedEventDict dict;
> +    NS_ENSURE_TRUE(dict.Init(nsDependentString(aData)));

NS_ENSURE_TRUE() needs a second parameter. And it has to be NS_ENSURE_SUCCESS() here, AFAICT.
Please do:
nsresult rv = dict.Init(nsDependentString(aData));
NS_ENSURE_SUCCESS(rv, rv);

@@ +166,3 @@
>      NS_ASSERTION(event, "This should never fail!");
>  
> +    nsresult rv = event->Dispatch(ToIDOMEventTarget(), USSDRECEIVED_EVENTNAME);

this will have to be:
rv = ...;

::: dom/network/src/USSDReceivedEvent.cpp
@@ +41,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +USSDReceivedEvent::GetSessionEnded(bool* aSessionEnded)

Could you put a comment saying that this method is [infallible]. Unfortunately, we currently don't have any way to check if the [infallible] promise is respected.

I would do:
/* [infallible] */ NS_IMETHODIMP
USSDReceivedEvent::GetSessionEnded(bool* aSessionEnded)

Comment 14

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Patch v5. Comments addressed

Comment 15

[Guillermo López :willyaranda (probably SLOW response)]

Bug 797806  has landed, so this can land safely

Comment 16

[Guillermo López :willyaranda (probably SLOW response)]

Fernando, Dietrich asked on IRC if this should be basecamp blocker because of device certification and I don't have an answer, do you know something?

Comment 17

[Beatriz Rodríguez [:brg]]

(In reply to Guillermo López (:willyaranda) from comment #16)
> Fernando, Dietrich asked on IRC if this should be basecamp blocker because
> of device certification and I don't have an answer, do you know something?

I can confirm that this is important for device certification.

Comment 18

[Mounir Lamouri (:mounir)]

Asking for basecamp-blocker per comment 16 and comment 17.

Comment 19

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04c1835aa9c7

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

After discussing with Guillermo and Beatriz we agreed that this bug should probably not be considered as blocker but as a nice-to-have.

The reason is that the addition of this flag within the 'ussdreceived' event would be useful to close an existing USSD dialog on the UI when the USSD session is closed. But the user can still close it manually when she decides to.

Comment 21

[Mounir Lamouri (:mounir)]

Comment on attachment 671831 [details] [diff] [review]
Patch v5. Comments addressed

B2G patch considered nice to have, see comments 16 to 20.

Comment 22

[Andrew Overholt [:overholt]]

Marking not a blocker based on comment #20.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04c1835aa9c7

Comment 24

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/04c1835aa9c7

Comment 25

[Alex Keybl [:akeybl]]

(In reply to Mounir Lamouri (:mounir) from comment #21)
> Comment on attachment 671831 [details] [diff] [review]
> Patch v5. Comments addressed
> 
> B2G patch considered nice to have, see comments 16 to 20.

We've sent email to the B2G triage guys to get their opinion.

Comment 26

[Andrew Overholt [:overholt]]

While this isn't a blocker, I don't see any problems with this landing on aurora as it is nice to have.

Comment 27

[bhavana bajaj [:bajaj]]

Comment on attachment 671831 [details] [diff] [review]
Patch v5. Comments addressed

Approving for aurora as it is a nice to have for b2g .

Comment 28

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a8f378b5da9

Comment 29

[Michael Wu [:mwu]]

On aurora:

/home/mikew/moz/B2G/gecko/dom/network/src/MobileConnection.cpp: In member function 'virtual nsresult mozilla::dom::network::MobileConnection::Observe(nsISupports*, const char*, const PRUnichar*)':
/home/mikew/moz/B2G/gecko/dom/network/src/MobileConnection.cpp:166: error: no matching function for call to 'mozilla::dom::USSDReceivedEventDict::Init(nsDependentString)'
../../../dist/include/mozilla/dom/USSDReceivedEventBinding.h:30: note: candidates are: bool mozilla::dom::USSDReceivedEventDict::Init(JSContext*, const JS::Value&)

I will be backing this out shortly unless someone tells me they're looking at it.

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

The cause of that error is that the patch for Bug 797806 is not on Aurora.

Comment 31

[Michael Wu [:mwu]]

Backed out on aurora.

http://hg.mozilla.org/releases/mozilla-aurora/rev/79200d477ad7

Comment 32

[Guillermo López :willyaranda (probably SLOW response)]

Comment on attachment 671831 [details] [diff] [review]
Patch v5. Comments addressed

This was approved for aurora before but backedout due to a missing bug (797806) that was not landed on aurora and caused a bustage because this patch depends on the other patch.

Bug 797806 was approved to land on Aurora so this can land again but _after the other patch lands_.

Requesting approval again.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a1f5eec91225