Closed Bug 1091454 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S1 (5dec)

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(3 files, 9 obsolete files)

79.00 KB, patch
edgar
: review+
Details | Diff | Splinter Review
6.24 KB, patch
edgar
: review+
Details | Diff | Splinter Review
40.17 KB, patch
bevis
: review+
Details | Diff | Splinter Review
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
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?
Blocks: b2g-stk
(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?
Flags: needinfo?(htsai)
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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?
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.
Flags: needinfo?(htsai)
Depends on: 1094142
Blocks: 1072808
This is to load the icons properly for SET_UP_CALL/LAUNCH_BROWSER.

Hi Hsinyi,

May I have your review for this change?

Thanks!
Attachment #8518135 - Flags: review?(htsai)
Modify and add the test cases to ensure the test coverage.
Attachment #8518136 - Flags: review?(htsai)
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.
Attachment #8518136 - Flags: review?(htsai)
update patch with positive test results in xpcshell & marionette.
Attachment #8518135 - Attachment is obsolete: true
Attachment #8518135 - Flags: review?(htsai)
Attachment #8519678 - Flags: review?(htsai)
update patch with positive test results in xpcshell & marionette.
Attachment #8518136 - Attachment is obsolete: true
Attachment #8519679 - Flags: review?(htsai)
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.
Attachment #8519678 - Flags: review?(htsai)
Attachment #8519678 - Flags: review?(echen)
Attachment #8519678 - Flags: review+
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 :)
Attachment #8519679 - Flags: review?(htsai) → review?(echen)
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.
Attachment #8519678 - Flags: review?(echen)
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.
Attachment #8519679 - Flags: review?(echen) → review+
Address the suggestion in comment 13 to refactor searchForTag/loadIconIfNecessary.
Attachment #8519678 - Attachment is obsolete: true
Attachment #8522015 - Flags: review?(echen)
Attachment #8522015 - Attachment description: Part 1.1: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons. → Part 1.1 v2: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons.
apply the new API in part1.1 to rewrite SETUP_CALL/LAUNCH_BROWSER.
Attachment #8522016 - Flags: review?(echen)
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.
Attachment #8522015 - Flags: review?(echen)
Attachment #8522016 - Flags: review?(echen)
(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.
1. Refactor |StkCommandParamsFactory.createParam| with |onComplete| callback.
2. Rewrite xpcshell test cases with |createParam| applied.
Attachment #8522015 - Attachment is obsolete: true
Attachment #8524249 - Flags: review?(echen)
update patch due to the changes in patch part 1.1.
Attachment #8522016 - Attachment is obsolete: true
Attachment #8524250 - Flags: review?(echen)
update patch due to the changes in patch part 1.1 and 1.2.
Attachment #8519679 - Attachment is obsolete: true
Attachment #8524254 - Flags: review+
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);
Attachment #8524249 - Flags: review?(echen)
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.
Attachment #8524250 - Flags: review?(echen)
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!
Attachment #8525081 - Flags: review?(echen) → review+
Attachment #8525078 - Flags: review?(echen) → review+
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.
Attachment #8525080 - Flags: review?(echen) → review+
this need dom peer review for:

 WebIDL file dom/webidl/MozStkCommandEvent.webidl altered
Flags: needinfo?(btseng)
Keywords: checkin-needed
Attachment #8525078 - Attachment description: Part 1.1 v4: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons. → Part 1.1 v4: Refactor searchForTag/loadIconIfNecessary for more flexibility to retrieve multiple tags and icons. r=echen
Attachment #8525080 - Attachment description: Part 1.2 v4: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER. → Part 1.2 v4: Reuse MozStkTextMessage as the container of Text/Icon to be displayed for SET_UP_CALL/LAUNCH_BROWSER. r=htsai,echen
Flags: needinfo?(btseng)
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. :)
https://hg.mozilla.org/mozilla-central/rev/22da7cf68b33
https://hg.mozilla.org/mozilla-central/rev/7d723aa47831
https://hg.mozilla.org/mozilla-central/rev/f55b542c195c
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S1 (5dec)
You need to log in before you can comment on or make changes to this bug.