968172 - Avoid refreshing HomeConfig on every onLocaleReady() call

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Lucas Rocha (:lucasr)]

onLocaleReady() is called every time fennec starts. This means we're doing an unnecessary HomeConfig refresh on every startup. We should only refresh the UI if the locale has actually changed.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: (Option 1) Bug 968172 - Trigger a full refresh when locale changes (r=rnewman)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Don't invalidate HomePager in onLocaleReady (r=rnewman)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (Option 2) Bug 968172 - Trigger a full refresh when locale changes (r=rnewman)

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8373936 [details] [diff] [review]
(Option 1) Bug 968172 - Trigger a full refresh when locale changes (r=rnewman)

Not entirely sure which one is the best approach here so I'll post the 2 options I considered to get a second opinion. Feel free to suggest more options :-)

The nice thing about Option 1 is that we keep all the HomeConfig invalidation code encapsulated in HomeConfigInvalidator. The drawback is that is that we're relying on the fact that the invalidation only runs after a delay in response to a Locale:Set event. The delayed execution implicitly ensures that the invalidation will happen after Locale:Set is handled in GeckoApp i.e. after setLocale() is called. I can add a comment explaining the assumptions around it.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8373938 [details] [diff] [review]
(Option 2) Bug 968172 - Trigger a full refresh when locale changes (r=rnewman)

And here's option 2. The nice thing about it is that we ensure that the HomeConfig invalidation only runs after the setLocale() call in GeckoApp.

This is still not ideal as it's not entirely obvious (from just reading the code) that Locale:Set should first be handled in GeckoApp. I can probably add a comment just before the refreshAll() call explaining why this is called in that order but still...

Furthermore, this involves scattering HomeConfig invalidation logic outside HomeConfigInvalidation which is unfortunate.

Given these 2 options, I'm leaning towards Option 1.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8373937 [details] [diff] [review]
Don't invalidate HomePager in onLocaleReady (r=rnewman)

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

We don't need to explicitly restart anything in HomePager anymore as the HomeConfig invalidation will take care of it.

Comment 7

[Richard Newman [:rnewman]]

There are two places that invoke onLocaleReady.

The first is during background onCreate. This is the startup path: we launch, and ASAP we hit prefs to figure out what locale we should really be using. We tell Java via onLocaleReady, and we don't tell Gecko (because it persists the locale in its own prefs).

The second is when a user picks a new locale. The picking process sends Locale:Set to GeckoApp, and GeckoApp passes the value to the LocaleManager. If the locale changed to something new, the act of setting that locale causes three things:

  Locale:Changed to be sent to Gecko
  GeckoApp.onLocaleReady to be called
  ... and, ultimately, onConfigurationChanged to be called.


onLocaleReady was intended to encode the following pattern: when we start up we *don't know* which locale to use, so it's called at the absolute earliest point that we know which locale we should be using, and you shouldn't do any string work beforehand. If the locale changes, again, it's called as soon as we know.


Locale:Set is a message, and -- as you observed -- GeckoApp.setLocale has to handle it before anyone else takes action.

onLocaleReady is intended to be the extension hook that runs after Locale:Set, and only if the locale has changed or we're just starting up. If the user picked the same locale that they were using before, Locale:Set is a no-op.

(Note that Locale:Set is not triggered at startup. onLocaleReady is called directly.)

As such, GeckoApp should be the only component that handles Locale:Set.


It sounds like what you want is this:

* A broadcast mechanism, so that you don't have to couple the home page system to GeckoApp.
* A notification when the locale has *changed*, not when it's ready, because you're OK with showing text in English for a second rather than a blank title before you replace it with German -- that is, you can't follow the pattern for which onLocaleReady was established.

If that's correct, let's do the following:

* Make onLocaleReady (or a wrapper) on startup aware of the two locales (we already are at the point that it's called), and conditionally broadcast a Java-side equivalent to the Locale:Changed that we send to Gecko.

* Go with something like Option 1, but listening to an event that's guaranteed to occur only (a) when everything locale-related has been handled, and (b) when the locale has changed.

If I'm misunderstanding, and you don't need to show strings up-front, then let's refine the concept of "invalidating" -- it's fine to parse and handle the home config immediately on startup, just not to hard-code or display any strings until onLocaleReady is called.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Trigger a full refresh when locale changes (r=rnewman)

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8373937 [details] [diff] [review]
Don't invalidate HomePager in onLocaleReady (r=rnewman)

Let me redirect this one to margaret as she wrote the HomePager.invalidate() code.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8375737 [details] [diff] [review]
Trigger a full refresh when locale changes (r=rnewman)

Adds an 'isNew' argument to onLocaleReady(). I'm not a big fan of boolean arguments but using a enum here felt a bit overkill.

Given that we don't currently have a formalized in-Java broadcast mechanism, I preferred to simplify things in the scope of this bug and just call HomeConfigInvalidator directly from BrowserApp. There's some discussion happening around using LocalBroadcastManager in bug 949637 so I'll keep an eye on it and update this code accordingly if necessary.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8373937 [details] [diff] [review]
Don't invalidate HomePager in onLocaleReady (r=rnewman)

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

This patch is easy to review, since it just depends on HomeConfigInvalidator to do the right thing, and that's rnewman's part to review! :)

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8375737 [details] [diff] [review]
Trigger a full refresh when locale changes (r=rnewman)

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

Nearly!

::: mobile/android/base/BrowserApp.java
@@ +1667,5 @@
>          }
>      }
>  
>      @Override
> +    public void onLocaleReady(final String locale, boolean isNew) {

Can we call this `localeChanged` or `localeDidChange` instead of `isNew`? I feel the latter is ambiguous -- perhaps `isNew` is true on the first invocation?

::: mobile/android/base/GeckoActivity.java
@@ +19,4 @@
>       *
>       * onLocaleReady must always be called on the UI thread.
> +     *
> +     * @param iNew {@code true} if the call was triggered because a new

Typo, but I hope this changes anyway! Also, I'd define this as "true if the call occurred because the new locale differed from the previous locale; false otherwise".

::: mobile/android/base/GeckoApp.java
@@ +1315,5 @@
>                  final String uiLocale = appLocale;
>                  ThreadUtils.postToUiThread(new Runnable() {
>                      @Override
>                      public void run() {
> +                        GeckoApp.this.onLocaleReady(uiLocale, false /* isNew */);

This should be:

  final boolean localeChanged = !TextUtils.equals(appLocale, uiLocale);
  GeckoApp.this.onLocaleReady(uiLocale, localeChanged);

In case it's not obvious why: we actually do fetch the locale here. If the OS locale is en_GB, then we already started up in en_GB. Then, here in a background runnable started from onCreate, we just found that we ought to be in es_ES.

What the `localeChanged` argument to `onLocaleReady` means is "not only is the locale ready, but if you did any work already based on the locale extant prior to this call, then you need to do it again".

@@ +2763,5 @@
>          if (!shouldRestart) {
>              ThreadUtils.postToUiThread(new Runnable() {
>                  @Override
>                  public void run() {
> +                    GeckoApp.this.onLocaleReady(resultant, true /* isNew */);

This is correct: leave a comment that we'll only reach this point if the locale was different, so we can hard-code true.

Personally I'd do that as

  // We short-circuit above if the locale was the same.
  final boolean localeChanged = true;
  GeckoApp.this.onLocaleReady(resultant, localeChanged);

Comment 13

[Lucas Rocha (:lucasr)]

Your review makes me realize that we're not talking about the exact same thing. Apologies, I should have been more clear about the what is needed here.

First of all, a bit of context. The HomeConfig framework is built with the goal that the about:home configuration should have everything the HomePager needs to display content immediately (including any strings) on startup without any external dependencies (such as gecko being up and running, current locale being set, etc). This means, in the specific case of locales, that we persist all strings directly in the configuration. This includes both strings for the built-in panels (top sites, bookmarks, history, reading list), which come from the Android resource system; and for add-on based panels, which come from the add-on l10n framework. For now, we only store the panel titles. So, whenever HomeConfig gets saved (e.g. when a panel is added, removed, or set as default) we will store the strings using the currently set locale in Gecko/Android.

This is why we only need to 'refresh' the strings in HomeConfig when the locale *changes*. The startup path for onLocaleReady() is irrelevant for HomeConfig because we already have the strings for the current locale.

With that in mind, I see two possible ways to approach this (feel free to suggest more options):

1. Add something like a 'LocaleReadyReason' enum argument to onLocaleReady() that can be either STARTUP or NEW_LOCALE_SELECTED so that I can tell the different calls apart and only refresh HomeConfig when the user selects a *new* locale.

2. Add a separate callback like onNewLocaleSelected() that gets triggered just before onLocaleReady() when GeckoApp.setLocale() is called.

I hope that clarifies what the HomeConfig framework needs from the Locale infrastructure.

Comment 14

[Richard Newman [:rnewman]]

Ah, gotcha. Apologies.

There are a bunch of ways that the locale can be changed, including behind your back with no notification (e.g., a system locale change when Fennec isn't running).

My suggestion, then:

* The home config should persist the locale (via locale.toString()) used when it was generated. That's a 100%-sure marker for which locale is going to be shown when this config is used, and allows us to confidently invalidate it.

* App.onLocaleReady calls HomeConfigInvalidator.onLocaleReady. This catches background locale changes, changes when the user has picked one, and when the OS switches during the app's lifecycle.

* HomeConfigInvalidator.onLocaleReady should compare the possibly-new input locale to the config's persisted locale. If they're the same, do nothing. If they differ, invalidate the config.

That should simplify things!


The test plan for this, in order:

* Strings should be correct on first launch.
* Force-quit the app. Change OS locale. Relaunch. Strings should change.
* Without quitting the app, switch back to settings and change the OS locale again. Switch back. Strings should change.
* Switch locale in-app: strings should change.

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Rename PREFS_KEY constant to PREFS_CONFIG_KEY (r=rnewman)

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Trigger a full refresh when locale changes (r=rnewman)

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8378515 [details] [diff] [review]
Rename PREFS_KEY constant to PREFS_CONFIG_KEY (r=rnewman)

Some prep work, doing it in a separate patch to reduce noise in the main one.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8378515 [details] [diff] [review]
Rename PREFS_KEY constant to PREFS_CONFIG_KEY (r=rnewman)

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

r=trivial :D

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 8378516 [details] [diff] [review]
Trigger a full refresh when locale changes (r=rnewman)

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

Works on all cases in the suggested test plan (comment #14). Some follow-ups:
- There's a visible delay between the locale change and the UI updates in HomePager (bug 974598)
- A full refresh causes the HomeConfig state to be saved to disk, even if the user is still using the default setup (which is loaded from memory). We should avoid doing that (bug 974601)
- Add-on based panels are not updated properly on locale changes because the Home.panels API registry is not refreshed on locale changes (bug 974139)

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8378516 [details] [diff] [review]
Trigger a full refresh when locale changes (r=rnewman)

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

::: mobile/android/base/home/HomeConfig.java
@@ +660,5 @@
> +    public String getLocale() {
> +        String locale = mBackend.getLocale();
> +        if (locale == null) {
> +            locale = getCurrentLocale();
> +            mBackend.setLocale(locale);

Worth commenting that this is an upgrade path, and assumes that the current locale is the same as the one used to generate the backend's original contents.

But in that case, it might be worth just triggering a refresh here (which will presumably correct the locale). Your call.

@@ +670,3 @@
>      public void save(List<PanelConfig> panelConfigs) {
>          mBackend.save(panelConfigs);
> +        mBackend.setLocale(getCurrentLocale());

I have a gentle conceptual preference for getting the locale prior to saving the backend.

But see comment in the backend code which makes this moot.

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +151,5 @@
> +        final SharedPreferences prefs = getSharedPreferences();
> +        final SharedPreferences.Editor editor = prefs.edit();
> +
> +        editor.putString(PREFS_LOCALE_KEY, locale);
> +        editor.commit();

This is a little problematic -- we're incurring an additional prefs commit (which blocks this thread until the prefs are written to disk). You're also running the risk of splitting the two prefs writes, and having the latter not occur, leaving a stale value on disk.

Bearing in mind that the persisted config is always tied to a locale, I'd suggest taking the locale as an argument to config.save (as a proxy for when the config items are actually created; if you can find a good way to do that instead, great).

That way you can use the same editor for both prefs, incur only one commit, and get an atomic write for them both.

You might still need to keep this method around in order to do the upgrade/no-locale path, but I don't think it should be routine.

The alternative is to *really* tie the locale to the config, storing it in the same JSON string that you're putting in PREFS_CONFIG_KEY, and use only a single prefs value. That works for me too.

Comment 21

[Lucas Rocha (:lucasr)]

Attachment: Ensure consistent locale string on onLocaleReady() calls (r=rnewman)

Comment 22

[Lucas Rocha (:lucasr)]

Attachment: Trigger a full refresh when locale changes (r=rnewman)

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 8378986 [details] [diff] [review]
Ensure consistent locale string on onLocaleReady() calls (r=rnewman)

Using the persisted locale string directly returns pt-BR instead of pt_BR. This patch ensures we consistently use the result of updateLocale() as the locale argument for onLocaleReady().

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 8378987 [details] [diff] [review]
Trigger a full refresh when locale changes (r=rnewman)

(In reply to Richard Newman [:rnewman] from comment #20)
> Comment on attachment 8378516 [details] [diff] [review]
> Trigger a full refresh when locale changes (r=rnewman)
> 
> Review of attachment 8378516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeConfig.java
> @@ +660,5 @@
> > +    public String getLocale() {
> > +        String locale = mBackend.getLocale();
> > +        if (locale == null) {
> > +            locale = getCurrentLocale();
> > +            mBackend.setLocale(locale);
> 
> Worth commenting that this is an upgrade path, and assumes that the current
> locale is the same as the one used to generate the backend's original
> contents.

Done.
 
> But in that case, it might be worth just triggering a refresh here (which
> will presumably correct the locale). Your call.

Good point. There was a window for failure in the previous patch if the user ran the upgrade path just after an OS locale change. We have to force a refresh if the upgrade path runs and the user had customized HomePager before. At the same time, we don't want to unconditionally refresh on the upgrade path as this would cause an unnecessary extra about:home refresh on first run, for example. This is properly handled in this new patch.

> @@ +670,3 @@
> >      public void save(List<PanelConfig> panelConfigs) {
> >          mBackend.save(panelConfigs);
> > +        mBackend.setLocale(getCurrentLocale());
> 
> I have a gentle conceptual preference for getting the locale prior to saving
> the backend.
> 
> But see comment in the backend code which makes this moot.
> 
> ::: mobile/android/base/home/HomeConfigPrefsBackend.java
> @@ +151,5 @@
> > +        final SharedPreferences prefs = getSharedPreferences();
> > +        final SharedPreferences.Editor editor = prefs.edit();
> > +
> > +        editor.putString(PREFS_LOCALE_KEY, locale);
> > +        editor.commit();
> 
> This is a little problematic -- we're incurring an additional prefs commit
> (which blocks this thread until the prefs are written to disk). You're also
> running the risk of splitting the two prefs writes, and having the latter
> not occur, leaving a stale value on disk.
> 
> Bearing in mind that the persisted config is always tied to a locale, I'd
> suggest taking the locale as an argument to config.save (as a proxy for when
> the config items are actually created; if you can find a good way to do that
> instead, great).

Based on how we're using the HomeConfig API, I feel confident enough to say that it's safe to use current locale at save time. 

> That way you can use the same editor for both prefs, incur only one commit,
> and get an atomic write for them both.
> 
> You might still need to keep this method around in order to do the
> upgrade/no-locale path, but I don't think it should be routine.

The setLocale() method is not needed anymore in this new patch as the upgrade path is run internally by the backend.

> The alternative is to *really* tie the locale to the config, storing it in
> the same JSON string that you're putting in PREFS_CONFIG_KEY, and use only a
> single prefs value. That works for me too.

I would rather not couple them as we need to load them independently in some cases (this bug is a good example).

Comment 25

[Richard Newman [:rnewman]]

Comment on attachment 8378986 [details] [diff] [review]
Ensure consistent locale string on onLocaleReady() calls (r=rnewman)

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

Good consistency catch! I think this is the right fix.

I think the reason this is the way it is: if `updateLocale` can't use the input, it'll return null, and so will `setSelectedLocale`. `getAndApplyPersistedLocale`, on the other hand, wants to always return a value for FHR.

But as written, the osLocale in FHR will be java_STYLE, and appLocale will depend on the caller of `setSelectedLocale` -- currently intl-STYLE, but some test code passes java_STYLE.

Returning the result of `updateLocale` will change this: sometimes now we'll turn "foo-BAR-baz" into null, and thus into osLocale, but everything else will be normalized into a Java locale code. In practice I'm not worried about this, and it's an improvement in some ways, despite losing a small amount of info.

However! This will get three locales wrong, because Java gets them wrong. I'll think about that for a little while.


Sidenote, just so this is written down somewhere: the input to setSelectedLocale is persisted and used in LocaleManager, even across restarts. That's an escape valve if we need to fix how those locale codes are processed.

That's what you're seeing here -- the input was "pt-BR". The equivalent post-processed Locale is available via LocaleManager.getCurrentLocale().

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 8378987 [details] [diff] [review]
Trigger a full refresh when locale changes (r=rnewman)

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

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +83,5 @@
> +        ThreadUtils.getBackgroundHandler().post(new Runnable() {
> +            @Override
> +            public void run() {
> +                final String configLocale = mHomeConfig.getLocale();
> +                if (!TextUtils.equals(configLocale, locale)) {

It's probably more efficient to do this check on the UI thread, outside the runnable -- decide whether to queue up any work at all. 99% of the time the two locales will be the same.

You can also pseudo-inline the TextUtils call: `locale` will never be null, so we can save a method call:

  final String configLocale = mHomeConfig.getLocale();
  if (configLocale == null || !configLocale.equals(locale)) {
      ThreadUtils.getBackgroundHandler().post(new Runnable() {
          @Override
          public void run() {
              handlePanelRefresh(null);
          }
      });
  }

Comment 27

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #26)
> Comment on attachment 8378987 [details] [diff] [review]
> Trigger a full refresh when locale changes (r=rnewman)
> 
> Review of attachment 8378987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeConfigInvalidator.java
> @@ +83,5 @@
> > +        ThreadUtils.getBackgroundHandler().post(new Runnable() {
> > +            @Override
> > +            public void run() {
> > +                final String configLocale = mHomeConfig.getLocale();
> > +                if (!TextUtils.equals(configLocale, locale)) {
> 
> It's probably more efficient to do this check on the UI thread, outside the
> runnable -- decide whether to queue up any work at all. 99% of the time the
> two locales will be the same.

Can't do this in the UI thread as getLocale() will to do blocking I/O call on the upgrade path. I'd rather avoid blocking the UI thread.
 
> You can also pseudo-inline the TextUtils call: `locale` will never be null,
> so we can save a method call:

Good point, done.

Comment 28

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/5e50f61a7c9b
https://hg.mozilla.org/integration/fx-team/rev/0304dd8dc1d9
https://hg.mozilla.org/integration/fx-team/rev/e0a8e36904bf
https://hg.mozilla.org/integration/fx-team/rev/6d779ac440d6

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e50f61a7c9b
https://hg.mozilla.org/mozilla-central/rev/0304dd8dc1d9
https://hg.mozilla.org/mozilla-central/rev/e0a8e36904bf
https://hg.mozilla.org/mozilla-central/rev/6d779ac440d6

Comment 30

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.