Closed Bug 361708 Opened 16 years ago Closed 16 years ago

Make the way links from mailnews behave pref controllable

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: iannbugzilla, Assigned: iannbugzilla)

Details

(Keywords: fixed-seamonkey1.1)

Attachments

(2 files, 5 obsolete files)

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"
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
Assignee: mail → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #246446 - Flags: review?(mnyromyr)
Attachment #246446 - Flags: review?(mnyromyr)
Attached patch Slightly revised patch v0.1a (obsolete) — Splinter Review
Changes since v0.1:
* Tweaked openNewTabWindowOrExistingWith to fix an issue with opening a new tab
Attachment #246446 - Attachment is obsolete: true
Attachment #246526 - Flags: review?(mnyromyr)
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.
Attachment #246526 - Flags: review?(mnyromyr) → review-
Attached patch patch with constants v0.1b (obsolete) — Splinter Review
Changes since v0.1a:
* Moved to using constants instead of numbers for the types
* Switched to using a prefixes for variables
Attachment #246526 - Attachment is obsolete: true
Attachment #246688 - Flags: review?(mnyromyr)
Attachment #246688 - Flags: review?(mnyromyr) → review+
Attachment #246688 - Flags: superreview?(neil)
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.
Attachment #246688 - Flags: superreview?(neil) → superreview-
Attached patch Patch using getTopWin v0.1c (obsolete) — Splinter Review
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
Attachment #246688 - Attachment is obsolete: true
Attachment #247491 - Flags: superreview?(neil)
Attachment #247491 - Flags: review+
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 ;-)
Attachment #247491 - Flags: superreview?(neil) → superreview-
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
Attachment #247491 - Attachment is obsolete: true
Attachment #247569 - Flags: superreview?(neil)
Attachment #247569 - Flags: review+
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)
Attachment #247569 - Flags: superreview?(neil) → superreview+
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
Attachment #247569 - Attachment is obsolete: true
Attachment #247900 - Flags: superreview+
Attachment #247900 - Flags: review+
Attachment #247900 - Flags: approval-seamonkey1.1?
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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
Attachment #247900 - Flags: approval-seamonkey1.1? → approval-seamonkey1.1+
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 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?
(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?
You need to log in before you can comment on or make changes to this bug.