Bug 1468980 - Add an annotations parameter to PlacesUtils.history.update to retrieve any annotations. r=mak

MozReview-Commit-ID: 9pobdfHodcG

--HG--
extra : rebase_source : 05e9dea7b9cced745023c424db07d8e64cbccca0

toolkit/components/places/History.jsm

@@ -608,7 +608,7 @@ var History = Object.freeze({
    /**
    * Update information for a page.
    *
-   * Currently, it supports updating the description and the preview image URL
+   * Currently, it supports updating the description, preview image URL and annotations
    * for a page, any other fields will be ignored.
    *
    * Note that this function will ignore the update if the target page has not
@@ -634,6 +634,14 @@ var History = Object.freeze({
    *      2). It throws if its length is greater than DB_URL_LENGTH_MAX
    *          defined in PlacesUtils.jsm.
    *
+   *      If a property `annotations` is provided, the annotations will be
+   *      updated. Note that:
+   *      1). It should be a Map containing key/value pairs to be updated.
+   *      2). If the value is falsy, the annotation will be removed.
+   *      3). If the value is non-falsy, the annotation will be added or updated.
+   *      For `annotations` the keys must all be strings, the values should be
+   *      Boolean, Number or Strings. null and undefined are supported as falsy values.
+   *
    * @return (Promise)
    *      A promise resolved once the update is complete.
    * @rejects (Error)
@@ -652,8 +660,9 @@ var History = Object.freeze({
   update(pageInfo) {
     let info = PlacesUtils.validatePageInfo(pageInfo, false);
 
-    if (info.description === undefined && info.previewImageURL === undefined) {
-      throw new TypeError("pageInfo object must at least have either a description or a previewImageURL property");
+    if (info.description === undefined && info.previewImageURL === undefined &&
+        info.annotations === undefined) {
+      throw new TypeError("pageInfo object must at least have either a description, previewImageURL or annotations property.");
     }
 
     return PlacesUtils.withConnectionWrapper("History.jsm: update", db => update(db, info));
@@ -1433,15 +1442,16 @@ var insertMany = function(db, pageInfos, onResult, onError) {
 var update = async function(db, pageInfo) {
   let updateFragments = [];
   let whereClauseFragment = "";
+  let baseParams = {};
   let params = {};
 
   // Prefer GUID over url if it's present
   if (typeof pageInfo.guid === "string") {
     whereClauseFragment = "guid = :guid";
-    params.guid = pageInfo.guid;
+    baseParams.guid = pageInfo.guid;
   } else {
     whereClauseFragment = "url_hash = hash(:url) AND url = :url";
-    params.url = pageInfo.url.href;
+    baseParams.url = pageInfo.url.href;
   }
 
   if (pageInfo.description || pageInfo.description === null) {
@@ -1452,12 +1462,74 @@ var update = async function(db, pageInfo) {
     updateFragments.push("preview_image_url");
     params.preview_image_url = pageInfo.previewImageURL ? pageInfo.previewImageURL.href : null;
   }
-  // Since this data may be written at every visit and is textual, avoid
-  // overwriting the existing record if it didn't change.
-  await db.execute(`
-    UPDATE moz_places
-    SET ${updateFragments.map(v => `${v} = :${v}`).join(", ")}
-    WHERE ${whereClauseFragment}
-      AND (${updateFragments.map(v => `IFNULL(${v}, "") <> IFNULL(:${v}, "")`).join(" OR ")})
-  `, params);
+  if (updateFragments.length > 0) {
+    // Since this data may be written at every visit and is textual, avoid
+    // overwriting the existing record if it didn't change.
+    await db.execute(`
+      UPDATE moz_places
+      SET ${updateFragments.map(v => `${v} = :${v}`).join(", ")}
+      WHERE ${whereClauseFragment}
+        AND (${updateFragments.map(v => `IFNULL(${v}, "") <> IFNULL(:${v}, "")`).join(" OR ")})
+    `, {...baseParams, ...params});
+  }
+
+  if (pageInfo.annotations) {
+    let annosToRemove = [];
+    let annosToUpdate = [];
+
+    for (let anno of pageInfo.annotations) {
+      anno[1] ? annosToUpdate.push(anno[0]) : annosToRemove.push(anno[0]);
+    }
+
+    await db.executeTransaction(async function() {
+      if (annosToUpdate.length) {
+        await db.execute(`
+          INSERT OR IGNORE INTO moz_anno_attributes (name)
+          VALUES ${Array.from(annosToUpdate.keys()).map(k => `(:${k})`).join(", ")}
+        `, Object.assign({}, annosToUpdate));
+
+        for (let anno of annosToUpdate) {
+          let content = pageInfo.annotations.get(anno);
+          // TODO: We only really need to save the type whilst we still support
+          // accessing page annotations via the annotation service.
+          let type = typeof content == "string" ? Ci.nsIAnnotationService.TYPE_STRING :
+            Ci.nsIAnnotationService.TYPE_INT64;
+          let date = PlacesUtils.toPRTime(new Date());
+
+          // This will replace the id every time an annotation is updated. This is
+          // not currently an issue as we're not joining on the id field.
+          await db.execute(`
+            INSERT OR REPLACE INTO moz_annos
+              (place_id, anno_attribute_id, content, flags,
+               expiration, type, dateAdded, lastModified)
+            VALUES ((SELECT id FROM moz_places WHERE ${whereClauseFragment}),
+                    (SELECT id FROM moz_anno_attributes WHERE name = :anno_name),
+                    :content, 0, :expiration, :type, :date_added,
+                    :last_modified)
+          `, {
+            ...baseParams,
+            anno_name: anno,
+            content,
+            expiration: PlacesUtils.annotations.EXPIRE_NEVER,
+            type,
+            // The date fields are unused, so we just set them both to the latest.
+            date_added: date,
+            last_modified: date,
+          });
+        }
+      }
+
+      for (let anno of annosToRemove) {
+        // We don't remove anything from the moz_anno_attributes table. If we
+        // delete the last item of a given name, that item really should go away.
+        // It will be cleaned up by expiration.
+        await db.execute(`
+          DELETE FROM moz_annos
+          WHERE place_id = (SELECT id FROM moz_places WHERE ${whereClauseFragment})
+          AND anno_attribute_id =
+            (SELECT id FROM moz_anno_attributes WHERE name = :anno_name)
+        `, { ...baseParams, anno_name: anno });
+      }
+    });
+  }
 };

toolkit/components/places/PlacesUtils.jsm

@@ -1084,6 +1084,32 @@ var PlacesUtils = {
       }
     }
 
+    if (pageInfo.annotations) {
+      if (typeof pageInfo.annotations != "object" ||
+          pageInfo.annotations.constructor.name != "Map") {
+        throw new TypeError("annotations must be a Map");
+      }
+
+      if (pageInfo.annotations.size == 0) {
+        throw new TypeError("there must be at least one annotation");
+      }
+
+      for (let [key, value] of pageInfo.annotations.entries()) {
+        if (typeof key != "string") {
+          throw new TypeError("all annotation keys must be strings");
+        }
+        if (typeof value != "string" &&
+            typeof value != "number" &&
+            typeof value != "boolean" &&
+            value !== null &&
+            value !== undefined) {
+          throw new TypeError("all annotation values must be Boolean, Numbers or Strings");
+        }
+      }
+
+      info.annotations = pageInfo.annotations;
+    }
+
     if (!validateVisits) {
       return info;
     }

toolkit/components/places/nsPlacesTables.h

@@ -47,6 +47,8 @@
   ")" \
 )
 
+// Note: flags, expiration, type, dateAdded and lastModified should be considered
+// deprecated but are kept to ease backwards compatibility.
 #define CREATE_MOZ_ANNOS NS_LITERAL_CSTRING( \
   "CREATE TABLE moz_annos (" \
     "  id INTEGER PRIMARY KEY" \

toolkit/components/places/tests/history/test_update.js

@@ -63,7 +63,41 @@ add_task(async function test_error_cases() {
   Assert.throws(
     () => PlacesUtils.history.update({ url: "http://valid.uri.com" }),
     /TypeError: pageInfo object must at least/,
-    "passing a pageInfo with neither description nor previewImageURL should throw a TypeError"
+    "passing a pageInfo with neither description, previewImageURL, nor annotations should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({ url: "http://valid.uri.com", annotations: "asd" }),
+    /TypeError: annotations must be a Map/,
+    "passing a pageInfo with incorrect annotations type should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({ url: "http://valid.uri.com", annotations: new Map() }),
+    /TypeError: there must be at least one annotation/,
+    "passing a pageInfo with an empty annotations type should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({
+      url: "http://valid.uri.com",
+      annotations: new Map([[1234, "value"]]),
+    }),
+    /TypeError: all annotation keys must be strings/,
+    "passing a pageInfo with an invalid key type should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({
+      url: "http://valid.uri.com",
+      annotations: new Map([["test", ["myarray"]]]),
+    }),
+    /TypeError: all annotation values must be Boolean, Numbers or Strings/,
+    "passing a pageInfo with an invalid key type should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({
+      url: "http://valid.uri.com",
+      annotations: new Map([["test", {anno: "value"}]]),
+    }),
+    /TypeError: all annotation values must be Boolean, Numbers or Strings/,
+    "passing a pageInfo with an invalid key type should throw a TypeError"
   );
 });
 
@@ -126,7 +160,7 @@ add_task(async function test_previewImageURL_change_saved() {
   Assert.equal(previewImageURL, previewImageURLInDB, "previewImageURL should be updated via GUID as expected");
 });
 
-add_task(async function test_change_both_saved() {
+add_task(async function test_change_description_and_preview_saved() {
   await PlacesUtils.history.clear();
 
   let TEST_URL = "http://mozilla.org/test_change_both_saved";
@@ -150,3 +184,177 @@ add_task(async function test_change_both_saved() {
   Assert.strictEqual(description, descriptionInDB, "description should be updated via URL as expected");
   Assert.equal(previewImageURL, previewImageURLInDB, "previewImageURL should not be updated");
 });
+
+async function getAnnotationInfoFromDB(pageUrl, annoName) {
+  let db = await PlacesUtils.promiseDBConnection();
+
+  let rows = await db.execute(`
+    SELECT a.content, a.flags, a.expiration, a.type FROM moz_anno_attributes n
+    JOIN moz_annos a ON n.id = a.anno_attribute_id
+    JOIN moz_places h ON h.id = a.place_id
+    WHERE h.url_hash = hash(:pageUrl) AND h.url = :pageUrl
+      AND n.name = :annoName
+  `, {annoName, pageUrl});
+
+  let result = rows.map(row => {
+    return {
+      content: row.getResultByName("content"),
+      flags: row.getResultByName("flags"),
+      expiration: row.getResultByName("expiration"),
+      type: row.getResultByName("type"),
+    };
+  });
+
+  return result;
+}
+
+add_task(async function test_simple_change_annotations() {
+  await PlacesUtils.history.clear();
+
+  const TEST_URL = "http://mozilla.org/test_change_both_saved";
+  await PlacesTestUtils.addVisits(TEST_URL);
+  Assert.ok(await PlacesTestUtils.isPageInDB(TEST_URL),
+    "Should have inserted the page into the database.");
+
+  await PlacesUtils.history.update({
+    url: TEST_URL,
+    annotations: new Map([["test/annotation", "testContent"]]),
+  });
+
+  let pageInfo = await PlacesUtils.history.fetch(TEST_URL, {includeAnnotations: true});
+
+  Assert.equal(pageInfo.annotations.size, 1,
+    "Should have one annotation for the page");
+  Assert.equal(pageInfo.annotations.get("test/annotation"), "testContent",
+    "Should have the correct annotation");
+
+  let annotationInfo = await getAnnotationInfoFromDB(TEST_URL, "test/annotation");
+  Assert.deepEqual({
+    content: "testContent",
+    flags: 0,
+    type: Ci.nsIAnnotationService.TYPE_STRING,
+    expiration: Ci.nsIAnnotationService.EXPIRE_NEVER,
+  }, annotationInfo[0], "Should have stored the correct annotation data in the db");
+
+  await PlacesUtils.history.update({
+    url: TEST_URL,
+    annotations: new Map([["test/annotation2", "testAnno"]]),
+  });
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_URL, {includeAnnotations: true});
+
+  Assert.equal(pageInfo.annotations.size, 2,
+    "Should have two annotations for the page");
+  Assert.equal(pageInfo.annotations.get("test/annotation"), "testContent",
+    "Should have the correct value for the first annotation");
+  Assert.equal(pageInfo.annotations.get("test/annotation2"), "testAnno",
+    "Should have the correct value for the second annotation");
+
+  await PlacesUtils.history.update({
+    url: TEST_URL,
+    annotations: new Map([["test/annotation", 1234]]),
+  });
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_URL, {includeAnnotations: true});
+
+  Assert.equal(pageInfo.annotations.size, 2,
+    "Should still have two annotations for the page");
+  Assert.equal(pageInfo.annotations.get("test/annotation"), 1234,
+    "Should have the updated the first annotation value");
+  Assert.equal(pageInfo.annotations.get("test/annotation2"), "testAnno",
+    "Should have kept the value for the second annotation");
+
+  annotationInfo = await getAnnotationInfoFromDB(TEST_URL, "test/annotation");
+  Assert.deepEqual({
+    content: 1234,
+    flags: 0,
+    type: Ci.nsIAnnotationService.TYPE_INT64,
+    expiration: Ci.nsIAnnotationService.EXPIRE_NEVER,
+  }, annotationInfo[0], "Should have updated the annotation data in the db");
+
+  await PlacesUtils.history.update({
+    url: TEST_URL,
+    annotations: new Map([["test/annotation", null]]),
+  });
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_URL, {includeAnnotations: true});
+
+  Assert.equal(pageInfo.annotations.size, 1,
+    "Should have removed only the first annotation");
+  Assert.strictEqual(pageInfo.annotations.get("test/annotation"), undefined,
+    "Should have removed only the first annotation");
+  Assert.equal(pageInfo.annotations.get("test/annotation2"), "testAnno",
+    "Should have kept the value for the second annotation");
+
+  await PlacesUtils.history.update({
+    url: TEST_URL,
+    annotations: new Map([["test/annotation2", null]]),
+  });
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_URL, {includeAnnotations: true});
+
+  Assert.equal(pageInfo.annotations.size, 0,
+    "Should have no annotations left");
+
+  let db = await PlacesUtils.promiseDBConnection();
+  let rows = await db.execute(`
+    SELECT * FROM moz_annos
+  `);
+  Assert.equal(rows.length, 0, "Should be no annotations left in the db");
+});
+
+add_task(async function test_change_multiple_annotations() {
+  await PlacesUtils.history.clear();
+
+  const TEST_URL = "http://mozilla.org/test_change_both_saved";
+  await PlacesTestUtils.addVisits(TEST_URL);
+  Assert.ok(await PlacesTestUtils.isPageInDB(TEST_URL),
+    "Should have inserted the page into the database.");
+
+  await PlacesUtils.history.update({
+    url: TEST_URL,
+    annotations: new Map([
+      ["test/annotation", "testContent"],
+      ["test/annotation2", "testAnno"],
+    ]),
+  });
+
+  let pageInfo = await PlacesUtils.history.fetch(TEST_URL, {includeAnnotations: true});
+
+  Assert.equal(pageInfo.annotations.size, 2,
+    "Should have inserted the two annotations for the page.");
+  Assert.equal(pageInfo.annotations.get("test/annotation"), "testContent",
+    "Should have the correct value for the first annotation");
+  Assert.equal(pageInfo.annotations.get("test/annotation2"), "testAnno",
+    "Should have the correct value for the second annotation");
+
+  await PlacesUtils.history.update({
+    url: TEST_URL,
+    annotations: new Map([
+      ["test/annotation", 123456],
+      ["test/annotation2", 135246],
+    ]),
+  });
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_URL, {includeAnnotations: true});
+
+  Assert.equal(pageInfo.annotations.size, 2,
+    "Should have two annotations for the page");
+  Assert.equal(pageInfo.annotations.get("test/annotation"), 123456,
+    "Should have the correct value for the first annotation");
+  Assert.equal(pageInfo.annotations.get("test/annotation2"), 135246,
+    "Should have the correct value for the second annotation");
+
+  await PlacesUtils.history.update({
+    url: TEST_URL,
+    annotations: new Map([
+      ["test/annotation", null],
+      ["test/annotation2", null],
+    ]),
+  });
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_URL, {includeAnnotations: true});
+
+  Assert.equal(pageInfo.annotations.size, 0,
+    "Should have no annotations left");
+});