361708 - Make the way links from mailnews behave pref controllable

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Ian Neal]

At the moment whenever you click on a link in mailnews it always opens in the most recently used browser window (or a new one if there's not one already open).
I would like to change this behaviour so that it looks at a pref and then from that pref decides whether to use an existing window/tab, open a new window or open a new tab in an existing window.
The pref that has been suggested is used is "browser.link.open_external"

Comment 1

[Ian Neal]

Attachment: Patch to implement pref controlled mailnews links v0.1

This patch:
* Adds new helper function openNewTabWindowOrExistingWith which pulls together code from openNewWindowWith, openNewTabWith and openTopBrowserWith and removes some duplicate code within contentAreaUtils.js
* Alters functions openNewWindowWith and openNewTabWith to just call new helper function
* Replaces openTopBrowserWith with new function openFromExternalWith to call the new helper function with setting from pref

Comment 2

[Ian Neal]

Attachment: Slightly revised patch v0.1a

Changes since v0.1:
* Tweaked openNewTabWindowOrExistingWith to fix an issue with opening a new tab

Comment 3

[Karsten Düsterloh]

Comment on attachment 246526 [details] [diff] [review]
Slightly revised patch v0.1a

Just a few nits...

>+  openNewTabWindowOrExistingWith(2, url, sendReferrer, null);

Create and use constants for these type integers, probably for all four values already.

>+  var aWhere = 1;

Using an a... prefix for a local variable in Mozilla code is confusing at best. Don't do it.

>+  try {
>+    if (pref)
>+      aWhere = pref.getIntPref("browser.link.open_external");
>+  } catch (ex) {
>+    aWhere =1;

If the try clause excepts, aWhere isn't changed, so there's no need to set it again. And if you're using try-catch, you don't need the "if (pref)" check either...

>+function openNewTabWindowOrExistingWith(aType, url, sendReferrer, reverseBackgroundPref)

For all functions you touched:
Either prefix all parameter names or none; prevalent style in this file tends towards prefixed.

Comment 4

[Ian Neal]

Attachment: patch with constants v0.1b

Changes since v0.1a:
* Moved to using constants instead of numbers for the types
* Switched to using a prefixes for variables

Comment 5

[[e-mail]]

Comment on attachment 246688 [details] [diff] [review]
patch with constants v0.1b

>+function openNewWindowWith(aURL, aSendReferrer)
>+function openFromExternalWith(aURL, aSendReferrer)
Strange place to put this one splitting as it does NewWIndow and NewTab.
Also I'd name it openAsExternal and there's never a referrer.

>+  try {
>+    where = pref.getIntPref("browser.link.open_external");
>+  } catch (ex) {}
How will this fail?

>+function openNewTabWindowOrExistingWith(aType, aURL, aSendReferrer, aReverseBackgroundPref)
Unwieldy function name but openSomewhere is probably worse ;-)

>+  // if we're looking to open in a tab
>+  if (aType == kNewTab) {
>+    try {
>+      // if we're running in a browser window, this should work
>+      browser = getBrowser();
>+    } catch (ex if ex instanceof ReferenceError) {}
>+  }
I don't think we need this, as we get the existing object later on anyway.

>+  // Make sure we are allowed to open this url
>+  urlSecurityCheck(aURL, browserDocument);
Although you don't know it because the bug is security-restricted, urlSecurityCheck is actually broken so I think you might as well use document all the time and thus you can move the check up to the start of the function.

>+    var wintype = browserDocument.documentElement.getAttribute('windowtype');
>+    if (wintype == "navigator:browser")
>+      originCharset = window.content.document.characterSet;
This only makes sense for document, not browserDocument.

>+  // We want to open in a new window or no existing window can be found.
>+  if (aType == kNewWindow || !browser) {
Since you don't look for an existing window when you want to open in a new window the lack of an existing window suffices to require a new window.

>+    browser.loadURI(aURL);
This should use the referrer and possibly the external load flag.

Comment 6

[Ian Neal]

Attachment: Patch using getTopWin v0.1c

Changes since v0.1b:
* Renamed function openFromExternalWith to openAsExternal
* Moved function openAsExternal to before openNewWindowWith
* Removed try/catch from round pref retrieval 
* Moved urlSecurityCheck to beginning of function
* Tweaked function to use getTopWin like openOneBookmark function

Carrying forward r= and requesting sr

Comment 7

[[e-mail]]

Comment on attachment 247491 [details] [diff] [review]
Patch using getTopWin v0.1c

>+  openNewTabWindowOrExistingWith(pref.getIntPref("browser.link.open_external"),
>+                                 aURL, false, null);
Nit: aReverseBackgroundPref is boolean, so null is inappropriate.

>+  // get referrer, if as external should be null
>+  var referrer = aSendReferrer ? getReferrer(browserDocument) : null;
Sorry for not spotting this before, but there's a security-sensitive bug relating getReferrer that was fixed for Firefox by passing the referring document where applicable as the parameter, but that's probably beyond the scope of this bug. In the interim I'd therefore say that it would be better to use getReferrer(document) and eliminate browserDocument.

Which conveniently allows you to move var browser = getBrowser(); to after the window opening code ;-)

Comment 8

[Ian Neal]

Attachment: Patch without browserDocument v0.1d

Changes since v0.1c:
* Moved getReferrer to near beginning of function and changed to using document instead of browserDocument in it.
* Removed declaring/setting browserDocument as not used anymore.
* Changed from null to false for aReverseBackgroundPref

Carrying forward r= and requesting sr

Comment 9

[[e-mail]]

Comment on attachment 247569 [details] [diff] [review]
Patch without browserDocument v0.1d

>+  var browser;
>+  // Get the existing browser object
>+  if (browserWin)
>     browser = browserWin.getBrowser();
Opening a window doesn't need a browser, so do this after the new window check...

>+  // We want to open in a new window or no existing window can be found.
>+  if (!browser) {
...which should then change to if (!browserWin)

Comment 10

[Ian Neal]

Attachment: Patch with later browser setting v0.1e (Checked into trunk)

Changes since v0.1d:
* Moved setting of browser variable to after window opening.

Checking in (trunk)
mailnews/base/resources/content/mailWindow.js;
new revision: 1.110; previous revision: 1.109
suite/common/contentAreaUtils.js;
new revision: 1.141; previous revision: 1.140
done

Requesting approval for 1.1 branch

Comment 11

[Robert Kaiser]

Comment on attachment 247900 [details] [diff] [review]
Patch with later browser setting v0.1e (Checked into trunk)

sounds good to me, a=me for 1.1

Comment 12

[Ian Neal]

Attachment: Branch patch (Checked into 1.8 branch)

Checking in (1.8 branch)
mailnews/base/resources/content/mailWindow.js;
new revision: 1.100.2.5; previous revision: 1.100.2.4
xpfe/communicator/resources/content/contentAreaUtils.js;
new revision: 1.134.2.5; previous revision: 1.134.2.4
done

Comment 13

[[e-mail]]

Comment on attachment 247900 [details] [diff] [review]
Patch with later browser setting v0.1e (Checked into trunk)

>+    window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no",
>+                      aURL, charsetArg, referrer);
>+    return;
>+  }
> 
>-  var referrer = sendReferrer ? getReferrer(browserDocument) : null;
>+  // Get the existing browser object
>+  browser = browserWin.getBrowser();
> 
>-  // As in openNewWindowWith(), we want to pass the charset of the
>-  // current document over to a new tab.
>-  var wintype = browserDocument.documentElement.getAttribute('windowtype');
>-  var originCharset;
>-  if (wintype == "navigator:browser") {
>-    originCharset = window.content.document.characterSet;
>+  // Open link in an existing window.
>+  if (aType == kExistingWindow) {
>+    browser.loadURI(aURL);
>+    browserWin.content.focus();
>+    return;
>   }
> 
>   // open link in new tab
>   var loadInBackground = false;
>   if (pref) {
>     loadInBackground = pref.getBoolPref("browser.tabs.loadInBackground");
>-    if (reverseBackgroundPref)
>+    if (aReverseBackgroundPref)
>       loadInBackground = !loadInBackground;
>   }
>-  browser.addTab(url, referrer, originCharset, !loadInBackground);
>+  browser.addTab(aURL, referrer, originCharset, !loadInBackground);
> }
Is it me or do we forget to use the referrer in the existing window case?

Comment 14

[Ian Neal]

(In reply to comment #13)
> Is it me or do we forget to use the referrer in the existing window case?
> 
I would presume that would mean switching to using .loadURIWithFlags?