Closed Bug 349309 Opened 13 years ago Closed 12 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: standard8, Assigned: philip.chee)

References

Details

Attachments

(1 file, 12 obsolete files)

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.
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".
Toolkit really needs a "These Components Are All Crap" component.
Component: Extension/Theme Manager → XUL Widgets
Product: Firefox → Toolkit
QA Contact: extension.manager → xul.widgets
Version: unspecified → Trunk
As soon as bug 263433 is fixed this will be fixed. Bug 345001 is also of interest.
Depends on: 263433
Blocks: 315452
Attached patch Patch v1.0 (obsolete) — Splinter Review
This works for browser apps (minefield and suiterunner) and for non browser apps (thunderbird and sunbird).
Attachment #264859 - Flags: review?(robert.bugzilla)
Attached patch Patch (WIP) (obsolete) — Splinter Review
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.
Attachment #264859 - Flags: review?(robert.bugzilla)
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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
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
Assignee: nobody → philip.chee
Attachment #264859 - Attachment is obsolete: true
Attachment #266376 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #266612 - Flags: review?
Attached patch Patch v1.2 (obsolete) — Splinter Review
Same fix for the Suite version of nsBrowserContentHandler.js
Attachment #266613 - Flags: review?(neil)
Attachment #266612 - Flags: review? → review?(benjamin)
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 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 ;-)
Attachment #266613 - Flags: review?(neil) → review-
(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 on attachment 266612 [details] [diff] [review]
Patch v1.1

>+    uri.QueryInterface(nsIURL);
Why?
Attached patch Patch 2.0 (obsolete) — Splinter Review
(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.
Attachment #266612 - Attachment is obsolete: true
Attachment #266613 - Attachment is obsolete: true
Attachment #266996 - Flags: review?(benjamin)
Attachment #266612 - Flags: review?(benjamin)
(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!
:)
Attachment #266997 - Flags: review?(neil)
Firefox version of the patch to nsBrowserContentHandler.js (using |nsIBrowserDOMWindow.OPEN_DEFAULTWINDOW|)
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).
Attachment #266997 - Flags: review?(neil) → review+
Comment on attachment 266998 [details] [diff] [review]
Patch for Firefox's nsBrowserContentHandler.js

Sorry forgot to set the review flags.
Attachment #266998 - Flags: review?(benjamin)
Duplicate of this bug: 383251
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.
Attachment #266998 - Flags: review?(benjamin) → review+
Comment on attachment 266996 [details] [diff] [review]
Patch 2.0

I'd like biesi to look over the networking usage as well.
Attachment #266996 - Flags: review?(cbiesinger)
Attachment #266996 - Flags: review?(benjamin)
Attachment #266996 - Flags: review+
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
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?
Blocks: 182121
(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.
(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.
Attached patch Patch 2.1 (fix nits) (obsolete) — Splinter Review
(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.
Attachment #268045 - Flags: review?(cbiesinger)
Blocks: 384139
Attached patch Patch 2.2 (un-bitrotted) (obsolete) — Splinter Review
Changes:
1. un-bitrot patch
2. Remove nsIDOMWindow as I've fixed the other end (nsBrowserContentHandler.js) not to need this interface.
Attachment #268045 - Attachment is obsolete: true
Attachment #270980 - Flags: review?(cbiesinger)
Attachment #268045 - Flags: review?(cbiesinger)
Update bitrot
Attachment #266997 - Attachment is obsolete: true
Update bitrot
Attachment #266998 - Attachment is obsolete: true
Attachment #266996 - Attachment is obsolete: true
Attachment #266996 - Flags: review?(cbiesinger)
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.
Attachment #270980 - Flags: review?(benjamin)
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?)
bsmedberg: please see the previous comment
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 on attachment 270980 [details] [diff] [review]
 Patch 2.2 (un-bitrotted)

But I'm certainly ok with having it there.
Attachment #270980 - Flags: review?(benjamin) → review+
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.
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.)
I think that in this code the loadgroup observer is kept alive by closures in the uriListener functions.
Attachment #270980 - Flags: review?(cbiesinger) → review+
oh, actually:
+    var channel = ios.newChannelFromURI(uri);
+    if (channel) {

no need for the if, newChannelFromURI throws if it fails
Attached patch Patch 2.3 with nits fixed. (obsolete) — Splinter Review
> oh, actually:
> +    var channel = ios.newChannelFromURI(uri);
> +    if (channel) {

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

fixed.
This patch for checkin. Un-bitrotted once more (and finally it is to be hoped).
Attachment #270980 - Attachment is obsolete: true
Attachment #270981 - Attachment is obsolete: true
Attachment #270983 - Attachment is obsolete: true
Attachment #272603 - Attachment is obsolete: true
Whiteboard: [checkin-needed]
Keywords: checkin-needed
Whiteboard: [checkin-needed]
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
Keywords: checkin-needed
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.
Philip,

Are you going to do any more work on this? Or can this be resolved as fixed now?
Duplicate of this bug: 384828
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
No longer depends on: 263433
See also: Bug 262575 – "Visit Homepage" and "Get More Extensions/Themes" in Extension and Theme manager should respect tabbed browsing preferences
You need to log in before you can comment on or make changes to this bug.