From 29b9ec51dc8ac4e0d15d1c02779c74f073464b7e Mon Sep 17 00:00:00 2001 From: Nan Jiang Date: Fri, 23 Jun 2017 14:30:27 -0400 Subject: [PATCH] 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 | 115 ++++++++++++- toolkit/components/places/PlacesUtils.jsm | 37 ++++- .../places/tests/history/test_fetch.js | 35 +++- .../places/tests/history/test_update.js | 152 ++++++++++++++++++ .../places/tests/history/xpcshell.ini | 1 + .../tests/migration/test_current_from_v38.js | 33 +--- 6 files changed, 335 insertions(+), 38 deletions(-) create mode 100644 toolkit/components/places/tests/history/test_update.js diff --git a/toolkit/components/places/History.jsm b/toolkit/components/places/History.jsm index 04fce654a244..485448844cff 100644 --- a/toolkit/components/places/History.jsm +++ b/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); +} diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index e885082e6037..0900a5d4725e 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/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"); } diff --git a/toolkit/components/places/tests/history/test_fetch.js b/toolkit/components/places/tests/history/test_fetch.js index 64a6a91dc261..091397a54b98 100644 --- a/toolkit/components/places/tests/history/test_fetch.js +++ b/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/ + ); }); diff --git a/toolkit/components/places/tests/history/test_update.js b/toolkit/components/places/tests/history/test_update.js new file mode 100644 index 000000000000..ce5cabeaab81 --- /dev/null +++ b/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"); +}); diff --git a/toolkit/components/places/tests/history/xpcshell.ini b/toolkit/components/places/tests/history/xpcshell.ini index 7fa6c1ad1c7c..aeb87a0e95ea 100644 --- a/toolkit/components/places/tests/history/xpcshell.ini +++ b/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] diff --git a/toolkit/components/places/tests/migration/test_current_from_v38.js b/toolkit/components/places/tests/migration/test_current_from_v38.js index b02967499621..e0bf9b85e6df 100644 --- a/toolkit/components/places/tests/migration/test_current_from_v38.js +++ b/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"); });