Bug 1479722 - Rework PlacesUtils.setCharsetForURI to PlacesUIUtils.setCharsetForPage and avoid main thread sync io. r=mak

MozReview-Commit-ID: HeO3Mm5Dibo

Differential Revision: https://phabricator.services.mozilla.com/D2582

--HG--
extra : moz-landing-system : lando

browser/base/content/browser-places.js

@@ -399,9 +399,8 @@ var PlacesCommandHook = {
 
       info.guid = await PlacesTransactions.NewBookmark(info).transact();
 
-      // Set the character-set
-      if (charset && !PrivateBrowsingUtils.isBrowserPrivate(browser)) {
-        PlacesUtils.setCharsetForURI(makeURI(url.href), charset);
+      if (charset) {
+        PlacesUIUtils.setCharsetForPage(url, charset, window).catch(Cu.reportError);
       }
     }
 

browser/base/content/browser.js

@@ -6225,8 +6225,9 @@ function BrowserSetForcedCharacterSet(aCharset) {
   if (aCharset) {
     gBrowser.selectedBrowser.characterSet = aCharset;
     // Save the forced character-set
-    if (!PrivateBrowsingUtils.isWindowPrivate(window))
-      PlacesUtils.setCharsetForURI(getWebNavigation().currentURI, aCharset);
+    PlacesUIUtils.setCharsetForPage(getWebNavigation().currentURI,
+                                    aCharset,
+                                    window).catch(Cu.reportError);
   }
   BrowserCharsetReload();
 }
@@ -8244,4 +8245,3 @@ var ConfirmationHint = {
     return this._message = document.getElementById("confirmation-hint-message");
   },
 };
-

browser/components/places/PlacesUIUtils.jsm

@@ -459,6 +459,32 @@ var PlacesUIUtils = {
     PlacesUtils.history.markPageAsFollowedLink(this.createFixedURI(aURL));
   },
 
+  /**
+   * Sets the character-set for a page. The character set will not be saved
+   * if the window is determined to be a private browsing window.
+   *
+   * @param {string|URL|nsIURI} url The URL of the page to set the charset on.
+   * @param {String} charset character-set value.
+   * @param {window} window The window that the charset is being set from.
+   * @return {Promise}
+   */
+  async setCharsetForPage(url, charset, window) {
+    if (PrivateBrowsingUtils.isWindowPrivate(window)) {
+      return;
+    }
+
+    // UTF-8 is the default. If we are passed the value then set it to null,
+    // to ensure any charset is removed from the database.
+    if (charset.toLowerCase() == "utf-8") {
+      charset = null;
+    }
+
+    await PlacesUtils.history.update({
+      url,
+      annotations: new Map([[PlacesUtils.CHARSET_ANNO, charset]])
+    });
+  },
+
   /**
    * Allows opening of javascript/data URI only if the given node is
    * bookmarked (see bug 224521).

browser/components/places/content/bookmarkProperties.js

@@ -468,8 +468,9 @@ var BookmarkPropertiesPanel = {
       if (this._postData)
         info.postData = this._postData;
 
-      if (this._charSet && !PrivateBrowsingUtils.isWindowPrivate(window))
-        PlacesUtils.setCharsetForURI(this._uri, this._charSet);
+      if (this._charSet) {
+        PlacesUIUtils.setCharsetForPage(this._uri, this._charSet, window).catch(Cu.reportError);
+      }
 
       itemGuid = await PlacesTransactions.NewBookmark(info).transact();
     } else if (this._itemType == LIVEMARK_CONTAINER) {

browser/components/places/tests/unit/test_PUIU_setCharsetForPage.js

@@ -0,0 +1,101 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const {PrivateBrowsingUtils} =
+  ChromeUtils.import("resource://gre/modules/PrivateBrowsingUtils.jsm", {});
+
+const UTF8 = "UTF-8";
+const UTF16 = "UTF-16";
+const CHARSET_ANNO = PlacesUtils.CHARSET_ANNO;
+
+const TEST_URI = "http://foo.com";
+const TEST_BOOKMARKED_URI = "http://bar.com";
+
+add_task(function setup() {
+  let savedIsWindowPrivateFunc = PrivateBrowsingUtils.isWindowPrivate;
+  PrivateBrowsingUtils.isWindowPrivate = () => false;
+
+  registerCleanupFunction(() => {
+    PrivateBrowsingUtils.isWindowPrivate = savedIsWindowPrivateFunc;
+  });
+});
+
+add_task(async function test_simple_add() {
+  // add pages to history
+  await PlacesTestUtils.addVisits(TEST_URI);
+  await PlacesTestUtils.addVisits(TEST_BOOKMARKED_URI);
+
+  // create bookmarks on TEST_BOOKMARKED_URI
+  await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    url: TEST_BOOKMARKED_URI,
+    title: TEST_BOOKMARKED_URI.spec,
+  });
+  await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    url: TEST_BOOKMARKED_URI,
+    title: TEST_BOOKMARKED_URI.spec,
+  });
+
+  // set charset on not-bookmarked page
+  await PlacesUIUtils.setCharsetForPage(TEST_URI, UTF16, {});
+  // set charset on bookmarked page
+  await PlacesUIUtils.setCharsetForPage(TEST_BOOKMARKED_URI, UTF16, {});
+
+  let pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeAnnotations: true});
+  Assert.equal(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), UTF16,
+    "Should return correct charset for a not-bookmarked page");
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_BOOKMARKED_URI, {includeAnnotations: true});
+  Assert.equal(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), UTF16,
+    "Should return correct charset for a bookmarked page");
+
+  await PlacesUtils.history.clear();
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeAnnotations: true});
+  Assert.ok(!pageInfo, "Should not return pageInfo for a page after history cleared");
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_BOOKMARKED_URI, {includeAnnotations: true});
+  Assert.equal(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), UTF16,
+    "Charset should still be set for a bookmarked page after history clear");
+
+  await PlacesUIUtils.setCharsetForPage(TEST_BOOKMARKED_URI, "");
+  pageInfo = await PlacesUtils.history.fetch(TEST_BOOKMARKED_URI, {includeAnnotations: true});
+  Assert.strictEqual(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), undefined,
+    "Should not have a charset after it has been removed from the page");
+});
+
+add_task(async function test_utf8_clears_saved_anno() {
+  await PlacesUtils.history.clear();
+  await PlacesTestUtils.addVisits(TEST_URI);
+
+  // set charset on bookmarked page
+  await PlacesUIUtils.setCharsetForPage(TEST_URI, UTF16, {});
+
+  let pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeAnnotations: true});
+  Assert.equal(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), UTF16,
+    "Should return correct charset for a not-bookmarked page");
+
+  // Now set the bookmark to a UTF-8 charset.
+  await PlacesUIUtils.setCharsetForPage(TEST_URI, UTF8, {});
+
+  pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeAnnotations: true});
+  Assert.strictEqual(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), undefined,
+    "Should have removed the charset for a UTF-8 page.");
+});
+
+add_task(async function test_private_browsing_not_saved() {
+  await PlacesUtils.history.clear();
+  await PlacesTestUtils.addVisits(TEST_URI);
+
+  // set charset on bookmarked page, but pretend this is a private browsing window.
+  PrivateBrowsingUtils.isWindowPrivate = () => true;
+  await PlacesUIUtils.setCharsetForPage(TEST_URI, UTF16, {});
+
+  let pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeAnnotations: true});
+  Assert.strictEqual(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), undefined,
+    "Should not have set the charset in a private browsing window.");
+});

browser/components/places/tests/unit/xpcshell.ini

@@ -18,3 +18,4 @@ support-files =
 [test_browserGlue_restore.js]
 [test_clearHistory_shutdown.js]
 [test_PUIU_batchUpdatesForNode.js]
+[test_PUIU_setCharsetForPage.js]

toolkit/components/places/Bookmarks.jsm

@@ -1395,7 +1395,7 @@ var Bookmarks = Object.freeze({
       JOIN moz_bookmarks c ON c.parent = b.id
       WHERE p.guid = :tagsGuid
       GROUP BY name
-      ORDER BY name COLLATE nocase ASC 
+      ORDER BY name COLLATE nocase ASC
     `, { tagsGuid: this.tagsGuid });
     return rows.map(r => ({
       name: r.getResultByName("name"),
@@ -1947,7 +1947,17 @@ async function handleBookmarkItemSpecialData(itemId, item) {
   }
   if ("charset" in item && item.charset) {
     try {
-      await PlacesUtils.setCharsetForURI(NetUtil.newURI(item.url), item.charset);
+      // UTF-8 is the default. If we are passed the value then set it to null,
+      // to ensure any charset is removed from the database.
+      let charset = item.charset;
+      if (item.charset.toLowerCase() == "utf-8") {
+        charset = null;
+      }
+
+      await PlacesUtils.history.update({
+        url: item.url,
+        annotations: new Map([[PlacesUtils.CHARSET_ANNO, charset]])
+      });
     } catch (ex) {
       Cu.reportError(`Failed to set charset "${item.charset}" for ${item.url}: ${ex}`);
     }

toolkit/components/places/PlacesUtils.jsm

@@ -1484,31 +1484,6 @@ var PlacesUtils = {
     return db.executeBeforeShutdown(name, task);
   },
 
-  /**
-   * Sets the character-set for a URI.
-   *
-   * @param {nsIURI} aURI
-   * @param {String} aCharset character-set value.
-   * @return {Promise}
-   */
-  setCharsetForURI: function PU_setCharsetForURI(aURI, aCharset) {
-    return new Promise(resolve => {
-      // Delaying to catch issues with asynchronous behavior while waiting
-      // to implement asynchronous annotations in bug 699844.
-      Services.tm.dispatchToMainThread(function() {
-        if (aCharset && aCharset.length > 0) {
-          PlacesUtils.annotations.setPageAnnotation(
-            aURI, PlacesUtils.CHARSET_ANNO, aCharset, 0,
-            Ci.nsIAnnotationService.EXPIRE_NEVER);
-        } else {
-          PlacesUtils.annotations.removePageAnnotation(
-            aURI, PlacesUtils.CHARSET_ANNO);
-        }
-        resolve();
-      });
-    });
-  },
-
   /**
    * Gets favicon data for a given page url.
    *

toolkit/components/places/tests/bookmarks/test_1129529.js

@@ -24,7 +24,7 @@ add_task(async function() {
           guid: "___guid1____",
           index: 0,
           id: 3,
-          charset: "UTF-8",
+          charset: "UTF-16",
           tags: "tag0",
           type: "text/x-moz-place",
           dateAdded: now,
@@ -35,7 +35,7 @@ add_task(async function() {
           guid: "___guid2____",
           index: 1,
           id: 4,
-          charset: "UTF-8",
+          charset: "UTF-16",
           tags: "tag1,a" + "0123456789".repeat(10), // 101 chars
           type: "text/x-moz-place",
           dateAdded: now,
@@ -46,7 +46,7 @@ add_task(async function() {
           guid: "___guid3____",
           index: 2,
           id: 5,
-          charset: "UTF-8",
+          charset: "UTF-16",
           tags: "tag2",
           type: "text/x-moz-place",
           dateAdded: now,
@@ -67,7 +67,7 @@ add_task(async function() {
 
   for (let i = 0; i < unsortedBookmarks.length; ++i) {
     let bookmark = unsortedBookmarks[i];
-    Assert.equal(bookmark.charset, "UTF-8");
+    Assert.equal(bookmark.charset, "UTF-16");
     Assert.equal(bookmark.dateAdded, now);
     Assert.equal(bookmark.lastModified, now);
     Assert.equal(bookmark.uri, "http://test" + i + ".com/");

toolkit/components/places/tests/unit/test_317472.js

@@ -1,65 +0,0 @@
-/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
-/* vim:set ts=2 sw=2 sts=2 et: */
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-const charset = "UTF-8";
-const CHARSET_ANNO = PlacesUtils.CHARSET_ANNO;
-
-const TEST_URI = uri("http://foo.com");
-const TEST_BOOKMARKED_URI = uri("http://bar.com");
-
-add_task(async function test_execute() {
-  // add pages to history
-  await PlacesTestUtils.addVisits(TEST_URI);
-  await PlacesTestUtils.addVisits(TEST_BOOKMARKED_URI);
-
-  // create bookmarks on TEST_BOOKMARKED_URI
-  await PlacesUtils.bookmarks.insert({
-    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-    url: TEST_BOOKMARKED_URI,
-    title: TEST_BOOKMARKED_URI.spec,
-  });
-  await PlacesUtils.bookmarks.insert({
-    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
-    url: TEST_BOOKMARKED_URI,
-    title: TEST_BOOKMARKED_URI.spec,
-  });
-
-  // set charset on not-bookmarked page
-  await PlacesUtils.setCharsetForURI(TEST_URI, charset);
-  // set charset on bookmarked page
-  await PlacesUtils.setCharsetForURI(TEST_BOOKMARKED_URI, charset);
-
-  // check that we have created a page annotation
-  Assert.equal(PlacesUtils.annotations.getPageAnnotation(TEST_URI, CHARSET_ANNO), charset);
-
-  let pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeAnnotations: true});
-  Assert.equal(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), charset,
-    "Should return correct charset for a not-bookmarked page");
-
-  pageInfo = await PlacesUtils.history.fetch(TEST_BOOKMARKED_URI, {includeAnnotations: true});
-  Assert.equal(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), charset,
-    "Should return correct charset for a bookmarked page");
-
-  await PlacesUtils.history.clear();
-
-  pageInfo = await PlacesUtils.history.fetch(TEST_URI, {includeAnnotations: true});
-  Assert.ok(!pageInfo, "Should not return pageInfo for a page after history cleared");
-
-  // check that page annotation has been removed
-  try {
-    PlacesUtils.annotations.getPageAnnotation(TEST_URI, CHARSET_ANNO);
-    do_throw("Charset page annotation has not been removed correctly");
-  } catch (e) {}
-
-  pageInfo = await PlacesUtils.history.fetch(TEST_BOOKMARKED_URI, {includeAnnotations: true});
-  Assert.equal(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), charset,
-    "Charset should still be set for a bookmarked page after history clear");
-
-  await PlacesUtils.setCharsetForURI(TEST_BOOKMARKED_URI, "");
-  pageInfo = await PlacesUtils.history.fetch(TEST_BOOKMARKED_URI, {includeAnnotations: true});
-  Assert.notEqual(pageInfo.annotations.get(PlacesUtils.CHARSET_ANNO), charset,
-    "Should not have a charset after it has been removed from the page");
-});

toolkit/components/places/tests/unit/test_bookmarks_html_escape_entities.js

@@ -25,9 +25,11 @@ add_task(async function() {
     title: unescaped
   });
   await PlacesUtils.keywords.insert({ url, keyword: unescaped, postData: unescaped });
-  let uri = Services.io.newURI(url);
-  PlacesUtils.tagging.tagURI(uri, [unescaped]);
-  await PlacesUtils.setCharsetForURI(uri, unescaped);
+  PlacesUtils.tagging.tagURI(Services.io.newURI(url), [unescaped]);
+  await PlacesUtils.history.update({
+    url,
+    annotations: new Map([[PlacesUtils.CHARSET_ANNO, unescaped]]),
+  });
   PlacesUtils.annotations.setItemAnnotation(
     await PlacesUtils.promiseItemId(bm.guid),
     DESCRIPTION_ANNO, unescaped, 0, PlacesUtils.annotations.EXPIRE_NEVER);

toolkit/components/places/tests/unit/test_promiseBookmarksTree.js

@@ -212,7 +212,10 @@ add_task(async function() {
                        annotations: [{ name: "TestAnnoA", value: "TestVal2"}]});
   let urlWithCharsetAndFavicon = uri("http://charset.and.favicon");
   await new_bookmark({ parentGuid: folderGuid, url: urlWithCharsetAndFavicon });
-  await PlacesUtils.setCharsetForURI(urlWithCharsetAndFavicon, "UTF-8");
+  await PlacesUtils.history.update({
+    url: urlWithCharsetAndFavicon,
+    annotations: new Map([[PlacesUtils.CHARSET_ANNO, "UTF-16"]]),
+  });
   await setFaviconForPage(urlWithCharsetAndFavicon, SMALLPNG_DATA_URI);
   // Test the default places root without specifying it.
   await test_promiseBookmarksTreeAgainstResult();

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

@@ -16,7 +16,6 @@ support-files =
   places.sparse.sqlite
 
 [test_000_frecency.js]
-[test_317472.js]
 [test_331487.js]
 [test_384370.js]
 [test_385397.js]