:Margaret Leibovic Reporter
	Description • 10 years ago

This bug should also cover doing the appropriate thing on the Java side.

	Lucas Rocha (:lucasr) Assignee
	Comment 1 • 10 years ago

Taking this while jdover is busy with home banner stuff.

	Lucas Rocha (:lucasr) Assignee
	Comment 10 • 10 years ago

Comment on attachment 8366963 [details] [diff] [review]
Bug 949174/964375 - Factor out method to transform Panel into JSON (r=margaret)

Prop work.

	Lucas Rocha (:lucasr) Assignee
	Comment 11 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 12 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 13 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 14 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 15 • 10 years ago

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().

	Lucas Rocha (:lucasr) Assignee
	Comment 16 • 10 years ago

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).

	Lucas Rocha (:lucasr) Assignee
	Comment 17 • 10 years ago

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.

	:Margaret Leibovic Reporter
	Comment 18 • 10 years ago

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.

	:Margaret Leibovic Reporter
	Comment 19 • 10 years ago

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.

	:Margaret Leibovic Reporter
	Comment 21 • 10 years ago

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.

	Richard Newman [:rnewman]
	Comment 22 • 10 years ago

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

	Lucas Rocha (:lucasr) Assignee
	Comment 23 • 10 years ago

(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.

	:Margaret Leibovic Reporter
	Comment 25 • 10 years ago

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)

	:Margaret Leibovic Reporter
	Comment 26 • 10 years ago

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.

	:Margaret Leibovic Reporter
	Comment 27 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 28 • 10 years ago

(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.

	Lucas Rocha (:lucasr) Assignee
	Comment 29 • 10 years ago

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.

	:Margaret Leibovic Reporter
	Comment 30 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 31 • 10 years ago

(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 :-)

	:Margaret Leibovic Reporter
	Comment 32 • 10 years ago

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()?

	Richard Newman [:rnewman]
	Comment 33 • 10 years ago

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));

	Lucas Rocha (:lucasr) Assignee
	Comment 34 • 10 years ago

(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.

	Lucas Rocha (:lucasr) Assignee
	Comment 38 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 39 • 10 years ago

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).

	Lucas Rocha (:lucasr) Assignee
	Comment 40 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 42 • 10 years ago

(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.

	Chenxia Liu [:liuche]
	Comment 43 • 10 years ago

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.

	Chenxia Liu [:liuche]
	Comment 44 • 10 years ago

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?

	Lucas Rocha (:lucasr) Assignee
	Comment 45 • 10 years ago

(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.

	Lucas Rocha (:lucasr) Assignee
	Comment 46 • 10 years ago

(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.

	Chenxia Liu [:liuche]
	Comment 47 • 10 years ago

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.

	Richard Newman [:rnewman]
	Comment 48 • 10 years ago

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?

	Richard Newman [:rnewman]
	Comment 50 • 10 years ago

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!

	Lucas Rocha (:lucasr) Assignee
	Comment 51 • 10 years ago

(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.

	Lucas Rocha (:lucasr) Assignee
	Comment 53 • 10 years ago

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.

	Lucas Rocha (:lucasr) Assignee
	Comment 54 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/4103c332f013
https://hg.mozilla.org/integration/fx-team/rev/31f1a0b3b5b8
https://hg.mozilla.org/integration/fx-team/rev/a49670361d8a
https://hg.mozilla.org/integration/fx-team/rev/3ce5275b17ed
https://hg.mozilla.org/integration/fx-team/rev/299bcc7ae1fe
https://hg.mozilla.org/integration/fx-team/rev/cfd371a2c63b
https://hg.mozilla.org/integration/fx-team/rev/1a5908740d35
https://hg.mozilla.org/integration/fx-team/rev/9385b83f32c5
https://hg.mozilla.org/integration/fx-team/rev/75b2985c88bb
https://hg.mozilla.org/integration/fx-team/rev/f9977589e824
https://hg.mozilla.org/integration/fx-team/rev/9897f4f42176

	Lucas Rocha (:lucasr) Assignee
	Comment 55 • 10 years ago

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.

	Richard Newman [:rnewman]
	Comment 56 • 10 years ago

(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!

	Lucas Rocha (:lucasr) Assignee
	Comment 57 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/4103c332f013
https://hg.mozilla.org/mozilla-central/rev/31f1a0b3b5b8
https://hg.mozilla.org/mozilla-central/rev/a49670361d8a
https://hg.mozilla.org/mozilla-central/rev/3ce5275b17ed
https://hg.mozilla.org/mozilla-central/rev/299bcc7ae1fe
https://hg.mozilla.org/mozilla-central/rev/cfd371a2c63b
https://hg.mozilla.org/mozilla-central/rev/1a5908740d35
https://hg.mozilla.org/mozilla-central/rev/9385b83f32c5
https://hg.mozilla.org/mozilla-central/rev/75b2985c88bb
https://hg.mozilla.org/mozilla-central/rev/f9977589e824
https://hg.mozilla.org/mozilla-central/rev/9897f4f42176