Bug 1433938 - Move Synced Tab matches above general history matches in the Address Bar. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 13 Feb 2018 14:34:44 +0100
changeset 756719 f5d37af3d0c0714e0f33b5548c4bb3b90519543b
parent 756717 dde7eb1a589f1ec5221ad2a8c78007094f3b3b01
push id99528
push usermak77@bonardo.net
push dateSat, 17 Feb 2018 13:12:52 +0000
reviewersadw
bugs1433938
milestone60.0a1
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 }),