Closed
Bug 349309
Opened 18 years ago
Closed 17 years ago
toolkit's extensions.js/OpenURL is app specific (GetMoreThemes/Extensions)
Categories
(Toolkit :: XUL Widgets, defect)
Toolkit
XUL Widgets
Tracking
()
RESOLVED
FIXED
People
(Reporter: standard8, Assigned: philip.chee)
References
Details
Attachments
(1 file, 12 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•18 years ago
|
||
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•18 years ago
|
||
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
Comment 3•18 years ago
|
||
As soon as bug 263433 is fixed this will be fixed. Bug 345001 is also of interest.
Depends on: 263433
Assignee | ||
Comment 4•18 years ago
|
||
This works for browser apps (minefield and suiterunner) and for non browser apps (thunderbird and sunbird).
Attachment #264859 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 5•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #264859 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 6•18 years ago
|
||
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.
Assignee | ||
Comment 7•18 years ago
|
||
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?
Assignee | ||
Comment 8•18 years ago
|
||
Same fix for the Suite version of nsBrowserContentHandler.js
Attachment #266613 -
Flags: review?(neil)
Assignee | ||
Updated•18 years ago
|
Attachment #266612 -
Flags: review? → review?(benjamin)
Comment 9•18 years ago
|
||
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•18 years ago
|
||
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-
Comment 11•18 years ago
|
||
(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•18 years ago
|
||
Comment on attachment 266612 [details] [diff] [review] Patch v1.1 >+ uri.QueryInterface(nsIURL); Why?
Assignee | ||
Comment 13•18 years ago
|
||
(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)
Assignee | ||
Comment 14•18 years ago
|
||
(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)
Assignee | ||
Comment 15•18 years ago
|
||
Firefox version of the patch to nsBrowserContentHandler.js (using |nsIBrowserDOMWindow.OPEN_DEFAULTWINDOW|)
Comment 16•18 years ago
|
||
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+
Assignee | ||
Comment 17•18 years ago
|
||
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)
Comment 19•18 years ago
|
||
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 20•18 years ago
|
||
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 21•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
(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•18 years ago
|
||
(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.
Assignee | ||
Comment 25•18 years ago
|
||
(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)
Assignee | ||
Comment 26•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #266996 -
Attachment is obsolete: true
Attachment #266996 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 29•17 years ago
|
||
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)
Comment 30•17 years ago
|
||
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 32•17 years ago
|
||
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•17 years ago
|
||
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+
Assignee | ||
Comment 34•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
I think that in this code the loadgroup observer is kept alive by closures in the uriListener functions.
Updated•17 years ago
|
Attachment #270980 -
Flags: review?(cbiesinger) → review+
Comment 37•17 years ago
|
||
oh, actually: + var channel = ios.newChannelFromURI(uri); + if (channel) { no need for the if, newChannelFromURI throws if it fails
Assignee | ||
Comment 38•17 years ago
|
||
> oh, actually: > + var channel = ios.newChannelFromURI(uri); > + if (channel) { > no need for the if, newChannelFromURI throws if it fails fixed.
Assignee | ||
Comment 39•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Whiteboard: [checkin-needed]
Updated•17 years ago
|
Keywords: checkin-needed
Whiteboard: [checkin-needed]
Reporter | ||
Comment 40•17 years ago
|
||
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
Assignee | ||
Comment 41•17 years ago
|
||
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•17 years ago
|
||
Philip, Are you going to do any more work on this? Or can this be resolved as fixed now?
Assignee | ||
Comment 44•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite?
Comment 45•17 years ago
|
||
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.
Description
•