846184 - Lightweight theme should be shown on startup, rather than when Gecko initializes

Status: VERIFIED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

Currently we wait for Gecko to start, to show the theme -- though the file is residing in the sdcard. We should probably poke into sdcard and get the file and use it, even before Gecko tells us to do it.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This shows the theme right on startup. Basically the bitmap is loaded even before we do a setContentView(). So, when the views load, they know there is a theme, use that image and show up. The experience is just soooo beaaautiful -- only if we don't think of the 800ms startup regression on Galaxy Nexus. :D

Thinking in terms of UX -- this is good.
Thinking in terms of startup regression and distribution -- let the users wait. :P

Comment 2

[Sriram Ramasubramanian [:sriram]]

There is a weird case, where Gecko would disable addons on startup -- and so will LWTheme be. But we would be showing the LWTheme -- as its in sdcard, and the preference is not flipped. That's an edge case that cannot be solved.

Comment 4

[:Margaret Leibovic]

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

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

I want to think about this a bit more. I know we talked about it on IRC yesterday, but I'm still wary of doing I/O on the main thread during startup. I also wonder if we can reduce this to checking if a file exists, rather than reading a preference first. Could we just store the image for the currently applied theme in a special spot? The theme won't change between shutdown/startup, so we wouldn't need to worry about it being incorrect.

::: mobile/android/base/LightweightTheme.java
@@ +138,5 @@
> +                mBitmapURL = headerURL;
> +
> +                // The url is just for preview, don't store it in preference.
> +                if (!mBitmapURL.startsWith("http"))
> +                    setPreferenceURL(headerURL);

What does the headerURL that get stored look like? Is it a path to a local file?

@@ +141,5 @@
> +                if (!mBitmapURL.startsWith("http"))
> +                    setPreferenceURL(headerURL);
> +
> +                mHandler.post(new Runnable() {
> +                    @Override

bnicholson would be happy to see this @Override :)

Comment 5

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 719330 [details] [diff] [review]
> Patch
> 
> Review of attachment 719330 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/LightweightTheme.java
> @@ +138,5 @@
> > +                mBitmapURL = headerURL;
> > +
> > +                // The url is just for preview, don't store it in preference.
> > +                if (!mBitmapURL.startsWith("http"))
> > +                    setPreferenceURL(headerURL);
> 
> What does the headerURL that get stored look like? Is it a path to a local
> file?
> 

Yup. Something like /mnt/sdcard/data/data/org.mozilla.firefox/_add_profile_/lightweight_theme.png or something. But not sure if the "sdcard" part can change on phones. Also, we need to know about profile! Oh wait! My patch won't work with multiple profiles :P

Comment 6

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > Comment on attachment 719330 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 719330 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/LightweightTheme.java
> > @@ +138,5 @@
> > > +                mBitmapURL = headerURL;
> > > +
> > > +                // The url is just for preview, don't store it in preference.
> > > +                if (!mBitmapURL.startsWith("http"))
> > > +                    setPreferenceURL(headerURL);
> > 
> > What does the headerURL that get stored look like? Is it a path to a local
> > file?
> > 
> 
> Yup. Something like
> /mnt/sdcard/data/data/org.mozilla.firefox/_add_profile_/lightweight_theme.
> png or something. But not sure if the "sdcard" part can change on phones.
> Also, we need to know about profile! Oh wait! My patch won't work with
> multiple profiles :P

Can you just use GeckoProfile to get at the correct profile?

Comment 7

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

(In reply to :Margaret Leibovic from comment #6)

> Can you just use GeckoProfile to get at the correct profile?

Yes

Comment 8

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to :Margaret Leibovic from comment #6)
> 
> > Can you just use GeckoProfile to get at the correct profile?
> 
> Yes

When do we know the GeckoProfile? Before or after Gecko starts?

Comment 9

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #8)
> (In reply to Mark Finkle (:mfinkle) from comment #7)
> > (In reply to :Margaret Leibovic from comment #6)
> > 
> > > Can you just use GeckoProfile to get at the correct profile?
> > 
> > Yes
> 
> When do we know the GeckoProfile? Before or after Gecko starts?

It looks like we choose the profile during GeckoApp.initialize:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1471

Comment 10

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #8)
> (In reply to Mark Finkle (:mfinkle) from comment #7)
> > (In reply to :Margaret Leibovic from comment #6)
> > 
> > > Can you just use GeckoProfile to get at the correct profile?
> > 
> > Yes
> 
> When do we know the GeckoProfile? Before or after Gecko starts?

Before. It does not depend on Gecko. It reads the profiles.ini itself.

Comment 11

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

Yes, by this line even a custom profile (used by testing for example) will be known:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1499

Comment 12

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

I see that GeckoApp.initialize is called from onWindowFocusChanged:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1947

I don't know how "late" in startup this fires, but I think we could move the GeckoProfile initialization code out of that function and into onCreate (or somewhere else) if we need the profile set sooner.

Comment 13

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> I see that GeckoApp.initialize is called from onWindowFocusChanged:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.
> java#1947
> 
> I don't know how "late" in startup this fires, but I think we could move the
> GeckoProfile initialization code out of that function and into onCreate (or
> somewhere else) if we need the profile set sooner.

onWindowFocusChanged() is called when the UI is seen by the user (for the first time). That's why we moved all bulk operations there (including starting gecko), so that we show the UI as fast as we can. I don't think we can/want to move initialize() to somewhere before this.

Comment 14

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #13)

> onWindowFocusChanged() is called when the UI is seen by the user (for the
> first time). That's why we moved all bulk operations there (including
> starting gecko), so that we show the UI as fast as we can. I don't think we
> can/want to move initialize() to somewhere before this.

I don't mean all of initialize, only the part that sets the profile. Of course, GeckoProfile might do some main thread work that is still slow enough for us to want it to stay put.

Comment 15

[:Margaret Leibovic]

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

Clearing my review queue... I think we should try to do the profile stuff mentioned in the more recent comments in this bug, rather than using a preference.

Comment 17

[Richard Newman [:rnewman]]

N.B., if the prefs file has already been loaded from disk -- which happens the first time it's used -- reading a pref is approximately free. This is the trick we use for locale switching. Reading a file from the profile will always be a stat.

Comment 18

[Christian Vielma (IRC :cvielma)]

Hi! I'd like to give it a try to this bug. I got a bit lost in the conversation, if it's ok for me to start working on this bug, can you please provide more information about what needs to be done?

Comment 19

[Richard Newman [:rnewman]]

In short: when the user applies (or unapplies) a theme, we should do one of two things:

* Save the new header URL (and other theme-related data?) in a "well-known" location, or
* Write the new header URL to a per-profile Android-side SharedPreference.

The goal is that the Android frontend should be able to figure out which theme image to show without waiting for Gecko to start. Right now we wait for logic in Gecko to tell us what to show, so the theme is applied later than we'd like.

I don't know offhand if the per-profile prefs have been loaded by the time we're ready to show a theme. That's the main determining factor between the two approaches.

Margaret will probably chime in with more when she's done with travel stuff.

Comment 20

[Christian Vielma (IRC :cvielma)]

Thanks Richard, I'll check this and ask for more feedback in case I get stuck.

Comment 21

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

Attachment: lwt-earlyshow WIP

This patch seems to get the URL and Color saved and then used to load the theme very early. I need to test the affect on performance. I also need to see if there is an easy way to save the color, if dominant color is used to set the color. I guess I could just duplicate the pref saving code.

Comment 22

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

Attachment: lwt-earlyshow v0.1

This patch does a few things:
1. Creates a Runnable used to load URL and Color, save URL and Color, create the bitmap, and setthe lightweight theme (on the UI thread). We needed to call this from the startup path and the enable/disable LWT path.
2. Drops the need to do dominant color fallback. I was looking at some docs [1][2] and it seems like adding an accentcolor is part of the best practices. Desktop falls back to transparent, so I did the same.

I tried to keep as much as possible in the new Runnable, but clearing the prefs on a reset is in the outer class. Meh.

Tested on a Nexus 7 and a Nexus S and it's pretty snappy on both. Definitely an improvement.

Comment 23

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

I forgot the links in comment 22:

[1] https://developer.mozilla.org/en-US/Add-ons/Themes/Lightweight_themes
[2] https://addons.mozilla.org/en-US/developers/docs/themes

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8493361 [details] [diff] [review]
lwt-earlyshow v0.1

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

The only high level concern: this adds a fetch for profile prefs during startup, which might move its first read to an earlier point in time. That's probably not a problem, but be on the lookout for autophone issues.

::: mobile/android/base/LightweightTheme.java
@@ +80,5 @@
> +            SharedPreferences prefs = GeckoSharedPrefs.forProfile(mApplication);
> +            SharedPreferences.Editor editor = prefs.edit();
> +            editor.putString(PREFS_URL, mHeaderURL);
> +            editor.putString(PREFS_COLOR, mColor);
> +            editor.apply();

We can simplify this to:

  GeckoSharedPrefs.forProfile(mApplication)
                  .edit()
                  .putString(PREFS_URL, mHeaderURL)
                  .putString(PREFS_COLOR, mColor)
                  .apply();

Ain't chaining beautiful?

@@ +81,5 @@
> +            SharedPreferences.Editor editor = prefs.edit();
> +            editor.putString(PREFS_URL, mHeaderURL);
> +            editor.putString(PREFS_COLOR, mColor);
> +            editor.apply();
> +        }

After `saveToPrefs`, mSavedColor and mSavedURL will be invalid. Consider either documenting that with a comment above, or just

  mSavedURL = mHeaderURL;
  mSavedColor = mColor;

@@ +108,5 @@
> +                saveToPrefs();
> +            }
> +
> +            String croppedURL = mHeaderURL;
> +            int mark = croppedURL.indexOf('?');

Can we move this work to line 107, so that our saved URL is always cropped? Less runtime work on every startup, one less string allocation, smaller SharedPreferences, and no need to do the indexOf call for the startup path.

@@ +110,5 @@
> +
> +            String croppedURL = mHeaderURL;
> +            int mark = croppedURL.indexOf('?');
> +            if (mark != -1)
> +                croppedURL = croppedURL.substring(0, mark);

{}

@@ +118,5 @@
> +
> +            ThreadUtils.postToUiThread(new Runnable() {
> +                @Override
> +                public void run() {
> +                    setLightweightTheme(bitmap, mColor);

Note that this is mostly a no-op if decodeUrl failed (and thus returns null).

You might consider checking whether bitmap is null, and save this UI thread runnable.

Also consider whether we should refuse to save a header URL that doesn't decode…

@@ +157,5 @@
>  
> +                ThreadUtils.postToBackgroundThread(new LightweightThemeRunnable(headerURL, color));
> +            } else if (event.equals("LightweightTheme:Disable")) {
> +                // Clear the saved data when a theme is disabled.
> +                // Called on the Gecko thread, but should be very lightweight.

Thanks for commenting this.

@@ +180,5 @@
> +        SharedPreferences prefs = GeckoSharedPrefs.forProfile(mApplication);
> +        SharedPreferences.Editor editor = prefs.edit();
> +        editor.putString(PREFS_URL, null);
> +        editor.putString(PREFS_COLOR, null);
> +        editor.apply();

GeckoSharedPrefs.forProfile(mApplication)
                .edit()
                .remove(PREFS_URL)
                .remove(PREFS_COLOR)
                .apply();

Comment 25

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

(In reply to Richard Newman [:rnewman] from comment #24)
> Comment on attachment 8493361 [details] [diff] [review]
> lwt-earlyshow v0.1
> 
> Review of attachment 8493361 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The only high level concern: this adds a fetch for profile prefs during
> startup, which might move its first read to an earlier point in time. That's
> probably not a problem, but be on the lookout for autophone issues.

Yep. Will watch.

> ::: mobile/android/base/LightweightTheme.java
> After `saveToPrefs`, mSavedColor and mSavedURL will be invalid. Consider
> either documenting that with a comment above, or just
> 
>   mSavedURL = mHeaderURL;
>   mSavedColor = mColor;

I did the latter

> > +            String croppedURL = mHeaderURL;
> > +            int mark = croppedURL.indexOf('?');
> 
> Can we move this work to line 107, so that our saved URL is always cropped?
> Less runtime work on every startup, one less string allocation, smaller
> SharedPreferences, and no need to do the indexOf call for the startup path.

The full URL is needed because all lightweight themes are saved to the same file:// URL location, but add the theme ID to the URL query string. It's the only way we can get uniqueness.

> > +            ThreadUtils.postToUiThread(new Runnable() {
> > +                @Override
> > +                public void run() {
> > +                    setLightweightTheme(bitmap, mColor);
> 
> Note that this is mostly a no-op if decodeUrl failed (and thus returns null).
> 
> You might consider checking whether bitmap is null, and save this UI thread
> runnable.

I considered this, but felt like we should set mBitmap to null too, like setLightweightTheme does, but then I'd be accessing mBitmap from two different threads and the code is commented about not doing that. In fact, we had crashes at one point because of it. So I left it alone for now.

I made the other changes...

Comment 26

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

https://hg.mozilla.org/integration/fx-team/rev/9efd03125b0b

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android reftest failures.
https://hg.mozilla.org/integration/fx-team/rev/84e0e76b8201

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

Comment 28

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

Bug 1072263 tries to handle the racy, intermittent orange. A Try run:
https://tbpl.mozilla.org/?tree=Try&rev=b6c3eef53540

No new orange

https://hg.mozilla.org/integration/fx-team/rev/689487642c03

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/689487642c03

Comment 30

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in latest builds:
Firefox for Android 37.0a1 (2014-12-09)
Firefox for Android 36.0a2 (2014-12-09)
Firefox for Android 35.b1

Device: Nexus 4 (Android 4.4.4)