458512 - Polish toEM() and also toOpenWindowByType() should support more parameters.

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Philip Chee]

While looking at Bug 456869 I filed Bug 458505 and then I came across Bug 407383 (Manage Add-ons in Preferences may open the add-on manager with an irrelevant pane selected).

We should probably take this polish as well in our toEM() function. However it's just a wrapper for toOpenWindowByType() which takes just two parameters:

function toOpenWindowByType( inType, uri )

The Firefox version is:

function toOpenWindowByType(inType, uri, features)

We probably would need something like:

function toOpenWindowByType(inType, uri, features, params)

*AND* return a handle to the window opened.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 toEM() and toOpenWindowByType()

Changes:

1. toEM() now supports the pane parameter like the Firefox equivalent BrowserOpenAddonsMgr(). It's still a wrapper around toOpenWindowByType() where all the work is actually done.

2. toOpenWindowByType()

2a. Supports a third parameter "features" for compatibility with Firefox.

2b. Added two more parameters: winArgs and callback.

2c. |winArgs| are the extra arguments to be passed to the openDialog() call. It can be a string or an array or strings.

2d. |callback| is a function that is called when an existing window is detected. A handle to the existing window is passed to |callback|. This allows toEM() to call the showView() function in the context of the Addons Manager window.

Comment 2

[[e-mail]]

Comment on attachment 356150 [details] [diff] [review]
Patch v1.0 toEM() and toOpenWindowByType()

So, I'm tempted to think that this patch is overkill, so here's my suggestion:

function toEM( pane )
{
  var win = toOpenWindowByType("Extension:Manager",
                               "chrome://mozapps/content/extensions/extensions.xul",
                               "all,dialog=no", pane);
  if (win) // found an existing window
    win.showView(pane);
}

function toOpenWindowByType( inType, uri, features, arg )
{
  // don't do several loads in parallel
  if (uri in window)
    return null;

  var topWindow = Components.classes['@mozilla.org/appshell/window-mediator;1']
                            .getService(nsIWindowMediator)
                            .getMostRecentWindow(inType);
  if (topWindow) {
    toOpenWindow(topWindow);
    return topWindow;
  }

  // open the requested window, but block it until it's fully loaded
  function newWindowLoaded(event)
  {
    // make sure that this handler is called only once
    window.removeEventListener("unload", newWindowLoaded, false);
    window[uri].removeEventListener("load", newWindowLoaded, false);
    delete window[uri];
  }

  // remember the newly loading window until it's fully loaded
  // or until the current window passes away
  window[uri] = window.openDialog(uri, "", features || "all,dialog=no", arg);
  window[uri].addEventListener("load", newWindowLoaded, false);
  window.addEventListener("unload", newWindowLoaded, false);
  return null;
}

Comment 3

[[e-mail]]

Comment on attachment 356150 [details] [diff] [review]
Patch v1.0 toEM() and toOpenWindowByType()

As discussed on IRC, both of our versions are wrong ;-)

Comment 4

[Philip Chee]

> As discussed on IRC, both of our versions are wrong ;-)

For the record in case someone reading this wonders why, Neils suggested code doesn't work well because the addons window doesn't like being passed a null in window.arguments[0] nor in showView().

Comment 5

[Philip Chee]

Attachment: Patch v1.1 Minimal Fix

As discussed over irc this version is far less ambitious.

1. toEM(aPane) is a direct port of the Firefox BrowserOpenAddonsMgr(aPane)

2. toOpenWindowByType() just adds support for window features as the third parameter for parity with the red panda.

Comment 6

[Caio Tiago Oliveira (asrail)]

(In reply to comment #5)
> Created an attachment (id=356329) [details]
> Patch v1.1 Minimal Fix
> 
> As discussed over irc this version is far less ambitious.
> 
> 1. toEM(aPane) is a direct port of the Firefox BrowserOpenAddonsMgr(aPane)

+  const EMTYPE = "Extension:Manager";

Why using a const only once?

Comment 7

[[e-mail]]

Comment on attachment 356329 [details] [diff] [review]
Patch v1.1 Minimal Fix

Only remove the constants if you feel like it.

Comment 8

[Philip Chee]

Attachment: Patch v1.2 [Checkin: Comment 9]

Patch for checkin. Carrying forward r+/sr+

> +  const EMTYPE = "Extension:Manager";
> Why using a const only once?

Too much cut and paste (from Firefox).

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 356370 [details] [diff] [review]
Patch v1.2
[Checkin: Comment 9]

http://hg.mozilla.org/comm-central/rev/d8b8a09219d2