Bug 1173359 - Exclude hidden pages and framed links from syncing. r=markh
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 22 Nov 2016 12:52:09 -0700
changeset 324206 4c39528f81a95acf65bab0825e03e825dcd53539
parent 324205 c55ce97752965e67ce78fa6ddefbb23f13168da0
child 324207 99c765086bf4e191a83919dda49874c18eac397a
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersmarkh
bugs1173359
milestone53.0a1
Bug 1173359 - Exclude hidden pages and framed links from syncing. r=markh MozReview-Commit-ID: 8I4Sulyr0H7
services/sync/modules/constants.js
services/sync/modules/engines.js
services/sync/modules/engines/history.js
services/sync/tests/unit/test_history_tracker.js
--- a/services/sync/modules/constants.js
+++ b/services/sync/modules/constants.js
@@ -187,12 +187,14 @@ SEAMONKEY_ID:                          "
 TEST_HARNESS_ID:                       "xuth@mozilla.org",
 
 MIN_PP_LENGTH:                         12,
 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);
 }
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -156,29 +156,35 @@ Tracker.prototype = {
     // Add/update the entry if we have a newer time.
     if ((this.changedIDs[id] || -Infinity) < when) {
       this._saveChangedID(id, when);
     }
 
     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;
+    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];
+      }
     }
-    if (this.changedIDs[id] != null) {
-      this._log.trace("Removing changed ID " + id);
-      delete this.changedIDs[id];
-      this.saveChangedIDs();
-    }
+    this.saveChangedIDs();
     return true;
   },
 
   clearChangedIDs: function () {
     this._log.trace("Clearing changed ID list");
     this.changedIDs = {};
     this.saveChangedIDs();
   },
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -59,25 +59,69 @@ HistoryEngine.prototype = {
     }
     notifyHistoryObservers("onBeginUpdateBatch");
     try {
       return SyncEngine.prototype._processIncoming.call(this, newitems);
     } finally {
       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) {
   Store.call(this, 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 = {};
   }, this);
 }
 HistoryStore.prototype = {
   __proto__: Store.prototype,
 
@@ -160,22 +204,28 @@ HistoryStore.prototype = {
     return this._getStmt(
       "SELECT url, title, frecency " +
       "FROM moz_places " +
       "WHERE guid = :guid");
   },
   _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"],
 
   // See bug 320831 for why we use SQL here
   _getVisits: function HistStore__getVisits(uri) {
     this._visitStm.params.url = uri;
     return Async.querySpinningly(this._visitStm, this._visitCols);
   },
--- a/services/sync/tests/unit/test_history_tracker.js
+++ b/services/sync/tests/unit/test_history_tracker.js
@@ -1,36 +1,38 @@
 /* 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");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines/history.js");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/util.js");
 
 Service.engineManager.clear();
 Service.engineManager.register(HistoryEngine);
 var engine = Service.engineManager.get("history");
 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);
 
   await PlacesTestUtils.addVisits({
     uri,
     visitDate: Date.now() * 1000,
-    transition: PlacesUtils.history.TRANSITION_LINK,
+    transition,
+    referrer,
   });
 
   return uri;
 }
 
 function run_test() {
   initTestLogging("Trace");
   Log.repository.getLogger("Sync.Tracker.History").level = Log.Level.Trace;
@@ -211,8 +213,39 @@ add_task(async function test_stop_tracki
 
   _("Notifying twice won't do any harm.");
   await stopTracking();
   await addVisit("stop_tracking_twice2");
   await verifyTrackerEmpty();
 
   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();
+});