Closed Bug 935377 Opened 6 years ago Closed 6 years ago

Firefox should fix common scheme typos

Categories

(Core :: DOM: Navigation, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox29 --- verified

People

(Reporter: christian, Assigned: christian)

References

Details

(Keywords: feature, Whiteboard: [bugday-20140122])

Attachments

(2 files, 9 obsolete files)

Attached patch url_fixup.patch (obsolete) — Splinter Review
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)
Attached patch url_fixup.patch (obsolete) — Splinter Review
Attachment #827817 - Attachment is obsolete: true
Attachment #827817 - Flags: review?(gavin.sharp)
Attachment #827818 - Flags: review?(gavin.sharp)
Attached patch url_fixup.patch (obsolete) — Splinter Review
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!)
Will fix this weekend, was in Boston and couldn't get to it.
Attachment #827819 - Flags: review?(gavin.sharp)
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)
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
(as an aside, I would love to be able to trivially instrument this to see how many times each fixup is happening)
Attached patch url_fixup.patch (obsolete) — Splinter Review
Cache the pref. Tweak the test. Blacklist scheme registration.
Attachment #827819 - Attachment is obsolete: true
Attachment #832073 - Flags: review?(bzbarsky)
Attached patch url_fixup.patch (obsolete) — Splinter Review
wut mercurial
Attachment #832073 - Attachment is obsolete: true
Attachment #832073 - Flags: review?(bzbarsky)
Attachment #832075 - Flags: review?(bzbarsky)
Attached patch url_fixup.patchSplinter Review
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+
Thanks for the review! I'll try to get to the changes tomorrow.
Landed with changes in https://hg.mozilla.org/mozilla-central/rev/604bf3977dc5

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

Nope.
Err, this didn't take after I changed to a bitfield
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch uri_fixup_part2.patch (obsolete) — Splinter Review
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-
Attached patch uri_fixup_part2.patch (obsolete) — Splinter Review
Brought typo fixup up a level
Attachment #8334365 - Attachment is obsolete: true
Attached patch uri_fixup_part2.patch (obsolete) — Splinter Review
Attachment #8335140 - Attachment is obsolete: true
Attachment #8335147 - Flags: feedback?(bzbarsky)
Comment on attachment 8335147 [details] [diff] [review]
uri_fixup_part2.patch

Seems reasonable.
Attachment #8335147 - Flags: feedback?(bzbarsky) → feedback+
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 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-
Attached patch firefox_url_fixup_part3.patch (obsolete) — Splinter Review
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)
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 :)
(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.
(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*
Keywords: feature
Target Milestone: mozilla28 → mozilla29
https://hg.mozilla.org/mozilla-central/rev/72fc7d58a274
Status: REOPENED → RESOLVED
Closed: 6 years ago6 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
(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)
Whiteboard: [bugday-20140122]
Status: RESOLVED → VERIFIED
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
Flags: needinfo?(petruta.rasa)
Flags: needinfo?(petruta.rasa)
Having a look at this patch, I got surprised it was fixing "tps" and "ps" (for https), but not "tp" and "p" (for http). I understand there is ambiguity with ftp, but as stated in comment #8, http is a much more common case.

Mainly for the sake of consistency between http and https, I would suggest adding back the fixes for "tp" and "p".
Alternatively, we could remove the fixes for "tps", "ps", and also "le". Is it really relevant to fix more than one missing letter?
You need to log in before you can comment on or make changes to this bug.