968188 - Pass a 'reason' to the Home.panels.add() and Home.panels.remove() APIs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

We need more context to decide how to handle these calls for each context. This is mostly to allow us to handle add-on updates properly. The autoInstall (see bug 964375) option is not enough to figure out the right thing to do.

Comment 1

[Lucas Rocha (:lucasr)]

Forgot to mention: By 'reason' I mean the ADDON_INSTALL, ADDON_UNINSTALL, etc bits provided by the API for bootstrapped add-ons: https://developer.mozilla.org/en-US/Add-ons/Bootstrapped_extensions#Bootstrap_entry_points

Comment 2

[Richard Newman [:rnewman]]

I also encourage you to use these as parts of UITelemetry events for the home panels stuff. (I assume you have a UITelem bug hanging around somewhere -- if not, please file one!)

Comment 3

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #2)
> I also encourage you to use these as parts of UITelemetry events for the
> home panels stuff. (I assume you have a UITelem bug hanging around somewhere
> -- if not, please file one!)

Nope, but filed bug 968308.

Comment 4

[:Margaret Leibovic]

I wonder if it would work to count on add-ons to only add the panel once, at install time, in the install method of the bootstrap API. I suppose the problem with this is that the set of panels we store in JS won't hold those panels after the app restarts, which is a problem if we depend on that.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Introduce 'action' option in Home.panels.add() API (r=margaret)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8375130 [details] [diff] [review]
Introduce 'action' option in Home.panels.add() API (r=margaret)

Ok, this can be a lot simpler than I expected. This patch replaces the autoInstall option with an 'action' option, which can be INSTALL (install this panel in the Hub immediately) or REFRESH (refresh this panel in Hub with these options, if present). Add-ons can use the reason given by the bootstrap framework to figure out what action to use. Here's what I have in my demo add-on:

function getActionForPanel(aReason) {
  switch(aReason) {
    case ADDON_INSTALL:
      return Home.panels.Action.INSTALL;

    case ADDON_UPGRADE:
    case ADDON_DOWNGRADE:
      return Home.panels.Action.REFRESH;

    default:
      return null;
  }
}

To properly implement panel upgrades, add-ons should only call Home.panels.remove() when it's either being uninstalled or disabled. Here's how my shutdown() method looks like:

function shutdown(aData, aReason) {
  if (aReason == ADDON_DISABLE || aReason == ADDON_UNINSTALL) {
    Home.panels.remove(PANEL_ID);
  }
}

With the 'action' option (associated with the 'reason' argument), we enable add-on developers to properly manage panels on all add-ons state changes: install, uninstall, enable, disable, upgrade, downgrade.

I'm happy enough with the 'action' option name but I'm open for better alternatives. Thoughts?

Comment 7

[:Margaret Leibovic]

Comment on attachment 8375130 [details] [diff] [review]
Introduce 'action' option in Home.panels.add() API (r=margaret)

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

Nice, I like this. I think it's a nice layer to avoid letting the panels API know about add-on details. The term "action" sounds good to me.

Be sure to update our API documentation here when this lands:
https://wiki.mozilla.org/Mobile/Projects/Third-party_service_integration_MVP#JS_APIs

::: mobile/android/modules/Home.jsm
@@ +162,5 @@
>      LIST: "list",
>      GRID: "grid"
>    }),
>  
> +  // Valid types of views for a dataset.

Oops, update this comment.

@@ +231,5 @@
>      }
>  
>      this._panels[panel.id] = panel;
>  
> +    if (action) {

We could save on indentation by just returning early if there's no action.

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #7)
> Comment on attachment 8375130 [details] [diff] [review]
> Introduce 'action' option in Home.panels.add() API (r=margaret)
> 
> Review of attachment 8375130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, I like this. I think it's a nice layer to avoid letting the panels API
> know about add-on details. The term "action" sounds good to me.
> 
> Be sure to update our API documentation here when this lands:
> https://wiki.mozilla.org/Mobile/Projects/Third-
> party_service_integration_MVP#JS_APIs

Done.

> ::: mobile/android/modules/Home.jsm
> @@ +162,5 @@
> >      LIST: "list",
> >      GRID: "grid"
> >    }),
> >  
> > +  // Valid types of views for a dataset.
> 
> Oops, update this comment.

Done.
 
> @@ +231,5 @@
> >      }
> >  
> >      this._panels[panel.id] = panel;
> >  
> > +    if (action) {
> 
> We could save on indentation by just returning early if there's no action.

I generally prefer to avoid early returns in the middle of methods as they are very easy to miss. I prefer to use early returns more as pre-condition checks for the method as a whole. There are exceptions, of course, but this doesn't seem to be the case here.

Comment 9

[Lucas Rocha (:lucasr)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64b5428726cd