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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P1)

Description

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

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

Comment 1

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

blocker as this is a regression from feature-b2g:2.2

Comment 2

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

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.

Comment 3

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

Attachment: Part 1: Define 'presentationType' in nsIStkSelectItemCmd instead of nsIStkSetUpMenuCmd. r=echen, a=2.2+

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!

Comment 4

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

Attachment: Part 2: Add test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM. r=echen, a=2.2+

Improve test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM.

Comment 5

[Anshul]

(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.

Comment 6

[Edgar Chen [:edgar]]

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.

Comment 7

[Edgar Chen [:edgar]]

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.

Comment 8

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

Attachment: Part 1: Define 'presentationType' in nsIStkSelectItemCmd instead of nsIStkSetUpMenuCmd. r=echen

remove a=2.2+ in patch description.

Comment 9

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

Attachment: Part 2: Add test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM. r=echen

Address comment 7.

Comment 10

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

Attachment: [v2.2] Part 1: Define 'presentationType' in nsIStkSelectItemCmd instead of nsIStkSetUpMenuCmd. r=echen

upload patch for v2.2

Comment 11

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

Attachment: [v2.2] Part 2: Add test coverage to ensure that 'presentationType' is only available in STK_CMD_SELECT_ITEM. r=echen

upload patch for v2.2

Comment 12

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

update try server result for m-c and v2.2:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e39f32c0e8f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5204e3da2df2

Comment 13

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

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.

Comment 14

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

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.

Comment 15

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

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.

Comment 16

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

(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 17

[bhavana bajaj [:bajaj]]

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.

Comment 18

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

(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.

Comment 19

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

'checkin-needed' per comment 17 and positive try server result in comment 12.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3e430f2ff603
https://hg.mozilla.org/integration/b2g-inbound/rev/72ac9b4c7b82

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e430f2ff603
https://hg.mozilla.org/mozilla-central/rev/72ac9b4c7b82

Comment 22

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

Remove [NO_UPLIFT] and set NI to uplift for v2.2 with approval in comment 17.

Thanks!

Comment 23

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

NI to uplift v2.2 with approval in comment 17.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

You don't need to ni? for uplifts. See:
https://wiki.mozilla.org/Release_Management/B2G_Landing#Automatic_Branch_Uplifts

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/03eefbb3bc0b
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ee6187bd28ad