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

--HG--
extra : rebase_source : f5d37af3d0c0714e0f33b5548c4bb3b90519543b

browser/base/content/test/urlbar/browser_autocomplete_enter_race.js

@@ -47,13 +47,14 @@ add_task(async function test_sametext() {
 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() {

browser/base/content/test/urlbar/browser_search_favicon.js

@@ -22,7 +22,7 @@ add_task(async function() {
   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");

browser/base/content/test/urlbar/browser_urlbar_remove_match.js

@@ -13,7 +13,7 @@ add_task(async function test_remove_history() {
   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");
 

toolkit/components/places/PlacesUtils.jsm

@@ -21,6 +21,10 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   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".
@@ -318,6 +322,8 @@ this.PlacesUtils = {
   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),
 
@@ -413,7 +419,38 @@ this.PlacesUtils = {
       }
       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;
+    }
   },
 
   /**

toolkit/components/places/UnifiedComplete.js

@@ -713,40 +713,18 @@ function stripHttpAndTrim(spec, trimSlash = true) {
  * 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);
+    return [stripHttpAndTrim(url), null];
   }
-  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);
-  }
-  // 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];
 }
 
 /**
@@ -868,7 +846,7 @@ function Search(searchString, searchParam, autocompleteListener,
   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.
@@ -1060,8 +1038,7 @@ Search.prototype = {
     // 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")) {
@@ -1132,14 +1109,22 @@ Search.prototype = {
       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;
     }
@@ -1859,23 +1844,6 @@ Search.prototype = {
                                        !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.
@@ -1891,6 +1859,8 @@ Search.prototype = {
     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);
     }
@@ -1911,6 +1881,43 @@ Search.prototype = {
   },
 
   _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).
@@ -1947,7 +1954,7 @@ Search.prototype = {
       }
     }
 
-    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.
@@ -1966,6 +1973,7 @@ Search.prototype = {
       bucket.insertIndex++;
       break;
     }
+    this._usedURLs[index] = {key: urlMapKey, action};
     return { index, replace };
   },
 

toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js

@@ -120,7 +120,7 @@ async function _check_autocomplete_matches(match, result) {
   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;

toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js

@@ -203,3 +203,31 @@ add_task(async function test_localtab_matches_override() {
              ],
   });
 });
+
+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" }),
+             ],
+  });
+});

toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js

@@ -26,8 +26,7 @@ add_task(async function test_tab_matches() {
   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 }) ]
   });