Right now, we always refresh every time a panel is installed removed. We should only when locale changes or when an add-on is upgraded.

Comment on attachment 8373413 [details] [diff] [review] Use base class for ConcurrentLinkedQueue (r=margaret) It's usually a good idea to use base types for data structures on class members.

Comment on attachment 8373414 [details] [diff] [review] Change HomeConfigInvalidator to operate on pending changes (r=margaret) This changes mPendingChanges to use ConfigChange instances instead of PanelConfig. Prep work for the new refresh change type. No functional changes.

Comment on attachment 8373415 [details] [diff] [review] Remove unused DELETED state from PanelConfig (r=margaret) No need to use the flag in PanelConfig directly anymore as the config change type covers that in HomeConfigInvalidator now.

Comment on attachment 8373416 [details] [diff] [review] Factor out method to replace a PanelConfig in a list (r=margaret) So that we can reuse the same code on the 'refresh' config change.

Comment on attachment 8373417 [details] [diff] [review] Only refresh all HomeConfig entries when requested (r=margaret) 'Refresh' changes can be triggered for a specific PanelConfig or for all current HomeConfig entries. I expect refreshAll() to be used on locale changes and HomePanels:Refresh to be used by the Home.panels.* API to refresh a specific panel when an add-ons is upgraded.

Comment on attachment 8373415 [details] [diff] [review] Remove unused DELETED state from PanelConfig (r=margaret) Review of attachment 8373415 [details] [diff] [review]: ----------------------------------------------------------------- Nice, I feel like this change type concept is clearer to follow than this deleted panel concept.

Comment on attachment 8373416 [details] [diff] [review] Factor out method to replace a PanelConfig in a list (r=margaret) Review of attachment 8373416 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/HomeConfigInvalidator.java @@ +124,5 @@ > > Log.d(LOGTAG, "scheduleInvalidation: scheduled new invalidation"); > } > > + private boolean replacePanelConfig(List<PanelConfig> panelConfigs, PanelConfig panelConfig) { This could use some documentation, since it really just *maybe* replaces the panel config :) @@ +154,5 @@ > } > break; > > case INSTALL: > + if (replacePanelConfig(panelConfigs, panelConfig)) { Shouldn't this be `if (!replacePanelConfig(...))` ?

Comment on attachment 8373417 [details] [diff] [review] Only refresh all HomeConfig entries when requested (r=margaret) Review of attachment 8373417 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/HomeConfigInvalidator.java @@ +185,5 @@ > + break; > + > + case REFRESH: > + if (panelConfig != null) { > + replacePanelConfig(panelConfigs, panelConfig); I think we should log an exception if this returns false, since something has gone wrong if we're trying to refresh a panel config that doesn't exist. @@ +187,5 @@ > + case REFRESH: > + if (panelConfig != null) { > + replacePanelConfig(panelConfigs, panelConfig); > + } else { > + shouldRefreshAll = true; Are you using this flag because you want to keep all return statements together down at the bottom of the method? I would probably just return here, but I don't have a strong preference if you prefer this style :)

Comment on attachment 8373416 [details] [diff] [review] Factor out method to replace a PanelConfig in a list (r=margaret) Review of attachment 8373416 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you fix the logic.

(In reply to :Margaret Leibovic from comment #12) > Comment on attachment 8373416 [details] [diff] [review] > Factor out method to replace a PanelConfig in a list (r=margaret) > > Review of attachment 8373416 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/HomeConfigInvalidator.java > @@ +124,5 @@ > > > > Log.d(LOGTAG, "scheduleInvalidation: scheduled new invalidation"); > > } > > > > + private boolean replacePanelConfig(List<PanelConfig> panelConfigs, PanelConfig panelConfig) { > > This could use some documentation, since it really just *maybe* replaces the > panel config :) Done. > @@ +154,5 @@ > > } > > break; > > > > case INSTALL: > > + if (replacePanelConfig(panelConfigs, panelConfig)) { > > Shouldn't this be `if (!replacePanelConfig(...))` ? Yep, I had fixed that locally but forgot to update the patch before sending it.

(In reply to :Margaret Leibovic from comment #13) > Comment on attachment 8373417 [details] [diff] [review] > Only refresh all HomeConfig entries when requested (r=margaret) > > Review of attachment 8373417 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/HomeConfigInvalidator.java > @@ +185,5 @@ > > + break; > > + > > + case REFRESH: > > + if (panelConfig != null) { > > + replacePanelConfig(panelConfigs, panelConfig); > > I think we should log an exception if this returns false, since something > has gone wrong if we're trying to refresh a panel config that doesn't exist. Good point. Added a warning log. > @@ +187,5 @@ > > + case REFRESH: > > + if (panelConfig != null) { > > + replacePanelConfig(panelConfigs, panelConfig); > > + } else { > > + shouldRefreshAll = true; > > Are you using this flag because you want to keep all return statements > together down at the bottom of the method? I would probably just return > here, but I don't have a strong preference if you prefer this style :) This is to track if there were any "refresh all" pending changes in the queue, in which case we want to run a full refresh on the final list of PanelConfigs after the queue has been fully processed - or just return the resulting list of PanelConfigs otherwise.

https://hg.mozilla.org/integration/fx-team/rev/dd871c955867 https://hg.mozilla.org/integration/fx-team/rev/eb6bff0ad110 https://hg.mozilla.org/integration/fx-team/rev/35bd2e5721d8 https://hg.mozilla.org/integration/fx-team/rev/ae8208a7fa57 https://hg.mozilla.org/integration/fx-team/rev/425cb2fb8536

https://hg.mozilla.org/mozilla-central/rev/dd871c955867 https://hg.mozilla.org/mozilla-central/rev/eb6bff0ad110 https://hg.mozilla.org/mozilla-central/rev/35bd2e5721d8 https://hg.mozilla.org/mozilla-central/rev/ae8208a7fa57 https://hg.mozilla.org/mozilla-central/rev/425cb2fb8536