1091454 - [B2G][STK] Icon Display for SET_UP_CALL and LAUNCH_BROWSER is not supported properly.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

When reviewing the implementation of these 2 commands among Gaia/MozStkCommandEvent/ril_worker.js,
I found 2 problems as followed:
1. In gaia/ril_worker, the confirmMessage and callMessage are String.
   However, in MozStkCommandEvent.weidl, they are specified as MozStkTextMessage
   which is a complicated structure with text, duration, icon, etc.
2. For SET_UP_CALL, it is possible to have 2 icons (one for confirmMessage; the 
   other for callMessage). 
   However, in ril_worker, only the first icon is retrieved.

File this bug to:
1. Specify confirmMessage/callMessage as DOM String.
2. Add new properties for MozStkSetUpCall:
   MozStkIconContainer confirmIcon;
   MozStkIconContainer callIcon;
3. Add new property for MozStkBrowserSetting:
   MozStkIconContainer confirmIcon;
4. Remove the extension from MozStkIconContainer for MozStkSetUpCall and 
   MozStkBrowserSetting.
5. Load icons properly in ril_worker.js for SET_UP_CALL and LAUNCH_BROWSER

Comment 1

[Jessica Jong [:jessica] (inactive)]

Thanks Bevis for reporting this. We have filed Bug 1062222 for the second icon of SET_UP_CALL. My idea was to put the confirm and call icons in MozStkTextMessage, since MozStkTextMessage inherits from MozStkIconContainer, then we don't need to add confirmIcon and callIcon. What do you think?

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #1)
> Thanks Bevis for reporting this. We have filed Bug 1062222 for the second
> icon of SET_UP_CALL. My idea was to put the confirm and call icons in
> MozStkTextMessage, since MozStkTextMessage inherits from
> MozStkIconContainer, then we don't need to add confirmIcon and callIcon.
> What do you think?

Oops! Sorry for late response of your question. :p

Yes, ideally it is.
However, as mentioned in comment 0, both gaia/ril_worker treat confirmMessage/callMessage as DOM String instead of MozStkTextMessage. (See Bug 744714 for the 1st time ril_worker supports STK.)

We need some migration plan in gaia/ril_worker for your proposal to prevent function broken:
For example, 
1. Add these 2 attributes to have both text/icon support.
  MozStkTextMessage confirmInfo;
  MozStkTextMessage callInfo;
2. Deprecate confirmMessage, callMessage after changes in gaia is done.

In addition, patch in bug 1072808 has to be changed accordingly because it was based on the assumption in comment 0 currently. :p

Hi Hsinyi,

What do you think?

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #2)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #1)
> > Thanks Bevis for reporting this. We have filed Bug 1062222 for the second
> > icon of SET_UP_CALL. My idea was to put the confirm and call icons in
> > MozStkTextMessage, since MozStkTextMessage inherits from
> > MozStkIconContainer, then we don't need to add confirmIcon and callIcon.
> > What do you think?
> 
> Oops! Sorry for late response of your question. :p
> 
> Yes, ideally it is.
> However, as mentioned in comment 0, both gaia/ril_worker treat
> confirmMessage/callMessage as DOM String instead of MozStkTextMessage. (See
> Bug 744714 for the 1st time ril_worker supports STK.)
> 
> We need some migration plan in gaia/ril_worker for your proposal to prevent
> function broken:
> For example, 
> 1. Add these 2 attributes to have both text/icon support.
>   MozStkTextMessage confirmInfo;
>   MozStkTextMessage callInfo;
> 2. Deprecate confirmMessage, callMessage after changes in gaia is done.

Note: MozStkBrowserSetting can also follow the same rule for this change.
> 
> In addition, patch in bug 1072808 has to be changed accordingly because it
> was based on the assumption in comment 0 currently. :p
> Hi Hsinyi,
> 
> What do you think?

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Per offline discussion, we'll take the approach in comment 1 to comply what has been defined in MozStkCommandEvent.webidl.
That is, the implementation of the data type of confirmMessage/callMessage in MozStkSetUpCall/MozStkBrowserSetting will be changed to |MozStkTextMessage| instead of |DOMString|.
Hence, to be backward compatible, a Gaia bug is required to support both formats.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v1: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER.

This is to load the icons properly for SET_UP_CALL/LAUNCH_BROWSER.

Hi Hsinyi,

May I have your review for this change?

Thanks!

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v1: Modify Test Case to comply the new format of confirmMessage/callMessage.

Modify and add the test cases to ensure the test coverage.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8518136 [details] [diff] [review]
Part 2 v1: Modify Test Case to comply the new format of confirmMessage/callMessage.

Sorry, not aware of the stk_set_up_call test cases in xpcshell.
I'll update patch part 2 again for the review.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v2: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER.

update patch with positive test results in xpcshell & marionette.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v2: Modify Test Case to comply the new format of confirmMessage/callMessage.

update patch with positive test results in xpcshell & marionette.

Comment 11

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

Comment on attachment 8519678 [details] [diff] [review]
Part 1 v2: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER.

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

r=me for .webidl. The change makes sense and complies the spec.

Edgar, could you please help review the implementation part? Thank you.

Comment 12

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

Comment on attachment 8519679 [details] [diff] [review]
Part 2 v2: Modify Test Case to comply the new format of confirmMessage/callMessage.

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

Forwarding to Edgar :)

Comment 13

[Edgar Chen [:edgar]]

Comment on attachment 8519678 [details] [diff] [review]
Part 1 v2: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER.

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

Thanks for your work. The implementation fixes the icon issue in SET_UP_CALL indeed, but I would like to make it having better modularity.
Please see my comments below.

::: dom/system/gonk/ril_worker.js
@@ +10955,5 @@
>      }
>      call.address = ctlv.value.number;
>  
> +    // 1st iteration for ALPHA_IDs.
> +    ctlv = StkProactiveCmdHelper.searchForNextTag(COMPREHENSIONTLV_TAG_ALPHA_ID, iter);

As per offline discussion, it seems it's worth to have utility functions which returns all results found in ctlvs and maybe accept multiple tags for searching?

@@ +11002,5 @@
> +      // It's impossible to have icons for call phase but none for confirm phase.
> +      if (iconIds["confirm"]) {
> +        confirmMessage.icons = result[0];
> +        confirmMessage.iconSelfExplanatory =
> +          iconIds["confirm"].qualifier == 0 ? true: false;

And could we also have an utility function for loading multiple stk icons? Something like |loadIconIfNecessary| without sending chrome message, but triggering the callback function passed by caller when it finishes the loading process.

Comment 14

[Edgar Chen [:edgar]]

Comment on attachment 8519679 [details] [diff] [review]
Part 2 v2: Modify Test Case to comply the new format of confirmMessage/callMessage.

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

Looks good, thank you.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.1 v2: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons.

Address the suggestion in comment 13 to refactor searchForTag/loadIconIfNecessary.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.2 v2: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER.

apply the new API in part1.1 to rewrite SETUP_CALL/LAUNCH_BROWSER.

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8522015 [details] [diff] [review]
Part 1.1 v2: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons.

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

Per off line discussion, the |StkCommandParamsFactory.createParam| wasn't designed for asynced process originally. Having an asynced operation (e.g. loading icons) in it makes the code complicated and hard to maintain.
So I think it's worth to refactor the code a bit more to make it support async operation. 
- Takes a callback function as argument and returns nothing.
- The callback will be triggered with stk data when the operations is finished.

Sorry for didn't point this out during the first review. :(

::: dom/system/gonk/ril_worker.js
@@ +11619,1 @@
>    searchForNextTag: function(tag, iter) {

Seems no one uses this any more, except `searchForTag`. Could we merge them together?

@@ +11635,5 @@
> +
> +    ctlvs.forEach((aCtlv) => {
> +      tags.forEach((aTag) => {
> +        if ((aCtlv.tag & ~COMPREHENSIONTLV_FLAG_CR) == aTag) {
> +          if (!ret[aTag]) { ret[aTag] = []; }

nit: coding style

if(...) {
  ...
}

even if the `if` block contains only one line.

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Edgar Chen [:edgar][:echen] from comment #17)
> Comment on attachment 8522015 [details] [diff] [review]
> Part 1.1 v2: Refactor searchForTag/loadIconIfNecessary for more flexibility
> to retrieve multiple tags and icons.
> 
> Review of attachment 8522015 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Per off line discussion, the |StkCommandParamsFactory.createParam| wasn't
> designed for asynced process originally. Having an asynced operation (e.g.
> loading icons) in it makes the code complicated and hard to maintain.
> So I think it's worth to refactor the code a bit more to make it support
> async operation. 
> - Takes a callback function as argument and returns nothing.
> - The callback will be triggered with stk data when the operations is
> finished.
> 
> Sorry for didn't point this out during the first review. :(
> 

Never mind. :-)
It's always good to refine our implementation when possible.

> ::: dom/system/gonk/ril_worker.js
> @@ +11619,1 @@
> >    searchForNextTag: function(tag, iter) {
> 
> Seems no one uses this any more, except `searchForTag`. Could we merge them
> together?

Will do.

> 
> @@ +11635,5 @@
> > +
> > +    ctlvs.forEach((aCtlv) => {
> > +      tags.forEach((aTag) => {
> > +        if ((aCtlv.tag & ~COMPREHENSIONTLV_FLAG_CR) == aTag) {
> > +          if (!ret[aTag]) { ret[aTag] = []; }
> 
> nit: coding style
> 
> if(...) {
>   ...
> }
> 

Will do.

> even if the `if` block contains only one line.

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.1 v3: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons.

1. Refactor |StkCommandParamsFactory.createParam| with |onComplete| callback.
2. Rewrite xpcshell test cases with |createParam| applied.

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.2 v3: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER.

update patch due to the changes in patch part 1.1.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v3: Modify Test Case to comply the new format of confirmMessage/callMessage. r=echen

update patch due to the changes in patch part 1.1 and 1.2.

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8524249 [details] [diff] [review]
Part 1.1 v3: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons.

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

Overall looks good, just still a few things to improve.
Thanks for the refactoring. ;)

::: dom/system/gonk/ril_worker.js
@@ +5271,5 @@
>      }
>  
> +    // The placeholder for the result of createParam().
> +    cmdDetails.options = null;
> +    this.context.StkCommandParamsFactory.createParam(cmdDetails, ctlvs, () => {

Carry the result of |createParam| in callback and assign it into |cmdDetails.options| here.

this.context.StkCommandParamsFactory.createParam(cmdDetails, ctlvs, (aResult) => {
  cmdDetails.options = aResult || null;
  ....
});

@@ +10554,5 @@
>        if (DEBUG) {
>          this.context.debug("Unknown proactive command " +
>                             cmdDetails.typeOfCommand.toString(16));
>        }
>        return null;

`return;` seems enough.

@@ +10556,5 @@
>                             cmdDetails.typeOfCommand.toString(16));
>        }
>        return null;
>      }
> +    return method.call(this, cmdDetails, ctlvs, onComplete);

Don't have to return anything.

method.call(this, cmdDetails, ctlvs, onComplete);

@@ +10584,5 @@
> +    this.loadIcons(cmdDetails, iconIdCtlvs, (aIcons, aIconCtlvs) => {
> +      if (aIcons) {
> +        result.icons = aIcons[0];
> +        result.iconSelfExplanatory =
> +          aIconCtlvs[0].value.qualifier == 0 ? true : false;

iconIdCtlvs is exactly the same as aIconCtlvs. Could we just use |iconIdCtlvs| here?
Then the callback won't have to carry the |aIconCtlvs| back.

@@ +10588,5 @@
> +          aIconCtlvs[0].value.qualifier == 0 ? true : false;
> +      }
> +
> +      cmdDetails.options = result;
> +      onComplete();

onComplete(result)

@@ +10639,5 @@
>        throw new Error("Stk Poll Interval: Required value missing : Duration");
>      }
>  
> +    cmdDetails.options = ctlv.value;
> +    onComplete();

onComplete(ctlv.value)

@@ +10673,5 @@
>        throw new Error("Stk Event List: Required value missing : Event List");
>      }
>  
> +    cmdDetails.options = ctlv.value || {eventList: null};
> +    onComplete();

onComplete(ctlv.value || {eventList: null});

@@ +10752,5 @@
> +    this.loadIcons(cmdDetails, iconIdCtlvs, (aIcons, aIconCtlvs) => {
> +      if (aIcons) {
> +        if (menuIconCtlv) {
> +          menu.iconSelfExplanatory =
> +            (aIconCtlvs.shift().value.qualifier == 0) ? true: false;

Same with above.
iconIdCtlvs is exactly the same as aIconCtlvs. Could we just use |iconIdCtlvs| here?
Then the callback won't have to carry the |aIconCtlvs| back.

@@ +10759,5 @@
> +
> +        for (let i = 0; i < aIcons.length; i++) {
> +          menu.items[i].icons = aIcons[i];
> +          menu.items[i].iconSelfExplanatory =
> +            (aIconCtlvs[i].value.qualifier == 0) ? true: false;

Ditto

@@ +10764,5 @@
> +        }
> +      }
> +
> +      cmdDetails.options = menu;
> +      onComplete();

onComplete(menu);

@@ +11097,5 @@
>      let provideLocalInfo = {
>        localInfoType: cmdDetails.commandQualifier
>      };
> +    cmdDetails.options = provideLocalInfo;
> +    onComplete();

onComplete(provideLocalInfo);

@@ +11122,5 @@
>        timer.timerValue = ctlv.value.timerValue;
>      }
>  
> +    cmdDetails.options = timer;
> +    onComplete();

onComplete(timer);

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8524250 [details] [diff] [review]
Part 1.2 v3: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER.

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

Looks good, but it needs a rebase for comment #22.
Thank you.

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8524249 [details] [diff] [review]
Part 1.1 v3: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons.

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

::: dom/system/gonk/ril_worker.js
@@ +5271,5 @@
>      }
>  
> +    // The placeholder for the result of createParam().
> +    cmdDetails.options = null;
> +    this.context.StkCommandParamsFactory.createParam(cmdDetails, ctlvs, () => {

oops! you are right. :(
I'll change this accordingly.

Thanks!

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.1 v4: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons. r=echen

address the problem in comment 22.

Comment 26

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.2 v4: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER. r=htsai,echen

revise according to the change in part 1.1 v4.

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v4: Modify Test Case to comply the new format of confirmMessage/callMessage. r=echen

rebase.

Comment 28

[Edgar Chen [:edgar]]

Comment on attachment 8525080 [details] [diff] [review]
Part 1.2 v4: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER. r=htsai,echen

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

Thank you.

Comment 29

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=8c7d863aa262

Comment 30

[Carsten Book [:Tomcat]]

this need dom peer review for:

 WebIDL file dom/webidl/MozStkCommandEvent.webidl altered

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

Th(In reply to Carsten Book [:Tomcat] from comment #30)
> this need dom peer review for:
> 
>  WebIDL file dom/webidl/MozStkCommandEvent.webidl altered

Hi,

This has been reviewed in comment 11. :)

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/22da7cf68b33
https://hg.mozilla.org/integration/b2g-inbound/rev/7d723aa47831
https://hg.mozilla.org/integration/b2g-inbound/rev/f55b542c195c

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22da7cf68b33
https://hg.mozilla.org/mozilla-central/rev/7d723aa47831
https://hg.mozilla.org/mozilla-central/rev/f55b542c195c