Closed Bug 908083 Opened 7 years ago Closed 6 years ago

B2G STK: support "browser termination event" Envelope command

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.2 C3(Oct25)

People

(Reporter: gwang, Assigned: gwang)

References

Details

Attachments

(3 files, 13 obsolete files)

2.72 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
3.55 KB, patch
Details | Diff | Splinter Review
5.04 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → gwang
Blocks: b2g-stk
Attachment #798746 - Flags: review?(allstars.chh)
Status: NEW → ASSIGNED
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 ?
Attachment #798746 - Flags: review?(allstars.chh) → review-
(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...
Attachment #798746 - Attachment is obsolete: true
Attachment #799310 - Flags: review?(allstars.chh)
Attachment #799312 - Flags: review?(allstars.chh)
Attachment #799314 - Flags: review?(allstars.chh)
Try server result pass!
https://tbpl.mozilla.org/?tree=Try&rev=90b402b69db8
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.
Attachment #799310 - Flags: review?(allstars.chh)
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.
Attachment #799312 - Flags: review?(allstars.chh) → review+
Attachment #799314 - Flags: review?(allstars.chh) → review+
Attachment #799312 - Attachment is obsolete: true
Attachment #799310 - Attachment is obsolete: true
Attachment #799328 - Flags: review?(allstars.chh)
Attachment #799328 - Attachment is obsolete: true
Attachment #799328 - Flags: review?(allstars.chh)
Attachment #799334 - Flags: review?(allstars.chh)
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 ...
Attachment #799334 - Flags: superreview?(jonas)
Attachment #799334 - Flags: review?(allstars.chh)
Attachment #799334 - Flags: review+
Attachment #799334 - Attachment is obsolete: true
Attachment #799334 - Flags: superreview?(jonas)
Attachment #799343 - Flags: superreview?(jonas)
Blocks: 908032
No longer blocks: 908032
Attachment #799325 - Attachment is obsolete: true
Attachment #799314 - Attachment is obsolete: true
Comment on attachment 799343 [details] [diff] [review]
Part1: "Browser Termination" event IDL. v4

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

Over to Hsin-Yi
Attachment #799343 - Flags: superreview?(jonas) → review?(htsai)
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?
Attachment #799343 - Flags: review?(htsai) → review+
According to reviewer's suggestion, modify comment in SimToolKit.idl and nsIDOMIccManager.idl
Attachment #799343 - Attachment is obsolete: true
Attachment #816921 - Attachment is obsolete: true
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Version: Trunk → unspecified
Please add r=me and run the try-server before landing these patches.
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
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 :)
Attachment #805827 - Attachment is obsolete: true
Attachment #817116 - Flags: review?(allstars.chh)
Attachment #817116 - Flags: review?(allstars.chh) → review+
Add reviewer = Yoshi into patch title.
Attachment #816935 - Attachment is obsolete: true
Please rebase your patches, I got conflict when trying pushing them.
Rebase conflict file.
Attachment #819660 - Attachment is obsolete: true
Rebase.
Attachment #805825 - Attachment is obsolete: true
Blocks: 908535
You need to log in before you can comment on or make changes to this bug.