Bug 1285577 - part 3: keep track of added visits in an import and allow removing them, r=mak draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 30 Nov 2016 11:56:25 +0000
changeset 451738 711f3575093b84bf6dfb18e7750f49c58858ebbc
parent 451737 fc36e4a137e6eef890091fb46e5f149a3cc1c46f
child 451739 e330deab2d1e2601fb9c435f3c6a5ae1d5d5cfc4
push id39276
push usergijskruitbosch@gmail.com
push dateTue, 20 Dec 2016 22:50:39 +0000
reviewersmak
bugs1285577
milestone53.0a1
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them, r=mak MozReview-Commit-ID: 7HCA7cKhws4
browser/components/migration/AutoMigrate.jsm
browser/components/migration/MigrationUtils.jsm
browser/components/migration/tests/unit/head_migration.js
browser/components/migration/tests/unit/test_automigration.js
--- a/browser/components/migration/AutoMigrate.jsm
+++ b/browser/components/migration/AutoMigrate.jsm
@@ -33,16 +33,18 @@ Cu.import("resource://gre/modules/Prefer
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
                                   "resource://gre/modules/LoginHelper.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 
+Cu.importGlobalProperties(["URL"]);
+
 const AutoMigrate = {
   get resourceTypesToUse() {
     let {BOOKMARKS, HISTORY, PASSWORDS} = Ci.nsIBrowserProfileMigrator;
     return BOOKMARKS | HISTORY | PASSWORDS;
   },
 
   _checkIfEnabled() {
     let pref = Preferences.get(kAutoMigrateEnabledPref, false);
@@ -448,14 +450,31 @@ const AutoMigrate = {
         foundLogin.QueryInterface(Ci.nsILoginMetaInfo);
         if (foundLogin.timePasswordChanged == login.timePasswordChanged) {
           Services.logins.removeLogin(foundLogin);
         }
       }
     }
   }),
 
+  _removeSomeVisits: Task.async(function* (visits) {
+    for (let urlVisits of visits) {
+      let urlObj;
+      try {
+        urlObj = new URL(urlVisits.url);
+      } catch (ex) {
+        continue;
+      }
+      yield PlacesUtils.history.removeVisitsByFilter({
+        url: urlObj,
+        beginDate: PlacesUtils.toDate(urlVisits.first),
+        endDate: PlacesUtils.toDate(urlVisits.last),
+        limit: urlVisits.visitCount,
+      });
+    }
+  }),
+
   QueryInterface: XPCOMUtils.generateQI(
     [Ci.nsIObserver, Ci.nsINavBookmarkObserver, Ci.nsISupportsWeakReference]
   ),
 };
 
 AutoMigrate.init();
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -11,16 +11,18 @@ const TOPIC_WILL_IMPORT_BOOKMARKS = "ini
 const TOPIC_DID_IMPORT_BOOKMARKS = "initial-migration-did-import-default-bookmarks";
 const TOPIC_PLACES_DEFAULTS_FINISHED = "places-browser-init-complete";
 
 Cu.import("resource://gre/modules/AppConstants.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
+Cu.importGlobalProperties(["URL"]);
+
 XPCOMUtils.defineLazyModuleGetter(this, "AutoMigrate",
                                   "resource:///modules/AutoMigrate.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "BookmarkHTMLUtils",
                                   "resource://gre/modules/BookmarkHTMLUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
                                   "resource://gre/modules/LoginHelper.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
@@ -964,16 +966,19 @@ this.MigrationUtils = Object.freeze({
         parentGuid, guid, lastModified, type
       });
       return bm;
     });
   },
 
   insertVisitsWrapper(places, options) {
     this._importQuantities.history += places.length;
+    if (gKeepUndoData) {
+      this._updateHistoryUndo(places);
+    }
     return PlacesUtils.asyncHistory.updatePlaces(places, options);
   },
 
   insertLoginWrapper(login) {
     this._importQuantities.logins++;
     let insertedLogin = LoginHelper.maybeImportLogin(login);
     // Note that this means that if we import a login that has a newer password
     // than we know about, we will update the login, and an undo of the import
@@ -982,17 +987,17 @@ this.MigrationUtils = Object.freeze({
     if (insertedLogin && gKeepUndoData) {
       let {guid, timePasswordChanged} = insertedLogin;
       gUndoData.get("logins").push({guid, timePasswordChanged});
     }
   },
 
   initializeUndoData() {
     gKeepUndoData = true;
-    gUndoData = new Map([["bookmarks", []], ["visits", new Map()], ["logins", []]]);
+    gUndoData = new Map([["bookmarks", []], ["visits", []], ["logins", []]]);
   },
 
   _postProcessUndoData: Task.async(function*(state) {
     if (!state) {
       return state;
     }
     let bookmarkFolders = state.get("bookmarks").filter(b => b.type == PlacesUtils.bookmarks.TYPE_FOLDER);
 
@@ -1017,16 +1022,42 @@ this.MigrationUtils = Object.freeze({
 
   stopAndRetrieveUndoData() {
     let undoData = gUndoData;
     gUndoData = null;
     gKeepUndoData = false;
     return this._postProcessUndoData(undoData);
   },
 
+  _updateHistoryUndo(places) {
+    let visits = gUndoData.get("visits");
+    let visitMap = new Map(visits.map(v => [v.url, v]));
+    for (let place of places) {
+      let visitCount = place.visits.length;
+      let first = Math.min.apply(Math, place.visits.map(v => v.visitDate));
+      let last = Math.max.apply(Math, place.visits.map(v => v.visitDate));
+      let url = place.uri.spec;
+      try {
+        new URL(url);
+      } catch (ex) {
+        // This won't save and we won't need to 'undo' it, so ignore this URL.
+        continue;
+      }
+      if (!visitMap.has(url)) {
+        visitMap.set(url, {url, visitCount, first, last});
+      } else {
+        let currentData = visitMap.get(url);
+        currentData.visitCount += visitCount;
+        currentData.first = Math.min(currentData.first, first);
+        currentData.last = Math.max(currentData.last, last);
+      }
+    }
+    gUndoData.set("visits", Array.from(visitMap.values()));
+  },
+
   /**
    * Cleans up references to migrators and nsIProfileInstance instances.
    */
   finishMigration: function MU_finishMigration() {
     gMigrators = null;
     gProfileStartup = null;
     gMigrationBundle = null;
   },
--- a/browser/components/migration/tests/unit/head_migration.js
+++ b/browser/components/migration/tests/unit/head_migration.js
@@ -1,25 +1,30 @@
 "use strict";
 
 /* exported gProfD, promiseMigration, registerFakePath */
 
 var { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
 
 Cu.importGlobalProperties([ "URL" ]);
 
+Cu.import("resource:///modules/MigrationUtils.jsm");
+Cu.import("resource://gre/modules/LoginHelper.jsm");
+Cu.import("resource://gre/modules/NetUtil.jsm");
+Cu.import("resource://gre/modules/PlacesUtils.jsm");
+Cu.import("resource://gre/modules/Preferences.jsm");
+Cu.import("resource://gre/modules/PromiseUtils.jsm");
+Cu.import("resource://gre/modules/Task.jsm");
+Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
-Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://testing-common/TestUtils.jsm");
+Cu.import("resource://testing-common/PlacesTestUtils.jsm");
 
-XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
-                                  "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "FileUtils",
                                   "resource://gre/modules/FileUtils.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "MigrationUtils",
-                                  "resource:///modules/MigrationUtils.jsm");
 
 // Initialize profile.
 var gProfD = do_get_profile();
 
 Cu.import("resource://testing-common/AppInfo.jsm"); /* globals updateAppInfo */
 updateAppInfo();
 
 /**
--- a/browser/components/migration/tests/unit/test_automigration.js
+++ b/browser/components/migration/tests/unit/test_automigration.js
@@ -1,16 +1,10 @@
 "use strict";
 
-Cu.import("resource:///modules/MigrationUtils.jsm");
-Cu.import("resource://gre/modules/LoginHelper.jsm");
-Cu.import("resource://gre/modules/PlacesUtils.jsm");
-Cu.import("resource://gre/modules/Preferences.jsm");
-Cu.import("resource://testing-common/TestUtils.jsm");
-Cu.import("resource://testing-common/PlacesTestUtils.jsm");
 let AutoMigrateBackstage = Cu.import("resource:///modules/AutoMigrate.jsm"); /* globals AutoMigrate */
 
 let gShimmedMigratorKeyPicker = null;
 let gShimmedMigrator = null;
 
 const kUsecPerMin = 60 * 1000000;
 
 // This is really a proxy on MigrationUtils, but if we specify that directly,
@@ -28,16 +22,32 @@ AutoMigrateBackstage.MigrationUtils = ne
     return MigrationUtils[name];
   },
 });
 
 do_register_cleanup(function() {
   AutoMigrateBackstage.MigrationUtils = MigrationUtils;
 });
 
+// This should be replaced by using History.fetch with a fetchVisits option,
+// once that becomes available
+function* visitsForURL(url)
+{
+  let visitCount = 0;
+  let db = yield PlacesUtils.promiseDBConnection();
+  visitCount = yield db.execute(
+    `SELECT count(*) FROM moz_historyvisits v
+     JOIN moz_places h ON h.id = v.place_id
+     WHERE url_hash = hash(:url) AND url = :url`,
+     {url});
+  visitCount = visitCount[0].getInt64(0);
+  return visitCount;
+}
+
+
 /**
  * Test automatically picking a browser to migrate from
  */
 add_task(function* checkMigratorPicking() {
   Assert.throws(() => AutoMigrate.pickMigrator("firefox"),
                 /Can't automatically migrate from Firefox/,
                 "Should throw when explicitly picking Firefox.");
 
@@ -435,8 +445,195 @@ add_task(function* testLoginsRemovalByUn
   yield AutoMigrate._removeUnchangedLogins(undoLoginData);
   Assert.equal(0, LoginHelper.searchLoginsWithObject({hostname: "https://example.com", formSubmitURL: "https://example.com/"}).length,
                    "unchanged example.com entry should have been removed.");
   Assert.equal(1, LoginHelper.searchLoginsWithObject({hostname: "https://example.org", formSubmitURL: "https://example.org/"}).length,
                    "changed example.org entry should have persisted.");
   Services.logins.removeAllLogins();
 });
 
+add_task(function* checkUndoVisitsState() {
+  MigrationUtils.initializeUndoData();
+  yield MigrationUtils.insertVisitsWrapper([{
+    uri: NetUtil.newURI("http://www.example.com/"),
+    title: "Example",
+    visits: [{
+      visitDate: new Date("2015-07-10").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }, {
+      visitDate: new Date("2015-09-10").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }, {
+      visitDate: new Date("2015-08-10").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }],
+  }, {
+    uri: NetUtil.newURI("http://www.example.org/"),
+    title: "Example",
+    visits: [{
+      visitDate: new Date("2016-04-03").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }, {
+      visitDate: new Date("2015-08-03").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }],
+  }, {
+    uri: NetUtil.newURI("http://www.example.com/"),
+    title: "Example",
+    visits: [{
+      visitDate: new Date("2015-10-10").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }],
+  }]);
+  let undoVisitData = (yield MigrationUtils.stopAndRetrieveUndoData()).get("visits");
+  Assert.deepEqual(Array.from(undoVisitData.map(v => v.url)).sort(),
+                   ["http://www.example.com/", "http://www.example.org/"]);
+  Assert.deepEqual(undoVisitData.find(v => v.url == "http://www.example.com/"), {
+    url: "http://www.example.com/",
+    visitCount: 4,
+    first: new Date("2015-07-10").getTime() * 1000,
+    last: new Date("2015-10-10").getTime() * 1000,
+  });
+  Assert.deepEqual(undoVisitData.find(v => v.url == "http://www.example.org/"), {
+    url: "http://www.example.org/",
+    visitCount: 2,
+    first: new Date("2015-08-03").getTime() * 1000,
+    last: new Date("2016-04-03").getTime() * 1000,
+  });
+
+  yield PlacesTestUtils.clearHistory();
+});
+
+add_task(function* checkUndoVisitsState() {
+  MigrationUtils.initializeUndoData();
+  yield MigrationUtils.insertVisitsWrapper([{
+    uri: NetUtil.newURI("http://www.example.com/"),
+    title: "Example",
+    visits: [{
+      visitDate: new Date("2015-07-10").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }, {
+      visitDate: new Date("2015-09-10").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }, {
+      visitDate: new Date("2015-08-10").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }],
+  }, {
+    uri: NetUtil.newURI("http://www.example.org/"),
+    title: "Example",
+    visits: [{
+      visitDate: new Date("2016-04-03").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }, {
+      visitDate: new Date("2015-08-03").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }],
+  }, {
+    uri: NetUtil.newURI("http://www.example.com/"),
+    title: "Example",
+    visits: [{
+      visitDate: new Date("2015-10-10").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }],
+  }, {
+    uri: NetUtil.newURI("http://www.mozilla.org/"),
+    title: "Example",
+    visits: [{
+      visitDate: new Date("2015-01-01").getTime() * 1000,
+      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
+    }],
+  }]);
+
+  // We have to wait until frecency updates have been handled in order
+  // to accurately determine whether we're doing the right thing.
+  let frecencyUpdatesHandled = new Promise(resolve => {
+    PlacesUtils.history.addObserver({
+      onFrecencyChanged(aURI) {
+        if (aURI.spec == "http://www.unrelated.org/") {
+          PlacesUtils.history.removeObserver(this);
+          resolve();
+        }
+      }
+    }, false);
+  });
+  yield PlacesUtils.history.insertMany([{
+    url: "http://www.example.com/",
+    title: "Example",
+    visits: [{
+      date: new Date("2015-08-16"),
+    }],
+  }, {
+    url: "http://www.example.org/",
+    title: "Example",
+    visits: [{
+      date: new Date("2016-01-03"),
+    }, {
+      date: new Date("2015-05-03"),
+    }],
+  }, {
+    url: "http://www.unrelated.org/",
+    title: "Unrelated",
+    visits: [{
+      date: new Date("2015-09-01"),
+    }],
+  }]);
+  yield frecencyUpdatesHandled;
+  let undoVisitData = (yield MigrationUtils.stopAndRetrieveUndoData()).get("visits");
+
+  let frecencyChangesExpected = new Map([
+    ["http://www.example.com/", PromiseUtils.defer()],
+    ["http://www.example.org/", PromiseUtils.defer()]
+  ]);
+  let uriDeletedExpected = new Map([
+    ["http://www.mozilla.org/", PromiseUtils.defer()],
+  ]);
+  let wrongMethodDeferred = PromiseUtils.defer();
+  let observer = {
+    onBeginUpdateBatch: function() {},
+    onEndUpdateBatch: function() {},
+    onVisit: function(uri) {
+      wrongMethodDeferred.reject(new Error("Unexpected call to onVisit " + uri.spec));
+    },
+    onTitleChanged: function(uri) {
+      wrongMethodDeferred.reject(new Error("Unexpected call to onTitleChanged " + uri.spec));
+    },
+    onClearHistory: function() {
+      wrongMethodDeferred.reject("Unexpected call to onClearHistory");
+    },
+    onPageChanged: function(uri) {
+      wrongMethodDeferred.reject(new Error("Unexpected call to onPageChanged " + uri.spec));
+    },
+    onFrecencyChanged: function(aURI) {
+      do_print("frecency change");
+      Assert.ok(frecencyChangesExpected.has(aURI.spec),
+                "Should be expecting frecency change for " + aURI.spec);
+      frecencyChangesExpected.get(aURI.spec).resolve();
+    },
+    onManyFrecenciesChanged: function() {
+      do_print("Many frecencies changed");
+      wrongMethodDeferred.reject(new Error("This test can't deal with onManyFrecenciesChanged to be called"));
+    },
+    onDeleteURI: function(aURI) {
+      do_print("delete uri");
+      Assert.ok(uriDeletedExpected.has(aURI.spec),
+                "Should be expecting uri deletion for " + aURI.spec);
+      uriDeletedExpected.get(aURI.spec).resolve();
+    },
+  };
+  PlacesUtils.history.addObserver(observer, false);
+
+  yield AutoMigrate._removeSomeVisits(undoVisitData);
+  PlacesUtils.history.removeObserver(observer);
+  yield Promise.all(uriDeletedExpected.values());
+  yield Promise.all(frecencyChangesExpected.values());
+
+  Assert.equal(yield visitsForURL("http://www.example.com/"), 1,
+               "1 example.com visit (out of 5) should have persisted despite being within the range, due to limiting");
+  Assert.equal(yield visitsForURL("http://www.mozilla.org/"), 0,
+               "0 mozilla.org visits should have persisted (out of 1).");
+  Assert.equal(yield visitsForURL("http://www.example.org/"), 2,
+               "2 example.org visits should have persisted (out of 4).");
+  Assert.equal(yield visitsForURL("http://www.unrelated.org/"), 1,
+               "1 unrelated.org visits should have persisted as it's not involved in the import.");
+  yield PlacesTestUtils.clearHistory();
+});
+