1143196 - Add Menu.FIRST to order values in GeckoActionProvider.onPrepareSubMenu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Order values should start at Menu.FIRST (as per bug 1122302 comment 76) and we currently start at 0. Menu.FIRST should be added to all of these values.

To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^

(NI self to paste code when bug 1122302 is merged to m-c)

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The code in question can be found here [1].

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java?rev=e1269f306413#154

Comment 2

[Rishi Baldawa]

I can pick this up.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Great - thanks Rishi! Once you post a patch, I'll assign you to the bug.

Comment 4

[Rishi Baldawa]

Attachment: Add Menu.FIRST to order values in GeckoActionProvider.onPrepareSubMenu

I was not sure how to handle the order wrt Menu.CATEGORY_SECONDARY. Hope I'm right.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8581336 [details] [diff] [review]
Add Menu.FIRST to order values in GeckoActionProvider.onPrepareSubMenu

Hi, Rishi.

Thanks for the patch and welcome to Bugzilla! :)

When you attach a patch to a bug, don't forget to add a review flag so your reviewer gets a notification. To do this, click the "Details" button next to the patch, select "?" next to the "review" item in the "Flags" section, and enter your reviewer's email. Since you generally won't know the reviewer's email, you can type the reviewer's IRC nick (from their Bugzilla name) into the field and it'll autocomplete, e.g. I'm ":mcomella".

I've done this for you so I have a notification to remember to review this, but be sure to do it next time! I'll get to the review sometime later today.

Comment 6

[Rishi Baldawa]

Sure. Sorry about that.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

No need to apologize - that's how we learn! :)

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8581336 [details] [diff] [review]
Add Menu.FIRST to order values in GeckoActionProvider.onPrepareSubMenu

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

Looks good to me - nice job, Rishi!

Here is a push to our try test servers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecec816415a7

Once it goes green, feel free to add the checkin-needed keyword [1]. Let me know if you need help reading the results. Note that all patches that use "checkin-needed" must also have an associated green try run.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 9

[Rishi Baldawa]

looks like it went green.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/1ce926882513

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Thanks, Rishi! If you'd like some followup bugs, perhaps you'd like to work on bug 1145252 or bug 1146730? Let me know if you would prefer something else too.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ce926882513

Comment 13

[Rishi Baldawa]

Looks like someone beat me to both the bugs. Anything else you can recommend me to fix ?

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sorry about that! For some front-end work, perhaps you'd like to take a look at bug 1079182 comment 6 or bug 1105271. For back-end, maybe bug 1116351.