Bug 1352502 - Part 2. Add API to update the page meta info for Places. r=mak

MozReview-Commit-ID: K3SjQr3ayjS

--HG--
extra : rebase_source : 8fda3d2d7ea5f5e1883b2a96e03175c7265f7e52

toolkit/components/places/History.jsm

@@ -23,6 +23,12 @@
  *     values.
  * - title: (string)
  *     The title associated with the page, if any.
+ * - description: (string)
+ *     The description of the page, if any.
+ * - previewImageURL: (URL)
+ *     or (nsIURI)
+ *     or (string)
+ *     The preview image URL of the page, if any.
  * - frecency: (number)
  *     The frecency of the page, if any.
  *     See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm
@@ -89,7 +95,6 @@ const ONRESULT_CHUNK_SIZE = 300;
 // This constant determines the maximum number of remove pages before we cycle.
 const REMOVE_PAGES_CHUNKLEN = 300;
 
-
 /**
  * Sends a bookmarks notification through the given observers.
  *
@@ -119,6 +124,8 @@ this.History = Object.freeze({
    *        - `includeVisits` (boolean) set this to true if `visits` in the
    *           PageInfo needs to contain VisitInfo in a reverse chronological order.
    *           By default, `visits` is undefined inside the returned `PageInfo`.
+   *        - `includeMeta` (boolean) set this to true to fetch page meta fields,
+   *           i.e. `description` and `preview_image_url`.
    *
    * @return (Promise)
    *      A promise resolved once the operation is complete.
@@ -147,6 +154,11 @@ this.History = Object.freeze({
       throw new TypeError("includeVisits should be a boolean if exists");
     }
 
+    let hasIncludeMeta = "includeMeta" in options;
+    if (hasIncludeMeta && typeof options.includeMeta !== "boolean") {
+      throw new TypeError("includeMeta should be a boolean if exists");
+    }
+
     return PlacesUtils.promiseDBConnection()
                       .then(db => fetch(db, guidOrURI, options));
   },
@@ -191,10 +203,6 @@ this.History = Object.freeze({
    *      If an element of `visits` has an invalid `transition`.
    */
   insert(pageInfo) {
-    if (typeof pageInfo != "object" || !pageInfo) {
-      throw new TypeError("pageInfo must be an object");
-    }
-
     let info = PlacesUtils.validatePageInfo(pageInfo);
 
     return PlacesUtils.withConnectionWrapper("History.jsm: insert",
@@ -563,6 +571,61 @@ this.History = Object.freeze({
     }
   },
 
+   /**
+   * Update information for a page.
+   *
+   * Currently, it supports updating the description and the preview image URL
+   * 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)
+   *      pageInfo must contain a URL of the target page. It will be ignored
+   *      if a valid page `guid` is also provided.
+   *
+   *      If a property `description` is provided, the description of the
+   *      page is updated. Note that:
+   *      1). An empty string or null `description` will clear the existing
+   *          value in the database.
+   *      2). Descriptions longer than DB_DESCRIPTION_LENGTH_MAX will be
+   *          truncated.
+   *
+   *      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.
+   *
+   * @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)
+   *      If `pageInfo` has an invalid `url` or an invalid `guid`.
+   * @throws (Error)
+   *      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");
+    }
+
+    return PlacesUtils.withConnectionWrapper("History.jsm: update", db => update(db, info));
+  },
+
+
   /**
    * Possible values for the `transition` property of `VisitInfo`
    * objects.
@@ -861,7 +924,13 @@ var fetch = async function(db, guidOrURL, options) {
     visitOrderFragment = "ORDER BY v.visit_date DESC";
   }
 
-  let query = `SELECT h.id, guid, url, title, frecency ${visitSelectionFragment}
+  let pageMetaSelectionFragment = "";
+  if (options.includeMeta) {
+    pageMetaSelectionFragment = ", description, preview_image_url";
+  }
+
+  let query = `SELECT h.id, guid, url, title, frecency
+               ${pageMetaSelectionFragment} ${visitSelectionFragment}
                FROM moz_places h ${joinFragment}
                ${whereClauseFragment}
                ${visitOrderFragment}`;
@@ -879,6 +948,11 @@ var fetch = async function(db, guidOrURL, options) {
           title: row.getResultByName("title") || ""
         };
       }
+      if (options.includeMeta) {
+        pageInfo.description = row.getResultByName("description") || ""
+        let previewImageURL = row.getResultByName("preview_image_url");
+        pageInfo.previewImageURL = previewImageURL ? new URL(previewImageURL) : null;
+      }
       if (options.includeVisits) {
         // On every row (not just the first), we need to collect visit data.
         if (!("visits" in pageInfo)) {
@@ -1254,3 +1328,32 @@ var insertMany = async function(db, pageInfos, onResult, onError) {
     }, true);
   });
 };
+
+// Inner implementation of History.update.
+var update = async function(db, pageInfo) {
+  let updateFragments = [];
+  let whereClauseFragment = "";
+  let info = {};
+
+  // Prefer GUID over url if it's present
+  if (typeof pageInfo.guid === "string") {
+    whereClauseFragment = "WHERE guid = :guid";
+    info.guid = pageInfo.guid;
+  } else {
+    whereClauseFragment = "WHERE url_hash = hash(:url) AND url = :url";
+    info.url = pageInfo.url.href;
+  }
+
+  if (pageInfo.description || pageInfo.description === null) {
+    updateFragments.push("description = :description");
+    info.description = pageInfo.description;
+  }
+  if (pageInfo.previewImageURL || pageInfo.previewImageURL === null) {
+    updateFragments.push("preview_image_url = :previewImageURL");
+    info.previewImageURL = pageInfo.previewImageURL ? pageInfo.previewImageURL.href : null;
+  }
+  let query = `UPDATE moz_places
+               SET ${updateFragments.join(", ")}
+               ${whereClauseFragment}`;
+  await db.execute(query, info);
+}

toolkit/components/places/PlacesUtils.jsm

@@ -210,6 +210,7 @@ function serializeNode(aNode, aIsLivemark) {
 // Imposed to limit database size.
 const DB_URL_LENGTH_MAX = 65536;
 const DB_TITLE_LENGTH_MAX = 4096;
+const DB_DESCRIPTION_LENGTH_MAX = 1024;
 
 /**
  * List of bookmark object validators, one per each known property.
@@ -991,14 +992,20 @@ this.PlacesUtils = {
       visits: [],
     };
 
+    if (typeof pageInfo != "object" || !pageInfo) {
+      throw new TypeError("pageInfo must be an object");
+    }
+
     if (!pageInfo.url) {
       throw new TypeError("PageInfo object must have a url property");
     }
 
     info.url = this.normalizeToURLOrGUID(pageInfo.url);
 
-    if (!validateVisits) {
-      return info;
+    if (typeof pageInfo.guid === "string" && this.isValidGuid(pageInfo.guid)) {
+      info.guid = pageInfo.guid;
+    } else if (pageInfo.guid) {
+      throw new TypeError(`guid property of PageInfo object: ${pageInfo.guid} is invalid`);
     }
 
     if (typeof pageInfo.title === "string") {
@@ -1007,6 +1014,32 @@ this.PlacesUtils = {
       throw new TypeError(`title property of PageInfo object: ${pageInfo.title} must be a string if provided`);
     }
 
+    if (typeof pageInfo.description === "string" || pageInfo.description === null) {
+      info.description = pageInfo.description ? pageInfo.description.slice(0, DB_DESCRIPTION_LENGTH_MAX) : null;
+    } else if (pageInfo.description !== undefined) {
+      throw new TypeError(`description property of pageInfo object: ${pageInfo.description} must be either a string or null if provided`);
+    }
+
+    if (pageInfo.previewImageURL || pageInfo.previewImageURL === null) {
+      let previewImageURL = pageInfo.previewImageURL;
+
+      if (previewImageURL === null) {
+        info.previewImageURL = null;
+      } else if (typeof(previewImageURL) === "string" && previewImageURL.length <= DB_URL_LENGTH_MAX) {
+        info.previewImageURL = new URL(previewImageURL);
+      } else if (previewImageURL instanceof Ci.nsIURI && previewImageURL.spec.length <= DB_URL_LENGTH_MAX) {
+        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 (!validateVisits) {
+      return info;
+    }
+
     if (!pageInfo.visits || !Array.isArray(pageInfo.visits) || !pageInfo.visits.length) {
       throw new TypeError("PageInfo object must have an array of visits");
     }

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

@@ -77,6 +77,35 @@ add_task(async function test_fetch_existent() {
   }
 });
 
+add_task(async function test_fetch_page_meta_info() {
+  await PlacesTestUtils.clearHistory();
+
+  let TEST_URI = NetUtil.newURI("http://mozilla.com/test_fetch_page_meta_info");
+  await PlacesTestUtils.addVisits(TEST_URI);
+  Assert.ok(page_in_database(TEST_URI));
+
+  // Test fetching the null values
+  let includeMeta = true;
+  let pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeMeta});
+  Assert.strictEqual(null, pageInfo.previewImageURL, "fetch should return a null previewImageURL");
+  Assert.equal("", pageInfo.description, "fetch should return a empty string description");
+
+  // Now set the pageMetaInfo for this page
+  let description = "Test description";
+  let previewImageURL = "http://mozilla.com/test_preview_image.png";
+  await PlacesUtils.history.update({ url: TEST_URI, description, previewImageURL });
+
+  includeMeta = true;
+  pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeMeta});
+  Assert.equal(previewImageURL, pageInfo.previewImageURL.href, "fetch should return a previewImageURL");
+  Assert.equal(description, pageInfo.description, "fetch should return a description");
+
+  includeMeta = false;
+  pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeMeta});
+  Assert.ok(!("description" in pageInfo), "fetch should not return a description if includeMeta is false")
+  Assert.ok(!("previewImageURL" in pageInfo), "fetch should not return a previewImageURL if includeMeta is false")
+});
+
 add_task(async function test_fetch_nonexistent() {
   await PlacesTestUtils.clearHistory();
   await PlacesUtils.bookmarks.eraseEverything();
@@ -104,7 +133,11 @@ add_task(async function test_error_cases() {
       /TypeError: options should be/
   );
   Assert.throws(
-    () => PlacesUtils.history.fetch("http://valid.uri.come", { includeVisits: "not a boolean"}),
+    () => PlacesUtils.history.fetch("http://valid.uri.come", {includeVisits: "not a boolean"}),
       /TypeError: includeVisits should be a/
   );
+  Assert.throws(
+    () => PlacesUtils.history.fetch("http://valid.uri.come", {includeMeta: "not a boolean"}),
+      /TypeError: includeMeta should be a/
+  );
 });

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

@@ -0,0 +1,152 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Tests for `History.update` as implemented in History.jsm
+
+"use strict";
+
+add_task(async function test_error_cases() {
+  Assert.throws(
+    () => PlacesUtils.history.update("not an object"),
+    /TypeError: pageInfo must be/,
+    "passing a string as pageInfo should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update(null),
+    /TypeError: pageInfo must be/,
+    "passing a null as pageInfo should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({url: "not a valid url string"}),
+    /TypeError: not a valid url string is/,
+    "passing an invalid url should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({
+      url: "http://valid.uri.com",
+      description: 123
+    }),
+    /TypeError: description property of/,
+    "passing a non-string description in pageInfo should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({
+      url: "http://valid.uri.com",
+      guid: "invalid guid",
+      description: "Test description"
+    }),
+    /TypeError: guid property of/,
+    "passing a invalid guid in pageInfo should throw a TypeError"
+  );
+  Assert.throws(
+    () => PlacesUtils.history.update({
+      url: "http://valid.uri.com",
+      previewImageURL: "not a valid url string"
+    }),
+    /TypeError: not a valid url string is/,
+    "passing an invlid preview image url in pageInfo should throw a TypeError"
+  );
+  Assert.throws(
+    () => {
+      let imageName = "a-very-long-string".repeat(10000);
+      let previewImageURL = `http://valid.uri.com/${imageName}.png`;
+      PlacesUtils.history.update({
+        url: "http://valid.uri.com",
+        previewImageURL
+      });
+    },
+    /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"
+  );
+});
+
+add_task(async function test_description_change_saved() {
+  await PlacesTestUtils.clearHistory();
+
+  let TEST_URL = NetUtil.newURI("http://mozilla.org/test_description_change_saved");
+  await PlacesTestUtils.addVisits(TEST_URL);
+  Assert.ok(page_in_database(TEST_URL));
+
+  let description = "Test description";
+  await PlacesUtils.history.update({ url: TEST_URL, description });
+  let descriptionInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "description");
+  Assert.equal(description, descriptionInDB, "description should be updated via URL as expected");
+
+  description = "";
+  await PlacesUtils.history.update({ url: TEST_URL, description });
+  descriptionInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "description");
+  Assert.strictEqual(null, descriptionInDB, "an empty description should set it to null in the database");
+
+  let guid = await PlacesTestUtils.fieldInDB(TEST_URL, "guid");
+  description = "Test description";
+  await PlacesUtils.history.update({ url: TEST_URL, guid, description });
+  descriptionInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "description");
+  Assert.equal(description, descriptionInDB, "description should be updated via GUID as expected");
+
+  description = "Test descipriton".repeat(1000);
+  await PlacesUtils.history.update({ url: TEST_URL, description });
+  descriptionInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "description");
+  Assert.ok(0 < descriptionInDB.length < description.length, "a long description should be truncated");
+
+  description = null;
+  await PlacesUtils.history.update({ url: TEST_URL, description});
+  descriptionInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "description");
+  Assert.strictEqual(description, descriptionInDB, "a null description should set it to null in the database");
+});
+
+add_task(async function test_previewImageURL_change_saved() {
+  await PlacesTestUtils.clearHistory();
+
+  let TEST_URL = NetUtil.newURI("http://mozilla.org/test_previewImageURL_change_saved");
+  let IMAGE_URL = "http://mozilla.org/test_preview_image.png";
+  await PlacesTestUtils.addVisits(TEST_URL);
+  Assert.ok(page_in_database(TEST_URL));
+
+  let previewImageURL = IMAGE_URL;
+  await PlacesUtils.history.update({ url: TEST_URL, previewImageURL });
+  let previewImageURLInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "preview_image_url");
+  Assert.equal(previewImageURL, previewImageURLInDB, "previewImageURL should be updated via URL as expected");
+
+  previewImageURL = null;
+  await PlacesUtils.history.update({ url: TEST_URL, previewImageURL});
+  previewImageURLInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "preview_image_url");
+  Assert.strictEqual(null, previewImageURLInDB, "a null previewImageURL should set it to null in the database");
+
+  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() {
+  await PlacesTestUtils.clearHistory();
+
+  let TEST_URL = NetUtil.newURI("http://mozilla.org/test_change_both_saved");
+  await PlacesTestUtils.addVisits(TEST_URL);
+  Assert.ok(page_in_database(TEST_URL));
+
+  let description = "Test description";
+  let previewImageURL = "http://mozilla.org/test_preview_image.png";
+
+  await PlacesUtils.history.update({ url: TEST_URL, description, previewImageURL });
+  let descriptionInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "description");
+  let previewImageURLInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "preview_image_url");
+  Assert.equal(description, descriptionInDB, "description should be updated via URL as expected");
+  Assert.equal(previewImageURL, previewImageURLInDB, "previewImageURL should be updated via URL as expected");
+
+  // 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");
+});

toolkit/components/places/tests/history/xpcshell.ini

@@ -11,4 +11,5 @@ head = head_history.js
 [test_removeByFilter.js]
 [test_removeVisitsByFilter.js]
 [test_sameUri_titleChanged.js]
+[test_update.js]
 [test_updatePlaces_embed.js]

toolkit/components/places/tests/migration/test_current_from_v38.js

@@ -11,33 +11,8 @@ add_task(function* database_is_valid() {
   Assert.equal((yield db.getSchemaVersion()), CURRENT_SCHEMA_VERSION);
 });
 
-add_task(function* test_new_fields() {
-  let path = OS.Path.join(OS.Constants.Path.profileDir, DB_FILENAME);
-  let db = yield Sqlite.openConnection({ path });
-
-  // Manually update these two fields for a places record.
-  yield db.execute(`
-    UPDATE moz_places
-    SET description = :description, preview_image_url = :previewImageURL
-    WHERE id = 1`, { description: "Page description",
-                     previewImageURL: "https://example.com/img.png" });
-  let rows = yield db.execute(`SELECT description FROM moz_places
-                               WHERE description IS NOT NULL
-                               AND preview_image_url IS NOT NULL`);
-  Assert.equal(rows.length, 1,
-    "should fetch one record with not null description and preview_image_url");
-
-  // Reset them to the default value
-  yield db.execute(`
-    UPDATE moz_places
-    SET description = NULL,
-        preview_image_url = NULL
-    WHERE id = 1`);
-  rows = yield db.execute(`SELECT description FROM moz_places
-                           WHERE description IS NOT NULL
-                           AND preview_image_url IS NOT NULL`);
-  Assert.equal(rows.length, 0,
-    "should fetch 0 record with not null description and preview_image_url");
-
-  yield db.close();
+add_task(function* test_select_new_fields() {
+  let db = yield PlacesUtils.promiseDBConnection();
+  yield db.execute(`SELECT description, preview_image_url FROM moz_places`);
+  Assert.ok(true, "should be able to select description and preview_image_url");
 });