Closed Bug 608791 Opened 14 years ago Closed 14 years ago

onBeforeLinkTraversal should compare host strings instead of top level domains


(Firefox :: General, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: Margaret, Assigned: Margaret)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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, links to 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.
Attachment #487375 - Flags: review?(
blocking2.0: --- → ?
Whiteboard: [needs review gavin]
Blocking since it meaningfully impacts the usefulness/surprisingness of app tabs
blocking2.0: ? → betaN+
Comment on attachment 487375 [details] [diff] [review]

>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      
>+              testLink(4, true, false, finish); 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.
Attachment #487375 - Flags: review?( → review-
Whiteboard: [needs review gavin] → [needs new patch]
Attached patch patch v2Splinter Review
Attachment #487375 - Attachment is obsolete: true
Attachment #488921 - Flags: review?(
Whiteboard: [needs new patch] → [needs review gavin]
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.
Attachment #488921 - Flags: review?( → review+
Blocks: 615291
I filed bug 615291 as a follow-up for refactoring the test.
Whiteboard: [needs review gavin] → [can land]
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
You need to log in before you can comment on or make changes to this bug.