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

RESOLVED FIXED in Firefox 30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 30
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Updated

5 years ago
Assignee: nobody → lucasr.at.mozilla
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

5 years ago
(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

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 968170
(Assignee)

Comment 5

5 years ago
Created attachment 8375130 [details] [diff] [review]
Introduce 'action' option in Home.panels.add() API (r=margaret)
(Assignee)

Comment 6

5 years ago
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?
Attachment #8375130 - Flags: review?(margaret.leibovic)

Comment 7

5 years ago
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.
Attachment #8375130 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 8

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/64b5428726cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30

Updated

5 years ago
Blocks: 972306
You need to log in before you can comment on or make changes to this bug.