Based on various bug reports, it seems clear that users are confused by the autofill heuristic result. Indeed, even if we will visit the https version, we show a trimmed url without a schema, so the user thinks we are forcing them to visit the unsecure version. Showing https may increase users trust into the locationbar choices.

Oh, so the autofill uses HTTPS? That was news to me! I agree that at least the "foo.com - Visit" entry should contained the schema, if not the input field.

The code returning autofill entries is in _matchKnownUrl http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/toolkit/components/places/UnifiedComplete.js#1154 it will then go through _processHostRow or _processUrlRow that set match.comment. For example http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/toolkit/components/places/UnifiedComplete.js#1603 This goes through AutocompleteController, then to autocomplete.xml (partially overriden by urlbarbindings.xml). To me looks like we are doing the right thing in UnifiedComplete.js already, by only stripping http, not https. The comment value we set looks correct... So the problem may rather be UI side? What we should have out of UnifiedComplete is the complete untrimmed url in comment and finalCompleteValue, and the trimmed url in value. I wonder if the problem is here http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/toolkit/content/widgets/autocomplete.xml#2022 For autofill entries we create visiturl entries, and the title for these is defined here, just same as the displayurl: http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/toolkit/content/widgets/autocomplete.xml#2114 So, the issue looks like being the fact we set a proper comment for autofill entries, but then we don't use it. Someone should debug the values in _adjustAcItem and maybe add a further param to the visiturl address so we set the title differently, or just make it use title instead of originalUrl (worth a try).

I think I've covered the various cases correctly now. I'll also set off a try build to make sure I haven't broken any tests.

Comment on attachment 8835513 [details] Bug 1322747 - Show https in autofill heuristic results. https://reviewboard.mozilla.org/r/111238/#review112854 I think this needs a couple more (simple) tests, but it looks good. ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:221 (Diff revision 1) > value: controller.getValueAt(i), > comment: controller.getCommentAt(i), > style: controller.getStyleAt(i), > image: controller.getImageAt(i), > } > - do_print(`Looking for "${result.value}", "${result.comment}" in expected results...`); > + do_print(`Found value: "${result.value}", comment: "${result.comment}" style: "${result.style}" in results...`); add a comma before style ::: toolkit/components/places/tests/unifiedcomplete/test_dupe_urls.js:22 (Diff revision 1) > completed: "mozilla.org/", > matches: [ { uri: NetUtil.newURI("http://mozilla.org/"), > - title: "mozilla.org", > + title: "mozilla.org/", > style: [ "autofill", "heuristic" ] } ] > }); > yield cleanup(); could you please add a further test to this test file for the makeKeyForURL bug you found? ::: toolkit/content/widgets/autocomplete.xml:2047 (Diff revision 1) > if (initialTypes.has("autofill")) { > // Treat autofills as visiturl actions. > action = { > type: "visiturl", > params: { > - url: originalUrl, > + url: this.getAttribute("title"), Unfortunately this is not tested anywhere, could you please add a test case to browser_urlbarAutoFillTrimURLs.js

Comment on attachment 8835513 [details] Bug 1322747 - Show https in autofill heuristic results. https://reviewboard.mozilla.org/r/111238/#review112854 > Unfortunately this is not tested anywhere, could you please add a test case to browser_urlbarAutoFillTrimURLs.js In writing for tests, I found another case that wasn't covered - searching for 'https://something' would show "something/" in the autofill result in the popup. That seemed inconsistent, so I fixed that as well. I've also rewritten the test file a bit to have an array of tests to try and make it clearer what all the cases and expected results are.

Comment on attachment 8835513 [details] Bug 1322747 - Show https in autofill heuristic results. https://reviewboard.mozilla.org/r/111238/#review114540 It looks quite good. Based on the IRC discussion with Shorlander, here I'm suggesting to drop the trailing slash for host autofills, only in the panel. The input field will continue having the trailing slash, because it helps further typing a path, after confirming the host. In the panel there's no valid reason to keep it and it clutters the text. If this change should cause anything bigger than "add www or remove slashes in the tests", please ask, I don't want to steal you a week of work for a dumb slash, we could just move this to a follow-up. I'm clearing the review just to give a very quick look at the final iteration, but we're 99% good here. ::: toolkit/components/places/UnifiedComplete.js:671 (Diff revision 2) > + let actionUrl = match.value; > + > // At this stage we only consider moz-action URLs. > if (!actionUrl.startsWith("moz-action:")) { > + // For autofill entries, we need to have a key based on the comment rather > + // than the value field. Could you please expand the comment with the reason. Something like: // For autofill entries, we need to have a key based on the comment rather // than the value field, because the latter may have been trimmed. ::: toolkit/components/places/UnifiedComplete.js:1729 (Diff revision 2) > + this._strippedPrefix == "https://"; > + > + // We don't strip the trailing slash, so that the value in displayed in the > + // autocomplete list has a slash, just like the match.value displayed in the > + // urlbar. > + match.comment = (addPrefix ? "https://" : "") + trimmedHost; I think we should actually show where the user is finally going, that is also with www. and so on. Basically I'd only strip "http://". This way we won't "lie" to the user, and if there are entries down in the panel that look like duplicates, it may be easier to understand they are not: I'd suggest this code path: // Comment should be the user's final destination, because ... (sum up your nice comments) match.comment = stripHttpAndTrim(match.finalCompleteValue || match.value); This should basically always return the final destination, modulo http and an eventual trailing slash. This will require fixing the trailing slashes in the tests (sorry!). If the trailing slash change should cause any issue other than that, please let me know before moving into large changes. ::: toolkit/components/places/tests/unifiedcomplete/test_dupe_urls.js:42 (Diff revision 2) > + style: [ "autofill", "heuristic" ] } ] > + }); > +}); > + > +add_task(function* teardown() { > yield cleanup(); Sounds like a good task for registerCleanupFunction()

Comment on attachment 8835513 [details] Bug 1322747 - Show https in autofill heuristic results. https://reviewboard.mozilla.org/r/111238/#review114540 > I think we should actually show where the user is finally going, that is also with www. and so on. > Basically I'd only strip "http://". This way we won't "lie" to the user, and if there are entries down in the panel that look like duplicates, it may be easier to understand they are not: > I'd suggest this code path: > > // Comment should be the user's final destination, because ... (sum up your nice comments) > match.comment = stripHttpAndTrim(match.finalCompleteValue || match.value); > > This should basically always return the final destination, modulo http and an eventual trailing slash. > This will require fixing the trailing slashes in the tests (sorry!). If the trailing slash change should cause any issue other than that, please let me know before moving into large changes. This is much nicer, tests seem happy as well, but I'll pass it through try to be sure. > Sounds like a good task for registerCleanupFunction() It appears that head.js does this already, so I've dropped it.

Comment on attachment 8835513 [details] Bug 1322747 - Show https in autofill heuristic results. https://reviewboard.mozilla.org/r/111238/#review114884 LGTM!

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0b13fb9ee2de Show https in autofill heuristic results. r=mak

Setting qe-verify- since this fix has automated coverage. Mark, if you think Manual QA should instead be looking at this, feel free to flip the qe-verify flag or ni? me directly.