Bug 1322747 - Show https in autofill heuristic results. r?mak draft
authorMark Banner <standard8@mozilla.com>
Thu, 26 Jan 2017 15:13:19 +0000
changeset 485397 06e19eee61558d667fd9acb0cbf63fbebdfa7f86
parent 485098 a9ec72f82299250e6023988e238931bbca0ef7fa
child 546004 990c7dcc2b97974f25d5a73304d6248e67d99566
push id45718
push userbmo:standard8@mozilla.com
push dateThu, 16 Feb 2017 16:42:25 +0000
reviewersmak
bugs1322747
milestone54.0a1
Bug 1322747 - Show https in autofill heuristic results. r?mak MozReview-Commit-ID: GlOoeQBOMIi
browser/base/content/test/urlbar/browser_urlbarAutoFillTrimURLs.js
toolkit/components/places/UnifiedComplete.js
toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
toolkit/components/places/tests/unifiedcomplete/test_autofill_default_behavior.js
toolkit/components/places/tests/unifiedcomplete/test_dupe_urls.js
toolkit/content/widgets/autocomplete.xml
--- a/browser/base/content/test/urlbar/browser_urlbarAutoFillTrimURLs.js
+++ b/browser/base/content/test/urlbar/browser_urlbarAutoFillTrimURLs.js
@@ -10,40 +10,107 @@ add_task(function* setup() {
     Services.prefs.clearUserPref(PREF_AUTOFILL);
     yield PlacesTestUtils.clearHistory();
     gURLBar.handleRevert();
   });
   Services.prefs.setBoolPref(PREF_TRIMURL, true);
   Services.prefs.setBoolPref(PREF_AUTOFILL, true);
 
   // Adding a tab would hit switch-to-tab, so it's safer to just add a visit.
-  yield PlacesTestUtils.addVisits({
+  yield PlacesTestUtils.addVisits([{
     uri: "http://www.autofilltrimurl.com/whatever",
     transition: Ci.nsINavHistoryService.TRANSITION_TYPED,
-  });
+  }, {
+    uri: "https://www.secureautofillurl.com/whatever",
+    transition: Ci.nsINavHistoryService.TRANSITION_TYPED,
+  }]);
 });
 
 function* promiseSearch(searchtext) {
   gURLBar.focus();
   gURLBar.inputField.value = searchtext.substr(0, searchtext.length - 1);
   EventUtils.synthesizeKey(searchtext.substr(-1, 1), {});
   yield promiseSearchComplete();
 }
 
-add_task(function* () {
-  yield promiseSearch("http://");
-  is(gURLBar.inputField.value, "http://", "Autofilled value is as expected");
+function* promiseTestResult(test) {
+  info("Searching for '${test.search}'");
+
+  yield promiseSearch(test.search);
+
+  is(gURLBar.inputField.value, test.autofilledValue,
+     `Autofilled value is as expected for search '${test.search}'`);
+
+  let result = gURLBar.popup.richlistbox.getItemAtIndex(0);
+
+  is(result._titleText.textContent, test.resultListDisplayTitle,
+     `Autocomplete result should have displayed title as expected for search '${test.search}'`);
+
+  is(result._actionText.textContent, test.resultListActionText,
+     `Autocomplete action text should be as expected for search '${test.search}'`);
+
+  is(result.getAttribute("type"), test.resultListType,
+     `Autocomplete result should have searchengine for the type for search '${test.search}'`);
+
+  is(gURLBar.mController.getFinalCompleteValueAt(0), test.finalCompleteValue,
+     `Autocomplete item should go to the expected final value for search '${test.search}'`);
+}
+
+const tests = [{
+    search: "http://",
+    autofilledValue: "http://",
+    resultListDisplayTitle: "http://",
+    resultListActionText: "Search with Google",
+    resultListType: "searchengine",
+    finalCompleteValue: 'moz-action:searchengine,{"engineName":"Google","input":"http%3A%2F%2F","searchQuery":"http%3A%2F%2F"}'
+  }, {
+    search: "https://",
+    autofilledValue: "https://",
+    resultListDisplayTitle: "https://",
+    resultListActionText: "Search with Google",
+    resultListType: "searchengine",
+    finalCompleteValue: 'moz-action:searchengine,{"engineName":"Google","input":"https%3A%2F%2F","searchQuery":"https%3A%2F%2F"}'
+  }, {
+    search: "au",
+    autofilledValue: "autofilltrimurl.com/",
+    resultListDisplayTitle: "www.autofilltrimurl.com",
+    resultListActionText: "Visit",
+    resultListType: "",
+    finalCompleteValue: "www.autofilltrimurl.com/"
+  }, {
+    search: "http://au",
+    autofilledValue: "http://autofilltrimurl.com/",
+    resultListDisplayTitle: "autofilltrimurl.com",
+    resultListActionText: "Visit",
+    resultListType: "",
+    finalCompleteValue: "http://autofilltrimurl.com/"
+  }, {
+    search: "sec",
+    autofilledValue: "secureautofillurl.com/",
+    resultListDisplayTitle: "https://www.secureautofillurl.com",
+    resultListActionText: "Visit",
+    resultListType: "",
+    finalCompleteValue: "https://www.secureautofillurl.com/"
+  }, {
+    search: "https://sec",
+    autofilledValue: "https://secureautofillurl.com/",
+    resultListDisplayTitle: "https://secureautofillurl.com",
+    resultListActionText: "Visit",
+    resultListType: "",
+    finalCompleteValue: "https://secureautofillurl.com/"
+  },
+];
+
+add_task(function* autofill_tests() {
+  for (let test of tests) {
+    yield promiseTestResult(test);
+  }
 });
 
-add_task(function* () {
-  yield promiseSearch("http://au");
-  is(gURLBar.inputField.value, "http://autofilltrimurl.com/", "Autofilled value is as expected");
-});
-
-add_task(function* () {
+add_task(function* autofill_complete_domain() {
   yield promiseSearch("http://www.autofilltrimurl.com");
   is(gURLBar.inputField.value, "http://www.autofilltrimurl.com/", "Autofilled value is as expected");
 
   // Now ensure selecting from the popup correctly trims.
   is(gURLBar.controller.matchCount, 2, "Found the expected number of matches");
   EventUtils.synthesizeKey("VK_DOWN", {});
   is(gURLBar.inputField.value, "www.autofilltrimurl.com/whatever", "trim was applied correctly");
 });
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -652,24 +652,31 @@ function stripHttpAndTrim(spec) {
   }
   if (spec.endsWith("/")) {
     spec = spec.slice(0, -1);
   }
   return spec;
 }
 
 /**
- * Returns the key to be used for a URL in a map for the purposes of removing
+ * Returns the key to be used for a match in a map for the purposes of removing
  * duplicate entries - any 2 URLs that should be considered the same should
  * return the same key. For some moz-action URLs this will unwrap the params
  * and return a key based on the wrapped URL.
  */
-function makeKeyForURL(actionUrl) {
+function makeKeyForURL(match) {
+  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, because the latter may have been trimmed.
+    if (match.hasOwnProperty("style") && match.style.includes("autofill")) {
+      return stripHttpAndTrim(match.comment);
+    }
     return stripHttpAndTrim(actionUrl);
   }
   let [, type, params] = actionUrl.match(/^moz-action:([^,]+),(.*)$/);
   try {
     params = JSON.parse(params);
   } catch (ex) {
     // This is unexpected in this context, so just return the input.
     return stripHttpAndTrim(actionUrl);
@@ -1615,17 +1622,17 @@ Search.prototype = {
 
   _addMatch(match) {
     // A search could be canceled between a query start and its completion,
     // in such a case ensure we won't notify any result for it.
     if (!this.pending)
       return;
 
     // Must check both id and url, cause keywords dynamically modify the url.
-    let urlMapKey = makeKeyForURL(match.value);
+    let urlMapKey = makeKeyForURL(match);
     if ((match.placeId && this._usedPlaceIds.has(match.placeId)) ||
         this._usedURLs.has(urlMapKey)) {
       return;
     }
 
     // Add this to our internal tracker to ensure duplicates do not end up in
     // the result.
     // Not all entries have a place id, thus we fallback to the url for them.
@@ -1700,19 +1707,26 @@ Search.prototype = {
     // If the untrimmed value doesn't preserve the user's input just
     // ignore it and complete to the found host.
     if (untrimmedHost &&
         !untrimmedHost.toLowerCase().includes(this._trimmedOriginalSearchString.toLowerCase())) {
       untrimmedHost = null;
     }
 
     match.value = this._strippedPrefix + trimmedHost;
-    // Remove the trailing slash.
-    match.comment = stripHttpAndTrim(trimmedHost);
     match.finalCompleteValue = untrimmedHost;
+
+    // The comment should be the user's final destination so that the user
+    // will definitely know where he is going to end up. For example, if the
+    // user is visiting a secure page, we'll leave the https on it, so that
+    // they know it'll be secure.
+    // We fallback to match.value, as that's what autocomplete does if
+    // finalCompleteValue is null.
+    match.comment = stripHttpAndTrim(match.finalCompleteValue || match.value);
+
     if (faviconUrl) {
       match.icon = PlacesUtils.favicons
                               .getFaviconLinkForIcon(NetUtil.newURI(faviconUrl)).spec;
     }
     // Although this has a frecency, this query is executed before any other
     // queries that would result in frecency matches.
     match.frecency = frecency;
     match.style = "autofill";
--- a/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
+++ b/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ -213,17 +213,17 @@ function* check_autocomplete(test) {
 
       for (let i = firstIndexToCheck; i < controller.matchCount; i++) {
         let result = {
           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...`);
         let lowerBound = test.checkSorting ? i : firstIndexToCheck;
         let upperBound = test.checkSorting ? i + 1 : matches.length;
         let found = false;
         for (let j = lowerBound; j < upperBound; ++j) {
           // Skip processed expected results
           if (matches[j] == undefined)
             continue;
           if (_check_autocomplete_matches(matches[j], result)) {
--- a/toolkit/components/places/tests/unifiedcomplete/test_autofill_default_behavior.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_autofill_default_behavior.js
@@ -7,28 +7,31 @@
  */
 
 add_task(function* test_default_behavior_host() {
   let uri1 = NetUtil.newURI("http://typed/");
   let uri2 = NetUtil.newURI("http://visited/");
   let uri3 = NetUtil.newURI("http://bookmarked/");
   let uri4 = NetUtil.newURI("http://tpbk/");
   let uri5 = NetUtil.newURI("http://tagged/");
+  let uri6 = NetUtil.newURI("https://secure/");
 
   yield PlacesTestUtils.addVisits([
     { uri: uri1, title: "typed", transition: TRANSITION_TYPED },
     { uri: uri2, title: "visited" },
     { uri: uri4, title: "tpbk", transition: TRANSITION_TYPED },
+    { uri: uri6, title: "secure", transition: TRANSITION_TYPED },
   ]);
   yield addBookmark( { uri: uri3, title: "bookmarked" } );
   yield addBookmark( { uri: uri4, title: "tpbk" } );
   yield addBookmark( { uri: uri5, title: "title", tags: ["foo"] } );
 
   yield setFaviconForHref(uri1.spec, "chrome://global/skin/icons/information-16.png");
   yield setFaviconForHref(uri3.spec, "chrome://global/skin/icons/error-16.png");
+  yield setFaviconForHref(uri6.spec, "chrome://global/skin/icons/question-16.png");
 
   // RESTRICT TO HISTORY.
   Services.prefs.setBoolPref("browser.urlbar.suggest.history", true);
   Services.prefs.setBoolPref("browser.urlbar.suggest.history.onlyTyped", false);
   Services.prefs.setBoolPref("browser.urlbar.suggest.bookmark", false);
 
   do_print("Restrict history, common visit, should not autoFill");
   yield check_autocomplete({
@@ -42,16 +45,25 @@ add_task(function* test_default_behavior
   yield check_autocomplete({
     search: "ty",
     matches: [ { uri: uri1, title: "typed", style: [ "autofill", "heuristic" ],
                  icon: "chrome://global/skin/icons/information-16.png" } ],
     autofilled: "typed/",
     completed: "typed/"
   });
 
+  do_print("Restrict history, secure typed visit, should autoFill with https");
+  yield check_autocomplete({
+    search: "secure",
+    matches: [ { uri: uri6, title: "https://secure", style: [ "autofill", "heuristic" ],
+                 icon: "chrome://global/skin/icons/question-16.png" } ],
+    autofilled: "secure/",
+    completed: "https://secure/"
+  });
+
   // Don't autoFill this one cause it's not typed.
   do_print("Restrict history, bookmark, should not autoFill");
   yield check_autocomplete({
     search: "bo",
     matches: [ ],
     autofilled: "bo",
     completed: "bo"
   });
--- a/toolkit/components/places/tests/unifiedcomplete/test_dupe_urls.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_dupe_urls.js
@@ -14,10 +14,26 @@ add_task(function* test_dupe_urls() {
   yield check_autocomplete({
     search: "moz",
     autofilled: "mozilla.org/",
     completed:  "mozilla.org/",
     matches: [ { uri: NetUtil.newURI("http://mozilla.org/"),
                  title: "mozilla.org",
                  style: [ "autofill", "heuristic" ] } ]
   });
-  yield cleanup();
 });
+
+add_task(function* test_dupe_secure_urls() {
+  yield PlacesTestUtils.addVisits({
+    uri: NetUtil.newURI("https://example.org/"),
+    transition: TRANSITION_TYPED
+  }, {
+    uri: NetUtil.newURI("https://example.org/?")
+  });
+  yield check_autocomplete({
+    search: "exam",
+    autofilled: "example.org/",
+    completed: "https://example.org/",
+    matches: [ { uri: NetUtil.newURI("https://example.org/"),
+                 title: "https://example.org",
+                 style: [ "autofill", "heuristic" ] } ]
+  });
+});
--- a/toolkit/content/widgets/autocomplete.xml
+++ b/toolkit/content/widgets/autocomplete.xml
@@ -2061,17 +2061,17 @@ extends="chrome://global/content/binding
 
           let action;
 
           if (initialTypes.has("autofill")) {
             // Treat autofills as visiturl actions.
             action = {
               type: "visiturl",
               params: {
-                url: originalUrl,
+                url: this.getAttribute("title"),
               },
             };
           }
 
           this.removeAttribute("actiontype");
           this.classList.remove("overridable-action");
 
           // If the type includes an action, set up the item appropriately.