Closed Bug 1197428 Opened 9 years ago Closed 9 years ago

Remove UrlBar.ImageButton.Icon style and change inheritors

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: mcomella, Assigned: tynn, Mentored)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file)

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

Then, you'll need to create a patch to upload - see
https://wiki.mozilla.org/Mobile/Fennec/Android#Creating_commits_and_submitting_patches

To fix this bug, remove the style from styles.xml and change the single user in browser_toolbar to inherit from the super style (UrlBar.ImageButton.Icon). See [1] for links to these files.

Why? The only place UrlBar.ImageButton.Icon is used is on edit_cancel in large*/browser_toolbar.xml, but the view is perpetually hidden – the style is effectively unused.

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! ^_^

[1]: https://mxr.mozilla.org/mozilla-central/search?string=imagebutton.icon&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Attached patch bug1197428.patchSplinter Review
There was also an occurrence of R.style.UrlBar_ImageButton_Icon in the PageActionLayout Java class
Attachment #8655902 - Flags: review?(michael.l.comella)
Assignee: nobody → tynn.dev
Comment on attachment 8655902 [details] [diff] [review]
bug1197428.patch

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

Looks good – thanks Christian!

I made a push to our try test servers (above).

Once the push goes green, you can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8655902 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #2)
> I made a push to our try test servers (above).

Oops: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68f0bb7bb32c
Thank you Michael.
But now I'm wondering, if this can be considered green.
rc3 failed once, while it completed the second time.
Our test suites often get intermittent failures and this looks like one of them. To check, you can see if the suite has been re-run (i.e. there is another test suite with the same name on the same device) and if that was green. Additionally, if you click a failing test suite, you can look at the bar on the bottom of the screen and see if it matches any existing known intermittent failures, which are also listed on that bar.

This looks good to me and is ready for check-in!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/94a35ea51e6a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Thanks again! If you're looking for another bug, how about bug 1155860 or bug 1164879? Note that I'm not sure how difficult the latter is – our menu code is quite complex since we do our own menu loading.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.