349309 - toolkit's extensions.js/OpenURL is app specific (GetMoreThemes/Extensions)

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Mark Banner (:standard8)]

In the OpenURL function of extensions.js, there is an app specific ifdef for MOZ_PHOENIX.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/content/extensions.js&rev=1.109&mark=428-444#425

This means that SeaMonkey as a toolkit application can't open URLs within itself, but relies on the external application handler.

I think this will also mean that the Get More ... link will be broken for Firefox when it becomes an xulrunner application.

Comment 1

[Mark Banner (:standard8)]

The openDialog call should also be changed to get the value of the pref browser.chromeURL rather than hard-coding "chrome://browser/content/browser.xul".

Comment 2

[Phil Ringnalda (:philor)]

Toolkit really needs a "These Components Are All Crap" component.

Comment 3

[Robert Strong (they/them - no direct email)]

As soon as bug 263433 is fixed this will be fixed. Bug 345001 is also of interest.

Comment 4

[Philip Chee]

Attachment: Patch v1.0

This works for browser apps (minefield and suiterunner) and for non browser apps (thunderbird and sunbird).

Comment 5

[Philip Chee]

Attachment: Patch (WIP)

WIP Patch. This works for SuiteRunner (though not optimally) but in Minefield I get a new browser window that tries to open |resource://gre/res/%5Bxpconnect%20wrapped%20nsIURI%5D| (which unescaped is resource://gre/res/[xpconnect wrapped nsIURI]) and naturally it can't and I get the usual error page.

Comment 6

[Philip Chee]

Neil tells me that this last error message is caused by a bug in Minefield's nsBrowserContentHandler.js actually and a new browser window is opened successfully if I locally fix nsBrowserContentHandler.js. Unfortunately it should open a new tab (current behaviour) not a new window.

Comment 7

[Philip Chee]

Attachment: Patch v1.1

1. Same as the WIP patch plus fix for handleContent in nsBrowserContentHandler.js so that:
1. It respects the "browser.link.open_newwindow" pref.
2. Hands off to handURIToExistingBrowser() instead of openWindow()

Works for/tested with:
Minefield: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070528 Minefield/3.0a5pre
Thunderbird: version 3.0a1pre (20070528)
SuiteRunner: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/2007052914 Mnenhy/0.7.5.0 SeaMonkey/2.0a1pre

Comment 8

[Philip Chee]

Attachment: Patch v1.2

Same fix for the Suite version of nsBrowserContentHandler.js

Comment 9

[Steffen Wilberg]

That's a lot of generic-looking code, yet it's in extensions.js.
It would be nice to have that in a generic location, so that it can be used by other toolkit components as well (bug 345001, bug 263433, bug 262808)?
Also see visitLink() in globalOverlay.js, which is for the about dialog:
http://mxr.mozilla.org/seamonkey/source/toolkit/content/globalOverlay.js#150

Comment 10

[[e-mail]]

Comment on attachment 266613 [details] [diff] [review]
Patch v1.2

>+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(nsIPrefBranch);
>+    var newWindowPref = prefs.getIntPref("browser.link.open_newwindow");
>+    var target = newWindowPref == 3 ?
>+      nsIBrowserDOMWindow.OPEN_NEWTAB : nsIBrowserDOMWindow.OPEN_NEWWINDOW;
You shouldn't need to massage this pref, it should already have the correct value, although IMHO you should never use this pref in this file.

>+    handURIToExistingBrowser(request.URI, target, "chrome,all,dialog=no");
Missing .spec ;-)

Comment 11

[[e-mail]]

(In reply to comment #10)
>>+    handURIToExistingBrowser(request.URI, target, "chrome,all,dialog=no");
>Missing .spec ;-)
Sorry, this isn't openWindow any more, is it... D'oh!

Comment 12

[[e-mail]]

Comment on attachment 266612 [details] [diff] [review]
Patch v1.1

>+    uri.QueryInterface(nsIURL);
Why?

Comment 13

[Philip Chee]

Attachment: Patch 2.0

(In reply to comment #12)
>>+    uri.QueryInterface(nsIURL);
>Why?
Sorry. Too much cut and paste. Fixed. I've also did some minor optimization by adding several constants to replace some repeated patterns.

Comment 14

[Philip Chee]

Attachment: Patch for SeaMonkey's nsBrowserContentHandler.js

(In reply to comment #10)
> You shouldn't need to massage this pref, it should already have the correct
> value, although IMHO you should never use this pref in this file.

Fixed, now using |nsIBrowserDOMWindow.OPEN_DEFAULTWINDOW| as recommended by Neil.

>>>+    handURIToExistingBrowser(request.URI, target, "chrome,all,dialog=no");
>> Missing .spec ;-)
> Sorry, this isn't openWindow any more, is it... D'oh!
:)

Comment 15

[Philip Chee]

Attachment: Patch for Firefox's nsBrowserContentHandler.js

Firefox version of the patch to nsBrowserContentHandler.js (using |nsIBrowserDOMWindow.OPEN_DEFAULTWINDOW|)

Comment 16

[[e-mail]]

Comment on attachment 266997 [details] [diff] [review]
Patch for SeaMonkey's nsBrowserContentHandler.js

While testing this I noticed that the handURIToExistingBrowser doesn't exclude popup windows :-\ (by comparison navigator windows only register content listeners if they were opened with a default set of chrome flags).

Comment 17

[Philip Chee]

Comment on attachment 266998 [details] [diff] [review]
Patch for Firefox's nsBrowserContentHandler.js

Sorry forgot to set the review flags.

Comment 19

[Benjamin Smedberg]

Comment on attachment 266998 [details] [diff] [review]
Patch for Firefox's nsBrowserContentHandler.js

Hrm, seems like it might be nice to pass the parent window in somehow (as a hint to getMostRecentBrowserWindow() perhaps?), but this is better than what's there so let's go with it.

Comment 20

[Benjamin Smedberg]

Comment on attachment 266996 [details] [diff] [review]
Patch 2.0

I'd like biesi to look over the networking usage as well.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 266996 [details] [diff] [review]
Patch 2.0

+  var iOService = Components.classes["@mozilla.org/network/io-service;1"]

interesting spelling. I'd name this either ioService or ios.


I don't really understand why you need the lastWindowClosingSurvivalArea, and why you don't need it before onStartRequest, but I hope bsmedberg understood that...

+    loadgroup.groupObserver = loadListener;

nothing keeps the listener alive...

+      getInterface: function(iid) {
+        if (iid.equals(Components.interfaces.nsIURIContentListener) ||
+            iid.equals(Components.interfaces.nsIInterfaceRequestor) ||
+            iid.equals(nsISupports))

no reason to list nsISupports or nsIInterfaceRequestor here

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

I don't think you want to give out your own loadgroup... it means that you don't get a docloader, but I do think you want one.

How did you come up with the list of interfaces to handle in your getInterface implementation? In particular, why have a DOM window there?

Comment 23

[[e-mail]]

(In reply to comment #22)
>How did you come up with the list of interfaces to handle in your getInterface
>implementation? In particular, why have a DOM window there?
Because the window watcher can't create a content window.

Comment 24

[[e-mail]]

(In reply to comment #23)
>(In reply to comment #22)
>>How did you come up with the list of interfaces to handle in your getInterface
>>implementation? In particular, why have a DOM window there?
>Because the window watcher can't create a content window.
Although, the content handler change might make that unnecessary.

Comment 25

[Philip Chee]

Attachment: Patch 2.1 (fix nits)

(In reply to comment #21)

>+  var iOService = Components.classes["@mozilla.org/network/io-service;1"]
>interesting spelling. I'd name this either ioService or ios.

Fixed

>+      getInterface: function(iid) {
>+        if (iid.equals(Components.interfaces.nsIURIContentListener) ||
>+            iid.equals(Components.interfaces.nsIInterfaceRequestor) ||
>+            iid.equals(nsISupports))
>no reason to list nsISupports or nsIInterfaceRequestor here

Fixed

>I don't really understand why you need the lastWindowClosingSurvivalArea, and
>why you don't need it before onStartRequest,
...
>I don't think you want to give out your own loadgroup... it means that you
>don't get a docloader, but I do think you want one.

I don't understand either but bsmedberg thinks it's necessary:
https://bugzilla.mozilla.org/show_bug.cgi?id=281847#c27

>but I hope bsmedberg understood that...
Well he should, I stole this code verbatim from something he wrote.

>+    loadgroup.groupObserver = loadListener;
>nothing keeps the listener alive...
bsmedberg wrote it this way.

(In reply to comment #22)
>How did you come up with the list of interfaces to handle in your getInterface
>implementation? In particular, why have a DOM window there?

I started with a minimal implementation of the listener based on:
http://lxr.mozilla.org/seamonkey/source/mail/components/compose/content/MsgComposeCommands.js#2858
http://lxr.mozilla.org/seamonkey/source/mail/components/nsMailDefaultHandler.js#60

But I got an NS_NO_INTERFACE error for |Components.interfaces.nsILoadGroup| so I borrowed benjamin's implementation. After that I got an NS_NO_INTERFACE error for |Components.interfaces.nsIDOMWindow| so I fixed that as well - see also Bug 262886.

Comment 26

[Philip Chee]

Attachment: Patch 2.2 (un-bitrotted)

Changes:
1. un-bitrot patch
2. Remove nsIDOMWindow as I've fixed the other end (nsBrowserContentHandler.js) not to need this interface.

Comment 27

[Philip Chee]

Attachment: Unbitrotted Patch for SeaMonkey's nsBrowserContentHandler.js

Update bitrot

Comment 28

[Philip Chee]

Attachment: UnBitrotted Patch for Firefox's nsBrowserContentHandler.js

Update bitrot

Comment 29

[Philip Chee]

Comment on attachment 270980 [details] [diff] [review]
 Patch 2.2 (un-bitrotted)

I seem to have accidentally removed the review flags in the previous patch.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

bsmedberg: Since this code is copied from you, can you explain:

- why you don't have to keep the loadgroup observer alive
- why you have to call enterLastWindowClosingSurvivalArea at all (it's the only reason you have this loadgroup code, right?)

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

bsmedberg: please see the previous comment

Comment 32

[Benjamin Smedberg]

In the browsercontenthandler I had to call enter/exitLastWindowClosingSurvivalArea because that code can be called before there is any window open, and the browser will fail to start the eventloop unless I make those calls.

I'm not sure it's necessary in this case: we know we have the EM window open already, so the browser isn't going to shut down.

Comment 33

[Benjamin Smedberg]

Comment on attachment 270980 [details] [diff] [review]
 Patch 2.2 (un-bitrotted)

But I'm certainly ok with having it there.

Comment 34

[Philip Chee]

I did some tests.
1. No loadgroup. Result: NS_NOINTERFACE error shows up in the Error Console and the link is not opened.
2. loadgroup with on listener attached. Result: Get more extension and Visit extension homepage works normally with no errors in the error console.

(Comment #32)
> I'm not sure it's necessary in this case: we know we have the EM window open
> already, so the browser isn't going to shut down.

(Comment #9)
> It would be nice to have that in a generic location, so that it can be used by
> other toolkit components as well (bug 345001, bug 263433, bug 262808)?

But if we move openURL() to somewhere more global so that other toolkit components can use it then we don't know what other consumers will be calling it.

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

bsmedberg, could you also answer the other question? :) The loadgroup only keeps a weak reference to the observer, and I see nothing else that keeps it alive...

hm, that the link isn't opened w/o a loadgroup is really strange... (the NS_NOINTERFACE in the console is expected though. fix xpconnect.)

Comment 36

[Benjamin Smedberg]

I think that in this code the loadgroup observer is kept alive by closures in the uriListener functions.

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, actually:
+    var channel = ios.newChannelFromURI(uri);
+    if (channel) {

no need for the if, newChannelFromURI throws if it fails

Comment 38

[Philip Chee]

Attachment: Patch 2.3 with nits fixed.

> oh, actually:
> +    var channel = ios.newChannelFromURI(uri);
> +    if (channel) {

> no need for the if, newChannelFromURI throws if it fails

fixed.

Comment 39

[Philip Chee]

Attachment: All in One patch for checkin

This patch for checkin. Un-bitrotted once more (and finally it is to be hoped).

Comment 40

[Mark Banner (:standard8)]

I've just checked this in on Philip's behalf:

Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.128; previous revision: 1.127
done
Checking in suite/browser/nsBrowserContentHandler.js;
/cvsroot/mozilla/suite/browser/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/nsBrowserContentHandler.js;
/cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.42; previous revision: 1.41
done

Comment 41

[Philip Chee]

I saw a recent blog post in planet.mozilla.org that suggests that an "in-litmus?" flag would be more appropriate to this bug as someone has to eyeball that the proper URL is opened. Unless there is some programatic way of checking for this.

Comment 42

[Stefan [:stefanh]]

Philip,

Are you going to do any more work on this? Or can this be resolved as fixed now?

Comment 44

[Philip Chee]

Yeah this is fixed, except for some sort of unit test. What's the flag for requesting assistance from QA to write some sort of mochikit test?

Also follow-up bugs should be filed to move this functionality to a more global part of toolkit by someone who wants this.

Comment 45

[Worcester12345]

See also: Bug 262575 – "Visit Homepage" and "Get More Extensions/Themes" in Extension and Theme manager should respect tabbed browsing preferences