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

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.

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!

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

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

remove a=2.2+ in patch description.

Address comment 7.

upload patch for v2.2

upload patch for v2.2

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

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.

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

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

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

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

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

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

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

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