Bug 1383621 - Use the async bookmarks API to insert the mobile query. r=markh
authorKit Cambridge <kit@yakshaving.ninja>
Mon, 24 Jul 2017 16:45:17 -0700
changeset 420402 3b56ac96c918e042194781d0021f5f243babc5e7
parent 420401 50ce68c9412ce80cf4bf4f1dc4be9f3c7fcd0c6d
child 420403 ec329722b2f8bad3b1b9d0829e8d89764a879fd1
child 420451 cc29c0db027bca894099bc3148ace3328e710ab8
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1383621
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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();
+});