845572 - GeckoView should automatically listen for LWTheme changes

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

GeckoView was created to switch between normal and private themes. And later functionality for LWTheme was added. But GeckoView was never made to listen for LWTheme messages by itself. It's better to make GeckoView listen to LWTheme and update their the theme, instead of a parent layout containing GeckoView doing it.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Phew! I did it! Killed all possible regressions!
Basically GeckoView is made to support LWTheme. However, there are few things like Awesomebar Tab indicator, the URL EditText (CustomEditText) and so on, that shouldn't change where there is a theme change. Instead, it should be changed by the app, as needed. Hence I introduced a "gecko:autoUpdateTheme" attribute that will block any unnecessary theme updates.

Checked this patch with normal, private tabs, installing a persona.

Comment 2

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

Comment on attachment 718717 [details] [diff] [review]
Patch

I like

Comment 3

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 718717 [details] [diff] [review]
> Patch
> 
> I like

I stole wesj's refactor :P

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 718717 [details] [diff] [review]
Patch

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

I still dont' love the auto-update bit, but its a good optimization probably. Nice!

::: mobile/android/base/AboutHomeContent.java
@@ -759,5 @@
> -             mLastTabs.setTheme(isLight);
> -             mRemoteTabs.setTheme(isLight);
> -             ((GeckoImageView) findViewById(R.id.abouthome_logo)).setTheme(isLight);
> -             ((GeckoTextView) findViewById(R.id.top_sites_title)).setTheme(isLight);
> -         }

Yay!

Comment 5

[Wesley Johnston (:wesj)]

The only other call to setTheme I see is in:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBarTabs.java#231

Can we kill it too?

Comment 6

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #5)
> The only other call to setTheme I see is in:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> AwesomeBarTabs.java#231
> 
> Can we kill it too?

Sadly no. This is one of the cases where we cannot "auto update" the theme. The theme has to be set in a weird fashion depending on if the tab is selected or not. (Selected tab should always have black text, no matter what theme it is).

Comment 7

[Sriram Ramasubramanian [:sriram]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> (In reply to Wesley Johnston (:wesj) from comment #5)
> > The only other call to setTheme I see is in:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> > AwesomeBarTabs.java#231
> > 
> > Can we kill it too?
> 
> Sadly no. This is one of the cases where we cannot "auto update" the theme.
> The theme has to be set in a weird fashion depending on if the tab is
> selected or not. (Selected tab should always have black text, no matter what
> theme it is).

Oh, and.. we don't want a race condition between Gecko telling the view to change the theme, and the setTarget used for setting private/normal tabs. Basically that part has to be handled by us. I'll try to optimize that piece of code using states in a separate patch.

Comment 8

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a50a209de1d9

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/a50a209de1d9