Closed
Bug 845572
Opened 12 years ago
Closed 12 years ago
GeckoView should automatically listen for LWTheme changes
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 22
People
(Reporter: sriram, Assigned: sriram)
References
Details
Attachments
(1 file)
32.61 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Attachment #718717 -
Flags: review?(wjohnston)
Comment 2•12 years ago
|
||
Comment on attachment 718717 [details] [diff] [review]
Patch
I like
Assignee | ||
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
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!
Attachment #718717 -
Flags: review?(wjohnston) → review+
Comment 5•12 years ago
|
||
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?
Assignee | ||
Comment 6•12 years ago
|
||
(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).
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Comment 8•12 years ago
|
||
Comment 9•12 years ago
|
||
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Updated•4 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
•