Bug 1468980 - Add an annotations parameter to PlacesUtils.history.update to retrieve any annotations. r=mak
authorMark Banner <standard8@mozilla.com>
Wed, 25 Jul 2018 07:34:35 +0100
changeset 429297 3c7eadf5e1ce2fab428e779ebdc8befb88dddbb1
parent 429296 147d99782475988dd63d3fbdc5334401b497ad32
child 429298 e48757a556f88b075f860f98c27f22f3e0db06ff
push id67110
push usermbanner@mozilla.com
push dateTue, 31 Jul 2018 08:35:52 +0000
treeherderautoland@bc2f6a85b93f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1468980
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1468980 - Add an annotations parameter to PlacesUtils.history.update to retrieve any annotations. r=mak MozReview-Commit-ID: 9pobdfHodcG
toolkit/components/places/History.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/nsPlacesTables.h
toolkit/components/places/tests/history/test_update.js
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -603,17 +603,17 @@ var History = Object.freeze({
     if (!arg || typeof arg != "object" || arg.constructor.name != "Date") {
       throw new TypeError("Expected a Date, got " + arg);
     }
   },
 
    /**
    * 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
    * yet been stored in the database. `History.fetch` could be used to check
    * whether the page and its meta information exist or not. Beware that
    * fetch&update might fail as they are not executed in a single transaction.
    *
    * @param pageInfo: (PageInfo)
@@ -629,16 +629,24 @@ var History = Object.freeze({
    *
    *      If a property `previewImageURL` is provided, the preview image
    *      URL of the page is updated. Note that:
    *      1). A null `previewImageURL` will clear the existing value in the
    *          database.
    *      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)
    *      Rejects if the update was unsuccessful.
    *
    * @throws (Error)
    *      If `pageInfo` has an unexpected type.
    * @throws (Error)
@@ -647,18 +655,19 @@ var History = Object.freeze({
    *      If `pageInfo` has neither `description` nor `previewImageURL`.
    * @throws (Error)
    *      If the length of `pageInfo.previewImageURL` is greater than
    *      DB_URL_LENGTH_MAX defined in PlacesUtils.jsm.
    */
   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));
   },
 
 
   /**
    * Possible values for the `transition` property of `VisitInfo`
@@ -1428,36 +1437,99 @@ var insertMany = function(db, pageInfos,
     }, true);
   });
 };
 
 // Inner implementation of History.update.
 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) {
     updateFragments.push("description");
     params.description = pageInfo.description;
   }
   if (pageInfo.previewImageURL || pageInfo.previewImageURL === null) {
     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 });
+      }
+    });
+  }
 };
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1079,16 +1079,42 @@ var PlacesUtils = {
         info.previewImageURL = new URL(previewImageURL.spec);
       } else if (previewImageURL instanceof URL && previewImageURL.href.length <= DB_URL_LENGTH_MAX) {
         info.previewImageURL = previewImageURL;
       } else {
         throw new TypeError("previewImageURL property of pageInfo object: ${previewImageURL} is invalid");
       }
     }
 
+    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;
     }
 
     if (!pageInfo.visits || !Array.isArray(pageInfo.visits) || !pageInfo.visits.length) {
       throw new TypeError("PageInfo object must have an array of visits");
     }
 
--- a/toolkit/components/places/nsPlacesTables.h
+++ b/toolkit/components/places/nsPlacesTables.h
@@ -42,16 +42,18 @@
   "CREATE TABLE moz_inputhistory (" \
     "  place_id INTEGER NOT NULL" \
     ", input LONGVARCHAR NOT NULL" \
     ", use_count INTEGER" \
     ", PRIMARY KEY (place_id, input)" \
   ")" \
 )
 
+// 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" \
     ", place_id INTEGER NOT NULL" \
     ", anno_attribute_id INTEGER" \
     ", content LONGVARCHAR" \
     ", flags INTEGER DEFAULT 0" \
     ", expiration INTEGER DEFAULT 0" \
--- a/toolkit/components/places/tests/history/test_update.js
+++ b/toolkit/components/places/tests/history/test_update.js
@@ -58,17 +58,51 @@ add_task(async function test_error_cases
       });
     },
     /TypeError: previewImageURL property of/,
     "passing an oversized previewImageURL in pageInfo should throw a TypeError"
   );
   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"
   );
 });
 
 add_task(async function test_description_change_saved() {
   await PlacesUtils.history.clear();
 
   let TEST_URL = "http://mozilla.org/test_description_change_saved";
   await PlacesTestUtils.addVisits(TEST_URL);
@@ -121,17 +155,17 @@ add_task(async function test_previewImag
 
   let guid = await PlacesTestUtils.fieldInDB(TEST_URL, "guid");
   previewImageURL = IMAGE_URL;
   await PlacesUtils.history.update({ url: TEST_URL, guid, previewImageURL });
   previewImageURLInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "preview_image_url");
   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";
   await PlacesTestUtils.addVisits(TEST_URL);
   Assert.ok(await PlacesTestUtils.isPageInDB(TEST_URL));
 
   let description = "Test description";
   let previewImageURL = "http://mozilla.org/test_preview_image.png";
@@ -145,8 +179,182 @@ add_task(async function test_change_both
   // Update description should not touch other fields
   description = null;
   await PlacesUtils.history.update({ url: TEST_URL, description });
   descriptionInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "description");
   previewImageURLInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "preview_image_url");
   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");
+});