Bug 1383621 - Use the async bookmarks API to insert the mobile query. r=markh draft
authorKit Cambridge <kit@yakshaving.ninja>
Mon, 24 Jul 2017 16:45:17 -0700
changeset 617595 c2c4cae2093b5323487cce8a5f270c529fa2e069
parent 617496 16ffc1d05422a81099ce8b9b59de66dde4c8b2f0
child 639856 8b9b11a35e28c438cab41ac9bc12ec6a02367d9c
push id71095
push userbmo:kit@mozilla.com
push dateFri, 28 Jul 2017 18:16:47 +0000
reviewersmarkh
bugs1383621
milestone56.0a1
Bug 1383621 - Use the async bookmarks API to insert the mobile query. r=markh MozReview-Commit-ID: KaeXwFHEg7K
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_tracker.js
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/tests/unit/test_sync_utils.js
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -518,17 +518,17 @@ BookmarksEngine.prototype = {
 
   async _orderChildren() {
     await this._store._orderChildren();
     this._store._childrenToOrder = {};
   },
 
   async _syncFinish() {
     await SyncEngine.prototype._syncFinish.call(this);
-    this._tracker._ensureMobileQuery();
+    await PlacesSyncUtils.bookmarks.ensureMobileQuery();
   },
 
   async _syncCleanup() {
     await SyncEngine.prototype._syncCleanup.call(this);
     delete this._guidMap;
   },
 
   async _createRecord(id) {
@@ -1018,62 +1018,16 @@ BookmarksTracker.prototype = {
     if (IGNORED_SOURCES.includes(source)) {
       return;
     }
 
     this._log.trace("onItemRemoved: " + itemId);
     this._upScore();
   },
 
-  _ensureMobileQuery: function _ensureMobileQuery() {
-    Services.prefs.setBoolPref("browser.bookmarks.showMobileBookmarks", true);
-    let find = val =>
-      PlacesUtils.annotations.getItemsWithAnnotation(ORGANIZERQUERY_ANNO, {}).filter(
-        id => PlacesUtils.annotations.getItemAnnotation(id, ORGANIZERQUERY_ANNO) == val
-      );
-
-    // Don't continue if the Library isn't ready
-    let all = find(ALLBOOKMARKS_ANNO);
-    if (all.length == 0)
-      return;
-
-    let mobile = find(MOBILE_ANNO);
-    let queryURI = Utils.makeURI("place:folder=" + PlacesUtils.mobileFolderId);
-    let title = PlacesBundle.GetStringFromName("MobileBookmarksFolderTitle");
-
-    // Don't add OR remove the mobile bookmarks if there's nothing.
-    if (PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.mobileFolderId, 0) == -1) {
-      if (mobile.length != 0)
-        PlacesUtils.bookmarks.removeItem(mobile[0], SOURCE_SYNC);
-    } else if (mobile.length == 0) {
-      // Add the mobile bookmarks query if it doesn't exist
-      let query = PlacesUtils.bookmarks.insertBookmark(all[0], queryURI, -1, title, /* guid */ null, SOURCE_SYNC);
-      PlacesUtils.annotations.setItemAnnotation(query, ORGANIZERQUERY_ANNO, MOBILE_ANNO, 0,
-                                  PlacesUtils.annotations.EXPIRE_NEVER, SOURCE_SYNC);
-      PlacesUtils.annotations.setItemAnnotation(query, PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO, 1, 0,
-                                  PlacesUtils.annotations.EXPIRE_NEVER, SOURCE_SYNC);
-    } else {
-      // Make sure the existing query URL and title are correct
-      if (!PlacesUtils.bookmarks.getBookmarkURI(mobile[0]).equals(queryURI)) {
-        PlacesUtils.bookmarks.changeBookmarkURI(mobile[0], queryURI,
-                                                SOURCE_SYNC);
-      }
-      let queryTitle = PlacesUtils.bookmarks.getItemTitle(mobile[0]);
-      if (queryTitle != title) {
-        PlacesUtils.bookmarks.setItemTitle(mobile[0], title, SOURCE_SYNC);
-      }
-      let rootTitle =
-        PlacesUtils.bookmarks.getItemTitle(PlacesUtils.mobileFolderId);
-      if (rootTitle != title) {
-        PlacesUtils.bookmarks.setItemTitle(PlacesUtils.mobileFolderId, title,
-                                           SOURCE_SYNC);
-      }
-    }
-  },
-
   // This method is oddly structured, but the idea is to return as quickly as
   // possible -- this handler gets called *every time* a bookmark changes, for
   // *each change*.
   onItemChanged: function BMT_onItemChanged(itemId, property, isAnno, value,
                                             lastModified, itemType, parentId,
                                             guid, parentGuid, oldValue,
                                             source) {
     if (IGNORED_SOURCES.includes(source)) {
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -1688,87 +1688,16 @@ add_task(async function test_onItemDelet
     await verifyTrackedItems([fx_guid, tb_guid, folder1_guid, folder2_guid]);
     do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE * 3);
   } finally {
     _("Clean up.");
     await cleanup();
   }
 });
 
-add_task(async function test_mobile_query() {
-  _("Ensure we correctly create the mobile query");
-
-  try {
-    await startTracking();
-
-    // Creates the organizer queries as a side effect.
-    let leftPaneId = PlacesUIUtils.leftPaneFolderId;
-    _(`Left pane root ID: ${leftPaneId}`);
-
-    let allBookmarksGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
-      "PlacesOrganizer/OrganizerQuery", "AllBookmarks");
-    equal(allBookmarksGuids.length, 1, "Should create folder with all bookmarks queries");
-    let allBookmarkGuid = allBookmarksGuids[0];
-
-    _("Try creating query after organizer is ready");
-    tracker._ensureMobileQuery();
-    let queryGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
-      "PlacesOrganizer/OrganizerQuery", "MobileBookmarks");
-    equal(queryGuids.length, 0, "Should not create query without any mobile bookmarks");
-
-    _("Insert mobile bookmark, then create query");
-    let mozBmk = await PlacesUtils.bookmarks.insert({
-      parentGuid: PlacesUtils.bookmarks.mobileGuid,
-      url: "https://mozilla.org",
-    });
-    tracker._ensureMobileQuery();
-    queryGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
-      "PlacesOrganizer/OrganizerQuery", "MobileBookmarks");
-    equal(queryGuids.length, 1, "Should create query once mobile bookmarks exist");
-
-    let queryGuid = queryGuids[0];
-
-    let queryInfo = await PlacesUtils.bookmarks.fetch(queryGuid);
-    equal(queryInfo.url, `place:folder=${PlacesUtils.mobileFolderId}`, "Query should point to mobile root");
-    equal(queryInfo.title, "Mobile Bookmarks", "Query title should be localized");
-    equal(queryInfo.parentGuid, allBookmarkGuid, "Should append mobile query to all bookmarks queries");
-
-    _("Rename root and query, then recreate");
-    await PlacesUtils.bookmarks.update({
-      guid: PlacesUtils.bookmarks.mobileGuid,
-      title: "renamed root",
-    });
-    await PlacesUtils.bookmarks.update({
-      guid: queryGuid,
-      title: "renamed query",
-    });
-    tracker._ensureMobileQuery();
-    let rootInfo = await PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.mobileGuid);
-    equal(rootInfo.title, "Mobile Bookmarks", "Should fix root title");
-    queryInfo = await PlacesUtils.bookmarks.fetch(queryGuid);
-    equal(queryInfo.title, "Mobile Bookmarks", "Should fix query title");
-
-    _("Point query to different folder");
-    await PlacesUtils.bookmarks.update({
-      guid: queryGuid,
-      url: "place:folder=BOOKMARKS_MENU",
-    });
-    tracker._ensureMobileQuery();
-    queryInfo = await PlacesUtils.bookmarks.fetch(queryGuid);
-    equal(queryInfo.url.href, `place:folder=${PlacesUtils.mobileFolderId}`,
-      "Should fix query URL to point to mobile root");
-
-    _("We shouldn't track the query or the left pane root");
-    await verifyTrackedItems([mozBmk.guid, "mobile"]);
-  } finally {
-    _("Clean up.");
-    await cleanup();
-  }
-});
-
 add_task(async function test_skip_migration() {
   await insertBookmarksToMigrate();
 
   let originalTombstones = await PlacesTestUtils.fetchSyncTombstones();
   let originalFields = await PlacesTestUtils.fetchBookmarkSyncFields(
     "0gtWTOgYcoJD", "0dbpnMdxKxfg", "r5ouWdPB3l28", "YK5Bdq5MIqL6");
 
   let filePath = OS.Path.join(OS.Constants.Path.profileDir, "weave", "changes",
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -5,16 +5,17 @@
 "use strict";
 
 this.EXPORTED_SYMBOLS = ["PlacesSyncUtils"];
 
 const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
 
 Cu.importGlobalProperties(["URL", "URLSearchParams"]);
 
+Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "Log",
                                   "resource://gre/modules/Log.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 
 /**
@@ -24,16 +25,21 @@ XPCOMUtils.defineLazyModuleGetter(this, 
  * tags, keywords, synced annotations, and missing parents.
  */
 var PlacesSyncUtils = {};
 
 const { SOURCE_SYNC } = Ci.nsINavBookmarksService;
 
 const MICROSECONDS_PER_SECOND = 1000000;
 
+const ORGANIZER_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
+const ORGANIZER_ALL_BOOKMARKS_ANNO_VALUE = "AllBookmarks";
+const ORGANIZER_MOBILE_QUERY_ANNO_VALUE = "MobileBookmarks";
+const MOBILE_BOOKMARKS_PREF = "browser.bookmarks.showMobileBookmarks";
+
 // These are defined as lazy getters to defer initializing the bookmarks
 // service until it's needed.
 XPCOMUtils.defineLazyGetter(this, "ROOT_SYNC_ID_TO_GUID", () => ({
   menu: PlacesUtils.bookmarks.menuGuid,
   places: PlacesUtils.bookmarks.rootGuid,
   tags: PlacesUtils.bookmarks.tagsGuid,
   toolbar: PlacesUtils.bookmarks.toolbarGuid,
   unfiled: PlacesUtils.bookmarks.unfiledGuid,
@@ -831,16 +837,96 @@ const BookmarkSyncUtils = PlacesSyncUtil
     const possible = [+existingMillis, +serverMillis].filter(n => !isNaN(n) && n > lowerBound);
     if (!possible.length) {
       return undefined;
     }
     return Math.min(...possible);
   },
 
   /**
+   * Rebuilds the left pane query for the mobile root under "All Bookmarks" if
+   * necessary. Sync calls this method at the end of each bookmark sync. This
+   * code should eventually move to `PlacesUIUtils#maybeRebuildLeftPane`; see
+   * bug 647605.
+   *
+   * - If there are no mobile bookmarks, the query will not be created, or
+   *   will be removed if it already exists.
+   * - If there are mobile bookmarks, the query will be created if it doesn't
+   *   exist, or will be updated with the correct title and URL otherwise.
+   */
+  async ensureMobileQuery() {
+    Services.prefs.setBoolPref(MOBILE_BOOKMARKS_PREF, true);
+
+    let db = await PlacesUtils.promiseDBConnection();
+
+    let maybeAllBookmarksGuids = await fetchGuidsWithAnno(db,
+      ORGANIZER_QUERY_ANNO, ORGANIZER_ALL_BOOKMARKS_ANNO_VALUE);
+    if (!maybeAllBookmarksGuids.length) {
+      return;
+    }
+
+    let hasMobileBookmarks = (await db.executeCached(`SELECT EXISTS(
+      SELECT 1 FROM moz_bookmarks b
+      JOIN moz_bookmarks p ON p.id = b.parent
+      WHERE p.guid = :mobileGuid
+    ) AS hasMobile`, {
+      mobileGuid: PlacesUtils.bookmarks.mobileGuid,
+    }))[0].getResultByName("hasMobile");
+
+    let allBookmarksGuid = maybeAllBookmarksGuids[0];
+    let mobileTitle = PlacesUtils.getString("MobileBookmarksFolderTitle");
+
+    let maybeMobileQueryGuids = await fetchGuidsWithAnno(db,
+      ORGANIZER_QUERY_ANNO, ORGANIZER_MOBILE_QUERY_ANNO_VALUE);
+    if (maybeMobileQueryGuids.length) {
+      let mobileQueryGuid = maybeMobileQueryGuids[0];
+      if (hasMobileBookmarks) {
+        // We have a left pane query for mobile bookmarks, and at least one
+        // mobile bookmark. Make sure the query title is correct.
+        await PlacesUtils.bookmarks.update({
+          guid: mobileQueryGuid,
+          url: "place:folder=MOBILE_BOOKMARKS",
+          title: mobileTitle,
+          source: SOURCE_SYNC,
+        });
+      } else {
+        // We have a left pane query for mobile bookmarks, but no mobile
+        // bookmarks. Remove the query.
+        await PlacesUtils.bookmarks.remove(mobileQueryGuid, {
+          source: SOURCE_SYNC,
+        });
+      }
+    } else if (hasMobileBookmarks) {
+      // We have mobile bookmarks, but no left pane query. Create the query.
+      let mobileQuery = await PlacesUtils.bookmarks.insert({
+        parentGuid: allBookmarksGuid,
+        url: "place:folder=MOBILE_BOOKMARKS",
+        title: mobileTitle,
+        source: SOURCE_SYNC,
+      });
+
+      let mobileQueryId = await PlacesUtils.promiseItemId(mobileQuery.guid);
+
+      PlacesUtils.annotations.setItemAnnotation(mobileQueryId,
+        ORGANIZER_QUERY_ANNO, ORGANIZER_MOBILE_QUERY_ANNO_VALUE, 0,
+        PlacesUtils.annotations.EXPIRE_NEVER, SOURCE_SYNC);
+      PlacesUtils.annotations.setItemAnnotation(mobileQueryId,
+        PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO, 1, 0,
+        PlacesUtils.annotations.EXPIRE_NEVER, SOURCE_SYNC);
+    }
+
+    // Make sure the mobile root title matches the query.
+    await PlacesUtils.bookmarks.update({
+      guid: PlacesUtils.bookmarks.mobileGuid,
+      title: mobileTitle,
+      source: SOURCE_SYNC,
+    });
+  },
+
+  /**
    * Fetches an array of GUIDs for items that have an annotation set with the
    * given value.
    */
   async fetchGuidsWithAnno(anno, val) {
     let db = await PlacesUtils.promiseDBConnection();
     return fetchGuidsWithAnno(db, anno, val);
   },
 });
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -2663,8 +2663,86 @@ add_task(async function test_migrateOldT
   deepEqual(tombstones, [{
     guid: tombstoneSyncId,
     dateRemoved: new Date(1479162463976),
   }], "Should write tombstone for nonexistent migrated item");
 
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
+
+add_task(async function test_ensureMobileQuery() {
+  do_print("Ensure we correctly create the mobile query");
+
+  const PlacesUIUtils = {};
+  try {
+    Cu.import("resource://gre/modules/PlacesUIUtils.jsm", PlacesUIUtils);
+  } catch (ex) {
+    do_print("Can't build left pane roots; skipping test");
+    return;
+  }
+
+  // Creates the organizer queries as a side effect.
+  let leftPaneId = PlacesUIUtils.leftPaneFolderId;
+  do_print(`Left pane root ID: ${leftPaneId}`);
+
+  let allBookmarksGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
+    "PlacesOrganizer/OrganizerQuery", "AllBookmarks");
+  equal(allBookmarksGuids.length, 1, "Should create folder with all bookmarks queries");
+  let allBookmarkGuid = allBookmarksGuids[0];
+
+  do_print("Try creating query after organizer is ready");
+  await PlacesSyncUtils.bookmarks.ensureMobileQuery()
+  let queryGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
+    "PlacesOrganizer/OrganizerQuery", "MobileBookmarks");
+  equal(queryGuids.length, 0, "Should not create query without any mobile bookmarks");
+
+  do_print("Insert mobile bookmark, then create query");
+  let mozBmk = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.mobileGuid,
+    url: "https://mozilla.org",
+  });
+  await PlacesSyncUtils.bookmarks.ensureMobileQuery()
+  queryGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
+    "PlacesOrganizer/OrganizerQuery", "MobileBookmarks");
+  equal(queryGuids.length, 1, "Should create query once mobile bookmarks exist");
+
+  let queryGuid = queryGuids[0];
+
+  let queryInfo = await PlacesUtils.bookmarks.fetch(queryGuid);
+  equal(queryInfo.url, `place:folder=MOBILE_BOOKMARKS`, "Query should point to mobile root");
+  equal(queryInfo.title, "Mobile Bookmarks", "Query title should be localized");
+  equal(queryInfo.parentGuid, allBookmarkGuid, "Should append mobile query to all bookmarks queries");
+
+  do_print("Rename root and query, then recreate");
+  await PlacesUtils.bookmarks.update({
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    title: "renamed root",
+  });
+  await PlacesUtils.bookmarks.update({
+    guid: queryGuid,
+    title: "renamed query",
+  });
+  await PlacesSyncUtils.bookmarks.ensureMobileQuery()
+  let rootInfo = await PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.mobileGuid);
+  equal(rootInfo.title, "Mobile Bookmarks", "Should fix root title");
+  queryInfo = await PlacesUtils.bookmarks.fetch(queryGuid);
+  equal(queryInfo.title, "Mobile Bookmarks", "Should fix query title");
+
+  do_print("Point query to different folder");
+  await PlacesUtils.bookmarks.update({
+    guid: queryGuid,
+    url: "place:folder=BOOKMARKS_MENU",
+  });
+  await PlacesSyncUtils.bookmarks.ensureMobileQuery()
+  queryInfo = await PlacesUtils.bookmarks.fetch(queryGuid);
+  equal(queryInfo.url.href, `place:folder=MOBILE_BOOKMARKS`,
+    "Should fix query URL to point to mobile root");
+
+  do_print("We shouldn't track the query or the left pane root");
+
+  let changes = await PlacesSyncUtils.bookmarks.pullChanges();
+  deepEqual(Object.keys(changes).sort(), [mozBmk.guid, "mobile"],
+    "Should not track mobile query");
+
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});