To clarify, in 1.8.0 a restriction of 1 still diverted target="_blank" according to the open_newwindow preference and you used to have to set that preference to 2 to make everything open in new windows.
Option "1" is basically the "ignore 'browser.link.open_external'" option. That seems to have been the original intent; the fact that it didn't apply to <a target="_blank"> was a copy-paste error in the code as far as I could tell.
Why's that even necessary? To ignore browser.link.open_external is essentially the same as setting that option to the default, which is to open links in new windows.
> Why's that even necessary? You'll have to ask danm. He wrote that code.
Is there UI for this pref?
So, has a decision on this pref been reached yet? I talked to Neil about this a month ago, and he was going to talk to bz about it.
Filter "spam" on "guifeatures-nobody-20080610".
Created attachment 452249 [details] [diff] [review] Possible patch By passing aCalledFromJS down to nsContentTreeOwner we can make the restriction only apply for window.open as documented in firefox.js/browser-prefs.js, i.e. 0: target="target" and window.open(url, target, args) diverted 1: target="target" diverted 2: target="target" and window.open(url, target) diverted Setting browser.link.open_newwindow to 2 still works to disable all diverts.
What happened to comment 2?
(In reply to comment #2) > Option "1" is basically the "ignore 'browser.link.open_external'" option. That > seems to have been the original intent; the fact that it didn't apply to <a > target="_blank"> was a copy-paste error in the code as far as I could tell. Paste where? Even the comment for where the restriction pref is read refers specifically to window.open, I don't see any restriction for target="blank".
> Paste where? In the original code that was checked in to handle the various window.open retargeting prefs by danm.
That is, the comments were written post-facto to "explain" what the code was doing, but the original code was trying to have the codepaths be the same and just failed. At least if I remember things correctly; it's been a while since I did the archeology.
Well, attachment 159442 [details] [diff] [review] seems to be the original patch that just diverted <a href="..." target="...">; attachment 159443 [details] [diff] [review] looks like it was designed as a window.open follow-up, with the restriction pref added as an afterthought, so that people could choose not to divert window.open only (given that the second option of only diverting without features had not yet been thought of).
OK, good. I'm going to need a few days here to find time to sort through the callers and make sure the aCalledFromJS thing does what we want it to.
How does this interact with showModalDialog (which passes false for aCalledFromJS)?
(In reply to comment #16) > How does this interact with showModalDialog (which passes false for > aCalledFromJS)? showModalDialog never tries to Provide a window, because it's a dialog; not only are CHROME_MODAL and CHROME_DIALOG both set, but because showModalDialog always passes a real nsIVariant, even for null, then aDialog gets set too.
Comment on attachment 452249 [details] [diff] [review] Possible patch Sounds good. Thanks for the explanation!
Pushed changeset a9d27938bb2d to mozilla-central.
Comment on attachment 452249 [details] [diff] [review] Possible patch Oops. I forgot that this was a -w patch and forgot to recreate the original patch to get the indentation right.
Could this have cause bug 651659?
It's only supposed to affect content windows opened using target attributes.
I figured as much, but maybe it's not working as expected. It seems that there weren't a lot of changes in this area from 1.9.2 to 2.0, so I'm not sure what else could be the culprit.