942740 - [B2G] [DSDS] USSD received event for DSDS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

Currently, the message indicated that ussd is received doesn't carry any information about serviceId. In DSDS, gaia needs to know this message is generated by which service.

The possible solution is adding "serviceId" into broadcast message.
something like,
gSystemMessenger.broadcastMessage("ussd-received", {serviceId: serviceId, ussd: ussd});

Comment 1

[Ken Chang[:kenkai]]

Move this bug in backlog.

Comment 2

[Gabriele Svelto [:gsvelto]]

While working on bug 1002327 I've noticed that the 'ussd-received' event currently looks like this:

{
  rilMessageType: USSDReceived,
  message: <a string>
  sessionEnded: <a boolean>
  rilMessageClientId: <a number>
}

What does the |rilMessageClientId| stand for? I've quickly glanced at the RIL code and this seems to originate from the same field used for the |serviceId| in other messages but I'm not 100% sure of that. Can you please confirm if this is the same field or not? If it is shall we just rename it so that it's called |serviceId| instead?

Comment 3

[Edgar Chen [:edgar]]

(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> While working on bug 1002327 I've noticed that the 'ussd-received' event
> currently looks like this:
> 
> {
>   rilMessageType: USSDReceived,
>   message: <a string>
>   sessionEnded: <a boolean>
>   rilMessageClientId: <a number>
> }
> 
> What does the |rilMessageClientId| stand for? I've quickly glanced at the
> RIL code and this seems to originate from the same field used for the
> |serviceId| in other messages but I'm not 100% sure of that. Can you please
> confirm if this is the same field or not? If it is shall we just rename it
> so that it's called |serviceId| instead?

Yes, the value of |rilMessageClientId| is the same as |serviceId|, but it is used for internal ril code, should not be exposed (|rilMessageType| should not, either).

So the event shall looks like:

{
   message: <a string>
   sessionEnded: <a boolean>
   serviceId: <a number>
}

Will address it in this bug.

Thank you for catching this.

Comment 4

[Edgar Chen [:edgar]]

Noming since this blocks a 1.3T blocker, bug 1002327.

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

triage: blocks a blocker. 1.3T+

Comment 6

[Edgar Chen [:edgar]]

Attachment: WIP, patch, v1

Comment 7

[Gabriele Svelto [:gsvelto]]

(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> Yes, the value of |rilMessageClientId| is the same as |serviceId|, but it is
> used for internal ril code, should not be exposed (|rilMessageType| should
> not, either).
> 
> So the event shall looks like:
> 
> {
>    message: <a string>
>    sessionEnded: <a boolean>
>    serviceId: <a number>
> }
> 
> Will address it in this bug.
> 
> Thank you for catching this.

Excellent, thanks Edgar, I'll proceed with the implementation of bug 1002327 using your WIP patch for testing.

Comment 8

[Edgar Chen [:edgar]]

I am trying to add some test cases for it now.

Comment 9

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> I am trying to add some test cases for it now.

To have a test case for ussd, we need emulator support first.
But now there are some works need to be done in bug 1007535, so I would like to land the fix first. And keeps working on test case part in bug 1007535.

Thank you.

Comment 10

[Edgar Chen [:edgar]]

Attachment: Patch, v2

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8420751 [details] [diff] [review]
Patch, v2

Hi Hsinyi, could you help to review this? Thank you.

Comment 12

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

Comment on attachment 8420751 [details] [diff] [review]
Patch, v2

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

Good!

Comment 13

[Edgar Chen [:edgar]]

Attachment: [1.3t] Patch, v2, r=hsinyi, a=1.3t+

Patch for 1.3t

Comment 14

[Edgar Chen [:edgar]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=b492c5d9dd0a

Comment 15

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/09bf28f2fa79

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09bf28f2fa79

Comment 17

[Anshul]

Comment on attachment 8420796 [details] [diff] [review]
[1.3t] Patch, v2, r=hsinyi, a=1.3t+

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2766,5 @@
>      }
>    },
>  
>    handleUSSDReceived: function handleUSSDReceived(ussd) {
> +    gSystemMessenger.broadcastMessage("ussd-received",

For all broadcast messages I see that we are creating a new format for the data being passed instead of following the format used by messages like sendCellBroadcastMessage for example, that sends data in the following format; just to be consistent.

      this._sendTargetMessage("cellbroadcast", message, {
        clientId: clientId,
        data: data
      });

Is there a specific reason?

Comment 18

[Edgar Chen [:edgar]]

(In reply to Anshul from comment #17)
> Comment on attachment 8420796 [details] [diff] [review]
> [1.3t] Patch, v2, r=hsinyi, a=1.3t+
> 
> Review of attachment 8420796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +2766,5 @@
> >      }
> >    },
> >  
> >    handleUSSDReceived: function handleUSSDReceived(ussd) {
> > +    gSystemMessenger.broadcastMessage("ussd-received",
> 
> For all broadcast messages I see that we are creating a new format for the
> data being passed instead of following the format used by messages like
> sendCellBroadcastMessage for example, that sends data in the following
> format; just to be consistent.
> 
>       this._sendTargetMessage("cellbroadcast", message, {
>         clientId: clientId,
>         data: data
>       });
> 
> Is there a specific reason?

|clientId| is used for indicating the sim slot (not part of |data|), and |data| could be null in some case, so we separate them into different attribute. Does this answer your question? Please let me know if it is not clear to you. Thank you.

Comment 19

[Anshul]

(In reply to Edgar Chen [:edgar][:echen] from comment #18)
> (In reply to Anshul from comment #17)
> > Comment on attachment 8420796 [details] [diff] [review]
> > [1.3t] Patch, v2, r=hsinyi, a=1.3t+
> > 
> > Review of attachment 8420796 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +2766,5 @@
> > >      }
> > >    },
> > >  
> > >    handleUSSDReceived: function handleUSSDReceived(ussd) {
> > > +    gSystemMessenger.broadcastMessage("ussd-received",
> > 
> > For all broadcast messages I see that we are creating a new format for the
> > data being passed instead of following the format used by messages like
> > sendCellBroadcastMessage for example, that sends data in the following
> > format; just to be consistent.
> > 
> >       this._sendTargetMessage("cellbroadcast", message, {
> >         clientId: clientId,
> >         data: data
> >       });
> > 
> > Is there a specific reason?
> 
> |clientId| is used for indicating the sim slot (not part of |data|), and
> |data| could be null in some case, so we separate them into different
> attribute. Does this answer your question? Please let me know if it is not
> clear to you. Thank you.
Thanks for the info Edgar. My only concern is that the format of message being send as system message and then one sent to a target is inconsistent.

Comment 20

[James Zhang (Spreadtrum)]

1.3t please

Comment 21

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/3b6c97b5fe8b

Comment 22

[yang.zhao]

(In reply to James Zhang from comment #20)
> 1.3t please

Gecko patch.
Seems already landed it from comment 21.

Comment 23

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8420751 [details] [diff] [review]
Patch, v2

I'm requesting approval for this as it blocks bug 1002327 which in turn blocks bug 1019370 which is a 1.4+ partner blocker.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Missing DSDS support for USSD messages in the platform
User impact if declined: Cannot support sending MMI codes and receiving USSD messages on the second SIM on DSDS-devices
Testing completed: This has been tested on both master and v1.3t where it already landed some time ago
Risk to taking this patch (and alternatives if risky): None that I know of, this patch also fixes the object handed over to the event receiver, without this patch the said objects contains some incorrect fields.
String or UUID changes made by this patch: None

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8420751 [details] [diff] [review]
Patch, v2

Approved for 1.4.

Comment 25

[Gabriele Svelto [:gsvelto]]

Attachment 8420751 [details] [diff] needs to land on the mozilla-b2g30_v1_4 branch. Last time I checked the patch applied cleanly.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c771eb11c80d