Closed
Bug 1197427
Opened 9 years ago
Closed 9 years ago
Move UrlBar.ImageButton.TabCount out of styles
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: mcomella, Assigned: an0nym0usdroid42, Mentored)
Details
(Whiteboard: [lang=java][good first bug])
Attachments
(2 files)
5.88 KB,
patch
|
mcomella
:
review-
|
Details | Diff | Splinter Review |
6.04 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
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, take the attributes from UrlBar.ImageButton.TabCount in styles.xml, move it into the single view that uses it in browser_toolbar, and remove the style, inheriting from the inherited style (i.e. "UrlBar.ImageButton"). Do the same for the large configuration. See [1] for the online version of the source. Why are we doing this? TabCount styles are only used once and so they can be declared in the original xml – this makes it easier to edit the content (i.e. only need to edit one file) and simplifies our styles.xml, making it easier to edit in the future. 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.tabcount&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Assignee | ||
Comment 1•9 years ago
|
||
The large configuration had only one attribute in styles.xml. So moved only that to its corresponding use in browser_toolbar.xml. Hope that was the expectation.
Attachment #8655551 -
Flags: review?(michael.l.comella)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → an0nym0usdroid42
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8655551 [details] [diff] [review] bug1197427.patch Review of attachment 8655551 [details] [diff] [review]: ----------------------------------------------------------------- Almost there. ::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml @@ -98,5 @@ > android:background="@drawable/browser_toolbar_action_bar_button"/> > > <!-- In a 56x60dp space, centering 24dp image will leave 16x18dp. --> > <org.mozilla.gecko.toolbar.TabCounter android:id="@+id/tabs_counter" > - style="@style/UrlBar.ImageButton.TabCount" You shouldn't remove this entirely - it should be "UrlBar.ImageButton". ::: mobile/android/base/resources/layout/browser_toolbar.xml @@ -64,5 @@ > 2 layers. Hence to center this, an additional 4dp is added to the left. > The margins will be 40dp on left, 8dp on right, instead of ideal 30dp > and 12dp. --> > <org.mozilla.gecko.toolbar.TabCounter android:id="@+id/tabs_counter" > - style="@style/UrlBar.ImageButton.TabCount" Same.
Attachment #8655551 -
Flags: review?(michael.l.comella) → review-
Assignee | ||
Comment 3•9 years ago
|
||
Hi, did as suggested. So we want to keep styles of the parent atleast, but since the TabCount style was used only once, so we moved that to browser_toolbar directly.
Attachment #8657201 -
Flags: review?(michael.l.comella)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8657201 [details] [diff] [review] bug1197427.patch Review of attachment 8657201 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! 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 #8657201 -
Flags: review?(michael.l.comella) → review+
Reporter | ||
Comment 5•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #4) > I made a push to our try test servers (above). Looks like the auto-push failed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc7099ee361
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #5) > (In reply to Michael Comella (:mcomella) from comment #4) > > I made a push to our try test servers (above). > > Looks like the auto-push failed: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc7099ee361 Didn't know about the treeherder link above for checking. Thanks. But on the link above , I could see status as success. I'll check on status of other bug now.
Flags: needinfo?(michael.l.comella)
Reporter | ||
Comment 7•9 years ago
|
||
(In reply to Prateek Arora from comment #6) > But on the link above , I could see status as success. I'll check on status > of other bug now. Oh, sorry – I meant that my client failed to push the link automatically, hence why the link was not above comment 4. The push looks good (and green!). Feel free to checkin-needed. :)
Flags: needinfo?(michael.l.comella) → needinfo?(an0nym0usdroid42)
Assignee | ||
Comment 8•9 years ago
|
||
Adding Checkin-needed -- Try server results https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc7099ee361 The correct patch is the one from 4th Sept. I don't know why I didn't marked previous one as obsolete. Sorry about that.
Flags: needinfo?(an0nym0usdroid42)
Keywords: checkin-needed
Reporter | ||
Comment 9•9 years ago
|
||
Thanks Prateek! 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.
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3aa50a207837
Keywords: checkin-needed
Assignee | ||
Comment 11•9 years ago
|
||
Michael, I had a doubt regarding the process flow of bug. So once checked-in, Pulsebot pushes it to main server ? And after that, someone marks it as resolved ? Ex, bug 1197441, this has been pushed to main servers right ? Our work there is done ?
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/3aa50a207837
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Reporter | ||
Comment 13•9 years ago
|
||
(In reply to Prateek Arora from comment #11) > Michael, I had a doubt regarding the process flow of bug. So once > checked-in, Pulsebot pushes it to main server ? And after that, someone > marks it as resolved ? Ex, bug 1197441, this has been pushed to main > servers right ? Our work there is done ? We have one main tree (mozilla-central) that our nightly builds are taken from and several trees on which development happens (e.g. fx-team, the tree for Firefox dev). These development trees are merged into mozilla-central once a day. This happens so that when a tree closes (e.g. build bustage), fewer people are affected as only one development tree needs to be closed, rather than the main tree as it used to be. In this case, Pulsebot (comment 10) pushed your patch to the fx-team development tree and Wes merged fx-team into mozilla-central in comment 12 (e.g. if you click "push id" from the link in Wes' post, you'll see the merge commit at [1]). Generally, once the patch is on fx-team your work is done. However, sometimes a push will break the build or tests, in which case it'll likely be backed out of fx-team. If it passes these, it will likely be merged into mozilla-central and any future regressions will be filed as new bugs. [1]: https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=3aa50a207837
Flags: needinfo?(michael.l.comella)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•