908083 - B2G STK: support "browser termination event" Envelope command

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Georgia Wang]

Per TS 102.223

If the browser termination event is part of the event list (as set up by the last SET UP EVENT LIST command, see
clause 6.4.16), then when the browser is terminated either by the user action or by an error, the terminal shall inform the
UICC that this has occurred, by using the ENVELOPE (EVENT DOWNLOAD - browser termination) command as
defined in clause 7.5.9.2.

Comment 1

[Georgia Wang]

Attachment: B2G STK: support "browser termination event" Envelope command

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 798746 [details] [diff] [review]
B2G STK: support "browser termination event" Envelope command

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

It seems you forgot updating IDL.

Please split your patches to
Part 1: IDL.
Part 2: implementation.
Part 3: tests.

::: dom/system/gonk/ril_worker.js
@@ +2635,5 @@
> +        command.deviceId = {
> +          sourceId: STK_DEVICE_ID_ME,
> +          destinationId: STK_DEVICE_ID_SIM
> +        };
> +        command.terminationCause=command.event.terminationCause;

Where does terminationCause come from ?

Comment 3

[Georgia Wang]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #2)
> Comment on attachment 798746 [details] [diff] [review]
> B2G STK: support "browser termination event" Envelope command
> 
> Review of attachment 798746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems you forgot updating IDL.
> 
> Please split your patches to
> Part 1: IDL.
> Part 2: implementation.
> Part 3: tests.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2635,5 @@
> > +        command.deviceId = {
> > +          sourceId: STK_DEVICE_ID_ME,
> > +          destinationId: STK_DEVICE_ID_SIM
> > +        };
> > +        command.terminationCause=command.event.terminationCause;
> 
> Where does terminationCause come from ?

Oh...yes I forget IDL part...

Comment 4

[Georgia Wang]

Attachment: Part1: "Browser Termination" event IDL

Comment 5

[Georgia Wang]

Attachment: Part2: "Browser Termination" event implement in RIL

Comment 6

[Georgia Wang]

Attachment: Part3: "Browser Termination" event xpeshell test

Comment 7

[Georgia Wang]

Try server result pass!
https://tbpl.mozilla.org/?tree=Try&rev=90b402b69db8

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 799310 [details] [diff] [review]
Part1: "Browser Termination" event IDL

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

::: dom/icc/interfaces/SimToolKit.idl
@@ +634,5 @@
> +  /**
> +   * This object shall contain the browser termination cause.
> +   * See TZ 102 223 8.51
> +   */
> +  unsigned short terminationCause;

Please add the constants for cause in nsIDOMIccManager.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 799312 [details] [diff] [review]
Part2: "Browser Termination" event implement in RIL

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

::: dom/system/gonk/ril_worker.js
@@ +2635,5 @@
> +        command.deviceId = {
> +          sourceId: STK_DEVICE_ID_ME,
> +          destinationId: STK_DEVICE_ID_SIM
> +        };
> +        command.terminationCause=command.event.terminationCause;

nit, space between operators.

Comment 10

[Georgia Wang]

Attachment: Part2: "Browser Termination" event implement in RIL. v2

Comment 11

[Georgia Wang]

Attachment: Part1: "Browser Termination" event IDL. v2

Comment 12

[Georgia Wang]

Attachment: Part1: "Browser Termination" event IDL. v3

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 799334 [details] [diff] [review]
Part1: "Browser Termination" event IDL. v3

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

Please update the comments for the terminationCause.

Ask for a sr? to Jonas.

::: dom/icc/interfaces/SimToolKit.idl
@@ +632,5 @@
> +  unsigned short eventType;
> +
> +  /**
> +   * This object shall contain the browser termination cause.
> +   * See TZ 102 223 8.51

The cause of this event,
one of nsIDOMIccManager.STK_BROWSER_TERMINATION_CAUSE_USER and ...

Comment 14

[Georgia Wang]

Attachment: Part1: "Browser Termination" event IDL. v4

Comment 15

[Georgia Wang]

Attachment: Part2: "Browser Termination" event implement in RIL. v3

Comment 16

[Georgia Wang]

Attachment: Part3: "Browser Termination" event xpeshell test. v2

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 799343 [details] [diff] [review]
Part1: "Browser Termination" event IDL. v4

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

Over to Hsin-Yi

Comment 18

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

Comment on attachment 799343 [details] [diff] [review]
Part1: "Browser Termination" event IDL. v4

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

r=me with comments addressed. Thank you.

::: dom/icc/interfaces/SimToolKit.idl
@@ +632,5 @@
> +  unsigned short eventType;
> +
> +  /**
> +   * This object shall contain the browser termination cause.
> +   * See TZ 102 223 8.51, It shall be one of following:

nit: See TZ 102 223 8.51. <-- period here ;)

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +229,5 @@
>     * @param command
>     *        Command received from ICC. See MozStkCommand.
>     * @param response
>     *        The response that will be sent to ICC.
>     * @see MozStkResponse for the detail of response.

As a new MozStkBrowserTerminationEvent is created, we need to modify the comments "@param event" for sendStkEventDownload(), right?

Comment 19

[Georgia Wang]

Attachment: Part1: "Browser Termination" event IDL. v5

According to reviewer's suggestion, modify comment in SimToolKit.idl and nsIDOMIccManager.idl

Comment 20

[Georgia Wang]

Attachment: Part1: "Browser Termination" event IDL. v5

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Please add r=me and run the try-server before landing these patches.

Comment 22

[Georgia Wang]

I do a little modification in Part3 since the change of ReadUint32()->ReadInt32().

Latest try-server result in below link:
https://tbpl.mozilla.org/?tree=Try&rev=c9a8b5b21a48

Comment 23

[Georgia Wang]

Attachment: Part3: "Browser Termination" event xpeshell test. v3

I try to apply patch on current codebase and find we need to use "ReadInt32" instead of "ReadUint32" now. So here's the new patch with this change :)

Comment 24

[Georgia Wang]

Attachment: Part1: "Browser Termination" event IDL. v6

Add reviewer = Yoshi into patch title.

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Please rebase your patches, I got conflict when trying pushing them.

Comment 26

[Georgia Wang]

Attachment: Part1: "Browser Termination" event IDL. v7

Rebase conflict file.

Comment 27

[Georgia Wang]

Attachment: Part2: "Browser Termination" event implement in RIL. v4

Rebase.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c44d0a4ab944
https://hg.mozilla.org/integration/b2g-inbound/rev/f5dbf24ff1f2
https://hg.mozilla.org/integration/b2g-inbound/rev/dcab3b214724

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c44d0a4ab944
https://hg.mozilla.org/mozilla-central/rev/f5dbf24ff1f2
https://hg.mozilla.org/mozilla-central/rev/dcab3b214724

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

CC Anshul here.