onBeforeLinkTraversal should compare host strings instead of top level domains

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 487375 [details] [diff] [review]
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.
Attachment #487375 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
blocking2.0: --- → ?
(Assignee)

Updated

8 years ago
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]
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.
Attachment #487375 - Flags: review?(gavin.sharp) → review-
Whiteboard: [needs review gavin] → [needs new patch]
(Assignee)

Comment 3

8 years ago
Created attachment 488921 [details] [diff] [review]
patch v2
Attachment #487375 - Attachment is obsolete: true
Attachment #488921 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
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?(gavin.sharp) → review+
(Assignee)

Updated

8 years ago
Blocks: 615291
(Assignee)

Comment 5

8 years ago
I filed bug 615291 as a follow-up for refactoring the test.
Whiteboard: [needs review gavin] → [can land]
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/mozilla-central/rev/50913ed5b777
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
You need to log in before you can comment on or make changes to this bug.