[B2G][STK] Wrong attribute of "presentationType" was defined in nsIStkSetUpMenuCmd.

RESOLVED FIXED in Firefox 38

Status

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

({regression})

unspecified
2.2 S5 (6feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

This is a regression of bug 1072808.

When introducing the new proxy of the system messaging to telephony modules in bug 1072808, the attribute |presentationType| for STK_CMD_SELECT_ITEM was improperly defined in the wrong interface in STK_CMD_SET_UP_MENU. [1]

Hence, file this bug to correct this problem.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/icc/interfaces/nsIIccMessenger.idl#321
blocker as this is a regression from feature-b2g:2.2
blocking-b2g: 2.2? → 2.2+
Hello Anshul,
I want to highlight this issue to you. This is a flaw from a feature-b2g:2.2 bug 1072808, and we cannot fix this without touching ril internal interfaces because the root cause as Bevis explained is from an API attribute misplaced. And as this is a new regression, we believe we shouldn't wait for the next version to land the fix, it's better to get this patch in v2.2. Sorry for any inconvenience.
Flags: needinfo?(anshulj)
Hi Edgar,

Sorry about making this regression.

This is a quick fix to 
1. replace nsIStkMenuCmd with nsIStkSetUpMenuCmd and move 'presentationType' from nsIStkSetUpMenuCmd to nsIStkSelectItemCmd.
2. append 'presentationType' attribute only for STK_CMD_SELECT_ITEM in ril_worker.js.

May I have your review for this changes?

Thanks!
Attachment #8555182 - Flags: review?(echen)
Improve test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM.
Attachment #8555183 - Flags: review?(echen)

Comment 5

4 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> Hello Anshul,
> I want to highlight this issue to you. This is a flaw from a feature-b2g:2.2
> bug 1072808, and we cannot fix this without touching ril internal interfaces
> because the root cause as Bevis explained is from an API attribute
> misplaced. And as this is a new regression, we believe we shouldn't wait for
> the next version to land the fix, it's better to get this patch in v2.2.
> Sorry for any inconvenience.
Thanks for letting us know Hsin-Yi; agree that this needs to be fixed on 2.2.
Flags: needinfo?(anshulj)
Comment on attachment 8555182 [details] [diff] [review]
Part 1: Define 'presentationType' in nsIStkSelectItemCmd instead of nsIStkSetUpMenuCmd. r=echen, a=2.2+

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

Thank you, Bevis.
Attachment #8555182 - Flags: review?(echen) → review+
Comment on attachment 8555183 [details] [diff] [review]
Part 2: Add test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM. r=echen, a=2.2+

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

Thank you.

::: dom/icc/tests/marionette/test_stk_setup_menu.js
@@ +157,5 @@
>  function testSetupMenu(aCommand, aExpect) {
>    is(aCommand.typeOfCommand, MozIccManager.STK_CMD_SET_UP_MENU, "typeOfCommand");
>    is(aCommand.commandQualifier, aExpect.commandQualifier, "commandQualifier");
>    is(aCommand.options.title, aExpect.title, "options.title");
> +  is(aCommand.options.presentationType, undefined, "presentationType");

Please help to add some comments for this checking.
Attachment #8555183 - Flags: review?(echen) → review+
remove a=2.2+ in patch description.
Attachment #8555182 - Attachment is obsolete: true
Attachment #8555660 - Flags: review+
Attachment #8555661 - Attachment description: 8555183: Part 2: Add test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM. r=echen → Part 2: Add test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM. r=echen
Comment on attachment 8555663 [details] [diff] [review]
[v2.2] Part 1: Define 'presentationType' in nsIStkSelectItemCmd instead of nsIStkSetUpMenuCmd. r=echen

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1072808
User impact if declined: presentationType in STK_SELECT_ITEM becomes invalid.
Testing completed: Yes. Test case included.
Risk to taking this patch (and alternatives if risky): No.
String or UUID changes made by this patch:No.
Attachment #8555663 - Flags: approval-mozilla-b2g37?
Comment on attachment 8555664 [details] [diff] [review]
[v2.2] Part 2: Add test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM. r=echen

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1072808
User impact if declined: presentationType in STK_SELECT_ITEM becomes invalid.
Testing completed: Yes. Test case included.
Risk to taking this patch (and alternatives if risky): No.
String or UUID changes made by this patch:No.
Attachment #8555664 - Flags: approval-mozilla-b2g37?
Hi bevis,
Sorry that I cleaned the "checkin-needed" keyword because we can't land this patch to m-c until we know we get approval to v2.2.

Hi Release Manager,
Due to the agreement b/w moz and CAF, we start locking down ril internal interfaces since v2.2. I.e., we need to keep the ril internal interfaces the same on m-c and v2.2. That's why we ask for your approval before landing it on m-c. The patch is simple, risk is low, and it's also agreed by CAF that this issue definitely needs to be fixed for v2.2. Please take a look. Thank you.
Keywords: checkin-needed
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #15)
> Hi bevis,
> Sorry that I cleaned the "checkin-needed" keyword because we can't land this
> patch to m-c until we know we get approval to v2.2.
> 
> Hi Release Manager,
> Due to the agreement b/w moz and CAF, we start locking down ril internal
> interfaces since v2.2. I.e., we need to keep the ril internal interfaces the
> same on m-c and v2.2. That's why we ask for your approval before landing it
> on m-c. The patch is simple, risk is low, and it's also agreed by CAF that
> this issue definitely needs to be fixed for v2.2. Please take a look. Thank
> you.

Thanks for clarifying this!
Comment on attachment 8555663 [details] [diff] [review]
[v2.2] Part 1: Define 'presentationType' in nsIStkSelectItemCmd instead of nsIStkSetUpMenuCmd. r=echen

Given comment #15, this sounds good to land on 2.2. But i'd still like this to be check-in on m-c before the branch uplift, so we get a nightly testing and check for any fallouts.

Adding the checkin-needed for m-c, and pre-approving for b2g37 with NO_UPLIFT until we are sure this sticks on a m-c nightly.
Attachment #8555663 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8555664 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Whiteboard: [NO_UPLIFT]
(In reply to bhavana bajaj [:bajaj] from comment #17)
> Comment on attachment 8555663 [details] [diff] [review]
> [v2.2] Part 1: Define 'presentationType' in nsIStkSelectItemCmd instead of
> nsIStkSetUpMenuCmd. r=echen
> 
> Given comment #15, this sounds good to land on 2.2. But i'd still like this
> to be check-in on m-c before the branch uplift, so we get a nightly testing
> and check for any fallouts.
> 

That does make sense. Thanks, Bhavana.

> Adding the checkin-needed for m-c, and pre-approving for b2g37 with
> NO_UPLIFT until we are sure this sticks on a m-c nightly.
'checkin-needed' per comment 17 and positive try server result in comment 12.
Keywords: checkin-needed
Remove [NO_UPLIFT] and set NI to uplift for v2.2 with approval in comment 17.

Thanks!
Whiteboard: [NO_UPLIFT]
NI to uplift v2.2 with approval in comment 17.
Flags: needinfo?(ryanvm)
You don't need to ni? for uplifts. See:
https://wiki.mozilla.org/Release_Management/B2G_Landing#Automatic_Branch_Uplifts
Flags: needinfo?(ryanvm)
Target Milestone: --- → 2.2 S5 (6feb)
You need to log in before you can comment on or make changes to this bug.