935628 - Remove BrowserToolbarBackground

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

It's not actually needed anymore. Let's handle LightweightThemes directly in BrowserToolbar.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Animate toolbar with translation instead of scroll (r=sriram)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Remove BrowserToolbarBackground from toolbar (r=sriram)

Comment 3

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 828156 [details] [diff] [review]
Animate toolbar with translation instead of scroll (r=sriram)

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

I always wondered why this wasn't done.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 828157 [details] [diff] [review]
Remove BrowserToolbarBackground from toolbar (r=sriram)

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

Few changes are needed. r+ with that.
If we face some overdraw with tabs button, we need a way to mitigate that.

::: mobile/android/base/BrowserToolbar.java
@@ +1893,5 @@
> +        if (drawable == null)
> +            return;
> +
> +        StateListDrawable stateList = new StateListDrawable();
> +        stateList.addState(new int[] { R.attr.state_private }, new ColorDrawable(mActivity.getResources().getColor(R.color.background_private)));

View has getResources(). GeckoRelativeLayout now can just do, "getColorDrawable(R.color.xyz);"
So,

new ColorDrawable(getColorDrawable(R.color.background_private)) should work.

Also, use PRIVATE_STATE_SET and EMPTY_STATE_SET.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +72,5 @@
>                  android:layout_width="fill_parent"
>                  android:layout_height="@dimen/browser_toolbar_height"
>                  android:clickable="true"
> +                android:focusable="true"
> +                android:background="@drawable/url_bar_bg"/>

I am a bit worried here. This is going to cause overdraw behind Tabs button. If there is a way to offset that, I would be happy.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Only show right edge in toolbar when necessary (r=sriram)

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Only show right edge in toolbar when necessary (r=sriram)

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)
> ::: mobile/android/base/BrowserToolbar.java
> @@ +1893,5 @@
> > +        if (drawable == null)
> > +            return;
> > +
> > +        StateListDrawable stateList = new StateListDrawable();
> > +        stateList.addState(new int[] { R.attr.state_private }, new ColorDrawable(mActivity.getResources().getColor(R.color.background_private)));
> 
> View has getResources(). GeckoRelativeLayout now can just do,
> "getColorDrawable(R.color.xyz);"
> So,
>
> new ColorDrawable(getColorDrawable(R.color.background_private)) should work.

Done.
 
> Also, use PRIVATE_STATE_SET and EMPTY_STATE_SET.

Done.

> ::: mobile/android/base/resources/layout/gecko_app.xml
> @@ +72,5 @@
> >                  android:layout_width="fill_parent"
> >                  android:layout_height="@dimen/browser_toolbar_height"
> >                  android:clickable="true"
> > +                android:focusable="true"
> > +                android:background="@drawable/url_bar_bg"/>
> 
> I am a bit worried here. This is going to cause overdraw behind Tabs button.
> If there is a way to offset that, I would be happy.

Good point. But this patch is not actually changing much i.e. we're painting the background directly in the parent instead of using an extra sibling view. While doubled-checking this I noticed that we're always showing the 'right edge' view, even when the toolbar is not in editing mode. Hiding it when it's not being used leads to less overdraw. See screenshots.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Overdraw results without these patches (Nightly)

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Overdraw results with these patches

Comment 10

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/f16921967806
https://hg.mozilla.org/integration/fx-team/rev/da1a4740a722
https://hg.mozilla.org/integration/fx-team/rev/5fdcf5a9697f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop failures.
https://hg.mozilla.org/integration/fx-team/rev/0053063dd872

https://tbpl.mozilla.org/php/getParsedLog.php?id=30430850&tree=Fx-Team

Comment 12

[Lucas Rocha (:lucasr)]

Pushed to try and it's all green. This probably was some intermittent state in the repo or something. Let's try again.

https://hg.mozilla.org/integration/fx-team/rev/e224bc414b8d
https://hg.mozilla.org/integration/fx-team/rev/0f9ebc032d4d
https://hg.mozilla.org/integration/fx-team/rev/2bd9d2e52dc9

Comment 13

[Lucas Rocha (:lucasr)]

Rebase problem: the BrowserToolbarBackground class was left in the tree. Pushed in a separate patch:
https://hg.mozilla.org/integration/fx-team/rev/47c6e250d985

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e224bc414b8d
https://hg.mozilla.org/mozilla-central/rev/0f9ebc032d4d
https://hg.mozilla.org/mozilla-central/rev/2bd9d2e52dc9
https://hg.mozilla.org/mozilla-central/rev/47c6e250d985