Bug 1548884 - Enable the synced bookmarks mirror on Nightly and Beta. r=tcsc
authorLina Cambridge <lina@yakshaving.ninja>
Mon, 06 May 2019 20:40:20 +0000
changeset 531599 e391c48bc166bf4cba9dafe7b5c1d1310444269f
parent 531598 07ec0bc698e352f0701a0079996ec9143886ffed
child 531600 0762225c24094c033013ab87c6574c4a0f1a39bd
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc
bugs1548884
milestone68.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 1548884 - Enable the synced bookmarks mirror on Nightly and Beta. r=tcsc The number of Sync users on these channels is low compared to Release, so we can do this without a gradual rollout. This also lets more users test the new bookmark sync engine without manually flipping the pref. Differential Revision: https://phabricator.services.mozilla.com/D29859
services/sync/services-sync.js
services/sync/tests/unit/head_helpers.js
services/sync/tests/unit/test_bookmark_repair_responder.js
services/sync/tests/unit/test_engine_changes_during_sync.js
services/sync/tests/unit/test_telemetry.js
--- a/services/sync/services-sync.js
+++ b/services/sync/services-sync.js
@@ -17,17 +17,23 @@ pref("services.sync.scheduler.fxa.single
 // Note that new engines are typically added with a default of disabled, so
 // when an existing sync user gets the Firefox upgrade that supports the engine
 // it starts as disabled until the user has explicitly opted in.
 // The sync "create account" process typically *will* offer these engines, so
 // they may be flipped to enabled at that time.
 pref("services.sync.engine.addons", true);
 pref("services.sync.engine.addresses", false);
 pref("services.sync.engine.bookmarks", true);
+#ifdef EARLY_BETA_OR_EARLIER
+// Enable the new bookmark sync engine through early Beta, but not release
+// candidates or Release.
+pref("services.sync.engine.bookmarks.buffer", true);
+#else
 pref("services.sync.engine.bookmarks.buffer", false);
+#endif
 pref("services.sync.engine.creditcards", false);
 pref("services.sync.engine.history", true);
 pref("services.sync.engine.passwords", true);
 pref("services.sync.engine.prefs", true);
 pref("services.sync.engine.tabs", true);
 pref("services.sync.engine.tabs.filteredUrls", "^(about:.*|resource:.*|chrome:.*|wyciwyg:.*|file:.*|blob:.*|moz-extension:.*)$");
 
 // The addresses and CC engines might not actually be available at all.
--- a/services/sync/tests/unit/head_helpers.js
+++ b/services/sync/tests/unit/head_helpers.js
@@ -602,8 +602,13 @@ async function assertBookmarksTreeMatche
   let actual = bookmarkNodesToInfos(root.children);
 
   if (!ObjectUtils.deepEqual(actual, expected)) {
     _(`Expected structure for ${rootGuid}`, JSON.stringify(expected));
     _(`Actual structure for ${rootGuid}`, JSON.stringify(actual));
     throw new Assert.constructor.AssertionError({ actual, expected, message });
   }
 }
+
+function bufferedBookmarksEnabled() {
+  return Services.prefs.getBoolPref("services.sync.engine.bookmarks.buffer",
+    false);
+}
--- a/services/sync/tests/unit/test_bookmark_repair_responder.js
+++ b/services/sync/tests/unit/test_bookmark_repair_responder.js
@@ -105,31 +105,31 @@ add_task(async function test_responder_n
 
   await cleanup(server);
 });
 
 // One item requested and we have it locally, but it's not yet on the server.
 add_task(async function test_responder_upload() {
   let server = await makeServer();
 
-  // Pretend we've already synced this bookmark, so that we can ensure it's
-  // uploaded in response to our repair request.
-  let bm = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                title: "Get Firefox",
-                                                url: "http://getfirefox.com/",
-                                                source: PlacesUtils.bookmarks.SOURCES.SYNC });
-
   await Service.sync();
   deepEqual(getServerBookmarks(server).keys().sort(), [
     "menu",
     "mobile",
     "toolbar",
     "unfiled",
   ], "Should only upload roots on first sync");
 
+  // Pretend we've already synced this bookmark, so that we can ensure it's
+  // uploaded in response to our repair request.
+  let bm = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                                title: "Get Firefox",
+                                                url: "http://getfirefox.com/",
+                                                source: PlacesUtils.bookmarks.SOURCES.SYNC });
+
   let request = {
     request: "upload",
     ids: [bm.guid],
     flowID: Utils.makeGUID(),
   };
   let responder = new BookmarkRepairResponder();
   await responder.repair(request, null);
 
@@ -219,33 +219,34 @@ add_task(async function test_responder_t
 add_task(async function test_responder_missing_items() {
   let server = await makeServer();
 
   let fxBmk = await PlacesUtils.bookmarks.insert({
     parentGuid: PlacesUtils.bookmarks.unfiledGuid,
     title: "Get Firefox",
     url: "http://getfirefox.com/",
   });
-  let tbBmk = await PlacesUtils.bookmarks.insert({
-    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-    title: "Get Thunderbird",
-    url: "http://getthunderbird.com/",
-    // Pretend we've already synced Thunderbird.
-    source: PlacesUtils.bookmarks.SOURCES.SYNC,
-  });
 
   await Service.sync();
   deepEqual(getServerBookmarks(server).keys().sort(), [
     "menu",
     "mobile",
     "toolbar",
     "unfiled",
     fxBmk.guid,
   ].sort(), "Should upload roots and Firefox on first sync");
 
+  let tbBmk = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    title: "Get Thunderbird",
+    url: "http://getthunderbird.com/",
+    // Pretend we've already synced Thunderbird.
+    source: PlacesUtils.bookmarks.SOURCES.SYNC,
+  });
+
   _("Request Firefox, Thunderbird, and nonexistent GUID");
   let request = {
     request: "upload",
     ids: [fxBmk.guid, tbBmk.guid, Utils.makeGUID()],
     flowID: Utils.makeGUID(),
   };
   let responder = new BookmarkRepairResponder();
   await responder.repair(request, null);
--- a/services/sync/tests/unit/test_engine_changes_during_sync.js
+++ b/services/sync/tests/unit/test_engine_changes_during_sync.js
@@ -371,16 +371,43 @@ add_task(async function test_bookmark_ch
       // A bookmark that should appear in the results for the tag query.
       let remoteTaggedBmk = new Bookmark("bookmarks", bmk4_guid);
       remoteTaggedBmk.bmkUri     = "https://example.org/";
       remoteTaggedBmk.title      = "Tagged bookmark";
       remoteTaggedBmk.tags       = ["taggy"];
       remoteTaggedBmk.parentName = "Folder 2";
       remoteTaggedBmk.parentid   = folder2_guid;
       collection.insert(bmk4_guid, encryptPayload(remoteTaggedBmk.cleartext));
+
+      collection.insert("toolbar", encryptPayload({
+        id: "toolbar",
+        type: "folder",
+        title: "toolbar",
+        children: [folder1.guid],
+        parentName: "places",
+        parentid: "places",
+      }));
+
+      collection.insert("menu", encryptPayload({
+        id: "menu",
+        type: "folder",
+        title: "menu",
+        children: [bzBmk.guid, folder2_guid],
+        parentName: "places",
+        parentid: "places",
+      }));
+
+      collection.insert(folder1.guid, encryptPayload({
+        id: folder1.guid,
+        type: "folder",
+        title: "Folder 1",
+        children: [bmk2_guid],
+        parentName: "toolbar",
+        parentid: "toolbar",
+      }));
     }
 
     await assertChildGuids(folder1.guid, [tbBmk.guid],
       "Folder should have 1 child before first sync");
 
     let pingsPromise = wait_for_pings(2);
 
     let changes = await PlacesSyncUtils.bookmarks.pullChanges();
@@ -411,34 +438,42 @@ add_task(async function test_bookmark_ch
     ok(bmk3, "Should insert bookmark during first sync to simulate change");
     ok(collection.wbo(bmk3.guid),
       "Changed bookmark should be uploaded after follow-up sync");
 
     let bmk2 = await PlacesUtils.bookmarks.fetch({
       guid: bmk2_guid,
     });
     ok(bmk2, "Remote bookmark should be applied during first sync");
-    await assertChildGuids(folder1.guid, [tbBmk.guid, bmk2_guid, bmk3.guid],
-      "Folder 1 should have 3 children after first sync");
+    {
+      // We only check child GUIDs, and not their order, because the legacy and
+      // buffered bookmarks engines use different logic to merge children.
+      let folder1Children = await PlacesSyncUtils.bookmarks.fetchChildRecordIds(
+        folder1.guid);
+      deepEqual(folder1Children.sort(), [bmk2_guid, tbBmk.guid, bmk3.guid].sort(),
+        "Folder 1 should have 3 children after first sync");
+    }
     await assertChildGuids(folder2_guid, [bmk4_guid, tagQuery_guid],
       "Folder 2 should have 2 children after first sync");
     let taggedURIs = [];
     await PlacesUtils.bookmarks.fetch({tags: ["taggy"]}, b => taggedURIs.push(b.url));
     equal(taggedURIs.length, 1, "Should have 1 tagged URI");
     equal(taggedURIs[0].href, "https://example.org/",
       "Synced tagged bookmark should appear in tagged URI list");
 
     changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(changes, {},
       "Should have already uploaded changes in follow-up sync");
 
     // First ping won't include validation data, since we've changed bookmarks
     // and `canValidate` will indicate it can't proceed.
-    let engineData = pings.map(p =>
-      p.syncs[0].engines.find(e => e.name == "bookmarks")
-    );
+    let engineData = pings.map(p => {
+      let name = bufferedBookmarksEnabled() ? "bookmarks-buffered" :
+                                              "bookmarks";
+      return p.syncs[0].engines.find(e => e.name == name);
+    });
     ok(!engineData[0].validation, "Should not validate after first sync");
     ok(engineData[1].validation, "Should validate after second sync");
   } finally {
     engine._uploadOutgoing = uploadOutgoing;
     await cleanup(engine, server);
   }
 });
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -3,17 +3,16 @@
 
 const {Service} = ChromeUtils.import("resource://services-sync/service.js");
 const {WBORecord} = ChromeUtils.import("resource://services-sync/record.js");
 const {Resource} = ChromeUtils.import("resource://services-sync/resource.js");
 const {BookmarksEngine} = ChromeUtils.import("resource://services-sync/engines/bookmarks.js");
 const {RotaryEngine} = ChromeUtils.import("resource://testing-common/services/sync/rotaryengine.js");
 const {OS} = ChromeUtils.import("resource://gre/modules/osfile.jsm");
 
-
 function SteamStore(engine) {
   Store.call(this, "Steam", engine);
 }
 
 SteamStore.prototype = {
   __proto__: Store.prototype,
 };
 
@@ -158,17 +157,20 @@ add_task(async function test_processInco
 
     equal(fullPing.uid, "f".repeat(32)); // as setup by SyncTestingInfrastructure
     deepEqual(pingPayload.failureReason, {
       name: "httperror",
       code: 500,
     });
 
     equal(pingPayload.engines.length, 1);
-    equal(pingPayload.engines[0].name, "bookmarks");
+
+    let engineName = bufferedBookmarksEnabled() ? "bookmarks-buffered" :
+                                                  "bookmarks";
+    equal(pingPayload.engines[0].name, engineName);
     deepEqual(pingPayload.engines[0].failureReason, {
       name: "httperror",
       code: 500,
     });
   } finally {
     await store.wipe();
     await cleanAndGo(engine, server);
   }
@@ -183,35 +185,38 @@ add_task(async function test_uploading()
 
   let bmk = await PlacesUtils.bookmarks.insert({
     parentGuid: PlacesUtils.bookmarks.toolbarGuid,
     url: "http://getfirefox.com/",
     title: "Get Firefox!",
   });
 
   try {
+    let engineName = bufferedBookmarksEnabled() ? "bookmarks-buffered" :
+                                                  "bookmarks";
+
     let ping = await sync_engine_and_validate_telem(engine, false);
     ok(!!ping);
     equal(ping.engines.length, 1);
-    equal(ping.engines[0].name, "bookmarks");
+    equal(ping.engines[0].name, engineName);
     ok(!!ping.engines[0].outgoing);
     greater(ping.engines[0].outgoing[0].sent, 0);
     ok(!ping.engines[0].incoming);
 
     await PlacesUtils.bookmarks.update({
       guid: bmk.guid,
       title: "New Title",
     });
 
     await store.wipe();
     await engine.resetClient();
 
     ping = await sync_engine_and_validate_telem(engine, false);
     equal(ping.engines.length, 1);
-    equal(ping.engines[0].name, "bookmarks");
+    equal(ping.engines[0].name, engineName);
     equal(ping.engines[0].outgoing.length, 1);
     ok(!!ping.engines[0].incoming);
   } finally {
     // Clean up.
     await store.wipe();
     await cleanAndGo(engine, server);
   }
 });
@@ -523,33 +528,38 @@ add_task(async function test_clean_urls(
 
 add_task(async function test_initial_sync_engines() {
   enableValidationPrefs();
 
   await Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
   engine.enabled = true;
   // These are the only ones who actually have things to sync at startup.
-  let engineNames = ["clients", "bookmarks", "prefs", "tabs"];
+  let telemetryEngineNames = ["clients", "prefs", "tabs"];
+  if (bufferedBookmarksEnabled()) {
+    telemetryEngineNames.push("bookmarks-buffered");
+  } else {
+    telemetryEngineNames.push("bookmarks");
+  }
   let server = await serverForEnginesWithKeys({"foo": "password"}, ["bookmarks", "prefs", "tabs"].map(name =>
     Service.engineManager.get(name)
   ));
   await SyncTestingInfrastructure(server);
   try {
     const changes = await engine._tracker.getChangedIDs();
     _(`test_initial_sync_engines: Steam tracker contents: ${
       JSON.stringify(changes)}`);
     let ping = await wait_for_ping(() => Service.sync(), true);
 
     equal(ping.engines.find(e => e.name === "clients").outgoing[0].sent, 1);
     equal(ping.engines.find(e => e.name === "tabs").outgoing[0].sent, 1);
 
     // for the rest we don't care about specifics
     for (let e of ping.engines) {
-      if (!engineNames.includes(engine.name)) {
+      if (!telemetryEngineNames.includes(engine.name)) {
         continue;
       }
       greaterOrEqual(e.took, 1);
       ok(!!e.outgoing);
       equal(e.outgoing.length, 1);
       notEqual(e.outgoing[0].sent, undefined);
       equal(e.outgoing[0].failed, undefined);
     }