958192 - Use gecko messages to request current set of available panels from JS

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We should get rid of the shared preferences implementation of data storage in Home.lists/ListManager, and instead we should keep track of the set of registered lists in memory in JS, then request that list from Java when necessary (e.g. when the settings UI is loaded).

Comment 1

[:Margaret Leibovic]

Attachment: patch

This API to request the set of available panels feels a bit clunky, but I couldn't think of a better way to do it, given the asynchronous nature of gecko message passing.

I verified this API works, using my own local hacks. This actually could be a good candidate for a robocop test... load some privileged JS that registers a panel, then create a new PanelManager object and request the available panels. I can file a follow-up to write that test, since I want to give us a little time to shake things out before investing in test writing.

Also, I wonder if we can get rid of PanelInfo and just use PanelEntry instead, since these concepts have now converged to mean the same thing.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8358131 [details] [diff] [review]
patch

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

The HomePanels:Get/HomePanels:Data approach makes sense to me. I just wonder if we'll have any performance problems if the list of available panels grows too big to pass over the JNI bridge. But we can worry about this later. The patch needs work in terms of thread-safety though.

::: mobile/android/base/home/PanelManager.java
@@ +46,5 @@
>  
> +    /**
> +     * Asynchronously fetches list of available panels from Gecko.
> +     */
> +    public void requestAvailablePanels(String requestId, RequestCallback callback) {

I'd expect the requestId to be something that is returned from requestAvailablePanels(), not something the caller defines. Each requestAvailablePanels should return a new unique ID. You can probably use something like AtomicInteger to ensure thread-safety.

@@ +47,5 @@
> +    /**
> +     * Asynchronously fetches list of available panels from Gecko.
> +     */
> +    public void requestAvailablePanels(String requestId, RequestCallback callback) {
> +        // If there are no pending callbacks, register the event listener.

I think something like "If there were no pending callbacks before, ..." would be more clear.

@@ +76,3 @@
>  
> +            final String requestId = message.getString("requestId");
> +            mCallbackMap.get(requestId).onComplete(panelInfos);

Two things here:

1. handleMessage is called in the Gecko thread. This means you're using mCallbackMap from multiple threads. Here's what you can do:

- Turn mCallbackMap into a synchronized HashMap with something like Collections.synchronizedMap().
- Enclose the block where you call get() + remove() on mCallbackMap in a synchronized block (on mCallbackMap). Same for the isEmpty() + put() block in requestAvailablePanels().

2. The callback is going to be called in the Gecko thread which might lead to unintended results and/or crashes. We better always call the callbacks in the UI thread, no?

@@ +82,5 @@
>              Log.e(LOGTAG, "Exception handling " + event + " message", e);
>          }
> +
> +        // Unregister the event listener if there are no more pending callbacks.
> +        if (mCallbackMap.isEmpty()) {

I'd move this into the same synchronized block than get() and remove() above.

::: mobile/android/modules/Home.jsm
@@ +189,5 @@
>  
> +    // If this is the first panel we're adding, add an
> +    // observer to listen for requests from the Java UI.
> +    if (Object.keys(this._panels).length == 1) {
> +      Services.obs.addObserver(this, "HomePanels:Get", false);

We need to listen to HomePanels:Get even if there are no exported panels, no? I mean, the user might still go to the Settings UI and try to get a list of available panels, even with no panels add-ons installed (in which case the HomePanels:Get will never get a response?).

@@ +195,3 @@
>    },
>  
>    remove: function(id) {

Just wondering: do we require add-ons to explicitly call this to unregister their panels when the add-on is uninstalled/disabled?

Comment 3

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #2)

> ::: mobile/android/modules/Home.jsm
> @@ +189,5 @@
> >  
> > +    // If this is the first panel we're adding, add an
> > +    // observer to listen for requests from the Java UI.
> > +    if (Object.keys(this._panels).length == 1) {
> > +      Services.obs.addObserver(this, "HomePanels:Get", false);
> 
> We need to listen to HomePanels:Get even if there are no exported panels,
> no? I mean, the user might still go to the Settings UI and try to get a list
> of available panels, even with no panels add-ons installed (in which case
> the HomePanels:Get will never get a response?).

This raises an interesting point... with the current design, we don't run any Home.jsm initialization code anywhere, it only comes into action once an add-on loads it and starts making API calls (exception being Snippets.js also does this now).

> @@ +195,3 @@
> >    },
> >  
> >    remove: function(id) {
> 
> Just wondering: do we require add-ons to explicitly call this to unregister
> their panels when the add-on is uninstalled/disabled?

I assumed so, yes. It is the responsibility of bootstrapped add-ons to clean up after themselves (removing menuitems, etc), so I assumed this would follow the same convention.

I haven't thought about the case of a non-restartless add-on using these APIs, but I think we can assume for now that we'll only support restartless add-ons.

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8358131 [details] [diff] [review]
patch

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

A few thoughts in the comments, but I think the structure looks right. +1 on the thread-safety changes suggested by lucasr.

One thing that is kind out of the scope of this bug that this patch made me think about was, what else should PanelInfo contain, beyond title and ID? For instance, an icon for that add-on/panel (for adding a new panel), etc., and how that fits into this add-on panel API. I'm not sure if that should be passed in through HomePanels.add, or fetched on _handleGet. Thoughts?

::: mobile/android/base/home/PanelManager.java
@@ +69,5 @@
> +            final JSONArray panels = message.getJSONArray("panels");
> +            final int count = panels.length();
> +            for (int i = 0; i < count; i++) {
> +                final JSONObject panel = panels.getJSONObject(i);
> +                final PanelInfo info = new PanelInfo(panel.getString("id"), panel.getString("title"));

Since the PanelInfo interface isn't set and probably will get more complicated, this could be moved into a makePanelInfoFromJSON().

::: mobile/android/modules/Home.jsm
@@ +177,1 @@
>    add: function(options) {

Will add be called directly by something when add-ons with panels are installed? Should there be an observer for adding panels?

@@ +189,5 @@
>  
> +    // If this is the first panel we're adding, add an
> +    // observer to listen for requests from the Java UI.
> +    if (Object.keys(this._panels).length == 1) {
> +      Services.obs.addObserver(this, "HomePanels:Get", false);

+1.

As a side note, what sends requests through PanelManager? I can think of "Add more lists", or requesting extra data about the Panel that HomeConfig doesn't provide (isn't needed for startup) - what else?

@@ +199,3 @@
>  
> +    sendMessageToJava({
> +      type: "HomePanels:Removed",

What do you think about calling this "HomePanels:Remove"? I can see how it could be either, though, depending on if we want to think about this as notifying Java to remove a panel, or if we are informing Java that a panel has been removed.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #3)
> (In reply to Lucas Rocha (:lucasr) from comment #2)
> 
> > ::: mobile/android/modules/Home.jsm
> > @@ +189,5 @@
> > >  
> > > +    // If this is the first panel we're adding, add an
> > > +    // observer to listen for requests from the Java UI.
> > > +    if (Object.keys(this._panels).length == 1) {
> > > +      Services.obs.addObserver(this, "HomePanels:Get", false);
> > 
> > We need to listen to HomePanels:Get even if there are no exported panels,
> > no? I mean, the user might still go to the Settings UI and try to get a list
> > of available panels, even with no panels add-ons installed (in which case
> > the HomePanels:Get will never get a response?).
> 
> This raises an interesting point... with the current design, we don't run
> any Home.jsm initialization code anywhere, it only comes into action once an
> add-on loads it and starts making API calls (exception being Snippets.js
> also does this now).

Yeah. It seems to me that we'll need to be able to query the HomePanels system even when none of the active add-ons are using Home.jsm.  
 
> > @@ +195,3 @@
> > >    },
> > >  
> > >    remove: function(id) {
> > 
> > Just wondering: do we require add-ons to explicitly call this to unregister
> > their panels when the add-on is uninstalled/disabled?
> 
> I assumed so, yes. It is the responsibility of bootstrapped add-ons to clean
> up after themselves (removing menuitems, etc), so I assumed this would
> follow the same convention.

The reason I asked is because the process seems a bit error-prone. I realize that for UI changes (e.g. menu items) it would be hard to automatically revert the change. But for panels, it seems (and I might be totally wrong here) that it should be simple enough to just remove any panels exported by the add-on automatically when the add-on gets disabled/uninstalled?

On the other hand, if this is already a common practice for other parts of our add-on API, it would probably feel natural to have add-ons clean up panels too. 

> I haven't thought about the case of a non-restartless add-on using these
> APIs, but I think we can assume for now that we'll only support restartless
> add-ons.

Agree.

Comment 6

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #5)

> > > @@ +195,3 @@
> > > >    },
> > > >  
> > > >    remove: function(id) {
> > > 
> > > Just wondering: do we require add-ons to explicitly call this to unregister
> > > their panels when the add-on is uninstalled/disabled?
> > 
> > I assumed so, yes. It is the responsibility of bootstrapped add-ons to clean
> > up after themselves (removing menuitems, etc), so I assumed this would
> > follow the same convention.
> 
> The reason I asked is because the process seems a bit error-prone. I realize
> that for UI changes (e.g. menu items) it would be hard to automatically
> revert the change. But for panels, it seems (and I might be totally wrong
> here) that it should be simple enough to just remove any panels exported by
> the add-on automatically when the add-on gets disabled/uninstalled?
> 
> On the other hand, if this is already a common practice for other parts of
> our add-on API, it would probably feel natural to have add-ons clean up
> panels too. 

The issue here is that we don't actually know when the add-on is uninstalled. In fact, the whole add-on system is designed such that you don't know if some JS code is coming from an add-on or from within the browser itself. That's why that "clean up when you're uninstalled" pattern exists, and part of add-on review is checking to make sure an add-on does that.

In the end, we can never control if a random add-on will totally bust the browser, but we can make sure that add-on doesn't end up on a.m.o. or in our list of promoted panels :)

Comment 7

[:Margaret Leibovic]

Attachment: patch v2

This patch addressed previous concerns over thread safety and observer registration.

I added some logic in browser.js for lazy-loaded modules to observe notifications, similar to the code above it for lazy-loaded scripts. This is one instance where fat arrow functions were really handy, since they keep the same scope as the environment they're defined in.

(I also filed bug 959366 about how we don't actually unregister these observers right now, but I figured I might as well stay consistent and use LazyNotificationGetter.)

I think we should work in a follow-up to do something with the "HomePanel:Remove" case, but PanelManager will definitely need to listen for that and somehow propagate that information to the home config backend, as well as trigger a UI refresh.

Comment 8

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #7)

> I think we should work in a follow-up to do something with the
> "HomePanel:Remove" case, but PanelManager will definitely need to listen for
> that and somehow propagate that information to the home config backend, as
> well as trigger a UI refresh.

I just remembered bug 952311. We can take care of this in there.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8359483 [details] [diff] [review]
patch v2

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

Nice.

::: mobile/android/base/home/PanelManager.java
@@ +50,5 @@
> +    private static final Map<Integer, RequestCallback> sCallbacks = Collections.synchronizedMap(new HashMap<Integer, RequestCallback>());
> +
> +    /**
> +     * Asynchronously fetches list of available panels from Gecko.
> +     * 

nit: remove trailing space

@@ +88,4 @@
>  
> +            synchronized(sCallbacks) {
> +                callback = sCallbacks.get(requestId);
> +                sCallbacks.remove(requestId);

remove() already returns the value associated with requestId I think. You can just do:

callback = sCallbacks.remove(requestId);

@@ +94,5 @@
> +                if (sCallbacks.isEmpty()) {
> +                    GeckoAppShell.getEventDispatcher().unregisterEventListener("HomePanels:Data", this);
> +                }
> +            }
> +

Maybe we should safeguard against a potential null callback at this point? My gut feeling says no. Just wondering.

Comment 10

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #9)

> @@ +94,5 @@
> > +                if (sCallbacks.isEmpty()) {
> > +                    GeckoAppShell.getEventDispatcher().unregisterEventListener("HomePanels:Data", this);
> > +                }
> > +            }
> > +
> 
> Maybe we should safeguard against a potential null callback at this point?
> My gut feeling says no. Just wondering.

Yeah, callback shouldn't ever be null here, so I don't think we should add a null check.

Comment 11

[:Margaret Leibovic]

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

Comment 12

[Brian Nicholson (:bnicholson)]

Late drive-by: Collections.synchronizedMap was unnecessary here since you're already synchronizing all accesses to sCallbacks. Probably would have been better off just sticking with a standard HashMap.

Comment 13

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #12)
> Late drive-by: Collections.synchronizedMap was unnecessary here since you're
> already synchronizing all accesses to sCallbacks. Probably would have been
> better off just sticking with a standard HashMap.

Follow-up mentor bug?

Comment 14

[Ed Morley [:emorley]]

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