858687 - BrowserToolbar's menu button is optional

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

Since BrowserToolbar's menu button is optional and doesn't have any curve, we can base the entire layout with tabs-button as the right most element, if the menu button isn't present. This makes us have one single browser_toolbar file that is independent of whether we show menu button or not.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Remove browser_toolbar

This merges the browser_toolbar files into one. However the animation is a bit messed up. Lucas, could you take a look at this?

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: One BrowserToolbar

This uses dimens.xml to reduce the browser_toolbar to just one file shared between portrait and landscape mode on phones.
This can be used only if the animation in the previous WIP is fixed.

Comment 3

[Lucas Rocha (:lucasr)]

Taking this bug. I'm making massive changes to the original patch.

Comment 4

[Sriram Ramasubramanian [:sriram]]

You don't have to do it for the landscape mode. I have the landscape mode removed in Bug 861138. Please base your patch on top it. And please r+ the other one if you can, so that I can land ;)

Comment 5

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 733968 [details] [diff] [review]
Patch: One BrowserToolbar

As per bug 861138, we are removing the landscape mode. So, once this bug uses one browser_toolbar irrespective of menu button, we are done.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)
> You don't have to do it for the landscape mode. I have the landscape mode
> removed in Bug 861138. Please base your patch on top it. And please r+ the
> other one if you can, so that I can land ;)

Sure. I'm only changing layout/browser_toolbar_menu.xml and layout-large-v11/browser_toolbar_menu.xml. I'll rebase as soon as I stabilize my changes.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Rename browser_toolbar_menu.xml to browser_toolbar.xml

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Major simplification of browser toolbar layout

This patch does a major simplification on our browser toolbar layout:
- browser_toolbar.xml for both phones and tablets have a much simpler layout now with much less nested views.
- The animation to expand the entry is greatly simplified because the animation doesn't require layout updates anymore. No hardcoded values are needed.
- The sidebar animation is slightly simplified due to the new way we position the right edge view.
- No curves are needed on BrowserToolbarBackground anymore. Removed all the code to related to curves in it.
- No need for the custom view for RightEdge anymore as it doesn't need to match the toolbar background anymore. Removed.
- Forward button animation is slightly simpler with the new layout.
- Removed a few of dimens/style resources that are not necessary anymore.

This is just a first round of cleanups. I'll probably remove more cruft while working on the necessary changes for the new about:home behavior.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 741418 [details] [diff] [review]
Major simplification of browser toolbar layout

This is pretty big, but looks sane. Make sure you test on some phones and tablets.

Comment 10

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10bdfbe0195b
https://hg.mozilla.org/integration/mozilla-inbound/rev/06b0f332039d

Comment 11

[Lucas Rocha (:lucasr)]

Backed out due to Robocop failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00d40e2c278
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f111af2d094

Pushed a possible fix to try:
https://tbpl.mozilla.org/?tree=Try&rev=8a4840dd5b13

Comment 12

[Lucas Rocha (:lucasr)]

Tests now pass with a small view id fix on the tests.

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f70c5a5f09
https://hg.mozilla.org/integration/mozilla-inbound/rev/039a1de069ed

Comment 13

[Lucas Rocha (:lucasr)]

Pushed again now that the test infra is fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08dec3bd99a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd9b6bf7e2d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Turns out the backout caused more bustage. Backed out harder.
https://hg.mozilla.org/mozilla-central/rev/8058ffec4f32

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08dec3bd99a0
https://hg.mozilla.org/mozilla-central/rev/bcd9b6bf7e2d
https://hg.mozilla.org/mozilla-central/rev/f554f49e1ffa

Comment 16

[Alex Keybl [:akeybl]]

Do we need this in FF22 in support of bug 848719?

Comment 17

[Sriram Ramasubramanian [:sriram]]

Its good to have. But this would have dependencies on some other bugs. Lucas might be able to answer the dependencies.

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Alex Keybl [:akeybl] from comment #16)
> Do we need this in FF22 in support of bug 848719?

Yes, but this is a fairly complex change that is probably not Beta material at this point. Is bug 848719 a release blocker or something?

Comment 19

[Alex Keybl [:akeybl]]

(In reply to Lucas Rocha (:lucasr) from comment #18)
> (In reply to Alex Keybl [:akeybl] from comment #16)
> > Do we need this in FF22 in support of bug 848719?
> 
> Yes, but this is a fairly complex change that is probably not Beta material
> at this point. Is bug 848719 a release blocker or something?

Didn't want their to be per-version inconsistencies. Sounds like this isn't a critical change, or worth taking in our final beta.