Bug 1173359 - Exclude hidden pages and framed links from syncing. r=markh

MozReview-Commit-ID: 8I4Sulyr0H7

--HG--
extra : rebase_source : 55fb1ff567c0a2916bc0153a09c41b03d396cfeb

services/sync/modules/constants.js

@@ -192,6 +192,8 @@ MIN_PASS_LENGTH:                       8,
 DEVICE_TYPE_DESKTOP:                   "desktop",
 DEVICE_TYPE_MOBILE:                    "mobile",
 
+SQLITE_MAX_VARIABLE_NUMBER:            999,
+
 })) {
   this[key] = val;
   this.EXPORTED_SYMBOLS.push(key);

services/sync/modules/engines.js

@@ -161,19 +161,25 @@ Tracker.prototype = {
     return true;
   },
 
-  removeChangedID: function (id) {
-    if (!id) {
-      this._log.warn("Attempted to remove undefined ID to tracker");
+  removeChangedID: function (...ids) {
+    if (!ids.length || this.ignoreAll) {
       return false;
     }
-    if (this.ignoreAll || this._ignored.includes(id)) {
-      return false;
-    }
-    if (this.changedIDs[id] != null) {
-      this._log.trace("Removing changed ID " + id);
-      delete this.changedIDs[id];
-      this.saveChangedIDs();
+    for (let id of ids) {
+      if (!id) {
+        this._log.warn("Attempted to remove undefined ID from tracker");
+        continue;
+      }
+      if (this._ignored.includes(id)) {
+        this._log.debug(`Not removing ignored ID ${id} from tracker`);
+        continue;
+      }
+      if (this.changedIDs[id] != null) {
+        this._log.trace("Removing changed ID " + id);
+        delete this.changedIDs[id];
+      }
     }
+    this.saveChangedIDs();
     return true;
   },
 

services/sync/modules/engines/history.js

@@ -64,6 +64,50 @@ HistoryEngine.prototype = {
       notifyHistoryObservers("onEndUpdateBatch");
     }
   },
+
+  pullNewChanges() {
+    let modifiedGUIDs = Object.keys(this._tracker.changedIDs);
+    if (!modifiedGUIDs.length) {
+      return new Changeset();
+    }
+
+    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
+                        .DBConnection;
+
+    // Filter out hidden pages and `TRANSITION_FRAMED_LINK` visits. These are
+    // excluded when rendering the history menu, so we use the same constraints
+    // for Sync. We also don't want to sync `TRANSITION_EMBED` visits, but those
+    // aren't stored in the database.
+    for (let startIndex = 0;
+         startIndex < modifiedGUIDs.length;
+         startIndex += SQLITE_MAX_VARIABLE_NUMBER) {
+
+      let chunkLength = Math.min(SQLITE_MAX_VARIABLE_NUMBER,
+                                 modifiedGUIDs.length - startIndex);
+
+      let query = `
+        SELECT DISTINCT p.guid FROM moz_places p
+        JOIN moz_historyvisits v ON p.id = v.place_id
+        WHERE p.guid IN (${new Array(chunkLength).fill("?").join(",")}) AND
+              (p.hidden = 1 OR v.visit_type IN (0,
+                ${PlacesUtils.history.TRANSITION_FRAMED_LINK}))
+      `;
+
+      let statement = db.createAsyncStatement(query);
+      try {
+        for (let i = 0; i < chunkLength; i++) {
+          statement.bindByIndex(i, modifiedGUIDs[startIndex + i]);
+        }
+        let results = Async.querySpinningly(statement, ["guid"]);
+        let guids = results.map(result => result.guid);
+        this._tracker.removeChangedID(...guids);
+      } finally {
+        statement.finalize();
+      }
+    }
+
+    return new Changeset(this._tracker.changedIDs);
+  },
 };
 
 function HistoryStore(name, engine) {
@@ -72,7 +116,7 @@ function HistoryStore(name, engine) {
   // Explicitly nullify our references to our cached services so we don't leak
   Svc.Obs.add("places-shutdown", function() {
     for (let query in this._stmts) {
-      let stmt = this._stmts;
+      let stmt = this._stmts[query];
       stmt.finalize();
     }
     this._stmts = {};
@@ -165,12 +209,18 @@ HistoryStore.prototype = {
   _urlCols: ["url", "title", "frecency"],
 
   get _allUrlStm() {
-    return this._getStmt(
-      "SELECT url " +
-      "FROM moz_places " +
-      "WHERE last_visit_date > :cutoff_date " +
-      "ORDER BY frecency DESC " +
-      "LIMIT :max_results");
+    // Filter out hidden pages and framed link visits. See `pullNewChanges`
+    // for more info.
+    return this._getStmt(`
+      SELECT DISTINCT p.url
+      FROM moz_places p
+      JOIN moz_historyvisits v ON p.id = v.place_id
+      WHERE p.last_visit_date > :cutoff_date AND
+            p.hidden = 0 AND
+            v.visit_type NOT IN (0,
+              ${PlacesUtils.history.TRANSITION_FRAMED_LINK})
+      ORDER BY frecency DESC
+      LIMIT :max_results`);
   },
   _allUrlCols: ["url"],
 

services/sync/tests/unit/test_history_tracker.js

@@ -1,6 +1,7 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
+Cu.import("resource://gre/modules/PlacesDBUtils.jsm");
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://services-sync/engines.js");
@@ -17,7 +18,7 @@ var tracker = engine._tracker;
 // Don't write out by default.
 tracker.persistChangedIDs = false;
 
-async function addVisit(suffix) {
+async function addVisit(suffix, referrer = null, transition = PlacesUtils.history.TRANSITION_LINK) {
   let uriString = "http://getfirefox.com/" + suffix;
   let uri = Utils.makeURI(uriString);
   _("Adding visit for URI " + uriString);
@@ -25,7 +26,8 @@ async function addVisit(suffix) {
   await PlacesTestUtils.addVisits({
     uri,
     visitDate: Date.now() * 1000,
-    transition: PlacesUtils.history.TRANSITION_LINK,
+    transition,
+    referrer,
   });
 
   return uri;
@@ -216,3 +218,34 @@ add_task(async function test_stop_tracking_twice() {
 
   await cleanup();
 });
+
+add_task(async function test_filter_hidden() {
+  await startTracking();
+
+  _("Add visit; should be hidden by the redirect");
+  let hiddenURI = await addVisit("hidden");
+  let hiddenGUID = engine._store.GUIDForUri(hiddenURI);
+  _(`Hidden visit GUID: ${hiddenGUID}`);
+
+  _("Add redirect visit; should be tracked");
+  let trackedURI = await addVisit("redirect", hiddenURI,
+    PlacesUtils.history.TRANSITION_REDIRECT_PERMANENT);
+  let trackedGUID = engine._store.GUIDForUri(trackedURI);
+  _(`Tracked visit GUID: ${trackedGUID}`);
+
+  _("Add visit for framed link; should be ignored");
+  let embedURI = await addVisit("framed_link", null,
+    PlacesUtils.history.TRANSITION_FRAMED_LINK);
+  let embedGUID = engine._store.GUIDForUri(embedURI);
+  _(`Framed link visit GUID: ${embedGUID}`);
+
+  _("Run Places maintenance to mark redirect visit as hidden");
+  let maintenanceFinishedPromise =
+    promiseOneObserver("places-maintenance-finished");
+  PlacesDBUtils.maintenanceOnIdle();
+  await maintenanceFinishedPromise;
+
+  await verifyTrackedItems([trackedGUID]);
+
+  await cleanup();
+});