Bug 1588329 - Centralize array chunking in `PlacesUtils.chunkArray`. r?mak! draft
authorLina Cambridge <lina@yakshaving.ninja>
Sat, 12 Oct 2019 23:21:45 +0000
changeset 2378198 e31679dc527279c29f6049ba4c0f24c098913b39
parent 2378197 1363fe68bdd51223343e1d57b657dfe386b22c90
child 2378199 c7b03d292e49fffbeedf3ffdbd356a5de3837f8c
push id434587
push userreviewbot
push dateSat, 12 Oct 2019 23:22:06 +0000
treeherdertry@55835b18a856 [default view] [failures only]
reviewersmak
bugs1588329
milestone71.0a1
Bug 1588329 - Centralize array chunking in `PlacesUtils.chunkArray`. r?mak! Differential Diff: PHID-DIFF-efk7bitofl67la4qghav
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_syncengine_sync.js
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/head_sync.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -33,17 +33,17 @@ const { Collection, CryptoWrapper } = Ch
 const { Resource } = ChromeUtils.import("resource://services-sync/resource.js");
 const { SerializableSet, Svc, Utils } = ChromeUtils.import(
   "resource://services-sync/util.js"
 );
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   fxAccounts: "resource://gre/modules/FxAccounts.jsm",
   OS: "resource://gre/modules/osfile.jsm",
-  PlacesSyncUtils: "resource://gre/modules/PlacesSyncUtils.jsm",
+  PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
 });
 
 function ensureDirectory(path) {
   let basename = OS.Path.dirname(path);
   return OS.File.makeDir(basename, { from: OS.Constants.Path.profileDir });
 }
 
 /*
@@ -1305,17 +1305,17 @@ SyncEngine.prototype = {
 
     let backfilledItems = this.itemSource();
 
     backfilledItems.sort = "newest";
     backfilledItems.full = true;
 
     // `getBatched` includes the list of IDs as a query parameter, so we need to fetch
     // records in chunks to avoid exceeding URI length limits.
-    for (let [, ids] of PlacesSyncUtils.chunkArray(
+    for (let ids of PlacesUtils.chunkArray(
       idsToBackfill,
       this.guidFetchBatchSize
     )) {
       backfilledItems.ids = ids;
 
       let { response, records } = await backfilledItems.getBatched(
         this.downloadBatchSize
       );
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -1345,20 +1345,17 @@ BufferedBookmarksStore.prototype = {
           this.name
         );
       },
     });
   },
 
   async applyIncomingBatch(records) {
     let buf = await this.ensureOpenMirror();
-    for (let [, chunk] of PlacesSyncUtils.chunkArray(
-      records,
-      this._batchChunkSize
-    )) {
+    for (let chunk of PlacesUtils.chunkArray(records, this._batchChunkSize)) {
       await buf.store(chunk);
     }
     // Array of failed records.
     return [];
   },
 
   async applyIncoming(record) {
     let buf = await this.ensureOpenMirror();
--- a/services/sync/tests/unit/test_syncengine_sync.js
+++ b/services/sync/tests/unit/test_syncengine_sync.js
@@ -807,20 +807,17 @@ add_task(async function test_processInco
 
   // Engine that alternates between failing and applying every 2 records.
   let engine = makeRotaryEngine();
   engine._store._applyIncomingBatch = engine._store.applyIncomingBatch;
   engine._store.applyIncomingBatch = async function(records) {
     let sortedRecords = records.sort((a, b) => (a.id > b.id ? 1 : -1));
     let recordsToApply = [],
       recordsToFail = [];
-    let chunks = Array.from(
-      PlacesSyncUtils.chunkArray(sortedRecords, 2),
-      ([, chunk]) => chunk
-    );
+    let chunks = Array.from(PlacesUtils.chunkArray(sortedRecords, 2));
     for (let i = 0; i < chunks.length; i++) {
       (i % 2 === 0 ? recordsToFail : recordsToApply).push(...chunks[i]);
     }
     await engine._store._applyIncomingBatch(recordsToApply);
     return recordsToFail.map(record => record.id);
   };
 
   // Create a batch of server side records.
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -2212,17 +2212,20 @@ function insertBookmarkTree(items, sourc
          (SELECT id FROM moz_bookmarks WHERE guid = :parentGuid),
          IFNULL(:index, (SELECT count(*) FROM moz_bookmarks WHERE parent = :rootId)),
          NULLIF(:title, ''), :date_added, :last_modified, :guid,
          :syncChangeCounter, :syncStatus)`,
           items
         );
 
         // Remove stale tombstones for new items.
-        for (let chunk of chunkArray(items, SQLITE_MAX_VARIABLE_NUMBER)) {
+        for (let chunk of PlacesUtils.chunkArray(
+          items,
+          SQLITE_MAX_VARIABLE_NUMBER
+        )) {
           await db.executeCached(
             `DELETE FROM moz_bookmarks_deleted WHERE guid IN (${new Array(
               chunk.length
             )
               .fill("?")
               .join(",")})`,
             chunk.map(item => item.guid)
           );
@@ -2620,17 +2623,20 @@ function removeBookmarks(items, options)
               throw new Error("Cannot remove a non-empty folder.");
             }
             urls = urls.concat(
               await removeFoldersContents(db, [item.guid], options)
             );
           }
         }
 
-        for (let chunk of chunkArray(items, SQLITE_MAX_VARIABLE_NUMBER)) {
+        for (let chunk of PlacesUtils.chunkArray(
+          items,
+          SQLITE_MAX_VARIABLE_NUMBER
+        )) {
           // We don't go through the annotations service for this cause otherwise
           // we'd get a pointless onItemChanged notification and it would also
           // set lastModified to an unexpected value.
           await removeAnnotationsForItems(db, chunk);
 
           // Remove the bookmarks.
           await db.executeCached(
             `DELETE FROM moz_bookmarks WHERE guid IN (${new Array(chunk.length)
@@ -3385,27 +3391,16 @@ function adjustSeparatorsSyncCounter(
       delta: syncChangeDelta,
       parent: parentId,
       start_index: startIndex,
       item_type: Bookmarks.TYPE_SEPARATOR,
     }
   );
 }
 
-function* chunkArray(array, chunkLength) {
-  if (array.length <= chunkLength) {
-    yield array;
-    return;
-  }
-  let startIndex = 0;
-  while (startIndex < array.length) {
-    yield array.slice(startIndex, (startIndex += chunkLength));
-  }
-}
-
 /**
  * Generates a list of "?" SQL bindings based on input array length.
  * @param {array} values an array of values.
  * @param {string} [prefix] a string to prefix to the placeholder.
  * @param {string} [suffix] a string to suffix to the placeholder.
  * @returns {string} placeholders is a string made of question marks and commas,
  *          one per value.
  */
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -21,37 +21,17 @@ ChromeUtils.defineModuleGetter(
 );
 
 /**
  * This module exports functions for Sync to use when applying remote
  * records. The calls are similar to those in `Bookmarks.jsm` and
  * `nsINavBookmarksService`, with special handling for
  * tags, keywords, synced annotations, and missing parents.
  */
-var PlacesSyncUtils = {
-  /**
-   * Auxiliary generator function that yields an array in chunks
-   *
-   * @param  array
-   * @param  chunkLength
-   * @yields {Array}
-   *         New Array with the next chunkLength elements of array.
-   *         If the array has less than chunkLength elements, yields all of them
-   */
-  *chunkArray(array, chunkLength) {
-    if (!array.length || chunkLength <= 0) {
-      return;
-    }
-    let startIndex = 0;
-    while (startIndex < array.length) {
-      yield [startIndex, array.slice(startIndex, startIndex + chunkLength)];
-      startIndex += chunkLength;
-    }
-  },
-};
+var PlacesSyncUtils = {};
 
 const { SOURCE_SYNC } = Ci.nsINavBookmarksService;
 
 const MICROSECONDS_PER_SECOND = 1000000;
 const SQLITE_MAX_VARIABLE_NUMBER = 999;
 
 const MOBILE_BOOKMARKS_PREF = "browser.bookmarks.showMobileBookmarks";
 
@@ -253,17 +233,17 @@ const HistorySyncUtils = (PlacesSyncUtil
    */
   async determineNonSyncableGuids(guids) {
     // Filter out hidden pages and `TRANSITION_FRAMED_LINK` visits. These are
     // excluded when rendering the history menu, so we use the same constraints
     // for Sync. We also don't want to sync `TRANSITION_EMBED` visits, but those
     // aren't stored in the database.
     let db = await PlacesUtils.promiseDBConnection();
     let nonSyncableGuids = [];
-    for (let [, chunk] of PlacesSyncUtils.chunkArray(
+    for (let chunk of PlacesUtils.chunkArray(
       guids,
       SQLITE_MAX_VARIABLE_NUMBER
     )) {
       let rows = await db.execute(
         `
         SELECT DISTINCT p.guid FROM moz_places p
         JOIN moz_historyvisits v ON p.id = v.place_id
         WHERE p.guid IN (${new Array(chunk.length).fill("?").join(",")}) AND
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1902,16 +1902,34 @@ var PlacesUtils = {
         await new Promise(resolve => {
           Services.tm.dispatchToMainThread(resolve);
         });
       }
     }
 
     return rootItem;
   },
+
+  /**
+   * Returns a generator that iterates over `array` and yields slices of no
+   * more than `chunkLength` elements at a time.
+   */
+  *chunkArray(array, chunkLength) {
+    if (!array.length || chunkLength <= 0) {
+      return;
+    }
+    if (array.length <= chunkLength) {
+      yield array;
+      return;
+    }
+    let startIndex = 0;
+    while (startIndex < array.length) {
+      yield array.slice(startIndex, (startIndex += chunkLength));
+    }
+  },
 };
 
 XPCOMUtils.defineLazyGetter(PlacesUtils, "history", function() {
   let hs = Cc["@mozilla.org/browser/nav-history-service;1"].getService(
     Ci.nsINavHistoryService
   );
   return Object.freeze(
     new Proxy(hs, {
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -1020,17 +1020,18 @@ class SyncedBookmarksMirror {
         kind: Ci.mozISyncedBookmarksMerger.KIND_FOLDER,
         dateAdded,
         title,
       }
     );
 
     let children = record.children;
     if (children && Array.isArray(children)) {
-      for (let [offset, chunk] of PlacesSyncUtils.chunkArray(
+      let offset = 0;
+      for (let chunk of PlacesUtils.chunkArray(
         children,
         SQLITE_MAX_VARIABLE_NUMBER - 1
       )) {
         if (signal.aborted) {
           throw new SyncedBookmarksMirror.InterruptedError(
             "Interrupted while storing children for incoming folder"
           );
         }
@@ -1044,16 +1045,17 @@ class SyncedBookmarksMirror {
           (_, index) => `(?${index + 2}, ?1, ${offset + index})`
         ).join(",");
         await this.db.execute(
           `
           INSERT INTO structure(guid, parentGuid, position)
           VALUES ${valuesFragment}`,
           [guid, ...chunk.map(PlacesSyncUtils.bookmarks.recordIdToGuid)]
         );
+        offset += chunk.length;
       }
     }
   }
 
   async storeRemoteLivemark(record, { needsMerge }) {
     let guid = PlacesSyncUtils.bookmarks.recordIdToGuid(record.id);
     let parentGuid = PlacesSyncUtils.bookmarks.recordIdToGuid(record.parentid);
     let serverModified = determineServerModified(record);
--- a/toolkit/components/places/tests/sync/head_sync.js
+++ b/toolkit/components/places/tests/sync/head_sync.js
@@ -166,17 +166,17 @@ function inspectChangeRecords(changeReco
   results.updated.sort();
   results.deleted.sort();
   return results;
 }
 
 async function promiseManyDatesAdded(guids) {
   let datesAdded = new Map();
   let db = await PlacesUtils.promiseDBConnection();
-  for (let [, chunk] of PlacesSyncUtils.chunkArray(guids, 100)) {
+  for (let chunk of PlacesUtils.chunkArray(guids, 100)) {
     let rows = await db.executeCached(
       `
       SELECT guid, dateAdded FROM moz_bookmarks
       WHERE guid IN (${new Array(chunk.length).fill("?").join(",")})`,
       chunk
     );
     if (rows.length != chunk.length) {
       throw new TypeError("Can't fetch date added for nonexistent items");