Closed Bug 968170 Opened 6 years ago Closed 6 years ago

Trigger HomeConfig refreshes conditionally

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: lucasr, Assigned: lucasr)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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.
Attachment #8373413 - Flags: review?(margaret.leibovic)
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.
Attachment #8373414 - Flags: review?(margaret.leibovic)
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.
Attachment #8373415 - Flags: review?(margaret.leibovic)
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.
Attachment #8373416 - Flags: review?(margaret.leibovic)
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.
Attachment #8373417 - Flags: review?(margaret.leibovic)
Blocks: 968188
Blocks: 968172
Attachment #8373413 - Flags: review?(margaret.leibovic) → review+
Attachment #8373414 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8373415 - Flags: review?(margaret.leibovic) → review+
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 :)
Attachment #8373417 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8373416 - Flags: review?(margaret.leibovic) → review+
(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.
You need to log in before you can comment on or make changes to this bug.