User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041001 Firefox/0.10 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041001 Firefox/0.10 Theme manager and Extension manager open a new window for "Visit Homepage" even when "Force links that open new windows" is selected and set to "a new tab" in Tools -> Options -> Advanced Options -> Tabbed Browsing. Reproducible: Always Steps to Reproduce: 1. Go to Tools -> Options -> Advanced Options -> Tabbed Browsing 2. Select "Force links that open new windows" and set to "a new tab" 3. Go to Extensions manager, click an extension and click Visit Homepage. Actual Results: A new window with the extension's homepage is loaded Expected Results: A new tab in current browser with the Extensions homepage.

Forgot to mention: "Get more themes" and "Get more extensions" links in the theme/extension managers also don't use the Tabbed Browsing preferences.

This patch looks pref "browser.link.open_newwindow.ui", not "browser.link.open_newwindow". Firefox does not have UI for this pref.

(In reply to comment #2) > This patch looks pref "browser.link.open_newwindow.ui", not > "browser.link.open_newwindow". Firefox does not have UI for this pref. browser.link.open_newwindow is the correct pref; the other is a companion pref used to work around quirks in the way the UI configuration code was written. (At least, that's how I understood the attachments in the bug)

Will this patch also fix bug 262685? That might even be a dupe?

*** Bug 262685 has been marked as a duplicate of this bug. ***

I reported some time ago this one : https://bugzilla.mozilla.org/show_bug.cgi?id=251181 which max marked as WONTFIX...

(In reply to comment #6) > I reported some time ago this one : > https://bugzilla.mozilla.org/show_bug.cgi?id=251181 which max marked as WONTFIX... it is different. for bug#251181, we didn't have the tab browsing option so "get more extensions/themes" should open in a new window by default. but now we have already the tab browsing, and i think it is necessary to respect the preferences.

I also think the patch has to use the browser.link.open_newwindow Is more consistent and expected behavior to the Tabbed Browsing settings.

This patch does not care about 'link modifier' (ctrl+click to open in tab, shift+click to open in new window, etc), is it right?

Some general comments about the patch: has the if be on a single line, ie. "if(window.opener && window.opener.gBrowser)". Boolean short-circuit should take care of you. Follow the file syntax, i.e. if() { blah { code } } > case 3: // open in new tab > var tab = window.opener.gBrowser.addTab(aURL); > if(!pref.getBoolPref(PREF_BROWSER_LOADINBACKGROUND)) > window.opener.gBrowser.selectedTab = tab; > return; Shouldn't that be 'break;'? I've seen preferences being grabbed within a try{} catch(), but I'm not sure if that's needed or not.

(In reply to comment #10) > Some general comments about the patch: > has the if be on a single line, ie. "if(window.opener && > window.opener.gBrowser)". Boolean short-circuit should take care of you. Thank you. I'm confused about expression evaluating order. > > case 3: // open in new tab > > var tab = window.opener.gBrowser.addTab(aURL); > > if(!pref.getBoolPref(PREF_BROWSER_LOADINBACKGROUND)) > > window.opener.gBrowser.selectedTab = tab; > > return; > Shouldn't that be 'break;'? You say "'break;' after 'return;'"? ... oh, sorry. The previous 'case' is incorrect thing. I'll update it. > I've seen preferences being grabbed within a try{} catch(), but I'm not sure if > that's needed or not. In this case, try~catch clauses are not needed. An error will be thrown if there is no pref value, but default values are defined in all.js.

This patch still ignores link modifiers. Without it, this patch works well for me.

My opinion is that the 'link modifiers' can be ignored on UI links.

This patch uses whereToOpenLink() and openUILinkIn() function in utilityOverlay.js. When clicking mouse left button without modifiers, the link will open like target="_blank" (pref 'browser.link.open_newwindow'). Pressing middle button opens in new tab, and link modifiers work (e.g. shift+click to new window).

(In reply to comment #14) > Created an attachment (id=161465) > ugly hack > > This patch uses whereToOpenLink() and openUILinkIn() function in > utilityOverlay.js. Ugly hack indeed! I'm not sure if modifying gDoCommand() for the sake of a single command is necessarily a Good Thing. IMO openURL() would be better if it looked similar to this (taken from TBP): function TBP_openURL(aURL) { try { var targetPref = gPref.getIntPref("browser.link.open_newwindow"); } catch (e) {} if (aURL == null) aURL = "about:blank"; // check for an existing window and focus it; it's not application modal var anyWindow = WindowManager.getMostRecentWindow("navigator:browser"); if (anyWindow) { // get the charset of anyWindow, if it exists // XXX: is this even necessary? var charset; if (anyWindow.getBrowser() && anyWindow.getBrowser().characterSet) { charset = anyWindow.getBrowser().characterSet; } switch (targetPref) { case 2 : { anyWindow.openNewWindowWith(aURL, null, true, null); break; } case 3 : { var newTab = anyWindow.getBrowser().addTab(aURL, null, charset); anyWindow.getBrowser().selectedTab = newTab; break; } case 1 : { anyWindow.loadURI(aURL, null, originCharset); break; } } } else { // open browser.xul anyway anyWindow.openNewWindowWith(aURL, null, true, null); } return; } This way the user can use the existing tabbed UI prefs to choose how they want links from the EM and TM to be opened, without modifying any other part of extensions.xul or extensions.js.

(In reply to comment #15) > IMO openURL() would be better if it looked similar to this (taken from TBP): > (snip) > > This way the user can use the existing tabbed UI prefs to choose how they want > links from the EM and TM to be opened, without modifying any other part of > extensions.xul or extensions.js. Yes, the code you quoted is similar to attatchment 161384. By this code, you cannot use any link modifiers and/or middle mouse button. Are you sure?

This patch is based upon the function I just posted and teaches openURL() to honour the new pref, browser.link.open_newwindow. I have used similar code in TBP for some time. I also taught openURL() about browser.tabs.loadInBackground; however, ben has given review+ and approval-aviary+ to bug 262537 attachment 161279 [details] [diff] [review], which means that PREF_BROWSER_BACKGRND_OPEN may need to be changed to use browser.tabs.loadDivertedInBackground once the former patch is checked in, and if this patch is selected.

(In reply to comment #16) > Yes, the code you quoted is similar to attatchment 161384. > > By this code, you cannot use any link modifiers and/or middle mouse button. Are > you sure? I wish I had seen this before posting my patch... Anyway, to answer your question, do you think anyone is likely to mod+click a link in the Extensions or Theme Managers? Middle-click is a possibility, but mod+click sounds very unlikely to me - I have personally never done it, and no one has asked me to add it to TBP in the past.

Comment on attachment 161465 [details] [diff] [review] ugly hack (In reply to comment #18) Sorry, I misunderstood comment #13.

Comment on attachment 161467 [details] [diff] [review] teach openURL() about browser.link.open_newwindow Ben has checked in bug 262537 attachment 161279 [details] [diff] [review], so this patch is obsolete now. New patch coming up...

Same as before, only openURL() uses browser.tabs.loadDivertedInBackground for tab selection.

Please note that attachment 161476 [details] [diff] [review] is just for proof-of-concept; it was made against a compiled build and not against a source tree.

Comment on attachment 161476 [details] [diff] [review] teach openURL() about browser.link.open_newwindow (alternate) Obsoleted; real patch against latest Aviary snapshot coming up.

This is the final version of my idea, diff'ed with -urNp against the 0.10.1 extensions.js. The Mozilla download page makes references to daily snapshots but I was unable to find them.

Comment on attachment 162067 [details] [diff] [review] teach openURL() about browser.link.open_newwindow (final) Tentatively requesting review? from Ben. http://www.mozilla.org/owners.html is not communicative on the precise ownership of the EM/TM.

Comment on attachment 162067 [details] [diff] [review] teach openURL() about browser.link.open_newwindow (final) >+ var gPref = Components.classes[kPrefServiceID] >+ .getService(nsIPrefBranch); >+ var gMediator = Components.classes[kAppShellMediatorID] Never prefix locals with 'g'.

> This is the final version of my idea, diff'ed with -urNp against the 0.10.1 > extensions.js. The Mozilla download page makes references to daily snapshots > but I was unable to find them. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-0.9/ Even better: Get the source code and build yourself: http://www.mozilla.org/cvs.html http://www.mozilla.org/projects/firefox/build.html

Comment on attachment 162067 [details] [diff] [review] teach openURL() about browser.link.open_newwindow (final) Canceling review and obsoleting. New patch coming up.

Fixed variable naming nits, as well as a silly bug. Steffen: I don't have CVS access. My reference to snapshots was to _source_ snapshots.

Comment on attachment 162072 [details] [diff] [review] fixed patch Re-requesting review? from Ben.

> + // 1: open in currently selected tab, 2: open in new window, 3: open in new tab > + switch (targetPref) { > + case 1 : { > + anyWindow.loadURI(aURL, null, originCharset); > + break; > + } > + case 2 : { > + anyWindow.openDialog("chrome://browser/content/browser.xul", "_blank", "chrome,all,dialog=no", aURL, null, null); > + break; > + } > + case 3 : { > + var newTab = anyWindow.getBrowser().addTab(aURL, null, charset); > + if (!backgroundPref) anyWindow.getBrowser().selectedTab = newTab; > + break; > + } > + } I think 'default' is needed in this switch~case clause. See also: http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#3671

(In reply to comment #31) > I think 'default' is needed in this switch~case clause. > > See also: > http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#3671 Hmmmm. Are you saying that the case 2 block should also be able to be executed by default? Or just not bother to check for targetPref = 2?

amotohiko, is this what you had in mind?

(In reply to comment #33) > Created an attachment (id=162187) > patch incorporating amotohiko's suggestion > > amotohiko, is this what you had in mind? Sure. My opinion is the former one. The pref takes an integer value (because Mozilla does not support 3-state pref), so we need to think about fool proof...

*** Bug 266305 has been marked as a duplicate of this bug. ***

*** Bug 266467 has been marked as a duplicate of this bug. ***

*** Bug 266671 has been marked as a duplicate of this bug. ***

*** Bug 267380 has been marked as a duplicate of this bug. ***

*** Bug 268737 has been marked as a duplicate of this bug. ***

Comment on attachment 162072 [details] [diff] [review] fixed patch adding more app-specific stuff to toolkit isn't okay. We need a better solution for openURL than we have currently, hopefully that will feed into something we can control from /browser.

(In reply to comment #40) > (From update of attachment 162072 [details] [diff] [review] [edit]) > adding more app-specific stuff to toolkit isn't okay. We need a better > solution for openURL than we have currently, hopefully that will feed into > something we can control from /browser. What code should I look at to implement such a solution?

*** Bug 283835 has been marked as a duplicate of this bug. ***

This patch uses openURI method of browser window. This behaves as URI from 'external app', not 'internal link'. How about this one?

(In reply to comment #43) > Created an attachment (id=175711) [edit] > patch based on attachment 162072 [details] [diff] [review] [edit] > > This patch uses openURI method of browser window. > This behaves as URI from 'external app', not 'internal link'. > > How about this one? FWIW this looks much better, albeit slightly more complex.

Comment on attachment 177004 [details] [diff] [review] revised patch don't expect this to be reviewed without asking a particular browser/toolkit peer.

(In reply to comment #45) > Created an attachment (id=177004) [edit] > revised patch > I imagine there should be some error checking in the "if (anyWindow){...}" clause. Otherwise, there could be a silent failure to open the url (just an error message in the JS console). It should probably fall back to the old behavior in case of failures. However, I don't really know much about the code involved.

*** Bug 288703 has been marked as a duplicate of this bug. ***

Comment on attachment 177004 [details] [diff] [review] revised patch This doesn't hurt us in the app-specific ifdef world, its still something that needs to go, but in the meantime, lets at least do the right thing.

(In reply to comment #49) Thank you for reviewing. There are several related bugs. I know Bug 262808 (for Help window) and Bug 263433 (for Update dialog). Share this function, fixing these bugs will be easy. I think this 'openURL()' function should be moved to another file, like 'contentAreaUtils.js'. And renaming is needed to avoid confusion. There are functions named 'openNewTabWith' and 'openNewWindowWith'. I think 'openURIfromDialog' is good (not 'URL'). How about this?

Comment on attachment 177004 [details] [diff] [review] revised patch we're trying to wrap up alpha1. moving request out to alpha2.

(In reply to comment #51) > (From update of attachment 177004 [details] [diff] [review] [edit]) > we're trying to wrap up alpha1. moving request out to alpha2. > Should there be new milestone and/or blocking flags for this? Thanks.

This patch also fixes Bug 262808 and Bug 263433 (maybe. I cannot check update dialog's link). - move 'openURL' function to globalOverlay.js and rename to 'openFromDialog'. - remove 'visitLink' function since this is no longer used from anywhere. - add 'contentAreaClick' function to help.js (this is a simple version of browser.js's one) and call it from browser of help viewer. - remove XBL binding of 'link' element from updates.xml, and add event handlers to handle opening link to update.xul.

Looks like you need XPCNativeWrapper anymore :) http://lxr.mozilla.org/seamonkey/source/toolkit/content/XPCNativeWrapper.js#39 http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#4600

(In reply to comment #54) > Looks like you need XPCNativeWrapper anymore :) Thank you. I update patch and request for review. openFromDialog() function checks whether HTTP/HTTPS schemes is exposed or not. Non-browser apps (at least Thunderbird and Nvu) don't expose such schemes, I think this method can be used to detect app is browser or not. Is this correct? Or I should change same method as attachment 177004 [details] [diff] [review]?

Rerequest blocking since there is now a patch. Thanks.

Comment on attachment 177004 [details] [diff] [review] revised patch a=shaver.

Was it intentional that the patch that was approved is not the newest one?

im not sure. but in the id: 2005060106 build, it doesnt work. the patch was checked in at 5:06, so the fix should be in 6 oclock build. could be because of the xpcnativewrapper thing in comment 54.

Comment on attachment 184796 [details] [diff] [review] another patch, revised >+function openFromDialog(href) >+{ this isn't even the style of the function you removed. When in Rome generally applies, especially with widely-used conventions like this. >+ if (protocolSvc.isExposedProtocol(uri.scheme)) { >+ var mediator = Components.classes["@mozilla.org/appshell/window-mediator;1"] what if this is a mailto: link? this isn't quite right. >+ // locate the most recently used window >+ var anyWindow = mediator.getMostRecentWindow("navigator:browser"); navigator:browser may or may not be what we're looking for here. Yes this works for Firefox, but this breaks a mailto: link in tbird. And if this is a mostly-http function its poorly named, but we want a generic solution here. Copy/pasting darin's comments here for an idea of a potential solution. <darin> it seems to me that we need something like ExternalProtocolService, but a version that doesn't talk to the OS if the app can support the protocol <darin> and if the app can support the protocol it should treat it like a URL that entered via ShellExecute (i.e., on the command line) <darin> app.loadUrl(foo) >+ else { >+ // original behaviour; browser dependent. >+ openDialog("chrome://browser/content/browser.xul", "_blank", "chrome,all,dialog=no", href, null, null); >+ } >+ } so if they're _not_ using navigator:browser, we force them to use that URL for their browser chrome? not ok either. The extraneous extra brackets aren't needed. >+ else { >+ // non-browser >+ protocolSvc.loadUrl(uri); >+ } it'd be better to do if (!exposedprotocol) loadURI(uri) and return right up front. Note that this should be loadURI, and should pass in an nsIPrompt, per darin. >@@ -155,7 +156,9 @@ > <label id="found.app.features" hidden="true"/> > <vbox id="found.app.featuresList"/> > <separator class="thin"/> >- <link id="found.app.infoLink" label="&found.app.infoLink;"/> >+ <link id="found.app.infoLink" label="&found.app.infoLink;" >+ onclick="if (event.button == 0) openFromDialog(this.getAttribute('href'));" >+ onkeypress="if (event.keyCode == event.DOM_VK_ENTER || event.keyCode == event.DOM_VK_RETURN) openFromDialog(this.getAttribute('href'));"/> if you remove the link binding, doesn't this break?

*** Bug 297110 has been marked as a duplicate of this bug. ***

Thank you for reviewing. (In reply to comment #60) > (From update of attachment 184796 [details] [diff] [review] [edit]) > >+function openFromDialog(href) > >+{ > > this isn't even the style of the function you removed. When in Rome generally > applies, especially with widely-used conventions like this. Only 'visitURL' function is different from all of others. http://lxr.mozilla.org/mozilla/source/toolkit/content/globalOverlay.js Should I change the style? > <darin> it seems to me that we need something like ExternalProtocolService, but > a version that doesn't talk to the OS if the app can support the protocol > <darin> and if the app can support the protocol it should treat it like a URL > that entered via ShellExecute (i.e., on the command line) > <darin> app.loadUrl(foo) I agree with that. Add 'openURI' method to nsICommandLineHandler? Or create new interface to handle this? > >+ else { > >+ // non-browser > >+ protocolSvc.loadUrl(uri); > >+ } > > it'd be better to do if (!exposedprotocol) loadURI(uri) and return right up > front. Note that this should be loadURI, and should pass in an nsIPrompt, per > darin. OK. > >@@ -155,7 +156,9 @@ > > <label id="found.app.features" hidden="true"/> > > <vbox id="found.app.featuresList"/> > > <separator class="thin"/> > >- <link id="found.app.infoLink" label="&found.app.infoLink;"/> > >+ <link id="found.app.infoLink" label="&found.app.infoLink;" > >+ onclick="if (event.button == 0) openFromDialog(this.getAttribute('href'));" > >+ onkeypress="if (event.keyCode == event.DOM_VK_ENTER || event.keyCode == event.DOM_VK_RETURN) openFromDialog(this.getAttribute('href'));"/> > > if you remove the link binding, doesn't this break? I don't think so. There are no binding to link element in extensions.xml and link works. http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/content/extensions.xml

Sorry I never saw the reply here, I don't know if the right fix can be done in the right timeframe. Style should be left along unless you're touching the function, and if it doesn't break, ok. This isn't a blocker though, and maybe we should wait on 1.9 before we attempt the "right" fix here, this bug is not really that serious.

- In openFromDialog() function, add support for mailto: protocol for internal use. news: protocol seems to work, but always open new Thunderbird window. - Add patches for reporter. - Updated for new update service and help viewer. In openFromDialog() function, handlers for schemes are hard-coded. This is not good, and should be fixed by adding new generic protocol handler. Comments are welcomed. Thank you.

(In reply to comment #63) >maybe we should wait on 1.9 before we attempt the "right" fix here not that we've branched and the 1.9a1 trunk is open, are we going to try to attempt the "right" fix? just trying to feel things out.

*** Bug 306829 has been marked as a duplicate of this bug. ***

*** Bug 307190 has been marked as a duplicate of this bug. ***

*** Bug 309519 has been marked as a duplicate of this bug. ***

ive tried applying the patch on trunk and it seems to have bit rotted.

This is really annoying with all the new incompatibilities and updated extensions, especially since most don't update UMO any more, or at least right away. Is there and easier way (extension) to accomplish this? I use "Duplicate Tab" extension, which has a "merge windows" feature; so I just open a blank tab, then merge windows which at least grabs all of them and consolidates to one, with a second window remaining open, that being the actual extensions window (there might be another bug to open that in the regular windows as well). This is still too many steps to accomplish what the browser claims to be one of its advantages to real world users. Any other workarounds? Thanks.

(In reply to comment #69) > ive tried applying the patch on trunk and it seems to have bit rotted. Indeed. The lack of movement on this bug implies that the approval+ granted by shaver was not followed through on. Will this bug be fixed for 1.5 as part of the rc1-final cleanups or will it be deferred for a 1.5 maintenance release?

it's too late for this to make 1.5. Leaving the 2.0 nomination alone for the next train.

This is a nice-to-have, not a blocker.

(In reply to comment #73) > This is a nice-to-have, not a blocker. > We're talking version 2 here, right, which is quite a ways away, not version 1.5. Correct?

*** Bug 318258 has been marked as a duplicate of this bug. ***

This 'patch' contains patches for extension/theme manager, help viewer and text-link widget. The text-link widget is used in 'get more extensions/themes' link in EM, 'privacy policy' link in report wizard and 'details' link in software update wizard. This 'patch' isn't tested because I don't have build environment, so I don't ask review. Thank you.

since there is no c++ in the patch, there is no neeed to have a build environment. i just modified my firefox files directory, as i have on several other occassions. anyway, it could have been because i made a mistake applying the patch, but when i click "get more extensions/themes" or "visit homepage" nothing happens. but i don't get any errors. i have it show chrome errors in console and javascript in strict mode.

Comment on attachment 205297 [details] [diff] [review] another try (In reply to comment #77) > anyway, it could have been because i made a mistake applying > the patch, but when i click "get more extensions/themes" or "visit homepage" > nothing happens. but i don't get any errors. i have it show chrome errors in > console and javascript in strict mode. Actually. One semicolon is not needed. > since there is no c++ in the patch, there is no neeed to have a build > environment. Yes, I write the patch on this way. Hmm... I think review requestor should test their patches. I don't want to waste developer's time.

This is mostly same as attachment 205297 [details] [diff] [review]. Sorry for bug spams.

(In reply to comment #79) > Created an attachment (id=206172) [edit] Some details: * You can get chromeURL from the pref browser.chromeURL which exists for Firefox and SeaMonkey (when they migrate to Toolkit). * There's no need to get chromeURL if you managed to open the URI in an existing browser window. * Instead of using loadSubScript directly, first check whether openURI is already present and use that; if it's not, add an object as a second argument to loadSubScript and get openURI from that object: var context = {}; loader.loadSubScript("chrome://global/content/globalOverlay.js", context); context.openURI(href); That way, you won't unnecessary pollute the namespace. Finally I'd like to support the fact that you consider links from Chrome as external rather than internal links. Since some users might want to ignore target attributes in websites (and set browser.link.open_newwindow to 1) but still won't be aware that they just "lost" a website through clicking a Chrome link (which is why browser.link.open_external now defaults to 3 which IMHO makes sense, since there is no obvious "current" tab in this context). Even worse, you can force website links to another tab by choice while keyboard modifiers don't work on Chrome links.

Thank you for replying. > * You can get chromeURL from the pref browser.chromeURL which exists for > Firefox and SeaMonkey (when they migrate to Toolkit). And Thunderbird (for Mac OS X). http://lxr.mozilla.org/mozilla/source/mail/app/profile/all-thunderbird.js#43 I think I cannot trust this pref. > * There's no need to get chromeURL if you managed to open the URI in an > existing browser window. OK. > * Instead of using loadSubScript directly, first check whether openURI is > already present and use that; if it's not, add an object as a second argument > to loadSubScript and get openURI from that object: > > var context = {}; > loader.loadSubScript("chrome://global/content/globalOverlay.js", context); > context.openURI(href); > > That way, you won't unnecessary pollute the namespace. Thank you for your information. > Finally I'd like to support the fact that you consider links from Chrome as > external rather than internal links. Since some users might want to ignore > target attributes in websites (and set browser.link.open_newwindow to 1) but > still won't be aware that they just "lost" a website through clicking a Chrome > link (which is why browser.link.open_external now defaults to 3 which IMHO > makes sense, since there is no obvious "current" tab in this context). Even > worse, you can force website links to another tab by choice while keyboard > modifiers don't work on Chrome links. I think current code is wrong one when they open XUL content in a tab. In this case chrome links (xul:text-link widget) should open as the regular link.

*** Bug 340962 has been marked as a duplicate of this bug. ***

*** Bug 342269 has been marked as a duplicate of this bug. ***

*** Bug 343632 has been marked as a duplicate of this bug. ***

This is a quick, clean addons manager only fix.

*** Bug 344382 has been marked as a duplicate of this bug. ***

Same thing as before, except with all things in the extensions/content directory. I removed the link xbl binding from update.xml since nothing seems to use it AFAICT, so there is no need to fix that code.

Just an fyi: the extension update ui files (e.g. update.xml) are a complete mess and will be dealt with in Bug 342606. I should have time this weekend to review the remainder of the patch.

Comment on attachment 229211 [details] [diff] [review] Quick, complete fix for addons manager >Index: toolkit/mozapps/extensions/content/about.js ... >@@ -101,17 +101,17 @@ function init() ... >- extensionHomepage.setAttribute("href", homepage); >+ extensionHomepage.setAttribute('onclick', "loadHomepage(\"" + homepage + "\");"); nit: use double quotes around onclick. >Index: toolkit/mozapps/extensions/content/extensions.js ... >@@ -335,17 +335,17 @@ function showView(aView) { > getMore.setAttribute("tooltiptext", getMore.getAttribute(isThemes ? "tooltiptextthemes" : > "tooltiptextextensions")); > getMore.setAttribute("value", getMore.getAttribute(isThemes ? "valuethemes" : > "valueextensions")); > var getMoreURL = gPref.getComplexValue(isThemes ? PREF_EXTENSIONS_GETMORETHEMESURL > : PREF_EXTENSIONS_GETMOREEXTENSIONSURL, > Components.interfaces.nsIPrefLocalizedString).data; > getMoreURL = getMoreURL.replace(/%APPID%/g, gAppID); >- getMore.setAttribute('href', getMoreURL); >+ getMore.setAttribute('onclick', "openURL(\"" + getMoreURL + "\");"); nit: use double quotes around onclick. >@@ -429,17 +429,22 @@ function openURL(aURL) ... > # If we're a browser, open a new browser window instead. > #else >- openDialog("chrome://browser/content/browser.xul", "_blank", "chrome,all,dialog=no", aURL, null, null); >+ if (window.opener && window.opener.openUILinkIn) { >+ var newWindowPref = gPref.getIntPref("browser.link.open_newwindow"); >+ var where = newWindowPref == 3 ? "tab" : "window"; >+ window.opener.openUILinkIn(aURL, where); >+ } else >+ openDialog("chrome://browser/content/browser.xul", "_blank", "chrome,all,dialog=no", aURL, null, null); Add a comment that references this bug. >Index: toolkit/mozapps/extensions/content/list.js ... >@@ -164,17 +164,18 @@ function init() { > if ("moreInfoURL" in params && params["moreInfoURL"]) { > message = document.getElementById("moreInfo"); > message.value = extensionsBundle.getString("moreInfoText"); >- message.setAttribute("href", params["moreInfoURL"]); >+ message.setAttribute("onclick", "window.opener.openURL(\"" + >+ params["moreInfoURL"] + "\");"); This ui can be opened from the EM service without the EM ui opened when a new blocklisted item is detected. Since this ui will only be shown when an item is first blocklisted let's just leave this one as is and fix this after a generic solution is created. >Index: toolkit/mozapps/extensions/content/update.css >Index: toolkit/mozapps/extensions/content/update.xml Both of these will be taken care of in bug 342606 so just remove them from the patch. r=me with those changes

Removing blocks since none of them make any sense in the context of the Extension Manager... if anything this bug should have depended on bug 263433. The patch that will land after the tree opens is just wallpaper around the actual problem of not having a generic way for toolkit ui to support this for Firefox. Bug 263433 will be part of the real fix for this bug.

> Removing blocks since none of them make any sense in the context of the > Extension Manager... if anything this bug should have depended on bug 263433. The dependencies did make sense, see e.g. bug 262808 comment 4. > The patch that will land after the tree opens is just wallpaper around the > actual problem of not having a generic way for toolkit ui to support this for > Firefox. Attachment 206172 [details] [diff] in this bug was trying to fix this. Is there a new bug for fixing this for toolkit in general?

(In reply to comment #92) > > Removing blocks since none of them make any sense in the context of the > > Extension Manager... if anything this bug should have depended on bug 263433. > The dependencies did make sense, see e.g. bug 262808 comment 4. Right, this bug got morphed to also include help viewer which I would be fine with if this bug got traction and was fixed. To get this fixed for Firefox 2 I am un-morphing it. > > The patch that will land after the tree opens is just wallpaper around the > > actual problem of not having a generic way for toolkit ui to support this for > > Firefox. > Attachment 206172 [details] [diff] [edit] in this bug was trying to fix this. Is there a new bug for > fixing this for toolkit in general? bug 263433 covers text-link which this bug should have depended on and I don't believe there is a bug to cover the other cases.

Check in to trunk. bug 263433 covers a generic fix for label's with class="text-link" I don't believe a bug has been opened for supporting tabbed browsing preferences when opening urls from code... if one is filed please post the bug number in this bug. Please file new bugs if there is any fallout from this.

Version: unspecified. Any chance of branch?

(In reply to comment #95) > Version: unspecified. > > Any chance of branch? > If nothing breaks, it should make beta2.

Filed Bug 345001 for supporting respecting tabbed browser settings when opening web pages from code.

Comment on attachment 229496 [details] [diff] [review] Quick, complete fix for addons manager, v2 This patch is fairly safe and worthwhile for beta 2.

This patch looks like, at the very least, it needs careful security review. It's doing the equivalent of eval by concatenating strings to form the value of an onclick attribute. If it's possible for those strings to have untrusted input with quotes, parentheses, or semicolons in them, then this could be a security hole. Are the URLs all coming from trusted sources?

For the cmd_homepage case in the add-ons mgr context menu we get the value from the extensions rdf datasource and then make sure the scheme is http or https http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/content/extensions.js#1599 For the getMoreURL we are getting the url from getComplexValue from the locale's extensions.properties file. For the blocklist we are getting the url from the pref extensions.blocklist.detailsURL For the about dialog we get the value from the extensions rdf datasource. Lots of inconsistency... I'll submit an update to this patch that verifies these are all have a scheme of either http or https

The about dialog verifies the scheme is http or https already.

Comment on attachment 229496 [details] [diff] [review] Quick, complete fix for addons manager, v2 We're going to minus this for now. Feel free to renominate if you have more information about the security issues or a new patch.

On second thought, that patch was too ugly for me bear.

Ok?

Comment on attachment 230195 [details] [diff] [review] patch what's this: 1) -// manages the last-selected attribute for the view buttons and richlistbox +// manages the last-selected attribute for the view b7/21/2006uttons and richlistbox other than that, r=sspitzer

Checked in attachment #230195 [details] [diff] [review] to trunk. I'll submit a combined patch for 1.8.1

Carrying forward review and requesting 1.8.1

*** Bug 345713 has been marked as a duplicate of this bug. ***

Comment on attachment 230296 [details] [diff] [review] branch patch a=drivers. Please go ahead and land this on the branch.

Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0

So now Add-ons links are opened in new tab when browser.link.open_newwindow is set to 3. But I prefer browser.link.open_newwindow=1 so all window options are ignored, but then it makes Add-ons manager open links in new window.

This isn't by default or even by any easy choice yet, is it? I haven't run a trunk test build in a while. Thanks.