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)
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
Comment 1•10 years ago
|
||
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?
Assignee | ||
Comment 2•10 years ago
|
||
(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)
Assignee | ||
Comment 3•10 years ago
|
||
(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?
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Blocks: b2g-ril-interface
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Modify and add the test cases to ensure the test coverage.
Attachment #8518136 -
Flags: review?(htsai)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
update patch with positive test results in xpcshell & marionette.
Attachment #8518136 -
Attachment is obsolete: true
Attachment #8519679 -
Flags: review?(htsai)
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Address the suggestion in comment 13 to refactor searchForTag/loadIconIfNecessary.
Attachment #8519678 -
Attachment is obsolete: true
Attachment #8522015 -
Flags: review?(echen)
Assignee | ||
Updated•10 years ago
|
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.
Assignee | ||
Comment 16•10 years ago
|
||
apply the new API in part1.1 to rewrite SETUP_CALL/LAUNCH_BROWSER.
Attachment #8522016 -
Flags: review?(echen)
Comment 17•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8522016 -
Flags: review?(echen)
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
update patch due to the changes in patch part 1.1.
Attachment #8522016 -
Attachment is obsolete: true
Attachment #8524250 -
Flags: review?(echen)
Assignee | ||
Comment 21•10 years ago
|
||
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 22•10 years ago
|
||
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 23•10 years ago
|
||
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)
Assignee | ||
Comment 24•10 years ago
|
||
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!
Assignee | ||
Comment 25•10 years ago
|
||
address the problem in comment 22.
Attachment #8524249 -
Attachment is obsolete: true
Attachment #8525078 -
Flags: review?(echen)
Assignee | ||
Comment 26•10 years ago
|
||
revise according to the change in part 1.1 v4.
Attachment #8524250 -
Attachment is obsolete: true
Attachment #8525080 -
Flags: review?(echen)
Assignee | ||
Comment 27•10 years ago
|
||
rebase.
Attachment #8524254 -
Attachment is obsolete: true
Attachment #8525081 -
Flags: review?(echen)
Assignee | ||
Updated•10 years ago
|
Attachment #8525081 -
Flags: review?(echen) → review+
Updated•10 years ago
|
Attachment #8525078 -
Flags: review?(echen) → review+
Comment 28•10 years ago
|
||
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+
Assignee | ||
Comment 29•10 years ago
|
||
update try server result: https://tbpl.mozilla.org/?tree=Try&rev=8c7d863aa262
Keywords: checkin-needed
Comment 30•10 years ago
|
||
this need dom peer review for: WebIDL file dom/webidl/MozStkCommandEvent.webidl altered
Flags: needinfo?(btseng)
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Updated•10 years ago
|
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)
Assignee | ||
Comment 31•10 years ago
|
||
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. :)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 32•9 years ago
|
||
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
Keywords: checkin-needed
Comment 33•9 years ago
|
||
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.
Description
•