351235 - preference browser.link.restriction=1 is essentially useless

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Benoît]

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

[[e-mail]]

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 [:bzbarsky]]

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]

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 [:bzbarsky]]

> Why's that even necessary?

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

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Is there UI for this pref?

Comment 6

[Boris Zbarsky [:bzbarsky]]

No.

Comment 7

[Benoît]

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)]

Filter "spam" on "guifeatures-nobody-20080610".

Comment 9

[[e-mail]]

Attachment: 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 [:bzbarsky]]

What happened to comment 2?

Comment 11

[[e-mail]]

(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 [:bzbarsky]]

> Paste where? 

In the original code that was checked in to handle the various window.open retargeting prefs by danm.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

[[e-mail]]

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 [:bzbarsky]]

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 [:bzbarsky]]

How does this interact with showModalDialog (which passes false for aCalledFromJS)?

Comment 17

[[e-mail]]

(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 [:bzbarsky]]

Comment on attachment 452249 [details] [diff] [review]
Possible patch

Sounds good.  Thanks for the explanation!

Comment 19

[[e-mail]]

Pushed changeset a9d27938bb2d to mozilla-central.

Comment 20

[[not reading bugmail]]

http://hg.mozilla.org/mozilla-central/rev/a9d27938bb2d
http://hg.mozilla.org/mozilla-central/rev/f469d7357b5b

Comment 21

[[e-mail]]

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]]

Could this have cause bug 651659?

Comment 23

[[e-mail]]

It's only supposed to affect content windows opened using target attributes.

Comment 24

[Dão Gottwald [:dao]]

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.