Closed Bug 935628 Opened 6 years ago Closed 6 years ago

Remove BrowserToolbarBackground

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(5 files, 1 obsolete file)

It's not actually needed anymore. Let's handle LightweightThemes directly in BrowserToolbar.
Attachment #828156 - Flags: review?(sriram)
Attachment #828157 - Flags: review?(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.
Attachment #828156 - Flags: review?(sriram) → review+
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.
Attachment #829305 - Flags: review?(sriram)
(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.
Attachment #829309 - Attachment is obsolete: true
Attachment #828157 - Flags: review?(sriram) → review+
Attachment #829305 - Flags: review?(sriram) → review+
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
Rebase problem: the BrowserToolbarBackground class was left in the tree. Pushed in a separate patch:
https://hg.mozilla.org/integration/fx-team/rev/47c6e250d985
You need to log in before you can comment on or make changes to this bug.