Bug 878677 - Part 2: Sync changes to support FormHistory.jsm. r=rnewman

services/sync/modules/engines/forms.js

@@ -33,120 +33,63 @@ Utils.deferGetSet(FormRec, "cleartext", ["name", "value"]);
 let FormWrapper = {
   _log: Log4Moz.repository.getLogger("Sync.Engine.Forms"),
 
-  _getEntryCols: ["name", "value"],
+  _getEntryCols: ["fieldname", "value"],
   _guidCols:     ["guid"],
 
-  _stmts: {},
+  // Do a "sync" search by spinning the event loop until it completes.
-  _getStmt: function _getStmt(query) {
+  _searchSpinningly: function(terms, searchData) {
-    if (query in this._stmts) {
+    let results = [];
-      return this._stmts[query];
+    let cb = Async.makeSpinningCallback();
-    }
+    let callbacks = {
-
+      handleResult: function(result) {
-    this._log.trace("Creating SQL statement: " + query);
+        results.push(result);
-    let db = Svc.Form.DBConnection;
+      },
-    return this._stmts[query] = db.createAsyncStatement(query);
+      handleCompletion: function(reason) {
+        cb(null, results);
+      }
+    };
+    Svc.FormHistory.search(terms, searchData, callbacks);
+    return cb.wait();
   },
 
-  _finalize : function () {
+  _updateSpinningly: function(changes) {
-    for each (let stmt in FormWrapper._stmts) {
+    let cb = Async.makeSpinningCallback();
-      stmt.finalize();
+    let callbacks = {
-    }
+      handleCompletion: function(reason) {
-    FormWrapper._stmts = {};
+        cb();
+      }
+    };
+    Svc.FormHistory.update(changes, callbacks);
+    return cb.wait();
   },
 
-  get _getAllEntriesStmt() {
+  getEntry: function (guid) {
-    const query =
+    let results = this._searchSpinningly(this._getEntryCols, {guid: guid});
-      "SELECT fieldname name, value FROM moz_formhistory " +
+    if (!results.length) {
-      "ORDER BY 1.0 * (lastUsed - (SELECT lastUsed FROM moz_formhistory ORDER BY lastUsed ASC LIMIT 1)) / " +
-        "((SELECT lastUsed FROM moz_formhistory ORDER BY lastUsed DESC LIMIT 1) - (SELECT lastUsed FROM moz_formhistory ORDER BY lastUsed ASC LIMIT 1)) * " +
-        "timesUsed / (SELECT timesUsed FROM moz_formhistory ORDER BY timesUsed DESC LIMIT 1) DESC " +
-      "LIMIT 500";
-    return this._getStmt(query);
-  },
-
-  get _getEntryStmt() {
-    const query = "SELECT fieldname name, value FROM moz_formhistory " +
-                  "WHERE guid = :guid";
-    return this._getStmt(query);
-  },
-
-  get _getGUIDStmt() {
-    const query = "SELECT guid FROM moz_formhistory " +
-                  "WHERE fieldname = :name AND value = :value";
-    return this._getStmt(query);
-  },
-
-  get _setGUIDStmt() {
-    const query = "UPDATE moz_formhistory SET guid = :guid " +
-                  "WHERE fieldname = :name AND value = :value";
-    return this._getStmt(query);
-  },
-
-  get _hasGUIDStmt() {
-    const query = "SELECT guid FROM moz_formhistory WHERE guid = :guid LIMIT 1";
-    return this._getStmt(query);
-  },
-
-  get _replaceGUIDStmt() {
-    const query = "UPDATE moz_formhistory SET guid = :newGUID " +
-                  "WHERE guid = :oldGUID";
-    return this._getStmt(query);
-  },
-
-  getAllEntries: function getAllEntries() {
-    return Async.querySpinningly(this._getAllEntriesStmt, this._getEntryCols);
-  },
-
-  getEntry: function getEntry(guid) {
-    let stmt = this._getEntryStmt;
-    stmt.params.guid = guid;
-    return Async.querySpinningly(stmt, this._getEntryCols)[0];
-  },
-
-  getGUID: function getGUID(name, value) {
-    // Query for the provided entry.
-    let getStmt = this._getGUIDStmt;
-    getStmt.params.name = name;
-    getStmt.params.value = value;
-
-    // Give the GUID if we found one.
-    let item = Async.querySpinningly(getStmt, this._guidCols)[0];
-
-    if (!item) {
-      // Shouldn't happen, but Bug 597400...
-      // Might as well just return.
-      this._log.warn("GUID query returned " + item + "; turn on Trace logging for details.");
-      this._log.trace("getGUID(" + JSON.stringify(name) + ", " +
-                      JSON.stringify(value) + ") => " + item);
       return null;
     }
+    return {name: results[0].fieldname, value: results[0].value};
+  },
 
-    if (item.guid != null) {
+  getGUID: function (name, value) {
-      return item.guid;
+    // Query for the provided entry.
+    let query = { fieldname: name, value: value };
+    let results = this._searchSpinningly(this._guidCols, query);
+    return results.length ? results[0].guid : null;
+  },
+
+  hasGUID: function (guid) {
+    // We could probably use a count function here, but searchSpinningly exists...
+    return this._searchSpinningly(this._guidCols, {guid: guid}).length != 0;
+  },
+
+  replaceGUID: function (oldGUID, newGUID) {
+    let changes = {
+      op: "update",
+      guid: oldGUID,
+      newGuid: newGUID,
     }
-
+    this._updateSpinningly(changes);
-    // We need to create a GUID for this entry.
-    let setStmt = this._setGUIDStmt;
-    let guid = Utils.makeGUID();
-    setStmt.params.guid = guid;
-    setStmt.params.name = name;
-    setStmt.params.value = value;
-    Async.querySpinningly(setStmt);
-
-    return guid;
-  },
-
-  hasGUID: function hasGUID(guid) {
-    let stmt = this._hasGUIDStmt;
-    stmt.params.guid = guid;
-    return Async.querySpinningly(stmt, this._guidCols).length == 1;
-  },
-
-  replaceGUID: function replaceGUID(oldGUID, newGUID) {
-    let stmt = this._replaceGUIDStmt;
-    stmt.params.oldGUID = oldGUID;
-    stmt.params.newGUID = newGUID;
-    Async.querySpinningly(stmt);
   }
 
 };
@@ -164,9 +107,7 @@ FormEngine.prototype = {
   get prefName() "history",
 
   _findDupe: function _findDupe(item) {
-    if (Svc.Form.entryExists(item.name, item.value)) {
+    return FormWrapper.getGUID(item.name, item.value);
-      return FormWrapper.getGUID(item.name, item.value);
-    }
   }
 };
 
@@ -176,34 +117,47 @@ function FormStore(name, engine) {
 FormStore.prototype = {
   __proto__: Store.prototype,
 
-  applyIncomingBatch: function applyIncomingBatch(records) {
+  _processChange: function (change) {
-    return Utils.runInTransaction(Svc.Form.DBConnection, function() {
+    // If this._changes is defined, then we are applying a batch, so we
-      return Store.prototype.applyIncomingBatch.call(this, records);
+    // can defer it.
-    }, this);
+    if (this._changes) {
+      this._changes.push(change);
+      return;
+    }
+
+    // Otherwise we must handle the change synchronously, right now.
+    FormWrapper._updateSpinningly(change);
   },
 
-  applyIncoming: function applyIncoming(record) {
+  applyIncomingBatch: function (records) {
-    Store.prototype.applyIncoming.call(this, record);
+    // We collect all the changes to be made then apply them all at once.
-    this._sleep(0); // Yield back to main thread after synchronous operation.
+    this._changes = [];
+    let failures = Store.prototype.applyIncomingBatch.call(this, records);
+    if (this._changes.length) {
+      FormWrapper._updateSpinningly(this._changes);
+    }
+    delete this._changes;
+    return failures;
   },
 
-  getAllIDs: function FormStore_getAllIDs() {
+  getAllIDs: function () {
+    let results = FormWrapper._searchSpinningly(["guid"], [])
     let guids = {};
-    for each (let {name, value} in FormWrapper.getAllEntries()) {
+    for (let result of results) {
-      guids[FormWrapper.getGUID(name, value)] = true;
+      guids[result.guid] = true;
     }
     return guids;
   },
 
-  changeItemID: function FormStore_changeItemID(oldID, newID) {
+  changeItemID: function (oldID, newID) {
     FormWrapper.replaceGUID(oldID, newID);
   },
 
-  itemExists: function FormStore_itemExists(id) {
+  itemExists: function (id) {
     return FormWrapper.hasGUID(id);
   },
 
-  createRecord: function createRecord(id, collection) {
+  createRecord: function (id, collection) {
     let record = new FormRec(collection, id);
     let entry = FormWrapper.getEntry(id);
     if (entry != null) {
@@ -215,29 +169,34 @@ FormStore.prototype = {
     return record;
   },
 
-  create: function FormStore_create(record) {
+  create: function (record) {
     this._log.trace("Adding form record for " + record.name);
-    Svc.Form.addEntry(record.name, record.value);
+    let change = {
+      op: "add",
+      fieldname: record.name,
+      value: record.value
+    };
+    this._processChange(change);
   },
 
-  remove: function FormStore_remove(record) {
+  remove: function (record) {
     this._log.trace("Removing form record: " + record.id);
-
+    let change = {
-    // Just skip remove requests for things already gone
+      op: "remove",
-    let entry = FormWrapper.getEntry(record.id);
+      guid: record.id
-    if (entry == null) {
+    };
-      return;
+    this._processChange(change);
-    }
-
-    Svc.Form.removeEntry(entry.name, entry.value);
   },
 
-  update: function FormStore_update(record) {
+  update: function (record) {
     this._log.trace("Ignoring form record update request!");
   },
 
-  wipe: function FormStore_wipe() {
+  wipe: function () {
-    Svc.Form.removeAllEntries();
+    let change = {
+      op: "remove"
+    };
+    FormWrapper._updateSpinningly(change);
   }
 };
 
@@ -255,13 +214,8 @@ FormTracker.prototype = {
     Ci.nsIObserver,
     Ci.nsISupportsWeakReference]),
 
-  trackEntry: function trackEntry(name, value) {
-    this.addChangedID(FormWrapper.getGUID(name, value));
-    this.score += SCORE_INCREMENT_MEDIUM;
-  },
-
   _enabled: false,
-  observe: function observe(subject, topic, data) {
+  observe: function (subject, topic, data) {
     switch (topic) {
       case "weave:engine:start-tracking":
         if (!this._enabled) {
@@ -287,13 +241,10 @@ FormTracker.prototype = {
         }
         break;
       case "satchel-storage-changed":
-        if (data == "addEntry" || data == "before-removeEntry") {
+        if (data == "formhistory-add" || data == "formhistory-remove") {
-          subject = subject.QueryInterface(Ci.nsIArray);
+          let guid = subject.QueryInterface(Ci.nsISupportsString).toString();
-          let name = subject.queryElementAt(0, Ci.nsISupportsString)
+          this.addChangedID(guid);
-                            .toString();
+          this.score += SCORE_INCREMENT_MEDIUM;
-          let value = subject.queryElementAt(1, Ci.nsISupportsString)
-                             .toString();
-          this.trackEntry(name, value);
         }
         break;
     case "profile-change-teardown":
@@ -302,7 +253,7 @@ FormTracker.prototype = {
     }
   },
 
-  notify: function FormTracker_notify(formElement, aWindow, actionURI) {
+  notify: function (formElement, aWindow, actionURI) {
     if (this.ignoreAll) {
       return;
     }

services/sync/modules/util.js

@@ -621,13 +621,15 @@ let _sessionCID = Services.appinfo.ID == SEAMONKEY_ID ?
   "@mozilla.org/suite/sessionstore;1" :
   "@mozilla.org/browser/sessionstore;1";
 
-[["Form", "@mozilla.org/satchel/form-history;1", "nsIFormHistory2"],
+[
  ["Idle", "@mozilla.org/widget/idleservice;1", "nsIIdleService"],
  ["Session", _sessionCID, "nsISessionStore"]
 ].forEach(function([name, contract, iface]) {
   XPCOMUtils.defineLazyServiceGetter(Svc, name, contract, iface);
 });
 
+XPCOMUtils.defineLazyModuleGetter(Svc, "FormHistory", "resource://gre/modules/FormHistory.jsm");
+
 Svc.__defineGetter__("Crypto", function() {
   let cryptoSvc;
   let ns = {};

services/sync/tests/unit/head_appinfo.js

@@ -3,10 +3,18 @@
 
 const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
 
+Cu.import("resource://gre/modules/Services.jsm");
+
 let gSyncProfile;
 
 gSyncProfile = do_get_profile();
 
+// Init FormHistoryStartup and pretend we opened a profile.
+let fhs = Cc["@mozilla.org/satchel/form-history-startup;1"]
+            .getService(Ci.nsIObserver);
+fhs.observe(null, "profile-after-change", null);
+
+
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 // Make sure to provide the right OS so crypto loads the right binaries

services/sync/tests/unit/test_forms_store.js

@@ -8,7 +8,8 @@ Cu.import("resource://services-sync/util.js");
 
 function run_test() {
   let baseuri = "http://fake/uri/";
-  let store = new FormEngine(Service)._store;
+  let engine = new FormEngine(Service);
+  let store = engine._store;
 
   function applyEnsureNoFailures(records) {
     do_check_eq(store.applyIncomingBatch(records).length, 0);
@@ -37,6 +38,9 @@ function run_test() {
   }
   do_check_true(store.itemExists(id));
 
+  _("Should be able to find this entry as a dupe");
+  do_check_eq(engine._findDupe({name: "name!!", value: "value??"}), id);
+
   let rec = store.createRecord(id);
   _("Got record for id", id, rec);
   do_check_eq(rec.name, "name!!");
@@ -120,9 +124,7 @@ function run_test() {
     value: "entry"
   }]);
 
-  Utils.runInTransaction(Svc.Form.DBConnection, function() {
+  store.wipe();
-    store.wipe();
-  });
 
   for (let id in store.getAllIDs()) {
     do_throw("Shouldn't get any ids!");

services/sync/tests/unit/test_forms_tracker.js

@@ -8,46 +8,51 @@ Cu.import("resource://services-sync/util.js");
 
 function run_test() {
   _("Verify we've got an empty tracker to work with.");
-  let tracker = new FormEngine(Service)._tracker;
+  let engine = new FormEngine(Service);
+  let tracker = engine._tracker;
   // Don't do asynchronous writes.
   tracker.persistChangedIDs = false;
 
   do_check_empty(tracker.changedIDs);
   Log4Moz.repository.rootLogger.addAppender(new Log4Moz.DumpAppender());
 
+  function addEntry(name, value) {
+    engine._store.create({name: name, value: value});
+  }
+  function removeEntry(name, value) {
+    guid = engine._findDupe({name: name, value: value});
+    engine._store.remove({id: guid});
+  }
+
   try {
     _("Create an entry. Won't show because we haven't started tracking yet");
-    Svc.Form.addEntry("name", "John Doe");
+    addEntry("name", "John Doe");
     do_check_empty(tracker.changedIDs);
 
     _("Tell the tracker to start tracking changes.");
     Svc.Obs.notify("weave:engine:start-tracking");
-    Svc.Form.removeEntry("name", "John Doe");
+    removeEntry("name", "John Doe");
-    Svc.Form.addEntry("email", "john@doe.com");
+    addEntry("email", "john@doe.com");
     do_check_attribute_count(tracker.changedIDs, 2);
 
     _("Notifying twice won't do any harm.");
     Svc.Obs.notify("weave:engine:start-tracking");
-    Svc.Form.addEntry("address", "Memory Lane");
+    addEntry("address", "Memory Lane");
     do_check_attribute_count(tracker.changedIDs, 3);
 
     _("Let's stop tracking again.");
     tracker.clearChangedIDs();
     Svc.Obs.notify("weave:engine:stop-tracking");
-    Svc.Form.removeEntry("address", "Memory Lane");
+    removeEntry("address", "Memory Lane");
     do_check_empty(tracker.changedIDs);
 
     _("Notifying twice won't do any harm.");
     Svc.Obs.notify("weave:engine:stop-tracking");
-    Svc.Form.removeEntry("email", "john@doe.com");
+    removeEntry("email", "john@doe.com");
     do_check_empty(tracker.changedIDs);
 
-    _("Test error detection.");
-    // This throws an exception without the fix for Bug 597400.
-    tracker.trackEntry("foo", "bar");
-
   } finally {
     _("Clean up.");
-    Svc.Form.removeAllEntries();
+    engine._store.wipe();
   }
 }