Closed Bug 964375 Opened 7 years ago Closed 7 years ago

Add auto-install option to Home.panels.add API

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: Margaret, Assigned: lucasr)

References

(Blocks 2 open bugs)

Details

Attachments

(12 files, 4 obsolete files)

1.11 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
889 bytes, patch
Margaret
: review+
Details | Diff | Splinter Review
1.34 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
3.11 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
7.64 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
4.52 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
7.44 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
3.99 KB, patch
liuche
: review+
Details | Diff | Splinter Review
1.00 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
3.09 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
12.34 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
1.76 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
This bug should also cover doing the appropriate thing on the Java side.
Taking this while jdover is busy with home banner stuff.
Assignee: nobody → lucasr.at.mozilla
Comment on attachment 8366963 [details] [diff] [review]
Bug 949174/964375 - Factor out method to transform Panel into JSON (r=margaret)

Prop work.
Attachment #8366963 - Flags: review?(margaret.leibovic)
Comment on attachment 8366964 [details] [diff] [review]
Bug 949174/964375 - Send HomePanels:Install when autoInstall option is set (r=margaret)

Auto-install stuff on the JS side. Nothing on the Java side yet to handle this.
Attachment #8366964 - Flags: review?(margaret.leibovic)
Comment on attachment 8366965 [details] [diff] [review]
Bug 949174/964375 - Make PanelType optional in PanelConfig's JSON constructor (r=margaret)

The JSON data from JS doesn't contain a PanelType because it's implicit that we're dealing with a dynamic panel. Fallback automatically to dynamic type just for the JSON case.
Attachment #8366965 - Flags: review?(margaret.leibovic)
Comment on attachment 8366966 [details] [diff] [review]
Bug 949174/964375 - Encapsulate PanelInfo data behind getters (r=margaret)

Encapsulate things a bit more and provide API to convert PanelConfig to PanelInfo.
Attachment #8366966 - Flags: review?(margaret.leibovic)
Comment on attachment 8366967 [details] [diff] [review]
Bug 949174/964375 - Factor out code to create built-in panel configs (r=margaret)

We'll need the shared code that knows how to consistently create built-in PanelConfig instances. This will be used to refresh PanelConfig instances on HomeConfig invalidations.
Attachment #8366967 - Flags: review?(margaret.leibovic)
Comment on attachment 8366968 [details] [diff] [review]
Bug 949174/964375 - Add requestPanelsById() to PanelManager (r=margaret)

We'll need to request a specific set of PanelInfos when invalidating HomeConfig. requestAvailablePanels() becomes just a special-case of requestPanelsById().
Attachment #8366968 - Flags: review?(margaret.leibovic)
Comment on attachment 8366969 [details] [diff] [review]
Bug 949174/964375 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

Here's the real meat. HomeConfigInvalidator tries to batch as many HomeConfig updates as possible. It will install, remove, and 'refresh' panels all at-once. 
A PanelConfig 'refresh' re-fetches all layout/views/strings associated with a PanelConfig currently being displayed in HomePager (both for built-in and dynamic panels).
Attachment #8366969 - Flags: review?(margaret.leibovic)
Comment on attachment 8366970 [details] [diff] [review]
Bug 949174/964375 - Invalidate HomeConfig when the locale changes (r=margaret)

Triggering a loader restart is not enough anymore as we're now persisting the HomeConfig state. This means an invalidation requires re-fetching layout/view/strings for each panel and saving the new state. The invalidation will implicitly trigger a refresh on homepager via the OnChangeListener associated with the HomeConfig instance in HomePager.

It's a bit unfortunate that we have to use HomeConfigInvalidator directly in BrowserApp, but onLocaleReady seems to be the only safe spot to trigger an invalidation.
Attachment #8366970 - Flags: review?(margaret.leibovic)
Attachment #8366963 - Flags: review?(margaret.leibovic) → review+
Attachment #8366964 - Flags: review?(margaret.leibovic) → review+
Attachment #8366965 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8366966 [details] [diff] [review]
Bug 949174/964375 - Encapsulate PanelInfo data behind getters (r=margaret)

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

r+, but maybe we can get rid of more things in PanelInfo.

::: mobile/android/base/home/PanelManager.java
@@ +45,5 @@
> +        }
> +
> +        public String getTitle() {
> +            return mTitle;
> +        }

Does anything ever use these getId/getTitle methods? If we're just going to directly translate a PanelInfo into a PanelConfig from the JSON data, we don't need to hold onto the id/title separately.

@@ +51,5 @@
> +        public PanelConfig toPanelConfig() {
> +            try {
> +                return new PanelConfig(mJsonData);
> +            } catch (Exception e) {
> +                return null;

I think we should at least log this exception. It seems like it would be hard to debug this if a consumer ran into this.
Attachment #8366966 - Flags: review?(margaret.leibovic) → review+
Attachment #8366967 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8366969 [details] [diff] [review]
Bug 949174/964375 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

rnewman might like to look over the threading logic in here.
Attachment #8366969 - Flags: feedback?(rnewman)
Attachment #8367024 - Flags: review?(margaret.leibovic)
Comment on attachment 8367024 [details] [diff] [review]
Bug 949174/964375 - Fix typo in Home.panels.remove() (r=margaret)

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

This was already fixed by bug 964454.
Attachment #8367024 - Flags: review?(margaret.leibovic)
Comment on attachment 8366969 [details] [diff] [review]
Bug 949174/964375 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

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

I think there are two, possibly preferable, ways to implement HomeConfigInvalidator that would eliminate some synchronization.

The first is to simply use the background thread to serialize your operations. If all of your operations on an array occur on the same thread, it doesn't need any synchronization. So implement it this way: on whatever thread, parse the JSON blob. Then queue up a runnable that directly inserts the new config into mHomeConfig, or inserts it into a non-synchronized ArrayList, or whatever. You're basically using locks right now to avoid doing this, but then you queue up runnables anyway. As you do now, queue up a delayed runnable on the same thread to do the consuming.

The second is to use a real concurrent datastructure, like ConcurrentLinkedQueue. Then you don't need to lock at all: in your producer, parse items and push them into the right queue. In your background runnable, pop from the queue until it's empty, then do your processing. No locks.

The advantage of that approach is that you're not tying up the background thread -- which is doing other work for the application.

Consider using a new single-threaded Executor here if you expect this to be called frequently. I don't have enough context to tell; probably not.

Another optimization is to encourage callers to pass multiple installs or removals in one go. I presume that you're expecting that, because otherwise the delayed processing is unnecessary. This removes some of the inefficiency in whichever method you end up using.

Gentle f- for concurrency issues, and the potential for simplification here.

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +78,5 @@
> +
> +    private void handlePanelInstall(JSONObject json) {
> +        synchronized(mPendingInstalls) {
> +            try {
> +                mPendingInstalls.add(new PanelConfig(json));

Parse the panel config outside the lock.

@@ +89,5 @@
> +
> +        scheduleInvalidation();
> +    }
> +
> +    private void handlePanelRemove(String id) {

Check for null id up-front.

@@ +99,5 @@
> +        synchronized(mPendingInstalls) {
> +            for (Iterator<PanelConfig> i = mPendingInstalls.iterator(); i.hasNext();) {
> +                final PanelConfig panelConfig = i.next();
> +
> +                if (TextUtils.equals(panelConfig.getId(), id)) {

Don't ever create a PanelConfig with a null ID, and just compare these with .equals.

@@ +100,5 @@
> +            for (Iterator<PanelConfig> i = mPendingInstalls.iterator(); i.hasNext();) {
> +                final PanelConfig panelConfig = i.next();
> +
> +                if (TextUtils.equals(panelConfig.getId(), id)) {
> +                    i.remove();

Worth documenting that you don't `break` at this point because you allow for the same panel to be queued multiple times. Or `break`.

@@ +114,5 @@
> +        final Handler bgHandler = ThreadUtils.getBackgroundHandler();
> +
> +        if (mInvalidationRunnable != null) {
> +            bgHandler.removeCallbacks(mInvalidationRunnable);
> +            mInvalidationRunnable = null;

You access mInvalidationRunnable three times in this method: one check, two assignments, and a read. Even though the variable is marked as volatile, this is still unsafe, even with only a foreground and background thread. For example, here's a trivial race:

BG: line 122
FG: line 116
FG: line 118 -- assigns null!
BG: line 144 -- queues up a null runnable!

I suspect that you're fine to simply *always* queue up an invalidation runnable. Worst-case is it runs and finds that the pending lists are empty.

But if you don't want to do that, consider that the work done by mInvalidationRunnable is *always the same*. You should be defining it as a single instance and never discarding it! That's just wasting memory.

@@ +256,5 @@
> +    private void executeInvalidation(List<PanelConfig> installs, List<String> removals) {
> +        final List<PanelConfig> panelConfigs = new ArrayList<PanelConfig>(mHomeConfig.load());
> +
> +        executeRemovals(panelConfigs, removals);
> +        executeInstalls(panelConfigs, installs);

I'd be inclined to do isEmpty checks at this point, avoiding the need to construct iterators over each list (and do a bunch of extra method calls).

::: mobile/android/base/home/HomeConfigInvalidator.java.orig
@@ +1,1 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-

This file probably shouldn't be in the patch :P
Attachment #8366969 - Flags: feedback?(rnewman) → feedback-
(In reply to :Margaret Leibovic from comment #18)
> Comment on attachment 8366966 [details] [diff] [review]
> Bug 949174/964375 - Encapsulate PanelInfo data behind getters (r=margaret)
> 
> Review of attachment 8366966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, but maybe we can get rid of more things in PanelInfo.
> 
> ::: mobile/android/base/home/PanelManager.java
> @@ +45,5 @@
> > +        }
> > +
> > +        public String getTitle() {
> > +            return mTitle;
> > +        }
> 
> Does anything ever use these getId/getTitle methods? If we're just going to
> directly translate a PanelInfo into a PanelConfig from the JSON data, we
> don't need to hold onto the id/title separately.

I expect, for example, liuche to use PanelInfo.getTitle() in the panel choose UI to list the available panels. Also, we'll probably add an icon image to the list of panels in the future? 

> @@ +51,5 @@
> > +        public PanelConfig toPanelConfig() {
> > +            try {
> > +                return new PanelConfig(mJsonData);
> > +            } catch (Exception e) {
> > +                return null;
> 
> I think we should at least log this exception. It seems like it would be
> hard to debug this if a consumer ran into this.

Good point, done.
Comment on attachment 8366968 [details] [diff] [review]
Bug 949174/964375 - Add requestPanelsById() to PanelManager (r=margaret)

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

r+ with comments addressed.

::: mobile/android/base/home/PanelManager.java
@@ +73,2 @@
>       *
>       * @param callback onComplete will be called on the UI thread.

Update the @param documentation here, and indicate what passing a null value for ids will do, since you're supporting that.

@@ +85,5 @@
>          }
>  
> +        JSONObject message;
> +        try {
> +            message = new JSONObject();

Creating a new empty JSONObject shouldn't throw, you can probably just declare this up above, and then message can be final.

::: mobile/android/modules/Home.jsm
@@ +181,5 @@
>      let panels = [];
>      for (let id in this._panels) {
>        let panel = this._panels[id];
> +      if (ids == null || ids.indexOf(panel.id) >= 0)
> +        panels.push(this._panelToJSON(panel));

Nit: brace single-line ifs. Also add a comment explaining that a null value for ids means that the requester wants all panels.

Also, if you want, you could avoid declaring this ids variable and just do:

if (!data.ids || data.ids.indexOf(panel.id) >= 0)
Attachment #8366968 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8366969 [details] [diff] [review]
Bug 949174/964375 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

Clearing review until there's a new version of the patch.
Attachment #8366969 - Flags: review?(margaret.leibovic)
Comment on attachment 8366970 [details] [diff] [review]
Bug 949174/964375 - Invalidate HomeConfig when the locale changes (r=margaret)

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

::: mobile/android/base/home/HomePager.java
@@ -211,5 @@
>          mTabStrip.setVisibility(View.INVISIBLE);
>  
> -        // Load list of panels from configuration. Restart the loader if necessary.
> -        if (mRestartLoader) {
> -            lm.restartLoader(LOADER_ID_CONFIG, null, mConfigLoaderCallbacks);

So will the HomeConfigInvalidator handle redisplaying the HomePager, such that restarting the loader isn't necessary anymore? Looking at your previous patch, it's not clear to me how this will work.
(In reply to :Margaret Leibovic from comment #27)
> Comment on attachment 8366970 [details] [diff] [review]
> Bug 949174/964375 - Invalidate HomeConfig when the locale changes
> (r=margaret)
> 
> Review of attachment 8366970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomePager.java
> @@ -211,5 @@
> >          mTabStrip.setVisibility(View.INVISIBLE);
> >  
> > -        // Load list of panels from configuration. Restart the loader if necessary.
> > -        if (mRestartLoader) {
> > -            lm.restartLoader(LOADER_ID_CONFIG, null, mConfigLoaderCallbacks);
> 
> So will the HomeConfigInvalidator handle redisplaying the HomePager, such
> that restarting the loader isn't necessary anymore? Looking at your previous
> patch, it's not clear to me how this will work.

An invalidation now will cause a HomeConfig change (ie. OnChangeListener) which will do the trick for us. We only had to do that directly in the HomePager before because an HomePager.invalidate() didn't actually change HomeConfig's state.
Comment on attachment 8367602 [details] [diff] [review]
Bug 949174/964375/952311 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

Thanks for the feedback rnewman! Here's a new patch using ConcurrentLinkedQueue.
Attachment #8367602 - Flags: review?(margaret.leibovic)
Attachment #8367602 - Flags: feedback?(rnewman)
Attachment #8367024 - Attachment is obsolete: true
Attachment #8366969 - Attachment is obsolete: true
Comment on attachment 8366970 [details] [diff] [review]
Bug 949174/964375 - Invalidate HomeConfig when the locale changes (r=margaret)

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

Thanks for the clarification, that sounds good. It's nice to see this logic get cleaned up.
Attachment #8366970 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #25)
> Comment on attachment 8366968 [details] [diff] [review]
> Bug 949174/964375 - Add requestPanelsById() to PanelManager (r=margaret)
> 
> Review of attachment 8366968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed.
> 
> ::: mobile/android/base/home/PanelManager.java
> @@ +73,2 @@
> >       *
> >       * @param callback onComplete will be called on the UI thread.
> 
> Update the @param documentation here, and indicate what passing a null value
> for ids will do, since you're supporting that.

Nice catch, done.

> @@ +85,5 @@
> >          }
> >  
> > +        JSONObject message;
> > +        try {
> > +            message = new JSONObject();
> 
> Creating a new empty JSONObject shouldn't throw, you can probably just
> declare this up above, and then message can be final.

Done.

> ::: mobile/android/modules/Home.jsm
> @@ +181,5 @@
> >      let panels = [];
> >      for (let id in this._panels) {
> >        let panel = this._panels[id];
> > +      if (ids == null || ids.indexOf(panel.id) >= 0)
> > +        panels.push(this._panelToJSON(panel));
> 
> Nit: brace single-line ifs. Also add a comment explaining that a null value
> for ids means that the requester wants all panels.

Done.

> Also, if you want, you could avoid declaring this ids variable and just do:
> 
> if (!data.ids || data.ids.indexOf(panel.id) >= 0)

I kinda prefer the simpler ids local variable :-)
Comment on attachment 8367602 [details] [diff] [review]
Bug 949174/964375/952311 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

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

The panel logic looks good to me, but I don't trust myself enough to r+ the thread safety, so I'm counting on rnewman for that :D

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +83,5 @@
> +     * Runs in the gecko thread.
> +     */
> +    private void handlePanelInstall(JSONObject json) {
> +        if (json == null) {
> +            return;

I don't think you need this check, since getJSONObject will throw instead of returning null.

@@ +102,5 @@
> +     * Runs in the gecko thread.
> +     */
> +    private void handlePanelRemove(String id) {
> +        if (id == null) {
> +            return;

Same here.

@@ +180,5 @@
> +        if (installs.isEmpty()) {
> +            return;
> +        }
> +
> +        Log.d("BOOM", "executeInstalls...");

BOOM!

@@ +281,5 @@
> +    /**
> +     * Runs in the background thread.
> +     */
> +    private void executeInvalidation(List<PanelConfig> installs, List<String> removals) {
> +        final List<PanelConfig> panelConfigs = new ArrayList<PanelConfig>(mHomeConfig.load());

Why do you need to make a new ArrayList, instead of just setting panelConfigs directly to mHomeConfig.load()?
Attachment #8367602 - Flags: review?(rnewman)
Attachment #8367602 - Flags: review?(margaret.leibovic)
Attachment #8367602 - Flags: feedback?(rnewman)
Attachment #8367602 - Flags: feedback+
Blocks: 942878
Comment on attachment 8367602 [details] [diff] [review]
Bug 949174/964375/952311 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

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

One more round of simplification, I think.

::: mobile/android/base/GeckoApplication.java
@@ +68,5 @@
>          GeckoBatteryManager.getInstance().init(getApplicationContext());
>          GeckoBatteryManager.getInstance().start();
>          GeckoNetworkManager.getInstance().init(getApplicationContext());
>          MemoryMonitor.getInstance().init(getApplicationContext());
> +        HomeConfigInvalidator.getInstance().init(getApplicationContext());

Does this need to be uninited?

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +34,5 @@
> +    public static final String LOGTAG = "HomeConfigInvalidator";
> +
> +    private static final HomeConfigInvalidator sInstance = new HomeConfigInvalidator();
> +
> +    private static final int INVALIDATION_DELAY = 500;

Suffix these with _MSEC.

@@ +58,5 @@
> +        mPendingInstalls = new ConcurrentLinkedQueue<PanelConfig>();
> +        mPendingRemovals = new ConcurrentLinkedQueue<String>();
> +
> +        GeckoAppShell.getEventDispatcher().registerEventListener("HomePanels:Install", this);
> +        GeckoAppShell.getEventDispatcher().registerEventListener("HomePanels:Remove", this);

Extract constants: those message names (EVENT_HOMEPANELS_INSTALL, …), and the JSON keys "panel" and "id" (JSON_KEY_ID, …).

@@ +70,5 @@
> +                final JSONObject json = message.getJSONObject("panel");
> +                handlePanelInstall(json);
> +            } else if (event.equals("HomePanels:Remove")) {
> +                Log.d(LOGTAG, "HomePanels:Remove");
> +                final String id = message.getString("id");

if (message.isNull("id")) {
    Log.e(LOGTAG, "Ignoring removal message with null id.");
    return;
}
final String id = message.getString("id");

@@ +102,5 @@
> +     * Runs in the gecko thread.
> +     */
> +    private void handlePanelRemove(String id) {
> +        if (id == null) {
> +            return;

However, if the JSON object contains 

  "id": null

this id string will be the Java String "null". Yes, I know, this is ridiculous.

@@ +111,5 @@
> +
> +        for (Iterator<PanelConfig> i = mPendingInstalls.iterator(); i.hasNext();) {
> +            final PanelConfig panelConfig = i.next();
> +
> +            if (panelConfig.getId().equals(id)) {

id.equals(panelConfig.getId())

@@ +113,5 @@
> +            final PanelConfig panelConfig = i.next();
> +
> +            if (panelConfig.getId().equals(id)) {
> +                i.remove();
> +                Log.d(LOGTAG, "handlePanelRemove: removed pending install: " + panelConfig.getId());

Just use `id` — you've already determined it's equal!

@@ +281,5 @@
> +    /**
> +     * Runs in the background thread.
> +     */
> +    private void executeInvalidation(List<PanelConfig> installs, List<String> removals) {
> +        final List<PanelConfig> panelConfigs = new ArrayList<PanelConfig>(mHomeConfig.load());

The HomeConfig load method doesn't specify that it returns a fresh copy, even though the current implementation does. Fix the docs there, and yeah, skip the allocation.

@@ +284,5 @@
> +    private void executeInvalidation(List<PanelConfig> installs, List<String> removals) {
> +        final List<PanelConfig> panelConfigs = new ArrayList<PanelConfig>(mHomeConfig.load());
> +
> +        executeRemovals(panelConfigs, removals);
> +        executeInstalls(panelConfigs, installs);

There's a timing issue here.

If I add item A, wait 500ms, remove item A, and wait 600ms, the config will not contain item A.

If I add item A, wait 50ms, remove item A, and wait 600ms, the config will contain item A, because the removal was processed before the insertion.

You cannot safely maintain two queues without having this granularity issue.

A trivial solution to this would be to use a single queue to preserve ordering. Add a DELETED_PANEL flag to PanelConfig, or otherwise push sentinels into the same queue you use for installs. Merge executeRemovals and executeInstalls.

@@ +297,5 @@
> +    private class InvalidationRunnable implements Runnable {
> +        @Override
> +        public void run() {
> +            final List<PanelConfig> installs = new ArrayList<PanelConfig>();
> +            while(!mPendingInstalls.isEmpty()) {

This is unnecessary: the queue is thread-safe! Just have the execute method take the queue(s) directly, popping until empty and then returning.

Half of this file just turns into this one-liner inside a runnable:

  mHomeConfig.save(processQueue(mHomeConfig.load(), mPendingOperations));
Attachment #8367602 - Flags: review?(rnewman) → feedback+
(In reply to Richard Newman [:rnewman] from comment #33)
> Comment on attachment 8367602 [details] [diff] [review]
> Bug 949174/964375/952311 - Introduce HomeConfigInvalidator to handle
> install/invalidation (r=margaret)
> 
> Review of attachment 8367602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One more round of simplification, I think.
> 
> ::: mobile/android/base/GeckoApplication.java
> @@ +68,5 @@
> >          GeckoBatteryManager.getInstance().init(getApplicationContext());
> >          GeckoBatteryManager.getInstance().start();
> >          GeckoNetworkManager.getInstance().init(getApplicationContext());
> >          MemoryMonitor.getInstance().init(getApplicationContext());
> > +        HomeConfigInvalidator.getInstance().init(getApplicationContext());
> 
> Does this need to be uninited?

Not really.

> ::: mobile/android/base/home/HomeConfigInvalidator.java
> @@ +34,5 @@
> > +    public static final String LOGTAG = "HomeConfigInvalidator";
> > +
> > +    private static final HomeConfigInvalidator sInstance = new HomeConfigInvalidator();
> > +
> > +    private static final int INVALIDATION_DELAY = 500;
> 
> Suffix these with _MSEC.

Done.

> @@ +58,5 @@
> > +        mPendingInstalls = new ConcurrentLinkedQueue<PanelConfig>();
> > +        mPendingRemovals = new ConcurrentLinkedQueue<String>();
> > +
> > +        GeckoAppShell.getEventDispatcher().registerEventListener("HomePanels:Install", this);
> > +        GeckoAppShell.getEventDispatcher().registerEventListener("HomePanels:Remove", this);
> 
> Extract constants: those message names (EVENT_HOMEPANELS_INSTALL, …), and
> the JSON keys "panel" and "id" (JSON_KEY_ID, …).

Done.

> @@ +70,5 @@
> > +                final JSONObject json = message.getJSONObject("panel");
> > +                handlePanelInstall(json);
> > +            } else if (event.equals("HomePanels:Remove")) {
> > +                Log.d(LOGTAG, "HomePanels:Remove");
> > +                final String id = message.getString("id");
> 
> if (message.isNull("id")) {
>     Log.e(LOGTAG, "Ignoring removal message with null id.");
>     return;
> }
> final String id = message.getString("id");

Not relevant in the new patch anymore.

> @@ +102,5 @@
> > +     * Runs in the gecko thread.
> > +     */
> > +    private void handlePanelRemove(String id) {
> > +        if (id == null) {
> > +            return;
> 
> However, if the JSON object contains 
> 
>   "id": null
> 
> this id string will be the Java String "null". Yes, I know, this is
> ridiculous.

Ditto.

> @@ +111,5 @@
> > +
> > +        for (Iterator<PanelConfig> i = mPendingInstalls.iterator(); i.hasNext();) {
> > +            final PanelConfig panelConfig = i.next();
> > +
> > +            if (panelConfig.getId().equals(id)) {
> 
> id.equals(panelConfig.getId())

Done.

> @@ +113,5 @@
> > +            final PanelConfig panelConfig = i.next();
> > +
> > +            if (panelConfig.getId().equals(id)) {
> > +                i.remove();
> > +                Log.d(LOGTAG, "handlePanelRemove: removed pending install: " + panelConfig.getId());
> 
> Just use `id` — you've already determined it's equal!

Yep, done.

> @@ +281,5 @@
> > +    /**
> > +     * Runs in the background thread.
> > +     */
> > +    private void executeInvalidation(List<PanelConfig> installs, List<String> removals) {
> > +        final List<PanelConfig> panelConfigs = new ArrayList<PanelConfig>(mHomeConfig.load());
> 
> The HomeConfig load method doesn't specify that it returns a fresh copy,
> even though the current implementation does. Fix the docs there, and yeah,
> skip the allocation.

I had made it return an unmodifiable list as a defensive measure. But it turns out this is not necessary in practice (and it's causing redundant code like this one here). I'll change it to return a ordinary list.

> @@ +284,5 @@
> > +    private void executeInvalidation(List<PanelConfig> installs, List<String> removals) {
> > +        final List<PanelConfig> panelConfigs = new ArrayList<PanelConfig>(mHomeConfig.load());
> > +
> > +        executeRemovals(panelConfigs, removals);
> > +        executeInstalls(panelConfigs, installs);
> 
> There's a timing issue here.
> 
> If I add item A, wait 500ms, remove item A, and wait 600ms, the config will
> not contain item A.
> 
> If I add item A, wait 50ms, remove item A, and wait 600ms, the config will
> contain item A, because the removal was processed before the insertion.
> 
> You cannot safely maintain two queues without having this granularity issue.
> 
> A trivial solution to this would be to use a single queue to preserve
> ordering. Add a DELETED_PANEL flag to PanelConfig, or otherwise push
> sentinels into the same queue you use for installs. Merge executeRemovals
> and executeInstalls.

Nice catch. Following patch will cover that.

> @@ +297,5 @@
> > +    private class InvalidationRunnable implements Runnable {
> > +        @Override
> > +        public void run() {
> > +            final List<PanelConfig> installs = new ArrayList<PanelConfig>();
> > +            while(!mPendingInstalls.isEmpty()) {
> 
> This is unnecessary: the queue is thread-safe! Just have the execute method
> take the queue(s) directly, popping until empty and then returning.
> 
> Half of this file just turns into this one-liner inside a runnable:
> 
>   mHomeConfig.save(processQueue(mHomeConfig.load(), mPendingOperations));

Yep, new patches coming in a minute.
Comment on attachment 8368713 [details] [diff] [review]
Bug 949174/964375/952311 - Don't return an unmodifiable list on HomeConfig.load() (r=liuche)

This is only causing redundancy on the caller side. We don't hold any copies of the loaded list internally so it's safe to return an ordinary list.
Attachment #8368713 - Flags: review?(liuche)
Comment on attachment 8368714 [details] [diff] [review]
Bug 949174/964375/952311 - Send panel JSON instead of just id in HomePanels:Remove (r=margaret)

We need to streamline how HomePanels:* are processed in Java so the type of data has to always be the same for all events (install and remove).
Attachment #8368714 - Flags: review?(margaret.leibovic)
Comment on attachment 8368715 [details] [diff] [review]
Bug 949174/964375/952311 - Add DELETED_PANEL flag to PanelConfig (r=margaret)

Add DELETED flag and an safe-guard assertion before saving a new list of PanelConfigs. This should make it easier to catch bugs in save() callers.
Attachment #8368715 - Flags: review?(margaret.leibovic)
Attachment #8367602 - Attachment is obsolete: true
(In reply to :Margaret Leibovic from comment #32)
> Comment on attachment 8367602 [details] [diff] [review]
> Bug 949174/964375/952311 - Introduce HomeConfigInvalidator to handle
> install/invalidation (r=margaret)
> 
> Review of attachment 8367602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The panel logic looks good to me, but I don't trust myself enough to r+ the
> thread safety, so I'm counting on rnewman for that :D
> 
> ::: mobile/android/base/home/HomeConfigInvalidator.java
> @@ +83,5 @@
> > +     * Runs in the gecko thread.
> > +     */
> > +    private void handlePanelInstall(JSONObject json) {
> > +        if (json == null) {
> > +            return;
> 
> I don't think you need this check, since getJSONObject will throw instead of
> returning null.

Yep, removed in the new patch.
 
> @@ +102,5 @@
> > +     * Runs in the gecko thread.
> > +     */
> > +    private void handlePanelRemove(String id) {
> > +        if (id == null) {
> > +            return;
> 
> Same here.

Ditto.

> @@ +180,5 @@
> > +        if (installs.isEmpty()) {
> > +            return;
> > +        }
> > +
> > +        Log.d("BOOM", "executeInstalls...");
> 
> BOOM!

Oops! Removed :-)

> @@ +281,5 @@
> > +    /**
> > +     * Runs in the background thread.
> > +     */
> > +    private void executeInvalidation(List<PanelConfig> installs, List<String> removals) {
> > +        final List<PanelConfig> panelConfigs = new ArrayList<PanelConfig>(mHomeConfig.load());
> 
> Why do you need to make a new ArrayList, instead of just setting
> panelConfigs directly to mHomeConfig.load()?

This was because the list returned by the load() used to be unmodifiable. I changed that in the patch I've just submitted for liuche's review.
Attachment #8368793 - Flags: review?(rnewman)
Comment on attachment 8368713 [details] [diff] [review]
Bug 949174/964375/952311 - Don't return an unmodifiable list on HomeConfig.load() (r=liuche)

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

This looks good, since HomeConfig just makes final Lists to return anyways.

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +23,5 @@
>  public class PanelsPreferenceCategory extends CustomListCategory {
>      public static final String LOGTAG = "PanelsPrefCategory";
>  
>      protected HomeConfig mHomeConfig;
> +    protected List<PanelConfig> mPanelConfigs;

Even though saveHomeConfig never gets called until the panel is loaded, it still assumes that mPanelConfigs is not null. Possibly add a null check to saveHomeConfig if you think that's useful.
Attachment #8368713 - Flags: review?(liuche) → review+
Attachment #8368714 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8366966 [details] [diff] [review]
Bug 949174/964375 - Encapsulate PanelInfo data behind getters (r=margaret)

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

::: mobile/android/base/home/PanelManager.java
@@ +31,5 @@
>  
>      public class PanelInfo {
> +        private final String mId;
> +        private final String mTitle;
> +        private final JSONObject mJsonData;

Nit: To be consistent with JSONObject capitalization, can we make this mJSONData?
(In reply to Chenxia Liu [:liuche] from comment #44)
> Comment on attachment 8366966 [details] [diff] [review]
> Bug 949174/964375 - Encapsulate PanelInfo data behind getters (r=margaret)
> 
> Review of attachment 8366966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/PanelManager.java
> @@ +31,5 @@
> >  
> >      public class PanelInfo {
> > +        private final String mId;
> > +        private final String mTitle;
> > +        private final JSONObject mJsonData;
> 
> Nit: To be consistent with JSONObject capitalization, can we make this
> mJSONData?

Sure, done.
(In reply to Chenxia Liu [:liuche] from comment #43)
> Comment on attachment 8368713 [details] [diff] [review]
> Bug 949174/964375/952311 - Don't return an unmodifiable list on
> HomeConfig.load() (r=liuche)
> 
> Review of attachment 8368713 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, since HomeConfig just makes final Lists to return anyways.
> 
> ::: mobile/android/base/preferences/PanelsPreferenceCategory.java
> @@ +23,5 @@
> >  public class PanelsPreferenceCategory extends CustomListCategory {
> >      public static final String LOGTAG = "PanelsPrefCategory";
> >  
> >      protected HomeConfig mHomeConfig;
> > +    protected List<PanelConfig> mPanelConfigs;
> 
> Even though saveHomeConfig never gets called until the panel is loaded, it
> still assumes that mPanelConfigs is not null. Possibly add a null check to
> saveHomeConfig if you think that's useful.

Good point, done.
Comment on attachment 8366966 [details] [diff] [review]
Bug 949174/964375 - Encapsulate PanelInfo data behind getters (r=margaret)

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

::: mobile/android/base/home/PanelManager.java
@@ +45,5 @@
> +        }
> +
> +        public String getTitle() {
> +            return mTitle;
> +        }

I kind of like having these accessible through getters, because then consumers of the PanelInfo don't have to parse JSON; we should do this for members that are commonly consumed. For example, Settings and adding panels will need to display title (and at some point, icon).

I wonder if we should just have a constructor that just takes a JSONObject, and populate the members from that?

@@ +51,5 @@
> +        public PanelConfig toPanelConfig() {
> +            try {
> +                return new PanelConfig(mJsonData);
> +            } catch (Exception e) {
> +                return null;

I noticed that the PanelConfig(JSONObject) constructor also fetches the id and title from the JSON - it seems that the id and title member variables of PanelInfo could be inconsistent from what's in the JSON.

We should either add another constructor that takes title and id in addition to JSONObject, or just stick id and title into the json.
Attachment #8368715 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8368793 [details] [diff] [review]
Bug 949174/964375/952311 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

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

I don't see any obvious concurrency issues here, but HomeConfigInvalidator really feels like it could benefit from some better data structures.

High-level summary of inline comments: there's a lot of bug-prone iteration, index lookup, replacement-via-index, etc. that would all disappear with either a suitable comparison method on PanelConfig, or an ordered data structure that implemented your desired operations. You might consider an ordered map, like Java's LinkedHashMap, or adding an index field to the PanelConfig and just using a Map instead of a List, or splitting order and id -> value into two separate data structures.

It would be worth thinking about that for a little while.

If you're super-confident in your List-and-index code, and you're in a real hurry to land this, then r+.

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +56,5 @@
> +    public void init(Context context) {
> +        mContext = context;
> +        mHomeConfig = HomeConfig.getDefault(context);
> +
> +        mInvalidationRunnable = new InvalidationRunnable();

Statically init.

@@ +57,5 @@
> +        mContext = context;
> +        mHomeConfig = HomeConfig.getDefault(context);
> +
> +        mInvalidationRunnable = new InvalidationRunnable();
> +        mPendingChanges = new ConcurrentLinkedQueue<PanelConfig>();

Just make it final and init in the var declaration? Or do you need the old changes to be thrown away between inits?

@@ +117,5 @@
> +
> +    /**
> +     * Runs in the background thread.
> +     */
> +    private static int indexOfPanelId(List<PanelConfig> panelConfigs, String id) {

If PanelConfig defined a useful equals method, this would be equivalent to .indexOf(panelConfig).

@@ +144,5 @@
> +            final String id = panelConfig.getId();
> +            final int index = indexOfPanelId(panelConfigs, id);
> +
> +            if (panelConfig.isDeleted() && index >= 0) {
> +                panelConfigs.remove(index);

Consider a definition of PanelConfig.equals that will avoid you doing all of this hellish indexOfPanelId nonsense.

@@ +148,5 @@
> +                panelConfigs.remove(index);
> +                Log.d(LOGTAG, "executePendingChanges: removed panel " + id);
> +            } else if (!panelConfig.isDeleted()) {
> +                if (index >= 0) {
> +                    panelConfigs.set(index, panelConfig);

Worth commenting that what you're doing here is *replacing the existing PanelConfig with the same ID at the same position in the array*.

Which is to say: you're never using the index here at all. You only support adding panels to the end, or replacing an existing panel config with its updated equivalent.

(You'll notice that you're Greenspunning an OrderedHashMap here.)

@@ +173,5 @@
> +        for (int i = 0; i < count; i++) {
> +            final PanelConfig panelConfig = panelConfigs.get(i);
> +
> +            PanelConfig refreshedPanelConfig = null;
> +            if (panelConfig.getType() == PanelType.DYNAMIC) {

Use method dispatch to do this?

@@ +175,5 @@
> +
> +            PanelConfig refreshedPanelConfig = null;
> +            if (panelConfig.getType() == PanelType.DYNAMIC) {
> +                for (PanelInfo panelInfo : panelInfos) {
> +                    if (TextUtils.equals(panelInfo.getId(), panelConfig.getId())) {

equals!

@@ +250,5 @@
> +        public void run() {
> +            final List<PanelConfig> panelConfigs = mHomeConfig.load();
> +
> +            executePendingChanges(panelConfigs);
> +            executeRefresh(panelConfigs);

Why not have these return suitable data structures (indeed, perhaps without modifying the originals if it simplifies things), and chain them?
Comment on attachment 8370104 [details] [diff] [review]
Bug 949174/964375/952311 - Introduce HomeConfigInvalidator to handle install/invalidation (r=margaret)

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

LGTM. Thanks for your patience!
Attachment #8370104 - Flags: review+
Blocks: 967742
(In reply to Richard Newman [:rnewman] from comment #48)
> Comment on attachment 8368793 [details] [diff] [review]
> Bug 949174/964375/952311 - Introduce HomeConfigInvalidator to handle
> install/invalidation (r=margaret)
> 
> Review of attachment 8368793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see any obvious concurrency issues here, but HomeConfigInvalidator
> really feels like it could benefit from some better data structures.
> 
> High-level summary of inline comments: there's a lot of bug-prone iteration,
> index lookup, replacement-via-index, etc. that would all disappear with
> either a suitable comparison method on PanelConfig, or an ordered data
> structure that implemented your desired operations. You might consider an
> ordered map, like Java's LinkedHashMap, or adding an index field to the
> PanelConfig and just using a Map instead of a List, or splitting order and
> id -> value into two separate data structures.
> 
> It would be worth thinking about that for a little while.
> 
> If you're super-confident in your List-and-index code, and you're in a real
> hurry to land this, then r+.

Just to wrap-up the discussion here: I agree with the overall sentiment that we can do a better job representing the HomeConfig state, especially for the editing case. A simple list for the read-only case works fairly well I think. So, I think we need a more formal API to edit HomeConfig state. I filed bug 967742 to track this.
Comment on attachment 8370255 [details] [diff] [review]
Bug 949174/964375/952311 - Add isDynamic() method to PanelConfig (r=margaret)

One final bit of API in PanelConfig.
Attachment #8370255 - Flags: review?(margaret.leibovic)
Attachment #8368793 - Attachment is obsolete: true
Attachment #8368793 - Flags: review?(rnewman)
Attachment #8370255 - Flags: review?(margaret.leibovic) → review+
Blocks: 968188
Blocks: 952311
Blocks: 949174
I'm skipping the patch that triggers an invalidation in onLocaleReady() btw. It causes a HomeConfig refresh on every startup which is not ideal. I'll be looking for a proper solution in bug 968172.
(In reply to Lucas Rocha (:lucasr) from comment #55)
> I'm skipping the patch that triggers an invalidation in onLocaleReady() btw.
> It causes a HomeConfig refresh on every startup which is not ideal. I'll be
> looking for a proper solution in bug 968172.

Sounds like the right course of action. Thanks, Lucas!
Status: NEW → ASSIGNED
Hardware: ARM → All
Target Milestone: --- → Firefox 30
Blocks: 968530
You need to log in before you can comment on or make changes to this bug.