974601 - Avoid saving new configuration on refresh if still using default HomeConfig

Status: RESOLVED FIXED

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

Description

[Lucas Rocha (:lucasr)]

When a full refresh is triggered (e.g. from a locale change) but the user hasn't changed the default list of panels, we should simply trigger a reload on all active HomeConfig instances instead of saving the refreshed state to disk. The default configuration is loaded from memory so we should only save it to disk if the new list of panels is actually different.

In order to do that, we'll a way to know if the returned list from HomeConfig.load() is the default one. Requires some API changes that are planned in bug 967742.

Comment 1

[Lucas Rocha (:lucasr)]

Not super relevant for Fx30, downgrading to P2.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Avoid saving HomeConfig state if still using default configuration (r=margaret)

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8395781 [details] [diff] [review]
Avoid saving HomeConfig state if still using default configuration (r=margaret)

Overview:
- Changes backend to take/return State instances in save()/load() -- to allow us to properly track the 'default' state of the config.
- Tracks changes to the State instance made with the Editor API and transfer the 'default' state to the newly created State instance accordingly on apply/commit calls. 
- Whenever we try to save the default config to disk, we simply send a local broadcast message to force loaders to refresh and skip writing the state to disk altogether.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8395781 [details] [diff] [review]
Avoid saving HomeConfig state if still using default configuration (r=margaret)

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

Sorry for the slow review, this looks good. I like that we're using State in HomeConfigPrefsBackend now. I just have one concern about the similarity of ForceLoadChangeListener and ReloadBroadcastReceiver.

::: mobile/android/base/home/HomeConfigLoader.java
@@ +109,5 @@
> +        @Override
> +        public void onReceive(Context context, Intent intent) {
> +            onContentChanged();
> +        }
> +    }

This seems a bit redundant with the OnChangeListener. Is there any way we could get rid of the OnChangeListener, and only use this ReloadBroadcastReceiver instead? That is, instead of having HomeConfigPrefsBackend call mChangeListener.onChange, it could just fire a RELOAD_BROADCAST.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 8395781 [details] [diff] [review]
> Avoid saving HomeConfig state if still using default configuration
> (r=margaret)
> 
> Review of attachment 8395781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the slow review, this looks good. I like that we're using State in
> HomeConfigPrefsBackend now. I just have one concern about the similarity of
> ForceLoadChangeListener and ReloadBroadcastReceiver.
> 
> ::: mobile/android/base/home/HomeConfigLoader.java
> @@ +109,5 @@
> > +        @Override
> > +        public void onReceive(Context context, Intent intent) {
> > +            onContentChanged();
> > +        }
> > +    }
> 
> This seems a bit redundant with the OnChangeListener. Is there any way we
> could get rid of the OnChangeListener, and only use this
> ReloadBroadcastReceiver instead? That is, instead of having
> HomeConfigPrefsBackend call mChangeListener.onChange, it could just fire a
> RELOAD_BROADCAST.

Good point. Let me try that.

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Avoid saving HomeConfig state if still using default configuration (r=margaret)

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8398485 [details] [diff] [review]
Avoid saving HomeConfig state if still using default configuration (r=margaret)

- Encapsulated the broadcast bits in the backend
- Consolidated both the reload and change cases into a single OnReloadListener

Comment 8

[:Margaret Leibovic]

Comment on attachment 8398485 [details] [diff] [review]
Avoid saving HomeConfig state if still using default configuration (r=margaret)

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

Nice improvement.

Comment 9

[Lucas Rocha (:lucasr)]

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

Comment 10

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

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

Comment 11

[:Margaret Leibovic]

Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.