730127 - Create a standard way to ask for the addon manager to be opened

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect)

Description

[Geoff Lankow (:darktrojan)]

We're going to need this if we're to fix bug 715716. Notifying AddonManagerListeners seems to be the way to go, as we talked about on IRC last night. Any further thoughts, Blair?

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

CC'ing some people from bug 593687, as they've thought about this sort of thing more than I have.

The problem is that we need a way for the Add-ons Manager backend to open about:addons, and load a specific view. Geoff's idea was to have applications register as a listener (via AddonManager.addManagerListener) for a onRequestUIOpen event, and have the application open a tab (or window) and load about:addons. 

A similar approach would be to use something like AddonManager.setUIProvider(), which accepts a function.

Dave/Dao/Gavin: Thoughts?

Comment 2

[Dão Gottwald [:dao]]

Sounds good to me, if I understand the proposal correctly. Would BrowserOpenAddonsMgr also use this approach?

Comment 3

[Geoff Lankow (:darktrojan)]

(In reply to Dão Gottwald [:dao] from comment #2)
> Sounds good to me, if I understand the proposal correctly. Would
> BrowserOpenAddonsMgr also use this approach?

I was thinking more of the listener using BrowserOpenAddonsMgr, or whatever other function the app already uses to open it.

Comment 4

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #1)
> CC'ing some people from bug 593687, as they've thought about this sort of
> thing more than I have.
> 
> The problem is that we need a way for the Add-ons Manager backend to open
> about:addons, and load a specific view. Geoff's idea was to have
> applications register as a listener (via AddonManager.addManagerListener)
> for a onRequestUIOpen event, and have the application open a tab (or window)
> and load about:addons. 

This has the problem that if multiple listeners register we don't know which one should be used to open the window.

> A similar approach would be to use something like
> AddonManager.setUIProvider(), which accepts a function.

This is more of a single UI provider which I think works best. Bonus points for a default one that opens the UI in a full window. Bonus bonus points for making the test harness code we have use that instead of the hacky mechanism it does right now!

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

openOptionsInTab() (used for OPTIONS_TYPE_TAB) could also be switched to use this too.

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: WIP

Imagine this but with better names for stuff, better error correction and more using AddonsManagerInternal like everything else does.

(accidentally also implemented bug 715716)

Comment 7

[Geoff Lankow (:darktrojan)]

I meant to make it fall back to just opening a window if opening a tab can't be done.

I also meant error handling not error correction. I'm not going to correct other people's errors.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 601238 [details] [diff] [review]
WIP

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

This looks along the right lines to me. I'd expect getMostRecentWindow to actually return the most recent about:addons window or null if there isn't one open, though we already have a way to find that through the observer service so do we really need the app to expose that? I guess that means the app can still totally replace extensions.xul with its own copy

Comment 9

[Geoff Lankow (:darktrojan)]

(In reply to Dave Townsend (:Mossop) from comment #8)
> I'd expect getMostRecentWindow to
> actually return the most recent about:addons window or null if there isn't
> one open

I don't follow... it's there to find the most appropriate window to open a tab in or to be the parent of a potentially modal dialog. The name probably doesn't help though.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 601238 [details] [diff] [review]
WIP

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

::: browser/base/content/browser.js
@@ +9036,5 @@
> +  getMostRecentWindow: function() {
> +    return Services.wm.getMostRecentWindow("navigator:browser");
> +  },
> +  onRequestUIOpen: function(aView) {
> +    this.getMostRecentWindow().BrowserOpenAddonsMgr(aView);

I don't suppose you also want to exorcize the ping/pong notifications from BrowserOpenAddonsMgr too? :)
In a separate bug, of course - not here. That was only ever meant to be a temporary thing - we really need something less hacky and that can be used by all in-content UI.

@@ +9038,5 @@
> +  },
> +  onRequestUIOpen: function(aView) {
> +    this.getMostRecentWindow().BrowserOpenAddonsMgr(aView);
> +  },
> +  onRequestTabOpen: function(aURI) {

"Tab" won't make sense to all applications, even if that's what the options type is called. 

I wonder if it should be framed as openPrimaryUI vs openSecondaryUI - that makes it generic to both applications and components. Going forward, we'll need something like this for a bunch of things. Which, of course, implies that openPrimaryUI's first argument would be "about:addons", and for now we just special case that.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +1911,5 @@
> +  foo: function(aView) {
> +    AddonThing.onRequestUIOpen(aView);
> +  },
> +
> +  bar: function(aId) {

I wonder if this should accept a callback, that's passed the resulting window/document.

@@ +1937,5 @@
> +        features += instantApply ? ",dialog=no" : ",modal";
> +      } catch (e) {
> +        features += ",modal";
> +      }
> +      AddonThing.getMostRecentWindow().openDialog(optionsURL, features);

Would rather have this just call AddonThing.openDialog() - where that function is the same API as normal openDialog(), but handles figuring out the dialog parent itself.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Geoff Lankow (:darktrojan) from comment #9)
> (In reply to Dave Townsend (:Mossop) from comment #8)
> > I'd expect getMostRecentWindow to
> > actually return the most recent about:addons window or null if there isn't
> > one open
> 
> I don't follow... it's there to find the most appropriate window to open a
> tab in or to be the parent of a potentially modal dialog. The name probably
> doesn't help though.

I mean I'd expect AddonThing.getMostRecentWindow to return an about:addons window (you're current implementation doesn't). It doesn't make much sense otherwise. That said I think Blair's idea is better and you shouldn't need this function at all.

Also you should implement AddonThing in browserglue or somewhere that is a singleton.

(In reply to Blair McBride (:Unfocused) from comment #10)
> Comment on attachment 601238 [details] [diff] [review]
> WIP
> 
> Review of attachment 601238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +9036,5 @@
> > +  getMostRecentWindow: function() {
> > +    return Services.wm.getMostRecentWindow("navigator:browser");
> > +  },
> > +  onRequestUIOpen: function(aView) {
> > +    this.getMostRecentWindow().BrowserOpenAddonsMgr(aView);
> 
> I don't suppose you also want to exorcize the ping/pong notifications from
> BrowserOpenAddonsMgr too? :)
> In a separate bug, of course - not here. That was only ever meant to be a
> temporary thing - we really need something less hacky and that can be used
> by all in-content UI.
> 
> @@ +9038,5 @@
> > +  },
> > +  onRequestUIOpen: function(aView) {
> > +    this.getMostRecentWindow().BrowserOpenAddonsMgr(aView);
> > +  },
> > +  onRequestTabOpen: function(aURI) {
> 
> "Tab" won't make sense to all applications, even if that's what the options
> type is called. 
> 
> I wonder if it should be framed as openPrimaryUI vs openSecondaryUI - that
> makes it generic to both applications and components. Going forward, we'll
> need something like this for a bunch of things. Which, of course, implies
> that openPrimaryUI's first argument would be "about:addons", and for now we
> just special case that.

Thinking over this last night I wonder if it would be better to just have the following functions:

openAddonsManager(aView)

Opens the add-ons manager to a particular view. Also used for opening inline options.

openAddonOptions(aURL, aWhere)

Opens an add-ons options, aWhere specifies whether to open in a new tab or window (some apps can just always open in a new window).

Comment 12

[Geoff Lankow (:darktrojan)]

(In reply to Dave Townsend (:Mossop) from comment #11)
> Thinking over this last night I wonder if it would be better to just have
> the following functions:
> 
> openAddonsManager(aView)
> 
> Opens the add-ons manager to a particular view. Also used for opening inline
> options.
> 
> openAddonOptions(aURL, aWhere)
> 
> Opens an add-ons options, aWhere specifies whether to open in a new tab or
> window (some apps can just always open in a new window).

Sounds good.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 730127-1.diff

As discussed recently, I've starting from scratch instead of developing the previous attempt.

I've used the function name openTab because it's by far the most likely type of UI to be opened, and also we use it elsewhere such as OPTIONS_TYPE_TAB in AddonManager.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8362069 [details] [diff] [review]
730127-1.diff

>+++ b/browser/components/nsBrowserGlue.js

>+  openTab: function BG_openTab(aURI) {
>+    this.getMostRecentBrowserWindow().switchToTabHavingURI(aURI, true);
>+    return this.getMostRecentBrowserWindow().gBrowser.selectedBrowser.contentWindow;
>+  },

This API isn't e10s-friendly.

>+++ b/toolkit/components/glue/ToolkitGlue.jsm

This module name seems pretty meaningless. Apparently you got inspired by BrowserGlue, but at least that contains some fundamental browser guts (i.e. code that's initiated early in the process and isn't browser-window specific), so the name kind of makes sense there.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8362069 [details] [diff] [review]
730127-1.diff

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

What Dao said.

AppIntegration.jsm or something? And it should just go in toolkit/modules. 

And we definitely need to figure out the e10s issue - everything these days needs to consider it.

Comment 16

[Andrew Swan [:aswan]]

This isn't relevant for WebExtensions.

Comment 17

[Carlos [nohamelin]]

This is it, see bug 1420406