Last Comment Bug 351235 - preference browser.link.restriction=1 is essentially useless
: preference browser.link.restriction=1 is essentially useless
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: 1.8 Branch
: x86 All
: -- minor with 1 vote (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-03 11:40 PDT by Benoît
Modified: 2011-09-25 16:17 PDT (History)
13 users (show)
csthomas: blocking‑seamonkey1.1b-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Possible patch (3.01 KB, patch)
2010-06-18 05:57 PDT, neil@parkwaycc.co.uk
bzbarsky: review+
Details | Diff | Splinter Review

Description Benoît 2006-09-03 11:40:19 PDT
This setting is related to the preferences that deal with how to open a link that calls for a new window.

On the 1.8.0 branch, setting this preference
-to 0 diverts everything
-to 1 diverts everything but window.open (JavaScript)
-to 2 diverts everything but window.open with attributes like width and height

Now, on the 1.8.1 branch, setting this preference
-to 0 diverts everything
-to 1 diverts nothing
-to 2 diverts everything but window.open with attributes

So option 1 got changed, and is now basically useless.
Comment 1 neil@parkwaycc.co.uk 2006-09-03 12:39:26 PDT
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.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2006-09-04 23:13:03 PDT
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.
Comment 3 Benoît 2006-09-10 10:27:22 PDT
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.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2006-09-10 10:31:38 PDT
> Why's that even necessary?

You'll have to ask danm.  He wrote that code.

Comment 5 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-09-24 14:22:43 PDT
Is there UI for this pref?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2006-09-24 14:25:21 PDT
No.
Comment 7 Benoît 2006-12-08 10:05:34 PST
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.
Comment 8 Serge Gautherie (:sgautherie) 2008-06-10 10:00:14 PDT
Filter "spam" on "guifeatures-nobody-20080610".
Comment 9 neil@parkwaycc.co.uk 2010-06-18 05:57:24 PDT
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.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2010-06-18 11:04:40 PDT
What happened to comment 2?
Comment 11 neil@parkwaycc.co.uk 2010-06-18 12:19:26 PDT
(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".
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2010-06-18 12:43:10 PDT
> Paste where? 

In the original code that was checked in to handle the various window.open retargeting prefs by danm.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2010-06-18 12:44:07 PDT
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.
Comment 14 neil@parkwaycc.co.uk 2010-06-18 12:58:33 PDT
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).
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2010-06-18 13:03:43 PDT
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.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2010-07-02 16:43:37 PDT
How does this interact with showModalDialog (which passes false for aCalledFromJS)?
Comment 17 neil@parkwaycc.co.uk 2010-07-03 16:32:31 PDT
(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 18 Boris Zbarsky [:bz] (still a bit busy) 2010-07-04 17:44:03 PDT
Comment on attachment 452249 [details] [diff] [review]
Possible patch

Sounds good.  Thanks for the explanation!
Comment 19 neil@parkwaycc.co.uk 2010-07-05 05:04:22 PDT
Pushed changeset a9d27938bb2d to mozilla-central.
Comment 21 neil@parkwaycc.co.uk 2010-07-25 09:07:33 PDT
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.
Comment 22 Dão Gottwald [:dao] 2011-05-10 12:23:25 PDT
Could this have cause bug 651659?
Comment 23 neil@parkwaycc.co.uk 2011-05-10 16:55:34 PDT
It's only supposed to affect content windows opened using target attributes.
Comment 24 Dão Gottwald [:dao] 2011-05-11 01:06:38 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.