Firefox should fix common scheme typos

VERIFIED FIXED in Firefox 29

Status

()

Core
Document Navigation
--
enhancement
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: christian, Assigned: christian)

Tracking

({feature})

Trunk
mozilla29
feature
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 verified)

Details

(Whiteboard: [bugday-20140122])

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 827817 [details] [diff] [review]
url_fixup.patch

Firefox should fix common scheme typos.

I frequently copy + paste URLs and it annoys me when I miss the "h" in http. So I did something about it.
Attachment #827817 - Flags: review?(gavin.sharp)
(Assignee)

Comment 1

4 years ago
Created attachment 827818 [details] [diff] [review]
url_fixup.patch
Attachment #827817 - Attachment is obsolete: true
Attachment #827817 - Flags: review?(gavin.sharp)
Attachment #827818 - Flags: review?(gavin.sharp)
(Assignee)

Comment 2

4 years ago
Created attachment 827819 [details] [diff] [review]
url_fixup.patch

Missed a test in my queue
Attachment #827818 - Attachment is obsolete: true
Attachment #827818 - Flags: review?(gavin.sharp)
Attachment #827819 - Flags: review?(gavin.sharp)
Comment on attachment 827819 [details] [diff] [review]
url_fixup.patch

Review of attachment 827819 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDefaultURIFixup.cpp
@@ +224,5 @@
> +            scheme.LowerCaseEqualsLiteral("ftp") ||
> +            scheme.LowerCaseEqualsLiteral("file")) {
> +            // Do nothing.
> +        } else if (scheme.LowerCaseEqualsLiteral("ttp") &&
> +            Preferences::GetBool("browser.fixup.typo.ttp.http", true)) {

Two things:
1) It seems like overkill to have a pref for every one of these fixups. I would suggest just a "browser.fixup.scheme" or "browser.fixup.typo.scheme" to cover all of them.
2) You probably want to look this pref up once and cache it instead of doing a pref lookup every time a URL is entered.
(Also thanks for the patch!)
(Assignee)

Comment 5

4 years ago
Will fix this weekend, was in Boston and couldn't get to it.
(Assignee)

Updated

4 years ago
Attachment #827819 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

4 years ago
I'm also going to remove the "tp" one, as it is ambiguous if it is "http" or "ftp".
Sorry for the slow response! Patch in general looks good to me, as do Ted's suggestions. You should ask bz for review, since I'm not a peer of this stuff.
(In reply to christian from comment #6)
> I'm also going to remove the "tp" one, as it is ambiguous if it is "http" or
> "ftp".

I think it would still be fair to just assume "http". OTOH, it's probably a much less common typo.

Should registerProtocolHandler blacklist these typos? (https://developer.mozilla.org/en-US/docs/Web/API/navigator.registerProtocolHandler)
(Assignee)

Comment 9

4 years ago
I thought about the blacklist, but if for some reason someone needs to register it, they still can and then override this with the pref. Probably super edge-case though
(Assignee)

Comment 10

4 years ago
(as an aside, I would love to be able to trivially instrument this to see how many times each fixup is happening)
(Assignee)

Comment 11

4 years ago
Created attachment 832073 [details] [diff] [review]
url_fixup.patch

Cache the pref. Tweak the test. Blacklist scheme registration.
Attachment #827819 - Attachment is obsolete: true
Attachment #832073 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

4 years ago
Created attachment 832075 [details] [diff] [review]
url_fixup.patch

wut mercurial
Attachment #832073 - Attachment is obsolete: true
Attachment #832073 - Flags: review?(bzbarsky)
Attachment #832075 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

4 years ago
Created attachment 832076 [details] [diff] [review]
url_fixup.patch

Whoops, older patch. I'll get the hang of hg again some day...
Attachment #832075 - Attachment is obsolete: true
Attachment #832075 - Flags: review?(bzbarsky)
Attachment #832076 - Flags: review?(bzbarsky)
Comment on attachment 832076 [details] [diff] [review]
url_fixup.patch

>+        } else if (scheme.LowerCaseEqualsLiteral("ps")) {
>+            // tps -> https.

"ps", not "tps", right?

I'm a bit worried about this one and the "le" one, but I can't find obvious existing schemes like that.

>+    const unsigned long FIXUP_FLAG_FIX_SCHEME_TYPOS = 5;

8, not 5, since this is a bitfield.

r=me
Attachment #832076 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

4 years ago
Thanks for the review! I'll try to get to the changes tomorrow.
(Assignee)

Comment 16

4 years ago
Landed with changes in https://hg.mozilla.org/mozilla-central/rev/604bf3977dc5

Do I need to update the patch here?
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
>Do I need to update the patch here?

Nope.
(Assignee)

Comment 18

4 years ago
Err, this didn't take after I changed to a bitfield
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

4 years ago
Created attachment 8334365 [details] [diff] [review]
uri_fixup_part2.patch

Actually turns on the feature.
Attachment #8334365 - Flags: review?(bzbarsky)
Comment on attachment 8334365 [details] [diff] [review]
uri_fixup_part2.patch

The docshell change doesn't look right to me.  We should certainly not default to doing this fixup in docshell.

The caps change is ok.

The other ones need review from someone who owns the relevant code.
Attachment #8334365 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 21

4 years ago
Created attachment 8335140 [details] [diff] [review]
uri_fixup_part2.patch

Brought typo fixup up a level
Attachment #8334365 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 8335147 [details] [diff] [review]
uri_fixup_part2.patch
Attachment #8335140 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8335147 - Flags: feedback?(bzbarsky)
Comment on attachment 8335147 [details] [diff] [review]
uri_fixup_part2.patch

Seems reasonable.
Attachment #8335147 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 24

4 years ago
Comment on attachment 8335147 [details] [diff] [review]
uri_fixup_part2.patch

Ok, let's see how this does in review. Added bz for docshell bits, dolske for frontend bits, and frank for metro bits.
Attachment #8335147 - Flags: ui-review?(fryn)
Attachment #8335147 - Flags: superreview?(dolske)
Attachment #8335147 - Flags: review?(bzbarsky)
Comment on attachment 8335147 [details] [diff] [review]
uri_fixup_part2.patch

r=me, I think
Attachment #8335147 - Flags: review?(bzbarsky) → review+

Comment 26

3 years ago
Comment on attachment 8335147 [details] [diff] [review]
uri_fixup_part2.patch

Review of attachment 8335147 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not handling code or UI reviews at the moment. Deferring to mbrubeck.
Attachment #8335147 - Flags: ui-review?(fryn) → review?(mbrubeck)
Comment on attachment 8335147 [details] [diff] [review]
uri_fixup_part2.patch

Review of attachment 8335147 [details] [diff] [review]:
-----------------------------------------------------------------

r=mbrubeck for the changes in /browser/metro
Attachment #8335147 - Flags: review?(mbrubeck) → review+
The patch bitrotted due to bug 945627.
Comment on attachment 8335147 [details] [diff] [review]
uri_fixup_part2.patch

Review of attachment 8335147 [details] [diff] [review]:
-----------------------------------------------------------------

I fear this is adding the new fixup in places it shouldn't. Is this just adding it to every loadURIWithFlags callsite?

I'd definitely expect the flag to be used when typing/pasting a URL in the URL bar. And maybe some other places where a URL is supplied by the local user's input (where typos are likely). But for the general-purpose helpers [like loadURI() in browser,js, openLinkIn() in utilityOverlay.js] no fixup should be used (use of LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP might be a good indicator of where it's suitable). Eg, I wouldn't expect the right-click context menu's "Open Link in New Tab/Window" to make broken content links suddenly start working. Seems like that would set a bad precedent for web compatibility.

Should be straight forward to scope this down to fewer places, but if that's actually difficult or doesn't make sense, happy to think about it more.
Attachment #8335147 - Flags: superreview?(dolske) → review-
(Assignee)

Comment 30

3 years ago
Created attachment 8351739 [details] [diff] [review]
firefox_url_fixup_part3.patch

Only fix up typos when we also want to contact a 3rd party service

Test Plan:

* Loaded ttps://google.com, saw it worked
* Loaded ttp://google.com, saw it worked & redirected me to the https site
* Loaded ttp://reddit.com, saw it worked
* Loaded ttps://reddit.com, saw it worked (and still had a SSL warning)
* Loaded ttps://google.com, bookmarked it, saw bookmark was fixed URL
* Created simple page linking to ttps://google.com, clicked it, and saw it still threw an error (confirming this shouldn't affect web content)
* added bootstrap css to head, saw it wasn't attempted to download via ttps, and still downloaded via https
Attachment #832076 - Attachment is obsolete: true
Attachment #8335147 - Attachment is obsolete: true
Attachment #8351739 - Flags: review?(dolske)
(Assignee)

Comment 31

3 years ago
Created attachment 8351741 [details] [diff] [review]
firefox_url_fixup_part3.patch
Attachment #8351739 - Attachment is obsolete: true
Attachment #8351739 - Flags: review?(dolske)
Attachment #8351741 - Flags: review?(dolske)
Comment on attachment 8351741 [details] [diff] [review]
firefox_url_fixup_part3.patch

Review of attachment 8351741 [details] [diff] [review]:
-----------------------------------------------------------------

God, every time I look at the (existing) way this stuff works I just hate it, for fear of subtle bugs. Seems like it would be so much straight-forward to have all the low-level code only deal with URIs, and require callers to explicitly get their shit in order upfront... If something's reading user input and wants "amazon cheap ties" work (or, now, "ttp://site.com"), that's the time to map user input to an actionable URI. There's already a number of places (such as those touched by this patch) that do something with a provided "url", but then pass it off for potential third_party_fixup. But I don't see anything overtly wrong or dangerous, so this patch isn't making things worse.

r+=me, congrats to clegnitto for daring to wade into this cesspit. :)
Attachment #8351741 - Flags: review?(dolske) → review+
Comment on attachment 8351741 [details] [diff] [review]
firefox_url_fixup_part3.patch

Review of attachment 8351741 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserContentHandler.js
@@ +59,4 @@
>  
>    if (!(uri instanceof nsIFileURL)) {
> +    return urifixup.createFixupURI(aArgument,
> +                                   urifixup.FIXUP_FLAG_FIX_SCHEME_TYPOS);

Why did you add URI fixup at all here?
(this touched some android code, heads up mfinkle)
Comment on attachment 8351741 [details] [diff] [review]
firefox_url_fixup_part3.patch

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>-            uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
>+            let flags = Ci.nsIURIFixup.FIXUP_FLAG_NONE;
>+            uri = uriFixup.createFixupURI(inputVal, flags);

>diff --git a/browser/metro/base/content/bindings/urlbar.xml b/browser/metro/base/content/bindings/urlbar.xml

>-              uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
>+              let flags = Ci.nsIURIFixup.FIXUP_FLAG_NONE;
>+              uri = uriFixup.createFixupURI(inputVal, flags);

These don't look like useful changes.

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js

> function resolveURIInternal(aCmdLine, aArgument) {
>   var uri = aCmdLine.resolveURI(aArgument);
>+  var urifixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>+                           .getService(nsIURIFixup);

You can use Services.uriFixup
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #34)
> (this touched some android code, heads up mfinkle)

rs=mfinkle for the android part :)
(Assignee)

Comment 37

3 years ago
(In reply to Masatoshi Kimura [:emk] from comment #33)
> Comment on attachment 8351741 [details] [diff] [review]
> firefox_url_fixup_part3.patch
> 
> Review of attachment 8351741 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserContentHandler.js
> @@ +59,4 @@
> >  
> >    if (!(uri instanceof nsIFileURL)) {
> > +    return urifixup.createFixupURI(aArgument,
> > +                                   urifixup.FIXUP_FLAG_FIX_SCHEME_TYPOS);
> 
> Why did you add URI fixup at all here?

So it works from the command line.
(Assignee)

Comment 38

3 years ago
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #35)
> Comment on attachment 8351741 [details] [diff] [review]
> firefox_url_fixup_part3.patch
> 
> >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
> 
> >-            uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
> >+            let flags = Ci.nsIURIFixup.FIXUP_FLAG_NONE;
> >+            uri = uriFixup.createFixupURI(inputVal, flags);
> 
> >diff --git a/browser/metro/base/content/bindings/urlbar.xml b/browser/metro/base/content/bindings/urlbar.xml
> 
> >-              uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
> >+              let flags = Ci.nsIURIFixup.FIXUP_FLAG_NONE;
> >+              uri = uriFixup.createFixupURI(inputVal, flags);
> 
> These don't look like useful changes.

Yeah, kinda a holdover. I thought it was better to keep consistent with everywhere else but I can remove.

> 
> >diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js
> 
> > function resolveURIInternal(aCmdLine, aArgument) {
> >   var uri = aCmdLine.resolveURI(aArgument);
> >+  var urifixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
> >+                           .getService(nsIURIFixup);
> 
> You can use Services.uriFixup

I just moved the line from below in the function *shrugs*
(Assignee)

Updated

3 years ago
Keywords: feature
Target Milestone: mozilla28 → mozilla29
https://hg.mozilla.org/integration/mozilla-inbound/rev/72fc7d58a274
status-firefox29: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/72fc7d58a274
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
Hi Christian!

Should the test plan provided in comment 30 and exploratory testing around it cover the necessary testing for this feature?

If not, what are the things to keep in mind while verifying this feature?

Thank you!
Flags: needinfo?(christian)
QA Contact: petruta.rasa
(Assignee)

Comment 42

3 years ago
(In reply to petruta.rasa from comment #41)
> Hi Christian!
> 
> Should the test plan provided in comment 30 and exploratory testing around
> it cover the necessary testing for this feature?
> 
> If not, what are the things to keep in mind while verifying this feature?
> 
> Thank you!

Yep! I didn't actually test metro, windows, and linux builds. Basically this should work in the location bar and not in web content. All bookmarks, history items, etc should point to the corrected URL.
Flags: needinfo?(christian)

Updated

3 years ago
status-firefox29: fixed → verified
Whiteboard: [bugday-20140122]

Updated

3 years ago
Status: RESOLVED → VERIFIED
Automation coverage: http://mxr.mozilla.org/mozilla-central/source/docshell/test/unit/test_nsDefaultURIFixup.js
Flags: in-testsuite+
Comment on attachment 832076 [details] [diff] [review]
url_fixup.patch

(Assuming this patch is roughly equivalent to what got checked in, it's better not to mark it as obsolete.)
Attachment #832076 - Attachment is obsolete: false
Depends on: 1029632

Updated

3 years ago
Flags: needinfo?(petruta.rasa)
Flags: needinfo?(petruta.rasa)
You need to log in before you can comment on or make changes to this bug.