Bug 1464272 - Remote Tab matches may dominate the address bar results. r=kitcambridge draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 06 Jun 2018 18:48:27 +0200
changeset 805282 d8a7dd15c9664dda6dafc8fb1e7dee53b5b2e9b5
parent 804634 cec4a3cecc29ff97860198969b6fdff24b9e93bb
child 805684 13b39abddd823f9a23f071dd519a31712dd76b2a
child 805792 6843a40368ddd7d06f84dc3acf413a53b3002362
child 806337 9a7e13cc62dfe0ae95210e7a75068d07a0b7c275
push id112618
push usermak77@bonardo.net
push dateThu, 07 Jun 2018 16:22:26 +0000
reviewerskitcambridge
bugs1464272
milestone62.0a1
Bug 1464272 - Remote Tab matches may dominate the address bar results. r=kitcambridge MozReview-Commit-ID: HfvFvM619V5
toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm
toolkit/components/places/UnifiedComplete.js
toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
--- a/toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm
+++ b/toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm
@@ -8,74 +8,63 @@
  */
 
 "use strict";
 
 var EXPORTED_SYMBOLS = ["PlacesRemoteTabsAutocompleteProvider"];
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Services.jsm");
+ChromeUtils.defineModuleGetter(this, "SyncedTabs",
+  "resource://services-sync/SyncedTabs.jsm");
 
 XPCOMUtils.defineLazyGetter(this, "weaveXPCService", function() {
   try {
     return Cc["@mozilla.org/weave/service;1"]
              .getService(Ci.nsISupports)
              .wrappedJSObject;
   } catch (ex) {
     // The app didn't build Sync.
   }
   return null;
 });
 
-XPCOMUtils.defineLazyGetter(this, "Weave", () => {
-  try {
-    let {Weave} = ChromeUtils.import("resource://services-sync/main.js", {});
-    return Weave;
-  } catch (ex) {
-    // The app didn't build Sync.
-  }
-  return null;
-});
-
 // from MDN...
 function escapeRegExp(string) {
   return string.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
 }
 
 // Build the in-memory structure we use.
-function buildItems() {
-  let clients = new Map(); // keyed by client guid, value is client
-  let tabs = new Map(); // keyed by string URL, value is {clientId, tab}
-
+async function buildItems() {
+  // This is sorted by most recent client, most recent tab.
+  let tabsData = [];
   // If Sync isn't initialized (either due to lag at startup or due to no user
   // being signed in), don't reach in to Weave.Service as that may initialize
   // Sync unnecessarily - we'll get an observer notification later when it
   // becomes ready and has synced a list of tabs.
   if (weaveXPCService.ready) {
-    let engine = Weave.Service.engineManager.get("tabs");
-
-    for (let [guid, client] of Object.entries(engine.getAllClients())) {
-      clients.set(guid, client);
+    let clients = await SyncedTabs.getTabClients();
+    SyncedTabs.sortTabClientsByLastUsed(clients);
+    for (let client of clients) {
       for (let tab of client.tabs) {
-        let url = tab.urlHistory[0];
-        tabs.set(url, { clientId: guid, tab });
+        tabsData.push({tab, client});
       }
     }
   }
-  return { clients, tabs };
+  return tabsData;
 }
 
 // Manage the cache of the items we use.
 // The cache itself.
 let _items = null;
 
 // Ensure the cache is good.
-function ensureItems() {
+async function ensureItems() {
   if (!_items) {
-    _items = buildItems();
+    _items = await buildItems();
   }
   return _items;
 }
 
 // A preference used to disable the showing of icons in remote tab records.
 const PREF_SHOW_REMOTE_ICONS = "services.sync.syncedTabs.showRemoteIcons";
 let showRemoteIcons;
 
@@ -112,35 +101,35 @@ Services.obs.addObserver(observe, "weave
 
 // Observe the pref for showing remote icons and prime our bool that reflects its value.
 Services.prefs.addObserver(PREF_SHOW_REMOTE_ICONS, observe);
 observe(null, "nsPref:changed", PREF_SHOW_REMOTE_ICONS);
 
 // This public object is a static singleton.
 var PlacesRemoteTabsAutocompleteProvider = {
   // a promise that resolves with an array of matching remote tabs.
-  getMatches(searchString) {
+  async getMatches(searchString) {
     // If Sync isn't configured we bail early.
     if (!weaveXPCService || !weaveXPCService.ready || !weaveXPCService.enabled) {
-      return Promise.resolve([]);
+      return [];
     }
 
     let re = new RegExp(escapeRegExp(searchString), "i");
     let matches = [];
-    let { tabs, clients } = ensureItems();
-    for (let [url, { clientId, tab }] of tabs) {
+    let tabsData = await ensureItems();
+    for (let {tab, client} of tabsData) {
+      let url = tab.url;
       let title = tab.title;
       if (url.match(re) || (title && title.match(re))) {
-        // lookup the client record.
-        let client = clients.get(clientId);
         let icon = showRemoteIcons ? tab.icon : null;
         // create the record we return for auto-complete.
         let record = {
           url, title, icon,
-          deviceClass: Weave.Service.clientsEngine.getClientType(clientId),
-          deviceName: client.clientName,
+          deviceName: client.name,
+          lastUsed: tab.lastUsed * 1000
         };
         matches.push(record);
       }
     }
-    return Promise.resolve(matches);
+
+    return matches;
   },
 };
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -77,16 +77,20 @@ const FRECENCY_DEFAULT = 1000;
 // with the omnibox API. This is the maximum number of suggestions an extension
 // is allowed to add for a given search string.
 // This value includes the heuristic result.
 const MAXIMUM_ALLOWED_EXTENSION_MATCHES = 6;
 
 // After this time, we'll give up waiting for the extension to return matches.
 const MAXIMUM_ALLOWED_EXTENSION_TIME_MS = 3000;
 
+// By default we add remote tabs that have been used less than this time ago.
+// Any remaining remote tabs are added in queue if no other results are found.
+const RECENT_REMOTE_TAB_THRESHOLD_MS = 259200000; // 72 hours.
+
 // A regex that matches "single word" hostnames for whitelisting purposes.
 // The hostname will already have been checked for general validity, so we
 // don't need to be exhaustive here, so allow dashes anywhere.
 const REGEXP_SINGLEWORD_HOST = new RegExp("^[a-z0-9-]+$", "i");
 
 // Regex used to match userContextId.
 const REGEXP_USER_CONTEXT_ID = /(?:^| )user-context-id:(\d+)/;
 
@@ -931,16 +935,19 @@ function Search(searchString, searchPara
       this._previousSearchMatchTypes.push(MATCHTYPE.GENERAL);
     }
   }
 
   // Used to limit the number of adaptive results.
   this._adaptiveCount = 0;
   this._extraAdaptiveRows = [];
 
+  // Used to limit the number of remote tab results.
+  this._extraRemoteTabRows = [];
+
   // 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 = [];
   this._usedPlaceIds = new Set();
@@ -1218,16 +1225,22 @@ Search.prototype = {
     }
 
     // If we have some unused adaptive matches, add them now.
     while (this._extraAdaptiveRows.length &&
            this._currentMatchCount < Prefs.get("maxRichResults")) {
       this._addFilteredQueryMatch(this._extraAdaptiveRows.shift());
     }
 
+    // If we have some unused remote tab matches, add them now.
+    while (this._extraRemoteTabRows.length &&
+          this._currentMatchCount < Prefs.get("maxRichResults")) {
+      this._addMatch(this._extraRemoteTabRows.shift());
+    }
+
     // 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);
 
     // If we do not have enough results, and our match type is
     // MATCH_BOUNDARY_ANYWHERE, search again with MATCH_ANYWHERE to get more
     // results.
     let count = this._counts[MATCHTYPE.GENERAL] + this._counts[MATCHTYPE.HEURISTIC];
@@ -1704,17 +1717,17 @@ Search.prototype = {
   },
 
   async _matchRemoteTabs() {
     // Bail out early for non-sync users.
     if (!syncUsernamePref) {
       return;
     }
     let matches = await PlacesRemoteTabsAutocompleteProvider.getMatches(this._originalSearchString);
-    for (let {url, title, icon, deviceName} of matches) {
+    for (let {url, title, icon, deviceName, lastUsed} of matches) {
       // It's rare that Sync supplies the icon for the page (but if it does, it
       // is a string URL)
       if (!icon) {
         icon = iconHelper(url);
       } else {
         icon = PlacesUtils.favicons
                           .getFaviconLinkForIcon(Services.io.newURI(icon)).spec;
       }
@@ -1725,17 +1738,21 @@ Search.prototype = {
         value: PlacesUtils.mozActionURI("remotetab", { url, deviceName }),
         comment: title || url,
         style: "action remotetab",
         // we want frecency > FRECENCY_DEFAULT so it doesn't get pushed out
         // by "remote" matches.
         frecency: FRECENCY_DEFAULT + 1,
         icon,
       };
-      this._addMatch(match);
+      if (lastUsed > (Date.now() - RECENT_REMOTE_TAB_THRESHOLD_MS)) {
+        this._addMatch(match);
+      } else {
+        this._extraRemoteTabRows.push(match);
+      }
     }
   },
 
   // TODO (bug 1054814): Use visited URLs to inform which scheme to use, if the
   // scheme isn't specificed.
   _matchUnknownUrl() {
     let flags = Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS |
                 Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP;
--- a/toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
@@ -23,16 +23,22 @@ MockTabsEngine.prototype = {
   },
 };
 
 // A clients engine that doesn't need to be a constructor.
 let MockClientsEngine = {
   getClientType(guid) {
     Assert.ok(guid.endsWith("desktop") || guid.endsWith("mobile"));
     return guid.endsWith("mobile") ? "phone" : "desktop";
+  },
+  remoteClientExists(id) {
+    return true;
+  },
+  getClientName(id) {
+    return id.endsWith("mobile") ? "My Phone" : "My Desktop";
   }
 };
 
 // Tell Sync about the mocks.
 Weave.Service.engineManager.register(MockTabsEngine);
 
 // Tell the Sync XPCOM service it is initialized.
 let weaveXPCService = Cc["@mozilla.org/weave/service;1"]
@@ -60,17 +66,17 @@ function makeRemoteTabMatch(url, deviceN
   };
 }
 
 // The tests.
 add_task(async function test_nomatch() {
   // Nothing matches.
   configureEngine({
     guid_desktop: {
-      clientName: "My Desktop",
+      id: "desktop",
       tabs: [{
         urlHistory: ["http://foo.com/"],
       }],
     }
   });
 
   // No remote tabs match here, so we only expect search results.
   await check_autocomplete({
@@ -79,17 +85,17 @@ add_task(async function test_nomatch() {
     matches: [ makeSearchMatch("ex", { heuristic: true }) ],
   });
 });
 
 add_task(async function test_minimal() {
   // The minimal client and tabs info we can get away with.
   configureEngine({
     guid_desktop: {
-      clientName: "My Desktop",
+      id: "desktop",
       tabs: [{
         urlHistory: ["http://example.com/"],
       }],
     }
   });
 
   await check_autocomplete({
     search: "ex",
@@ -98,17 +104,17 @@ add_task(async function test_minimal() {
                makeRemoteTabMatch("http://example.com/", "My Desktop") ],
   });
 });
 
 add_task(async function test_maximal() {
   // Every field that could possibly exist on a remote record.
   configureEngine({
     guid_mobile: {
-      clientName: "My Phone",
+      id: "mobile",
       tabs: [{
         urlHistory: ["http://example.com/"],
         title: "An Example",
         icon: "http://favicon",
       }],
     }
   });
 
@@ -123,17 +129,17 @@ add_task(async function test_maximal() {
              ],
   });
 });
 
 add_task(async function test_noShowIcons() {
   Services.prefs.setBoolPref("services.sync.syncedTabs.showRemoteIcons", false);
   configureEngine({
     guid_mobile: {
-      clientName: "My Phone",
+      id: "mobile",
       tabs: [{
         urlHistory: ["http://example.com/"],
         title: "An Example",
         icon: "http://favicon",
       }],
     }
   });
 
@@ -150,17 +156,17 @@ add_task(async function test_noShowIcons
   });
   Services.prefs.clearUserPref("services.sync.syncedTabs.showRemoteIcons");
 });
 
 add_task(async function test_matches_title() {
   // URL doesn't match search expression, should still match the title.
   configureEngine({
     guid_mobile: {
-      clientName: "My Phone",
+      id: "mobile",
       tabs: [{
         urlHistory: ["http://foo.com/"],
         title: "An Example",
       }],
     }
   });
 
   await check_autocomplete({
@@ -175,17 +181,17 @@ add_task(async function test_matches_tit
 
 add_task(async function test_localtab_matches_override() {
   // We have an open tab to the same page on a remote device, only "switch to
   // tab" should appear as duplicate detection removed the remote one.
 
   // First setup Sync to have the page as a remote tab.
   configureEngine({
     guid_mobile: {
-      clientName: "My Phone",
+      id: "mobile",
       tabs: [{
         urlHistory: ["http://foo.com/"],
         title: "An Example",
       }],
     }
   });
 
   // Setup Places to think the tab is open locally.
@@ -206,17 +212,17 @@ add_task(async function test_localtab_ma
 
 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",
+      id: "mobile",
       tabs: [{
         urlHistory: [url],
         title: "An Example",
       }],
     }
   });
 
   // Setup Places to think the tab is open locally.
@@ -226,8 +232,52 @@ add_task(async function test_remotetab_m
     search: "rem",
     searchParam: "enable-actions",
     matches: [ makeSearchMatch("rem", { heuristic: true }),
                makeRemoteTabMatch("http://foo.remote.com/", "My Phone",
                                   { title: "An Example" }),
              ],
   });
 });
+
+add_task(async function test_many_remotetab_matches() {
+  await PlacesUtils.history.clear();
+
+  // In case we have many results, the most recent ones should be added on top,
+  // while others should be appended.
+  let url = "http://foo.remote.com/";
+  // First setup Sync to have the page as a remote tab.
+  configureEngine({
+    guid_mobile: {
+      id: "mobile",
+      tabs: Array(5).fill(0).map((e, i) => ({
+        urlHistory: [`${url}${i}`],
+        title: "A title",
+        lastUsed: (Date.now() / 1000) - (i * 86400) // i days ago.
+      })),
+    }
+  });
+
+  // Also add a local history result.
+  let historyUrl = url + "history/";
+  await PlacesTestUtils.addVisits(historyUrl);
+
+  await check_autocomplete({
+    search: "rem",
+    searchParam: "enable-actions",
+    checkSorting: true,
+    matches: [
+      makeSearchMatch("rem", { heuristic: true }),
+      makeRemoteTabMatch("http://foo.remote.com/0", "My Phone",
+                        { title: "A title" }),
+      makeRemoteTabMatch("http://foo.remote.com/1", "My Phone",
+                        { title: "A title" }),
+      makeRemoteTabMatch("http://foo.remote.com/2", "My Phone",
+                        { title: "A title" }),
+      { uri: Services.io.newURI(historyUrl),
+        title: "test visit for " + historyUrl },
+      makeRemoteTabMatch("http://foo.remote.com/3", "My Phone",
+                        { title: "A title" }),
+      makeRemoteTabMatch("http://foo.remote.com/4", "My Phone",
+                        { title: "A title" }),
+    ],
+  });
+});