Remove UrlBar.ImageButton.Icon style and change inheritors

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: tynn, Mentored)

Tracking

unspecified
Firefox 43
All
Android
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

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

Attachments

(1 attachment)

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
(Assignee)

Comment 1

3 years ago
Created attachment 8655902 [details] [diff] [review]
bug1197428.patch

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
(Assignee)

Comment 4

3 years ago
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!
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/94a35ea51e6a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
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.
You need to log in before you can comment on or make changes to this bug.