608791 - onBeforeLinkTraversal should compare host strings instead of top level domains

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Margaret Leibovic]

Attachment: patch

Using the nightly this weekend, I realized that matching top-level domains isn't a strict enough way of determining whether or not a link navigates within an app. For example, if you are on tbpl.mozilla.org, links to bugzilla.mozilla.org should open in a new tab, but they currently don't.

Looking back at bug 575561 comment 5 and comment 10, and talking to Paul, I think matching the host strings is a better heuristic than what we currently have implemented.

Comment 1

[Johnathan Nightingale [:johnath]]

Blocking since it meaningfully impacts the usefulness/surprisingness of app tabs

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 487375 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   onBeforeLinkTraversal: function(originalTarget, linkURI, linkNode, isAppTab) {

>+    let linkHost = linkURI.asciiHost;
>+    let docHost = linkNode.ownerDocument.documentURIObject.asciiHost;

Getting asciiHost can throw for non-nsStandardURL nsIURIs (e.g. data: URIs, which are implemented using nsSimpleURI), so you need to protect against that (and perhaps add a test for it). There's no real reason to use asciiHost rather than host, is there?

>diff --git a/browser/base/content/test/browser_bug575561.js b/browser/base/content/test/browser_bug575561.js

>+              // Tests link to http://www.example.com/browser/browser/base/content/test/dummy_page.html      
>+              testLink(4, true, false, finish);

www.example.com isn't actually one of the test hosts (not listed in server-locations.txt), so this will make this test depend on network connectivity, which isn't good. It doesn't look like there's any suitable existing "www." prefixed host, so I guess you might need to just add this one, I guess.

Comment 3

[:Margaret Leibovic]

Attachment: patch v2

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 488921 [details] [diff] [review]
patch v2

It might be nice to refactor the test such that it uses an array of objects that define the combinations to test (link target and result, similar to e.g. browser_keywordSearch.js), and have it iterate through that rather than having those chained calls to testLink (getting a bit out of control). Could also have testLink dynamically generate the link and get rid of app_bug575561.html so that it's easier to see what's being tested without having to look at two files and compare link indexes. Don't need to do that now though.

Comment 5

[:Margaret Leibovic]

I filed bug 615291 as a follow-up for refactoring the test.

Comment 6

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/50913ed5b777