Closed
Bug 935377
Opened 11 years ago
Closed 10 years ago
Firefox should fix common scheme typos
Categories
(Core :: DOM: Navigation, enhancement)
Core
DOM: Navigation
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)
9.65 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
18.93 KB,
patch
|
Dolske
:
review+
|
Details | Diff | 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)
Attachment #827817 -
Attachment is obsolete: true
Attachment #827817 -
Flags: review?(gavin.sharp)
Attachment #827818 -
Flags: review?(gavin.sharp)
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 3•11 years ago
|
||
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.
Comment 4•11 years ago
|
||
(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".
Comment 7•11 years ago
|
||
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.
Comment 8•11 years ago
|
||
(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
Assignee | ||
Comment 10•11 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•11 years ago
|
||
Cache the pref. Tweak the test. Blacklist scheme registration.
Attachment #827819 -
Attachment is obsolete: true
Attachment #832073 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•11 years ago
|
||
wut mercurial
Attachment #832073 -
Attachment is obsolete: true
Attachment #832073 -
Flags: review?(bzbarsky)
Attachment #832075 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•11 years ago
|
||
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 14•11 years ago
|
||
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•11 years ago
|
||
Thanks for the review! I'll try to get to the changes tomorrow.
Assignee | ||
Comment 16•11 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
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Target Milestone: --- → mozilla28
Comment 17•11 years ago
|
||
>Do I need to update the patch here?
Nope.
Assignee | ||
Comment 18•11 years ago
|
||
Err, this didn't take after I changed to a bitfield
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 19•11 years ago
|
||
Actually turns on the feature.
Attachment #8334365 -
Flags: review?(bzbarsky)
Comment 20•11 years ago
|
||
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•11 years ago
|
||
Brought typo fixup up a level
Attachment #8334365 -
Attachment is obsolete: true
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #8335140 -
Attachment is obsolete: true
Attachment #8335147 -
Flags: feedback?(bzbarsky)
Comment 23•11 years ago
|
||
Comment on attachment 8335147 [details] [diff] [review] uri_fixup_part2.patch Seems reasonable.
Attachment #8335147 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 24•11 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 25•11 years ago
|
||
Comment on attachment 8335147 [details] [diff] [review] uri_fixup_part2.patch r=me, I think
Attachment #8335147 -
Flags: review?(bzbarsky) → review+
Comment 26•10 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 27•10 years ago
|
||
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+
Comment 28•10 years ago
|
||
The patch bitrotted due to bug 945627.
Comment 29•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
Attachment #8351739 -
Attachment is obsolete: true
Attachment #8351739 -
Flags: review?(dolske)
Attachment #8351741 -
Flags: review?(dolske)
Comment 32•10 years ago
|
||
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 33•10 years ago
|
||
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?
Comment 34•10 years ago
|
||
(this touched some android code, heads up mfinkle)
Comment 35•10 years ago
|
||
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
Comment 36•10 years ago
|
||
(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•10 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•10 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*
Comment 39•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/72fc7d58a274
status-firefox29:
--- → fixed
Comment 40•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/72fc7d58a274
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Comment 41•10 years ago
|
||
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•10 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•10 years ago
|
Whiteboard: [bugday-20140122]
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Comment 43•10 years ago
|
||
Automation coverage: http://mxr.mozilla.org/mozilla-central/source/docshell/test/unit/test_nsDefaultURIFixup.js
Flags: in-testsuite+
Comment 44•10 years ago
|
||
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
Updated•10 years ago
|
Flags: needinfo?(petruta.rasa)
Updated•10 years ago
|
Flags: needinfo?(petruta.rasa)
Comment 45•6 years ago
|
||
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".
Comment 46•6 years ago
|
||
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.
Description
•