Bug 1420571 - Don't write unchanged page metadata to places.sqlite. r=Mardak draft
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 27 Nov 2017 18:15:21 +0100
changeset 703736 3aa365b44dbd148f1f835e12a08100add8e5e4a2
parent 703662 cad9c9573579698c223b4b6cb53ca723cd930ad2
child 741892 3f2ea0388b975f789dcb9b9b8e7a2075cf1988a8
push id90948
push usermak77@bonardo.net
push dateMon, 27 Nov 2017 17:18:21 +0000
reviewersMardak
bugs1420571
milestone59.0a1
Bug 1420571 - Don't write unchanged page metadata to places.sqlite. r=Mardak MozReview-Commit-ID: Gdv1qVSsfnO
toolkit/components/places/History.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/history/test_update.js
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -1383,32 +1383,36 @@ var insertMany = function(db, pageInfos,
     }, true);
   });
 };
 
 // Inner implementation of History.update.
 var update = async function(db, pageInfo) {
   let updateFragments = [];
   let whereClauseFragment = "";
-  let info = {};
+  let params = {};
 
   // Prefer GUID over url if it's present
   if (typeof pageInfo.guid === "string") {
-    whereClauseFragment = "WHERE guid = :guid";
-    info.guid = pageInfo.guid;
+    whereClauseFragment = "guid = :guid";
+    params.guid = pageInfo.guid;
   } else {
-    whereClauseFragment = "WHERE url_hash = hash(:url) AND url = :url";
-    info.url = pageInfo.url.href;
+    whereClauseFragment = "url_hash = hash(:url) AND url = :url";
+    params.url = pageInfo.url.href;
   }
 
   if (pageInfo.description || pageInfo.description === null) {
-    updateFragments.push("description = :description");
-    info.description = pageInfo.description;
+    updateFragments.push("description");
+    params.description = pageInfo.description;
   }
   if (pageInfo.previewImageURL || pageInfo.previewImageURL === null) {
-    updateFragments.push("preview_image_url = :previewImageURL");
-    info.previewImageURL = pageInfo.previewImageURL ? pageInfo.previewImageURL.href : null;
+    updateFragments.push("preview_image_url");
+    params.preview_image_url = pageInfo.previewImageURL ? pageInfo.previewImageURL.href : null;
   }
-  let query = `UPDATE moz_places
-               SET ${updateFragments.join(", ")}
-               ${whereClauseFragment}`;
-  await db.execute(query, info);
+  // 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);
 };
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -201,17 +201,17 @@ function serializeNode(aNode, aIsLivemar
   }
 
   return JSON.stringify(data);
 }
 
 // Imposed to limit database size.
 const DB_URL_LENGTH_MAX = 65536;
 const DB_TITLE_LENGTH_MAX = 4096;
-const DB_DESCRIPTION_LENGTH_MAX = 1024;
+const DB_DESCRIPTION_LENGTH_MAX = 256;
 
 /**
  * List of bookmark object validators, one per each known property.
  * Validators must throw if the property value is invalid and return a fixed up
  * version of the value, if needed.
  */
 const BOOKMARK_VALIDATORS = Object.freeze({
   guid: simpleValidateFunc(v => PlacesUtils.isValidGuid(v)),
--- a/toolkit/components/places/tests/history/test_update.js
+++ b/toolkit/components/places/tests/history/test_update.js
@@ -65,19 +65,19 @@ add_task(async function test_error_cases
     /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");
+  let TEST_URL = "http://mozilla.org/test_description_change_saved";
   await PlacesTestUtils.addVisits(TEST_URL);
-  Assert.ok(page_in_database(TEST_URL));
+  Assert.ok(await PlacesTestUtils.isPageInDB(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 });
@@ -99,20 +99,20 @@ add_task(async function test_description
   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 TEST_URL = "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));
+  Assert.ok(await PlacesTestUtils.isPageInDB(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});
@@ -124,19 +124,19 @@ add_task(async function test_previewImag
   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");
+  let TEST_URL = "http://mozilla.org/test_change_both_saved";
   await PlacesTestUtils.addVisits(TEST_URL);
-  Assert.ok(page_in_database(TEST_URL));
+  Assert.ok(await PlacesTestUtils.isPageInDB(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");