Bug 1340108 - autocomplete prefill sites with proper protocol and www. management, r=Gijs
authorSvetlana Orlik <sveta.orlik.code@gmail.com>
Wed, 01 Mar 2017 12:18:23 +0300
changeset 345435 4af8a2761308
parent 345434 5ea643137f3e
child 345436 0ae0446f58c3
push id31437
push usercbook@mozilla.com
push dateThu, 02 Mar 2017 13:00:04 +0000
treeherdermozilla-central@180a160ae22a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1340108
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1340108 - autocomplete prefill sites with proper protocol and www. management, r=Gijs MozReview-Commit-ID: 1POpbPzYJTe
toolkit/components/places/UnifiedComplete.js
toolkit/components/places/mozIPlacesAutoComplete.idl
toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -551,16 +551,19 @@ XPCOMUtils.defineLazyGetter(this, "Prefs
 });
 
 // Prefill Sites related
 
 function PrefillSite(url, title) {
   this.uri = NetUtil.newURI(url);
   this.title = title;
   this._matchTitle = title.toLowerCase();
+  this._hasWWW = this.uri.host.startsWith("www.");
+  this._hostWithoutWWW = this._hasWWW ? this.uri.host.slice(4)
+                                      : this.uri.host;
 }
 
 /**
  * Storage object for Prefill Sites.
  *   add(url, title): adds a site to storage
  *   populate(sites) : populates the  storage with array of [url,title]
  *   sites[]: resulting array of sites (PrefillSite objects)
  */
@@ -791,16 +794,24 @@ function Search(searchString, searchPara
   this._localMatchesStartIndex = 0;
 
   // Counts the number of inserted local matches.
   this._localMatchesCount = 0;
   // Counts the number of inserted remote matches.
   this._remoteMatchesCount = 0;
   // Counts the number of inserted extension matches.
   this._extensionMatchesCount = 0;
+
+  this._searchStringHasWWW = this._strippedPrefix.endsWith("www.");
+  this._searchStringWWW = this._searchStringHasWWW ? "www." : "";
+  this._searchStringFromWWW = this._searchStringWWW + this._searchString;
+
+  this._searchStringSchemeFound = this._strippedPrefix.match(/^(\w+):/i);
+  this._searchStringScheme = this._searchStringSchemeFound ?
+                             this._searchStringSchemeFound[1].toLowerCase() : "";
 }
 
 Search.prototype = {
   /**
    * Enables the desired AutoComplete behavior.
    *
    * @param type
    *        The behavior type to set.
@@ -1044,51 +1055,115 @@ Search.prototype = {
     if (!Prefs.prefillSitesEnabled)
       return;
     let profileCreationDate = yield ProfileAgeCreatedPromise;
     let daysSinceProfileCreation = (Date.now() - profileCreationDate) / MS_PER_DAY;
     if (daysSinceProfileCreation > Prefs.prefillSitesExpireDays)
       Services.prefs.setBoolPref("browser.urlbar.usepreloadedtopurls.enabled", false);
   },
 
-  // TODO: manage protocol and "www." like _matchSearchEngineUrl() does
   _matchPrefillSites() {
     if (!Prefs.prefillSitesEnabled)
       return;
+
+    // In case user typed just "https://" or "www." or "https://www."
+    // - we do not put out the whole lot of sites
+    if (!this._searchString)
+      return;
+
+    if (!(this._searchStringScheme === "" ||
+          this._searchStringScheme === "https" ||
+          this._searchStringScheme === "http"))
+      return;
+
+    let strictMatches = [];
+    let looseMatches = [];
+
     for (let site of PrefillSiteStorage.sites) {
-      if (site.uri.host.includes(this._searchString) ||
-          site._matchTitle.includes(this._searchString)) {
-        let match = {
-          value: site.uri.spec,
-          comment: site.title,
-          style: "prefill-site",
-          finalCompleteValue: site.uri.spec,
-          frecency: FRECENCY_DEFAULT - 1,
-        };
-        this._addMatch(match);
+      if (this._searchStringScheme && this._searchStringScheme !== site.uri.scheme)
+        continue;
+      let match = {
+        value: site.uri.spec,
+        comment: site.title,
+        style: "prefill-site",
+        frecency: FRECENCY_DEFAULT - 1,
+      };
+      if (site.uri.host.includes(this._searchStringFromWWW) ||
+          site._matchTitle.includes(this._searchStringFromWWW)) {
+        strictMatches.push(match);
+      } else if (site.uri.host.includes(this._searchString) ||
+                 site._matchTitle.includes(this._searchString)) {
+        looseMatches.push(match);
       }
     }
+    [...strictMatches, ...looseMatches].forEach(this._addMatch, this);
   },
 
   _matchPrefillSiteForAutofill() {
     if (!Prefs.prefillSitesEnabled)
       return false;
-    for (let site of PrefillSiteStorage.sites) {
-      if (site.uri.host.startsWith(this._searchString)) {
-        let match = {
-          value: stripPrefix(site.uri.spec),
-          style: "autofill",
-          finalCompleteValue: site.uri.spec,
-          frecency: FRECENCY_DEFAULT,
-        };
-        this._result.setDefaultIndex(0);
-        this._addMatch(match);
-        return true;
-      }
+
+    if (!(this._searchStringScheme === "" ||
+          this._searchStringScheme === "https" ||
+          this._searchStringScheme === "http"))
+      return false;
+
+    let searchStringSchemePrefix = this._searchStringScheme
+                                   ? (this._searchStringScheme + "://")
+                                   : "";
+
+    // If search string has scheme - we'll match it strictly
+    function matchScheme(site, search) {
+      return !search._searchStringScheme ||
+             search._searchStringScheme === site.uri.scheme;
+    }
+
+    // First we try to strict-match
+    // If search string has "www."- we try to strict-match it along with "www."
+    function matchStrict(site) {
+      return site.uri.host.startsWith(this._searchStringFromWWW)
+             && matchScheme(site, this);
     }
+    let site = PrefillSiteStorage.sites.find(matchStrict, this)
+    if (site) {
+      let match = {
+        // We keep showing prefix that user typed, then what we match on
+        value: searchStringSchemePrefix + site.uri.host + "/",
+        style: "autofill",
+        finalCompleteValue: site.uri.spec,
+        frecency: FRECENCY_DEFAULT,
+      };
+      this._result.setDefaultIndex(0);
+      this._addMatch(match);
+      return true;
+    }
+
+    // If no strict result found - we try loose match
+    // regardless of "www." in prefill-sites or search string
+    function matchLoose(site) {
+      return site._hostWithoutWWW.startsWith(this._searchString)
+             && matchScheme(site, this);
+    }
+    site = PrefillSiteStorage.sites.find(matchLoose, this);
+    if (site) {
+      let match = {
+        // We keep showing prefix that user typed, then what we match on
+        value: searchStringSchemePrefix + this._searchStringWWW +
+               site._hostWithoutWWW + "/",
+        style: "autofill",
+        // On loose match, result should always have "www."
+        finalCompleteValue: site.uri.scheme + "://www." +
+                            site._hostWithoutWWW + "/",
+        frecency: FRECENCY_DEFAULT,
+      };
+      this._result.setDefaultIndex(0);
+      this._addMatch(match);
+      return true;
+    }
+
     return false;
   },
 
   *_matchFirstHeuristicResult(conn) {
     // We always try to make the first result a special "heuristic" result.  The
     // heuristics below determine what type of result it will be, if any.
 
     let hasSearchTerms = this._searchTokens.length > 0;
@@ -2138,20 +2213,16 @@ UnifiedComplete.prototype = {
   registerOpenPage(uri, userContextId) {
     SwitchToTabStorage.add(uri, userContextId);
   },
 
   unregisterOpenPage(uri, userContextId) {
     SwitchToTabStorage.delete(uri, userContextId);
   },
 
-  addPrefillSite(url, title) {
-    PrefillSiteStorage.add(url, title);
-  },
-
   populatePrefillSiteStorage(json) {
     PrefillSiteStorage.populate(json);
   },
 
   // nsIAutoCompleteSearch
 
   startSearch(searchString, searchParam, previousResult, listener) {
     // Stop the search in case the controller has not taken care of it.
--- a/toolkit/components/places/mozIPlacesAutoComplete.idl
+++ b/toolkit/components/places/mozIPlacesAutoComplete.idl
@@ -132,25 +132,15 @@ interface mozIPlacesAutoComplete : nsISu
    * @param aURI
    *        The URI to unregister as an open page.
    * @param aUserContextId
    *        The Container Id of the tab.
    */
   void unregisterOpenPage(in nsIURI aURI, in uint32_t aUserContextId);
 
   /**
-   * Add a site to list of Prefill Sites.
-   *
-   * @param url
-   *        The URL of added site.
-   * @param title
-   *        The title of added site.
-   */
-  void addPrefillSite(in AUTF8String url, in AUTF8String title);
-
-  /**
    * Populate list of Prefill Sites from JSON.
    *
    * @param sites
    *        Array of [url,title] to populate from.
    */
   void populatePrefillSiteStorage(in jsval sites);
 };
--- a/toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js
@@ -11,18 +11,20 @@ const autocompleteObject = Cc["@mozilla.
 
 Cu.importGlobalProperties(["fetch"]);
 
 // With or without trailing slash - no matter. URI.spec does have it always.
 // Then, check_autocomplete() doesn't cut it off (uses stripPrefix()).
 let yahoooURI = NetUtil.newURI("https://yahooo.com/");
 let gooogleURI = NetUtil.newURI("https://gooogle.com/");
 
-autocompleteObject.addPrefillSite(yahoooURI.spec, "Yahooo");
-autocompleteObject.addPrefillSite(gooogleURI.spec, "Gooogle");
+autocompleteObject.populatePrefillSiteStorage([
+  [yahoooURI.spec, "Yahooo"],
+  [gooogleURI.spec, "Gooogle"],
+]);
 
 function *assert_feature_works(condition) {
   do_print("List Results do appear " + condition);
   yield check_autocomplete({
     search: "ooo",
     matches: [
       { uri: yahoooURI, title: "Yahooo",  style: ["prefill-site"] },
       { uri: gooogleURI, title: "Gooogle", style: ["prefill-site"] },
@@ -111,16 +113,192 @@ add_task(function* test_sorting_against_
       { uri: yahoooURI, title: "Yahooo",  style: ["prefill-site"] },
       { uri: gooogleURI, title: "Gooogle", style: ["prefill-site"] },
     ],
   });
 
   yield cleanup();
 });
 
+add_task(function* test_scheme_and_www() {
+  // Order is important to check sorting
+  let sites = [
+    ["https://www.ooops-https-www.com/", "Ooops"],
+    ["https://ooops-https.com/",         "Ooops"],
+    ["HTTP://ooops-HTTP.com/",           "Ooops"],
+    ["HTTP://www.ooops-HTTP-www.com/",   "Ooops"],
+    ["https://foo.com/",     "Title with www"],
+    ["https://www.bar.com/", "Tile"],
+  ];
+
+  let titlesMap = new Map(sites)
+
+  autocompleteObject.populatePrefillSiteStorage(sites);
+
+  let tests =
+  [
+    // User typed,
+    // Inline autofill,
+    // Substitute after enter is pressed,
+    //   [List matches, with sorting]
+    //   not tested if omitted
+    //   !!! first one is always an autofill entry !!!
+
+    [// Protocol by itself doesn't match anything
+    "https://",
+    "https://",
+    "https://",
+      []
+    ],
+
+    [// "www." by itself doesn't match anything
+    "www.",
+    "www.",
+    "www.",
+      []
+    ],
+
+    [// Protocol with "www." by itself doesn't match anything
+    "http://www.",
+    "http://www.",
+    "http://www.",
+      []
+    ],
+
+    [// ftp: - ignore
+    "ftp://ooops",
+    "ftp://ooops",
+    "ftp://ooops",
+      []
+    ],
+
+    [// Edge case: no "www." in search string, autofill and list entries with "www."
+    "ww",
+    "www.ooops-https-www.com/",
+    "https://www.ooops-https-www.com/", // 2nd in list, but has priority as strict
+      [
+      ["https://www.ooops-https-www.com/", "https://www.ooops-https-www.com"],
+      "HTTP://www.ooops-HTTP-www.com/",
+      ["https://foo.com/", "Title with www", ["prefill-site"]],
+      "https://www.bar.com/",
+      ]
+    ],
+
+    [// Strict match, no "www."
+    "ooops",
+    "ooops-https.com/",
+    "https://ooops-https.com/", // 2nd in list, but has priority as strict
+      [// List entries are not sorted (initial sorting preserved)
+       // except autofill entry is on top as always
+      ["https://ooops-https.com/", "https://ooops-https.com"],
+      "https://www.ooops-https-www.com/",
+      "HTTP://ooops-HTTP.com/",
+      "HTTP://www.ooops-HTTP-www.com/",
+      ]
+    ],
+
+    [// Strict match with "www."
+    "www.ooops",
+    "www.ooops-https-www.com/",
+    "https://www.ooops-https-www.com/",
+      [// Matches with "www." sorted on top
+      ["https://www.ooops-https-www.com/", "https://www.ooops-https-www.com"],
+      "HTTP://www.ooops-HTTP-www.com/",
+      "https://ooops-https.com/",
+      "HTTP://ooops-HTTP.com/",
+      ]
+    ],
+
+    [// Loose match: search no "www.", result with "www."
+    "ooops-https-www",
+    "ooops-https-www.com/",
+    "https://www.ooops-https-www.com/",
+      [
+      ["https://www.ooops-https-www.com/", "https://www.ooops-https-www.com"],
+      ]
+    ],
+
+    [// Loose match: search "www.", no-www site gets "www."
+    "www.ooops-https.",
+    "www.ooops-https.com/",
+    "https://www.ooops-https.com/",
+      [// Only autofill entry gets "www."
+      ["https://www.ooops-https.com/", "https://www.ooops-https.com"],
+      "https://ooops-https.com/", // List entry with prefilled URl for match site
+      ]
+    ],
+
+    [// Explicit protocol, no "www."
+    "https://ooops",
+    "https://ooops-https.com/",
+    "https://ooops-https.com/",
+      [
+      ["https://ooops-https.com/", "https://ooops-https.com"],
+      "https://www.ooops-https-www.com/",
+      ]
+    ],
+
+    [// Explicit protocol, with "www."
+    "https://www.ooops",
+    "https://www.ooops-https-www.com/",
+    "https://www.ooops-https-www.com/",
+      [
+      ["https://www.ooops-https-www.com/", "https://www.ooops-https-www.com"],
+      "https://ooops-https.com/",
+      ]
+    ],
+
+    [// Explicit HTTP protocol, no-www site gets "www."
+    "http://www.ooops-http.",
+    "http://www.ooops-http.com/",
+    "http://www.ooops-http.com/",
+      [
+      ["HTTP://www.ooops-HTTP.com/", "www.ooops-http.com"],
+      "HTTP://ooops-HTTP.com/",
+      ]
+    ],
+
+    [// Wrong protocol
+    "http://ooops-https",
+    "http://ooops-https",
+    "http://ooops-https",
+      []
+    ],
+  ];
+
+  function toMatch(entry, index) {
+    if (Array.isArray(entry)) {
+      return {
+        uri: NetUtil.newURI(entry[0]),
+        title: entry[1],
+        style: entry[2] || ["autofill", "heuristic"],
+      };
+    }
+    return {
+      uri: NetUtil.newURI(entry),
+      title: titlesMap.get(entry),
+      style: ["prefill-site"],
+    };
+  }
+
+  for (let test of tests) {
+    let matches = test[3] ? test[3].map(toMatch) : null;
+    do_print("User types: " + test[0]);
+    yield check_autocomplete({
+      checkSorting: true,
+      search: test[0],
+      autofilled: test[1].toLowerCase(),
+      completed: test[2].toLowerCase(),
+      matches,
+    });
+  }
+
+  yield cleanup();
+});
+
 add_task(function* test_data_file() {
   let response = yield fetch("chrome://global/content/unifiedcomplete-top-urls.json");
 
   do_print("Source file is supplied and fetched OK");
   Assert.ok(response.ok);
 
   do_print("The JSON is parsed");
   let sites = yield response.json();
@@ -129,14 +307,14 @@ add_task(function* test_data_file() {
   autocompleteObject.populatePrefillSiteStorage(sites);
 
   let lastSite = sites.pop();
   let uri = NetUtil.newURI(lastSite[0]);
 
   do_print("Storage is populated from JSON correctly");
   yield check_autocomplete({
     search: uri.host,
-    autofilled: stripPrefix(uri.spec),
+    autofilled: uri.host + "/",
     completed: uri.spec,
   });
 
   yield cleanup();
 });