348423 - Camino fails to honor the pref for redirecting window.open links

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Prior to 11 Aug, we redirected all window.open calls.  After 11 Aug, we don't redirect *any* window.open calls *even ones that don't specify any chrome restrictions or size*.

We *still* fail to follow the pref that exists to allow users to determine what they want to happen (aka the happy medium).

We should obey the pref and set it to 2 by default in all-camino.js

user_pref("browser.link.open_newwindow.restriction", 2);

0 = divert all window.open calls according to user's SWM prefs
1 = don't divert any window.open calls
2 = divert window.open according to user's SWM pref unless the window.open specifies size or features

Comment 1

[Mike Pinkerton (not reading bugmail)]

define redirect. 

Are you saying that we now open new windows all the time regardless of the SWM setting? I tested that and it works fine.

Comment 2

[Mike Pinkerton (not reading bugmail)]

i believe this is the relevant code to copy in Ff:

http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsContentTreeOwner.cpp#799

I wish i knew what it was supposed to do or i'd just put it in.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

s/redirect/divert/ = send window.open calls to a new tab (or same tab) when SWM is enabled

Gah, I had a bad testcase for plain window.open; vanilla ones *are* diverted.  Sorry.

So this is just about honoring the pref (and setting it to 2 by default when people enable SWM).

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: working SWM testcase

Comment 5

[froodian (Ian Leue)]

Attachment: Partial Fix

This is a partial fix.  It makes us respect values of 0 and 2 for the pref.  I'm not sure how we'd go about fixing this entirely, since we'll actually have to detect window.open calls (instead of just looking for changed chrome or specified size/position like we do now).  I'll let the community/hwaara decide if this is acceptable (possibly with a followup bug).

Comment 6

[Håkan Waara]

Comment on attachment 236104 [details] [diff] [review]
Partial Fix

I don't really know how any our swm code works; I think it's better if pink reviews the change directly.

Comment 7

[Mike Pinkerton (not reading bugmail)]

I'm not really sure we should do this. Setting the pref to anything other than 2 will most likely break web apps that assume this behavior. Sure, caveat emptor, but should we really give users enough rope to hang themselves if we can avoid it?

Comment 8

[Mike Pinkerton (not reading bugmail)]

oh, also, I don't see |respectWindowOpenCallsWithSizeAndPosition| declared in any header. This should generate a warning, no?

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Some users really want a true *single* window mode, where normal-clicking on any link works as $DEITY intended--opening that link in the same window.  Users who set the pref to 0 are also more likely to know/pick out links that will cause problems when the pref is set to 0 and know how to "fix"/prevent it.  

E.g., during the time from the point SWM landed until bug 338777 landed, 0 was our default behavior with SWM on, and I knew which sorts of links I could click on and which sorts I needed to disable SWM before clicking in order for things not to be broken.  It's fairly easy *for the sorts of people who would set this pref to 0* to figure out the sorts of links that are problematic, but supporting this pref gives them the flexibility to obtain the behavior they desire.  (It's a shame we didn't get all these pref behaviors for free...such is the life of embedding, I guess....)

Comment 10

[froodian (Ian Leue)]

Attachment: Patch

I agree with smokey - there is a group of users who'll want this.  This patch addresses the issue with |respectWindowOpenCallsWithSizeAndPosition| being improperly declared.

Comment 11

[Samuel Sidler (old account; do not CC)]

Since I'm one of the users that would use this regularly, I agree with Smokey and froodian as well. We should get this in.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 236516 [details] [diff] [review]
Patch

sr=pink

Comment 13

[froodian (Ian Leue)]

Checked in on trunk and 1.8branch