Closed Bug 794011 Opened 7 years ago Closed 7 years ago

MMI Codes: Add 'sessionEnded' to nsIDOMUSSDReceivedEvent

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp -
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: ferjm, Assigned: willyaranda)

References

Details

(Whiteboard: [good first bug][lang=cpp+js][mentor=ferjm])

Attachments

(1 file, 5 obsolete files)

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)
Whiteboard: [good first bug][lang=cpp+js][mentor=ferjm]
Assignee: nobody → willyaranda
This functionality is important to have fully implemented USSD functionality in the platform.
Attached patch Patch (obsolete) — Splinter Review
Attachment #669592 - Flags: feedback?(ferjmoreno)
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
Attachment #669592 - Flags: feedback?(ferjmoreno) → feedback+
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.
Attached patch Patch v2 (obsolete) — Splinter Review
Also fixes the logic to the sessionEnded from a USSD session and progressing the message to the DOM even if it's empty.
Attachment #669592 - Attachment is obsolete: true
Attachment #669981 - Flags: feedback?(ferjmoreno)
Attachment #669981 - Attachment is patch: true
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);|
Attachment #669981 - Flags: feedback?(ferjmoreno) → feedback+
Depends on: 797806
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?
Attachment #669981 - Flags: review?(mounir)
Attachment #669981 - Flags: review-
Attachment #669981 - Flags: feedback+
This uses the patch from bug 797806 (and adds the content of such bug to re-review it).

Just tested and working.
Attachment #669981 - Attachment is obsolete: true
Attachment #671546 - Flags: review?(mounir)
Attachment #671546 - Flags: feedback?(bugs)
(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
Attachment #671546 - Attachment is obsolete: true
Attachment #671546 - Flags: review?(mounir)
Attachment #671546 - Flags: feedback?(bugs)
Attachment #671552 - Flags: review?(mounir)
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?= <willyaranda@gmail.com>
>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.
Attachment #671546 - Attachment is obsolete: false
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #671552 - Attachment is obsolete: true
Attachment #671552 - Flags: review?(mounir)
Attachment #671773 - Flags: review?(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)
Attachment #671773 - Flags: review?(mounir) → review+
Attachment #671546 - Attachment is obsolete: true
Attachment #671773 - Attachment is obsolete: true
Bug 797806  has landed, so this can land safely
Status: NEW → ASSIGNED
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?
Flags: needinfo?(ferjmoreno)
Status: ASSIGNED → NEW
(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.
Asking for basecamp-blocker per comment 16 and comment 17.
blocking-basecamp: --- → ?
https://hg.mozilla.org/integration/mozilla-inbound/rev/04c1835aa9c7
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla19
Attachment #671831 - Flags: checkin?(mounir) → checkin+
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.
Flags: needinfo?(ferjmoreno)
Comment on attachment 671831 [details] [diff] [review]
Patch v5. Comments addressed

B2G patch considered nice to have, see comments 16 to 20.
Attachment #671831 - Flags: approval-mozilla-aurora?
Marking not a blocker based on comment #20.
blocking-basecamp: ? → -
https://hg.mozilla.org/mozilla-central/rev/04c1835aa9c7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(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.
Flags: needinfo?(overholt)
While this isn't a blocker, I don't see any problems with this landing on aurora as it is nice to have.
Flags: needinfo?(overholt)
Comment on attachment 671831 [details] [diff] [review]
Patch v5. Comments addressed

Approving for aurora as it is a nice to have for b2g .
Attachment #671831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
The cause of that error is that the patch for Bug 797806 is not on Aurora.
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.
Attachment #671831 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Attachment #671831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 805789
You need to log in before you can comment on or make changes to this bug.