Bug 1433938 - Move Synced Tab matches above general history matches in the Address Bar. r=adw
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 13 Feb 2018 14:34:44 +0100
changeset 404321 6cfa628374192bc053d5837c892fc5d6f44a6778
parent 404320 3f96ca5e9d2deeaa6206313c981fa32d46062725
child 404322 be58311261fcad925faeaceefe3303ceb1653865
child 404337 90e5ccdd8c5fbd87e427e16dd5fd226fe79cedf6
push id59777
push usermak77@bonardo.net
push dateSat, 17 Feb 2018 13:15:33 +0000
treeherderautoland@6cfa62837419 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1433938
milestone60.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 1433938 - Move Synced Tab matches above general history matches in the Address Bar. r=adw Puts Remote (Synced) Tab matches before other history results. Changes deduping algorithm to replace simple history matches with tab matches when the url is the same. Keeps overriding a Remote Tab with a Local Tab when the url is the same. MozReview-Commit-ID: 76urDklKtRF
browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
browser/base/content/test/urlbar/browser_search_favicon.js
browser/base/content/test/urlbar/browser_urlbar_remove_match.js
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/UnifiedComplete.js
toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
--- a/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
+++ b/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
@@ -42,23 +42,24 @@ add_task(async function test_sametext() 
   info("wait for the page to load");
   await BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser,
                                        false, "http://example.com/");
 });
 
 add_task(async function test_after_empty_search() {
   await promiseAutocompleteResultPopup("");
   gURLBar.focus();
-  gURLBar.value = "e";
+  // Add www. to avoid a switch-to-tab.
+  gURLBar.value = "www.e";
   EventUtils.synthesizeKey("x", {});
   EventUtils.synthesizeKey("VK_RETURN", {});
 
   info("wait for the page to load");
   await BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser,
-                                       false, "http://example.com/");
+                                       false, "http://www.example.com/");
 });
 
 add_task(async function test_disabled_ac() {
   // Disable autocomplete.
   let suggestHistory = Preferences.get("browser.urlbar.suggest.history");
   Preferences.set("browser.urlbar.suggest.history", false);
   let suggestBookmarks = Preferences.get("browser.urlbar.suggest.bookmark");
   Preferences.set("browser.urlbar.suggest.bookmark", false);
--- a/browser/base/content/test/urlbar/browser_search_favicon.js
+++ b/browser/base/content/test/urlbar/browser_search_favicon.js
@@ -17,17 +17,17 @@ add_task(async function() {
 
   Services.search.addEngineWithDetails("SearchEngine", "", "", "",
                                        "GET", "http://s.example.com/search");
   gEngine = Services.search.getEngineByName("SearchEngine");
   gEngine.addParam("q", "{searchTerms}", null);
   gOriginalEngine = Services.search.currentEngine;
   Services.search.currentEngine = gEngine;
 
-  let uri = NetUtil.newURI("http://s.example.com/search?q=foo&client=1");
+  let uri = NetUtil.newURI("http://s.example.com/search?q=foobar&client=1");
   await PlacesTestUtils.addVisits({ uri, title: "Foo - SearchEngine Search" });
 
   await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:mozilla");
 
   // The first autocomplete result has the action searchengine, while
   // the second result is the "search favicon" element.
   await promiseAutocompleteResultPopup("foo");
   let result = await waitForAutocompleteResultAt(1);
--- a/browser/base/content/test/urlbar/browser_urlbar_remove_match.js
+++ b/browser/base/content/test/urlbar/browser_urlbar_remove_match.js
@@ -8,17 +8,17 @@ add_task(async function test_remove_hist
 
   registerCleanupFunction(async function() {
     await PlacesUtils.history.clear();
   });
 
   let promiseVisitRemoved = PlacesTestUtils.waitForNotification(
     "onDeleteURI", uri => uri.spec == TEST_URL, "history");
 
-  await promiseAutocompleteResultPopup("remove.me/from_urlbar");
+  await promiseAutocompleteResultPopup("from_urlbar");
   let result = await waitForAutocompleteResultAt(1);
   Assert.equal(result.getAttribute("ac-value"), TEST_URL, "Found the expected result");
 
   EventUtils.synthesizeKey("VK_DOWN", {});
   Assert.equal(gURLBar.popup.richlistbox.selectedIndex, 1);
   let options = AppConstants.platform == "macosx" ? { shiftKey: true } : {};
   EventUtils.synthesizeKey("VK_DELETE", options);
   await promiseVisitRemoved;
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -16,16 +16,20 @@ XPCOMUtils.defineLazyModuleGetters(this,
   OS: "resource://gre/modules/osfile.jsm",
   Sqlite: "resource://gre/modules/Sqlite.jsm",
   Bookmarks: "resource://gre/modules/Bookmarks.jsm",
   History: "resource://gre/modules/History.jsm",
   AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
   PlacesSyncUtils: "resource://gre/modules/PlacesSyncUtils.jsm",
 });
 
+XPCOMUtils.defineLazyGetter(this, "MOZ_ACTION_REGEX", () => {
+  return /^moz-action:([^,]+),(.*)$/;
+});
+
 // On Mac OSX, the transferable system converts "\r\n" to "\n\n", where
 // we really just want "\n". On other platforms, the transferable system
 // converts "\r\n" to "\n".
 const NEWLINE = AppConstants.platform == "macosx" ? "\n" : "\r\n";
 
 // Timers resolution is not always good, it can have a 16ms precision on Win.
 const TIMERS_RESOLUTION_SKEW_MS = 16;
 
@@ -313,16 +317,18 @@ this.PlacesUtils = {
   TOPIC_EXPIRATION_FINISHED: "places-expiration-finished",
   TOPIC_FEEDBACK_UPDATED: "places-autocomplete-feedback-updated",
   TOPIC_FAVICONS_EXPIRED: "places-favicons-expired",
   TOPIC_VACUUM_STARTING: "places-vacuum-starting",
   TOPIC_BOOKMARKS_RESTORE_BEGIN: "bookmarks-restore-begin",
   TOPIC_BOOKMARKS_RESTORE_SUCCESS: "bookmarks-restore-success",
   TOPIC_BOOKMARKS_RESTORE_FAILED: "bookmarks-restore-failed",
 
+  ACTION_SCHEME: "moz-action:",
+
   asContainer: aNode => asContainer(aNode),
   asQuery: aNode => asQuery(aNode),
 
   endl: NEWLINE,
 
   /**
    * Is a string a valid GUID?
    *
@@ -408,17 +414,48 @@ this.PlacesUtils = {
     for (let key in params) {
       // Strip null or undefined.
       // Regardless, don't encode them or they would be converted to a string.
       if (params[key] === null || params[key] === undefined) {
         continue;
       }
       encodedParams[key] = encodeURIComponent(params[key]);
     }
-    return "moz-action:" + type + "," + JSON.stringify(encodedParams);
+    return this.ACTION_SCHEME + type + "," + JSON.stringify(encodedParams);
+  },
+
+  /**
+   * Parses a moz-action URL and returns its parts.
+   *
+   * @param url A moz-action URI.
+   * @note URL is in the format moz-action:ACTION,JSON_ENCODED_PARAMS
+   */
+  parseActionUrl(url) {
+    if (url instanceof Ci.nsIURI)
+      url = url.spec;
+    else if (url instanceof URL)
+      url = url.href;
+    // Faster bailout.
+    if (!url.startsWith(this.ACTION_SCHEME))
+      return null;
+
+    try {
+      let [, type, params] = url.match(MOZ_ACTION_REGEX);
+      let action = {
+        type,
+        params: JSON.parse(params)
+      };
+      for (let key in action.params) {
+        action.params[key] = decodeURIComponent(action.params[key]);
+      }
+      return action;
+    } catch (ex) {
+      Cu.reportError(`Invalid action url "${url}"`);
+      return null;
+    }
   },
 
   /**
    * Parses matchBuckets strings (for example, "suggestion:4,general:Infinity")
    * like those used in the browser.urlbar.matchBuckets preference.
    *
    * @param   str
    *          A matchBuckets string.
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -708,50 +708,28 @@ function stripHttpAndTrim(spec, trimSlas
 
 /**
  * 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(match) {
-  let actionUrl = match.value;
-
+  let url = match.value;
+  let action = PlacesUtils.parseActionUrl(url);
   // At this stage we only consider moz-action URLs.
-  if (!actionUrl.startsWith("moz-action:")) {
+  if (!action || !("url" in action.params)) {
     // 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);
+      url = 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);
+    return [stripHttpAndTrim(url), null];
   }
-  // For now we only handle these 2 action types and treat them as the same.
-  switch (type) {
-    case "remotetab":
-    case "switchtab":
-      if (params.url) {
-        return "moz-action:tab:" + stripHttpAndTrim(params.url);
-      }
-      break;
-      // TODO (bug 1222435) - "switchtab" should be handled as an "autofill"
-      // entry.
-    default:
-      // do nothing.
-      // TODO (bug 1222436) - extend this method so it can be used instead of
-      // the |placeId| that's also used to remove duplicate entries.
-  }
-  return stripHttpAndTrim(actionUrl);
+  return [stripHttpAndTrim(action.params.url), action];
 }
 
 /**
  * Returns whether the passed in string looks like a url.
  */
 function looksLikeUrl(str, ignoreAlphanumericHosts = false) {
   // Single word not including special chars.
   return !REGEXP_SPACES.test(str) &&
@@ -863,17 +841,17 @@ function Search(searchString, searchPara
   }
 
   // This is a replacement for this._result.matchCount, to be used when you need
   // to check how many "current" matches have been inserted.
   // Indeed this._result.matchCount may include matches from the previous search.
   this._currentMatchCount = 0;
 
   // These are used to avoid adding duplicate entries to the results.
-  this._usedURLs = new Set();
+  this._usedURLs = [];
   this._usedPlaceIds = new Set();
 
   // Counters for the number of matches per MATCHTYPE.
   this._counts = Object.values(MATCHTYPE)
                        .reduce((o, p) => { o[p] = 0; return o; }, {});
 
   this._searchStringHasWWW = this._strippedPrefix.endsWith("www.");
   this._searchStringWWW = this._searchStringHasWWW ? "www." : "";
@@ -1055,18 +1033,17 @@ Search.prototype = {
     // enabled, the first result is always a special result (resulting from one
     // of the queries between (1) and (6) inclusive). As such, the UI is
     // expected to auto-select the first result when actions are enabled. If the
     // first result is an inline completion result, that will also be the
     // default result and therefore be autofilled (this also happens if actions
     // are not enabled).
 
     // Get the final query, based on the tokens found in the search string.
-    let queries = [ this._adaptiveQuery ];
-
+    let queries = [];
     // "openpage" behavior is supported by the default query.
     // _switchToTabQuery instead returns only pages not supported by history.
     if (this.hasBehavior("openpage")) {
       queries.push(this._switchToTabQuery);
     }
     queries.push(this._searchQuery);
 
     // Check for Preloaded Sites Expiry before Autofill
@@ -1127,24 +1104,32 @@ Search.prototype = {
         }
       }
     }
     // In any case, clear previous suggestions.
     searchSuggestionsCompletePromise.then(() => {
       this._cleanUpNonCurrentMatches(MATCHTYPE.SUGGESTION);
     });
 
-    for (let [query, params] of queries) {
-      await conn.executeCached(query, params, this._onResultRow.bind(this));
+    // Run the adaptive query first.
+    await conn.executeCached(this._adaptiveQuery[0], this._adaptiveQuery[1],
+                             this._onResultRow.bind(this));
+    if (!this.pending)
+      return;
+
+    // Then fetch remote tabs.
+    if (this._enableActions && this.hasBehavior("openpage")) {
+      await this._matchRemoteTabs();
       if (!this.pending)
         return;
     }
 
-    if (this._enableActions && this.hasBehavior("openpage")) {
-      await this._matchRemoteTabs();
+    // Finally run all the other queries.
+    for (let [query, params] of queries) {
+      await conn.executeCached(query, params, this._onResultRow.bind(this));
       if (!this.pending)
         return;
     }
 
     // Ideally we should wait until MATCH_BOUNDARY_ANYWHERE, but that query
     // may be really slow and we may end up showing old results for too long.
     this._cleanUpNonCurrentMatches(MATCHTYPE.GENERAL);
 
@@ -1854,48 +1839,33 @@ Search.prototype = {
       // We fallback to match.value, as that's what autocomplete does if
       // finalCompleteValue is null.
       // Trim only if the value looks like a domain, we want to retain the
       // trailing slash if we're completing a url to the next slash.
       match.comment = stripHttpAndTrim(match.finalCompleteValue || match.value,
                                        !this._searchString.includes("/"));
     }
 
-    // Must check both id and url, cause keywords dynamically modify the url.
-    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.
-    // We cannot use only the url since keywords entries are modified to
-    // include the search string, and would be returned multiple times.  Ids
-    // are faster too.
-    if (match.placeId)
-      this._usedPlaceIds.add(match.placeId);
-    this._usedURLs.add(urlMapKey);
-
     match.style = match.style || "favicon";
 
     // Restyle past searches, unless they are bookmarks or special results.
     if (Prefs.get("restyleSearches") && match.style == "favicon") {
       this._maybeRestyleSearchMatch(match);
     }
 
     if (this._addingHeuristicFirstMatch) {
       match.style += " heuristic";
     }
 
     match.icon = match.icon || "";
     match.finalCompleteValue = match.finalCompleteValue || "";
 
     let {index, replace} = this._getInsertIndexForMatch(match);
+    if (index == -1)
+      return;
     if (replace) { // Replacing an existing match from the previous search.
       this._result.removeMatchAt(index);
     }
     this._result.insertMatchAt(index,
                                match.value,
                                match.comment,
                                match.icon,
                                match.style,
@@ -1906,16 +1876,53 @@ Search.prototype = {
     if (this._currentMatchCount == 1)
       TelemetryStopwatch.finish(TELEMETRY_1ST_RESULT, this);
     if (this._currentMatchCount == 6)
       TelemetryStopwatch.finish(TELEMETRY_6_FIRST_RESULTS, this);
     this.notifyResult(true, match.type == MATCHTYPE.HEURISTIC);
   },
 
   _getInsertIndexForMatch(match) {
+    // Check for duplicates and either discard (by returning -1) the duplicate
+    // or suggest to replace the original match, in case the new one is more
+    // specific (for example a Remote Tab wins over History, and a Switch to Tab
+    // wins over a Remote Tab).
+    // Must check both id and url, cause keywords dynamically modify the url.
+    // Note: this partially fixes Bug 1222435,  but not if the urls differ more
+    // than just by "http://". We should still evaluate www and other schemes
+    // equivalences.
+    let [urlMapKey, action] = makeKeyForURL(match);
+    if ((match.placeId && this._usedPlaceIds.has(match.placeId)) ||
+        this._usedURLs.map(e => e.key).includes(urlMapKey)) {
+      // it's a duplicate.
+      if (action && ["switchtab", "remotetab"].includes(action.type)) {
+        // Look for the duplicate among current matches.
+        for (let i = 0; i < this._usedURLs.length; ++i) {
+          let {key: matchKey, action: matchAction} = this._usedURLs[i];
+          if (matchKey == urlMapKey) {
+            if (!matchAction || action.type == "switchtab") {
+              this._usedURLs[i] = {key: urlMapKey, action};
+              return { index:  i, replace: true };
+            }
+            break; // Found the duplicate, no reason to continue.
+          }
+        }
+      }
+      return { index: -1, replace: false };
+    }
+
+    // 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.
+    // We cannot use only the url since keywords entries are modified to
+    // include the search string, and would be returned multiple times.  Ids
+    // are faster too.
+    if (match.placeId)
+      this._usedPlaceIds.add(match.placeId);
+
     let index = 0;
     // The buckets change depending on the context, that is currently decided by
     // the first added match (the heuristic one).
     if (!this._buckets) {
       // Convert the buckets to readable objects with a count property.
       let buckets = match.type == MATCHTYPE.HEURISTIC &&
                     match.style.includes("searchengine") ? Prefs.get("matchBucketsSearch")
                                                          : Prefs.get("matchBuckets");
@@ -1942,17 +1949,17 @@ Search.prototype = {
               bucket.count++;
               break;
             }
           }
         }
       }
     }
 
-    let replace = false;
+    let replace = 0;
     for (let bucket of this._buckets) {
       // Move to the next bucket if the match type is incompatible, or if there
       // is no available space or if the frecency is below the threshold.
       if (match.type != bucket.type || !bucket.available) {
         index += bucket.count;
         continue;
       }
 
@@ -1961,16 +1968,17 @@ Search.prototype = {
       if (bucket.insertIndex < bucket.count) {
         replace = true;
       } else {
         bucket.count++;
       }
       bucket.insertIndex++;
       break;
     }
+    this._usedURLs[index] = {key: urlMapKey, action};
     return { index, replace };
   },
 
   /**
    * Removes matches from a previous search, that are no more returned by the
    * current search
    * @param type
    *        The MATCHTYPE to clean up.
--- a/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
+++ b/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ -115,17 +115,17 @@ async function _check_autocomplete_match
   let { uri, title, tags, style } = match;
   if (tags)
     title += " \u2013 " + tags.sort().join(", ");
   if (style)
     style = style.sort();
   else
     style = ["favicon"];
 
-  info(`Checking against expected "${uri.spec}", "${title}"`);
+  info(`Checking against expected "${uri.spec}", comment: "${title}", style: "${style}"`);
   // Got a match on both uri and title?
   if (stripPrefix(uri.spec) != stripPrefix(result.value) || title != result.comment) {
     return false;
   }
 
   let actualStyle = result.style.split(/\s+/).sort();
   if (style)
     Assert.equal(actualStyle.toString(), style.toString(), "Match should have expected style");
--- a/toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
@@ -198,8 +198,36 @@ add_task(async function test_localtab_ma
   await check_autocomplete({
     search: "ex",
     searchParam: "enable-actions",
     matches: [ makeSearchMatch("ex", { heuristic: true }),
                makeSwitchToTabMatch("http://foo.com/", { title: "An Example" }),
              ],
   });
 });
+
+add_task(async function test_remotetab_matches_override() {
+  // If We have an history result to the same page, we should only get the
+  // remote tab match.
+  let url = "http://foo.remote.com/";
+  // First setup Sync to have the page as a remote tab.
+  configureEngine({
+    guid_mobile: {
+      clientName: "My Phone",
+      tabs: [{
+        urlHistory: [url],
+        title: "An Example",
+      }],
+    }
+  });
+
+  // Setup Places to think the tab is open locally.
+  await PlacesTestUtils.addVisits(url);
+
+  await check_autocomplete({
+    search: "rem",
+    searchParam: "enable-actions",
+    matches: [ makeSearchMatch("rem", { heuristic: true }),
+               makeRemoteTabMatch("http://foo.remote.com/", "My Phone",
+                                  { title: "An Example" }),
+             ],
+  });
+});
--- a/toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
@@ -21,18 +21,17 @@ add_task(async function test_tab_matches
   // Pages that cannot be registered in history.
   addOpenPages(uri3, 1);
   addOpenPages(uri4, 1);
 
   info("two results, normal result is a tab match");
   await check_autocomplete({
     search: "abc.com",
     searchParam: "enable-actions",
-    matches: [ makeVisitMatch("abc.com", "http://abc.com/", { heuristic: true }),
-               makeSwitchToTabMatch("http://abc.com/", { title: "ABC rocks" }),
+    matches: [ makeSwitchToTabMatch("http://abc.com/", { title: "ABC rocks" }),
                makeSearchMatch("abc.com", { heuristic: false }) ]
   });
 
   info("three results, one tab match");
   await check_autocomplete({
     search: "abc",
     searchParam: "enable-actions",
     matches: [ makeSearchMatch("abc", { heuristic: true }),