Bug 1355663 - Don't de-duplicate tabs from multiple devices. r?eoger draft
authortiago <tiago.paez11@gmail.com>
Fri, 12 May 2017 20:54:54 -0300
changeset 577162 dedebc7a2008f27eab4c1092f82bd7776044223b
parent 577084 96b36c5f527dd42e680a230839519eee1fc2c9f3
child 628447 5e8133484882a4a87c7748fe2701a7aac7e80d8c
push id58634
push userbmo:tiago.paez11@gmail.com
push dateFri, 12 May 2017 23:55:08 +0000
reviewerseoger
bugs1355663
milestone55.0a1
Bug 1355663 - Don't de-duplicate tabs from multiple devices. r?eoger MozReview-Commit-ID: AciCTECtXJc
services/sync/modules/SyncedTabs.jsm
services/sync/tests/unit/test_syncedtabs.js
--- a/services/sync/modules/SyncedTabs.jsm
+++ b/services/sync/modules/SyncedTabs.jsm
@@ -105,47 +105,36 @@ let SyncedTabsInternal = {
       return result;
     }
 
     // A boolean that controls whether we should show the icon from the remote tab.
     const showRemoteIcons = Preferences.get("services.sync.syncedTabs.showRemoteIcons", true);
 
     let engine = Weave.Service.engineManager.get("tabs");
 
-    let seenURLs = new Set();
     let ntabs = 0;
 
     for (let client of Object.values(engine.getAllClients())) {
       if (!Weave.Service.clientsEngine.remoteClientExists(client.id)) {
         continue;
       }
       let clientRepr = await this._makeClient(client);
       log.debug("Processing client", clientRepr);
 
       for (let tab of client.tabs) {
         let url = tab.urlHistory[0];
         log.debug("remote tab", url);
-        // Note there are some issues with tracking "seen" tabs, including:
-        // * We really can't return the entire urlHistory record as we are
-        //   only checking the first entry - others might be different.
-        // * We don't update the |lastUsed| timestamp to reflect the
-        //   most-recently-seen time.
-        // In a followup we should consider simply dropping this |seenUrls|
-        // check and return duplicate records - it seems the user will be more
-        // confused by tabs not showing up on a device (because it was detected
-        // as a dupe so it only appears on a different device) than being
-        // confused by seeing the same tab on different clients.
-        if (!url || seenURLs.has(url)) {
+
+        if (!url) {
           continue;
         }
         let tabRepr = await this._makeTab(client, tab, url, showRemoteIcons);
         if (filter && !this._tabMatchesFilter(tabRepr, filter)) {
           continue;
         }
-        seenURLs.add(url);
         clientRepr.tabs.push(tabRepr);
       }
       // We return all clients, even those without tabs - the consumer should
       // filter it if they care.
       ntabs += clientRepr.tabs.length;
       result.push(clientRepr);
     }
     log.info(`Final tab list has ${result.length} clients with ${ntabs} tabs.`);
--- a/services/sync/tests/unit/test_syncedtabs.js
+++ b/services/sync/tests/unit/test_syncedtabs.js
@@ -212,8 +212,37 @@ add_task(async function test_filter() {
   equal(clients[0].tabs.length, 1);
   equal(clients[0].tabs[0].url, "http://foo.com/");
   // check it matches the title.
   clients = await SyncedTabs.getTabClients("test");
   equal(clients.length, 1);
   equal(clients[0].tabs.length, 1);
   equal(clients[0].tabs[0].url, "http://foo.com/");
 });
+
+add_task(async function test_duplicatesTabsAcrossClients() {
+
+  await configureClients({
+    guid_desktop: {
+      clientName: "My Desktop",
+      tabs: [
+      {
+        urlHistory: ["http://foo.com/"],
+        title: "A test page.",
+      }],
+    },
+    guid_mobile: {
+      clientName: "My Phone",
+      tabs: [
+      {
+          urlHistory: ["http://foo.com/"],
+          title: "A test page.",
+      }],
+    },
+  });
+
+  let clients = await SyncedTabs.getTabClients();
+  equal(clients.length, 2);
+  equal(clients[0].tabs.length, 1);
+  equal(clients[1].tabs.length, 1);
+  equal(clients[0].tabs[0].url, "http://foo.com/");
+  equal(clients[1].tabs[0].url, "http://foo.com/");
+});