978991 - Hook for add-ons when panel is added/removed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Chenxia Liu [:liuche]]

First, we should decide whether to display "Hide" for dynamic panels. It might be a little confusing to see both "Hide" and "Remove", but after installing a panel (and possibly authenticating), it seems reasonable to want the choice to show or hide that panel without "logging out" or losing any setup the user might have done.

We'll also need to add observers for uninstall/disable for add-ons.

Comment 1

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #0)
> First, we should decide whether to display "Hide" for dynamic panels. It
> might be a little confusing to see both "Hide" and "Remove", but after
> installing a panel (and possibly authenticating), it seems reasonable to
> want the choice to show or hide that panel without "logging out" or losing
> any setup the user might have done.

Whatever auth state the panel might have is not stored in HomeConfig. This means that removing a panel will not clear its data. So, the decision to allow hiding a dynamic panel is kind of orthogonal to this aspect.

> We'll also need to add observers for uninstall/disable for add-ons.

You mean observers to let the add-on know when one of its panels has been disabled/removed from about:home? What use case you have in mind here? Clearing the panel's data?

Comment 2

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> (In reply to Chenxia Liu [:liuche] from comment #0)
> > First, we should decide whether to display "Hide" for dynamic panels. It
> > might be a little confusing to see both "Hide" and "Remove", but after
> > installing a panel (and possibly authenticating), it seems reasonable to
> > want the choice to show or hide that panel without "logging out" or losing
> > any setup the user might have done.
> 
> Whatever auth state the panel might have is not stored in HomeConfig. This
> means that removing a panel will not clear its data. So, the decision to
> allow hiding a dynamic panel is kind of orthogonal to this aspect.

I think that users will perceive "Hide" and "Remove" differently, and may assume that "Remove" will do more permanent things, like deleting the panel data. Although we don't actually handle that in the HomeConfig, we could let add-ons register an uninstall handler for the panel that takes care of this.

> > We'll also need to add observers for uninstall/disable for add-ons.
> 
> You mean observers to let the add-on know when one of its panels has been
> disabled/removed from about:home? What use case you have in mind here?
> Clearing the panel's data?

I don't think add-ons need to know about whether or not the panel has been disabled, since right now JS has no concept of enabled/disabled panels, but I think it would be nice to let an add-on know whether its panel has been installed or uninstalled. This would also allow us to encourage things like only storing data when your panel is installed.

Comment 3

[Chenxia Liu [:liuche]]

Conclusion: let's just use "Remove" for dynamic panels.

Morphing this bug to handle adding a hook for letting add-ons know when the panel has been removed.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #3)
> Conclusion: let's just use "Remove" for dynamic panels.
> 
> Morphing this bug to handle adding a hook for letting add-ons know when the
> panel has been removed.

Just wondering: if the goal with this new hook is just to allow add-ons to clear datasets, maybe the Home.panels framework could just do it automatically under the hood i.e. no need to let the add-on know about it. What would be the pitfalls from clearing datasets automatically?

I'm just want to make sure we're not adding unnecessary add-on API here.

Comment 5

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> (In reply to Chenxia Liu [:liuche] from comment #3)
> > Conclusion: let's just use "Remove" for dynamic panels.
> > 
> > Morphing this bug to handle adding a hook for letting add-ons know when the
> > panel has been removed.
> 
> Just wondering: if the goal with this new hook is just to allow add-ons to
> clear datasets, maybe the Home.panels framework could just do it
> automatically under the hood i.e. no need to let the add-on know about it.
> What would be the pitfalls from clearing datasets automatically?
> 
> I'm just want to make sure we're not adding unnecessary add-on API here.

Good question... we should also think about other things add-ons might want to do when their panel is uninstalled, perhaps clear autentication like liuche suggested.

However, this raises an even bigger question. Right now we do allow add-ons to save data while their panel isn't installed (the data APIs are totally separate from the panel APIs), so should we also do something to prevent that? Or to let an add-on know when their panel is installed so that they can only start syncing the data then? Also, would we clear any registered sync listeners for that dataset?

My one concern with automatically clearing the data for add-ons is that right now it's totally the add-on's responsibility to manage the data, and this would change that, but it might not be clear what's going on. For example, I believe most of our demo add-ons just save data as soon as they're installed, and then maybe periodically after that. But if we wiped that data when a user removed a panel, what would happen when the user adds it back? The add-on would need some notification to know it should go save that data again.

Comment 6

[Lucas Rocha (:lucasr)]

To whoever working on this bug: you'll have to update HomeConfig.Editor to disallow dynamic panels from being disabled.

Comment 7

[:Margaret Leibovic]

I think we should expand this bug to also add a hook for when a panel is installed. Without this hook, add-ons won't be able to only update data when a panel is installed.

Comment 8

[:Margaret Leibovic]

The proposal here kinda conflicts with the proposal in bug 974035. I'll own these two bugs to figure out what we should do.

Comment 9

[Chenxia Liu [:liuche]]

I also like the idea of letting add-ons request sync (as described in bug 974035) instead of exposing the install API. I suppose an add-on might conceivably want to do something on install (display some one-time UI?) but this is something that we could potentially add later on.

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #9)
> I also like the idea of letting add-ons request sync (as described in bug
> 974035) instead of exposing the install API. I suppose an add-on might
> conceivably want to do something on install (display some one-time UI?) but
> this is something that we could potentially add later on.

This could easily be implemented in the add-on's install hook.

Comment 11

[:Margaret Leibovic]

I just realized we may want to increase the priority of this bug, since currently my RSS add-on will continue to register any panel that was added for all of time, even if it was removed in settings, since it doesn't know that the panel was removed. It will also continue to update the feed data, which isn't good.

Comment 12

[:Margaret Leibovic]

Attachment: (WIP) Add hooks to let an add-on know when a panel is installed/uninstalled

This is a first attempt at a simple approach. One possible concern here is that these onInstall/onUninstall callbacks will be called even if the add-on itself is the one calling install/uninstall, but I think that's fine (since the add-on knows what it's doing). I like that this is simple.

What do you two think?

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8398200 [details] [diff] [review]
(WIP) Add hooks to let an add-on know when a panel is installed/uninstalled

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

Just wondering: are you adding the 'installed' hook just for the sake of symmetry with 'uninstalled'? Do you have a use case for it?

::: mobile/android/modules/Home.jsm
@@ +161,5 @@
>  // private members without leaking it outside Home.jsm.
>  let handlePanelsGet;
>  let handlePanelsAuthenticate;
> +let handlePanelsInstalled;
> +let handlePanelsUninstalled;

This list is getting a bit long. Maybe this should become an object with the event names as keys? Something like:

let messageHandlers = {};

Then in Home.panels, you'd do:

messageHandlers["HomePanels:Installed"] = function() {
    ...
}

And in the observe function:

observe: function(subject, topic, data) {
    if (topic in messageHandlers) {
        messageHandler[topic]();
    }
}

Comment 15

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #14)
> Comment on attachment 8398200 [details] [diff] [review]
> (WIP) Add hooks to let an add-on know when a panel is installed/uninstalled
> 
> Review of attachment 8398200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just wondering: are you adding the 'installed' hook just for the sake of
> symmetry with 'uninstalled'? Do you have a use case for it?

The use case would be an add-on that only registers a panel. When the panel is installed, then the add-on would go fetch data for it (or, if the user uninstalled then re-installed the panel).

> ::: mobile/android/modules/Home.jsm
> @@ +161,5 @@
> >  // private members without leaking it outside Home.jsm.
> >  let handlePanelsGet;
> >  let handlePanelsAuthenticate;
> > +let handlePanelsInstalled;
> > +let handlePanelsUninstalled;
> 
> This list is getting a bit long. Maybe this should become an object with the
> event names as keys? Something like:
> 
> let messageHandlers = {};
> 
> Then in Home.panels, you'd do:
> 
> messageHandlers["HomePanels:Installed"] = function() {
>     ...
> }
> 
> And in the observe function:
> 
> observe: function(subject, topic, data) {
>     if (topic in messageHandlers) {
>         messageHandler[topic]();
>     }
> }

Good suggestion. I feel like this HomePanels code is getting a bit messy, so this would be a good way to fight that.

Comment 16

[:Margaret Leibovic]

Attachment: Add hooks to let an add-on know when a panel is installed/uninstalled

Named the message handler object to HomePanelsMessageHandlers to make it clear that it only contains HomePanels stuff.

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8398832 [details] [diff] [review]
Add hooks to let an add-on know when a panel is installed/uninstalled

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

::: mobile/android/base/home/HomeConfig.java
@@ +1101,5 @@
>  
>                  installed = true;
>              }
>  
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomePanels:Installed", panelConfig.getId()));

Strictly speaking, it's not safe to assume that an uninstall() call in HomeConfig.Editor will necessarily result in the panel being uninstalled. We should only send the message when the installation/uninstallation becomes effective i.e. once the Editor gets committed/applied.

Maybe Editor should have queue of pending messages that gets send once the changes are actually saved?

::: mobile/android/chrome/content/browser.js
@@ +130,5 @@
>  });
>  
>  // Lazily-loaded JS modules that use observer notifications
>  [
> +  ["Home", ["HomePanels:Get", "HomePanels:Authenticate", "HomePanels:Installed", "HomePanels:Uninstalled"], "resource://gre/modules/Home.jsm"],

nit: maybe indent this list so that it's easier to read?

::: mobile/android/modules/Home.jsm
@@ +203,5 @@
> +    },
> +
> +    "HomePanels:Installed": function handlePanelsInstalled(id) {
> +      let options = _registeredPanels[id]();
> +      if (!options.onInstall || typeof options.onInstall !== "function") {

Log an error if onInstall is not a function?

@@ +211,5 @@
> +    },
> +
> +    "HomePanels:Uninstalled": function handlePanelsUninstalled(id) {
> +      let options = _registeredPanels[id]();
> +      if (!options.onUninstall || typeof options.onUninstall !== "function") {

Ditto for onUninstall.

Comment 18

[Chenxia Liu [:liuche]]

Comment on attachment 8398832 [details] [diff] [review]
Add hooks to let an add-on know when a panel is installed/uninstalled

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

Looks good to me, but I think Lucas has a good point about the Editor install/uninstall not being a concurrent call.

::: mobile/android/modules/Home.jsm
@@ +406,5 @@
>  
>    // Lazy notification observer registered in browser.js
>    observe: function(subject, topic, data) {
> +    if (topic in HomePanelsMessageHandlers) {
> +      HomePanelsMessageHandlers[topic](data);

Log an error message if the topic isn't present?

Comment 19

[:Margaret Leibovic]

Attachment: (v2) Add hooks to let an add-on know when a panel is installed/uninstalled

Addressed comments.

Comment 20

[:Margaret Leibovic]

Comment on attachment 8399658 [details] [diff] [review]
(v2) Add hooks to let an add-on know when a panel is installed/uninstalled

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

This bit-rotted. I'll update and upload a new version.

::: mobile/android/modules/Home.jsm
@@ +203,5 @@
> +    },
> +
> +    "HomePanels:Installed": function handlePanelsInstalled(id) {
> +      let options = _registeredPanels[id]();
> +      if (!options.onInstall) {

I think these should change to oninstall/onuninstall, to be consistent with the event handler APIs we have in the Home.banner API.

Comment 21

[:Margaret Leibovic]

Attachment: (v3) Add hooks to let an add-on know when a panel is installed/uninstalled

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8400793 [details] [diff] [review]
(v3) Add hooks to let an add-on know when a panel is installed/uninstalled

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

Looks good!

One last thing, don't forget to remove disabling non-dynamic panels, from comment #6. In another patch, maybe?

::: mobile/android/base/home/HomeConfig.java
@@ +1256,5 @@
>              final State newConfigState =
>                      new State(mHomeConfig, makeOrderedCopy(true), isDefault());
>  
> +            // Copy the event queue to a new list, so that we only modify mEventQueue on
> +            // the original thread where it was created.

Nit: It might be clearer to say this is because we want to limit modification of mEventQueue to a single thread, but not a big deal because that's implied.

@@ +1257,5 @@
>                      new State(mHomeConfig, makeOrderedCopy(true), isDefault());
>  
> +            // Copy the event queue to a new list, so that we only modify mEventQueue on
> +            // the original thread where it was created.
> +            final LinkedList<GeckoEvent> eventQueueCopy = new LinkedList<GeckoEvent>(mEventQueue);

I'm not sure how worried we should be that this is as modifiable list. I think it should be fine since we're just handling this internally, right?

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 8400793 [details] [diff] [review]
(v3) Add hooks to let an add-on know when a panel is installed/uninstalled

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

Drive-by.

::: mobile/android/base/home/HomeConfig.java
@@ +1149,5 @@
>  
>                  installed = true;
> +
> +                // Add an event to the queue if a new panel is sucessfully installed.
> +                mEventQueue.add(GeckoEvent.createBroadcastEvent("HomePanels:Installed", panelConfig.getId()));

Strictly speaking, install/uninstall calls should replace each other to avoid sending unnecessary events to the add-on i.e. if you call install() after an uninstall(), the event queue should end up with only an 'install' event.

Not a big issue as I don't expect this case to happen very often in the current code. It would be nice if this code was a bit more future-proof though. Up to you.

Comment 24

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #23)
> Comment on attachment 8400793 [details] [diff] [review]
> (v3) Add hooks to let an add-on know when a panel is installed/uninstalled
> 
> Review of attachment 8400793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by.
> 
> ::: mobile/android/base/home/HomeConfig.java
> @@ +1149,5 @@
> >  
> >                  installed = true;
> > +
> > +                // Add an event to the queue if a new panel is sucessfully installed.
> > +                mEventQueue.add(GeckoEvent.createBroadcastEvent("HomePanels:Installed", panelConfig.getId()));
> 
> Strictly speaking, install/uninstall calls should replace each other to
> avoid sending unnecessary events to the add-on i.e. if you call install()
> after an uninstall(), the event queue should end up with only an 'install'
> event.
> 
> Not a big issue as I don't expect this case to happen very often in the
> current code. It would be nice if this code was a bit more future-proof
> though. Up to you.

Hm, that's a good point, but maybe something we can do in a follow-up, since I want to uplift this patch to 30 to use with my home feeds add-on.

Comment 25

[:Margaret Leibovic]

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

Comment 26

[:Margaret Leibovic]

Comment on attachment 8400793 [details] [diff] [review]
(v3) Add hooks to let an add-on know when a panel is installed/uninstalled

[Approval Request Comment]
Bug caused by (feature/regressing bug #): required for hub v1 (subscribe to feeds in panels on home page)
User impact if declined: an add-on can't know if a user removed a panel from their about:home, and therefore can't delete data when it is removed
Testing completed (on m-c, etc.): tested locally, just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, adds new add-on hook
String or IDL/UUID changes made by this patch: none

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f09c8c142d1

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Needs Aurora rebasing.

Comment 29

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ee4156b0686

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #28)
> Needs Aurora rebasing.

removing branch-patch-needed in that case

Comment 31

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.