993584 - Initial landing for CloudSync

Status: RESOLVED FIXED

(Cloud Services Graveyard :: cloudSync, enhancement)

Description

[Alan K [:ack]]

Updated CloudSync API.

* support for tabs and bookmarks only
* only bookmarks in a single, special folder are synced
* event listeners for tab/bookmark changes
* can update tabs/bookmarks from remote clients

API description here: https://gist.github.com/modeswitch/9916280

Comment 1

[Alan K [:ack]]

Attachment: initial implementation (rough, still needs work)

Comment 2

[Alan K [:ack]]

Attachment: updated cloudsync patch (still needs some cleanup)

Comment 3

[Alan K [:ack]]

Attachment: updated cloudsync patch (without UI changes, ready for initial review)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8407565 [details] [diff] [review]
updated cloudsync patch (without UI changes, ready for initial review)

I think you want Richard to review this :)

Comment 6

[Alan K [:ack]]

Making some more changes. I think there's a better way to exclude bookmarks from syncing using annotations.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Alan K [:ack] from comment #6)
> Making some more changes. I think there's a better way to exclude bookmarks
> from syncing using annotations.

Assuming you're talking about places annos, I was under the impression that we discourage their use these days. mak, am I misremembering?

(Also, I might not get to this review before Monday; if so, I apologize!)

Comment 8

[Alan K [:ack]]

(In reply to Richard Newman [:rnewman] from comment #7)
> (In reply to Alan K [:ack] from comment #6)
> > Making some more changes. I think there's a better way to exclude bookmarks
> > from syncing using annotations.
> 
> Assuming you're talking about places annos, I was under the impression that
> we discourage their use these days. mak, am I misremembering?
> 
> (Also, I might not get to this review before Monday; if so, I apologize!)

Yep, places annotations. If that's not correct, what is the new hotness?

Also, is it reasonable to create a folder like we have for Mobile Bookmarks, but for each adapter?

Comment 9

[Andreas Gal :gal]

Yeah, having a dedicated folder for each service seems like a reasonable starting point IMO.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Alan K [:ack] from comment #8)

> Yep, places annotations. If that's not correct, what is the new hotness?

Using the database schema itself.

(Bear in mind that only Firefox on desktop uses Places, and the Places DB schema has changed over time. Firefox Sync has a stable, abstract bookmarks record concept, and this is one reason.)


> Also, is it reasonable to create a folder like we have for Mobile Bookmarks,
> but for each adapter?

That really depends on what you're trying to do.

Mobile Bookmarks is a root, not a folder, and every client (desktop, Android, third-party) needs additional magic to handle the set of roots correctly. Having a dynamic set of roots is probably going to be painful, depending on how the set can change.

Note that Mobile Bookmarks isn't one specific folder that's synced; it's just the place that XUL Fennec put bookmarks, just as desktop uses Bookmarks Menu or Unsorted Bookmarks by default. Users can move bookmarks between these roots, and Sync syncs them all.

Having an extra root doesn't really save you from some pain -- e.g., what happens if a user moves a bookmark from their toolbar into the folder, and back out again? You need to make sure other clients don't delete that bookmark when processing the changes, if they're only syncing that one folder.

IIRC, mak isn't very fond of the roots concept, but it is what it is.

I strongly recommend figuring out exactly what the requirements are here, both now and going forward, and on which platforms, and then carefully mapping those requirements to the existing concepts in Places and elsewhere. There are a lot of places to twist an ankle.

Comment 11

[Alan K [:ack]]

(In reply to Richard Newman [:rnewman] from comment #10)
> Using the database schema itself.
> 
> (Bear in mind that only Firefox on desktop uses Places, and the Places DB
> schema has changed over time. Firefox Sync has a stable, abstract bookmarks
> record concept, and this is one reason.)

OK, I'll need to change a few places where I've used PlacesUtils for this.

> That really depends on what you're trying to do.
> 
> Mobile Bookmarks is a root, not a folder, and every client (desktop,
> Android, third-party) needs additional magic to handle the set of roots
> correctly. Having a dynamic set of roots is probably going to be painful,
> depending on how the set can change.
> 
> Note that Mobile Bookmarks isn't one specific folder that's synced; it's
> just the place that XUL Fennec put bookmarks, just as desktop uses Bookmarks
> Menu or Unsorted Bookmarks by default. Users can move bookmarks between
> these roots, and Sync syncs them all.
> 
> Having an extra root doesn't really save you from some pain -- e.g., what
> happens if a user moves a bookmark from their toolbar into the folder, and
> back out again? You need to make sure other clients don't delete that
> bookmark when processing the changes, if they're only syncing that one
> folder.
> 
> IIRC, mak isn't very fond of the roots concept, but it is what it is.
> 
> I strongly recommend figuring out exactly what the requirements are here,
> both now and going forward, and on which platforms, and then carefully
> mapping those requirements to the existing concepts in Places and elsewhere.
> There are a lot of places to twist an ankle.

A third-party has asked for this functionality and I'm just trying to figure out how reasonable that is. Does the problem arise because those roots are understood to use the same IDs by convention rather than using GUIDs?

Comment 12

[Marco Bonardo [:mak]]

(In reply to Richard Newman [:rnewman] from comment #7)
> Assuming you're talking about places annos, I was under the impression that
> we discourage their use these days. mak, am I misremembering?

We discourage their usage mostly cause they are a synchronous API and thus causing jank... We have a plan to introduce an async alternative, but nothing ready.
And yes, as you said, Places is desktop only.

Comment 13

[Alan K [:ack]]

Since creating new root folders looks perilous, what about protecting a regular folder (from deletion, etc) instead?

Comment 14

[Richard Newman [:rnewman]]

Sorry for the delay on this; it's lucky that we're waiting for input from Marco, 'cos I'm failing to clear a large enough chunk of time to review something this big. I haven't forgotten about you!

Comment 15

[Marco Bonardo [:mak]]

So, the roots concept is very broken and there's no support to extend them, cause there's also poor will to bring them on... the system has been overly complicated in the past to achieve some UX that could have been obtained with a simpler system...
The only advantage a folder has being a root is that the user cannot remove it, though it may get lost in other cases like corruption (we don't backup stuff we don't know about).
It's also not exposed in the UI unless you do some special handling. First of all, let me clarify in the UI we never expose roots, we expose shortcuts to roots (what we call a folder shortcut, a bookmark with url place:folder=ID), so even if the user figures a way to remove the shortcut, the root is safe.

In this case I think you should indeed have a "root" folder, that is a folder with parent PlacesUtils.PlacesRootId and then a shortcut pointing to your folder should be added to the UI. Where to add this is up for debate:
- you may add to PlacesUIUtils.leftPaneFolderId, or to PlacesUIUtils.allBookmarksFolderId... this way it will appear in the Library left pane, under one of those. The advantage is that those folders are readonly, so the user cannot remove them through the default UI, he can only make copies.
- you may add it to the toolbar or the menu or any other place. Here it's easier for the user to move or remove it.

Now it may look that the best option is 1 cause you just create the shortcut and it will be there forever, unfortunately that's not true, in case of Library corruption we must throw away the whole Library left pane folder, and re-create it from scratch, that means your shortcuts are gone as well (The root is safe though).
What people implementing mobile/metro "roots" did so far, was having some code checking if they needed to re-create the shortcuts, since there's no way to guarantee they won't be lost. But actually you should also check your root is there, cause if the database gets completely corrupt, then we lose everything (Apart what is in the backups, but as I said we don't backup stuff we don't know about), so the shortcut may be the smaller deal.

Comment 16

[Alan K [:ack]]

This is what I've done, and it seems to work:

      let placesRootId = PlacesUtils.placesRootId;
      let adapterRootId = PlacesUtils.bookmarks.createFolder(placesRootId, name, PlacesUtils.bookmarks.DEFAULT_INDEX);
      PlacesUtils.bookmarks.insertBookmark(PlacesUIUtils.allBookmarksFolderId, NetUtil.newURI("place:folder=" + adapterRootId), PlacesUtils.bookmarks.DEFAULT_INDEX, name);
      PlacesUtils.annotations.setItemAnnotation(adapterRootId, EXCLUDEBACKUP_ANNO, 1, 0, PlacesUtils.annotations.EXPIRE_NEVER);
      PlacesUtils.annotations.setItemAnnotation(adapterRootId, CLOUDSYNC_BOOKMARKS_ROOT_BASE_ANNO + "/" + name, 1, 0, PlacesUtils.annotations.EXPIRE_NEVER);

I'm not too worried about corruption here. The code checks for the root folder, and I can add a check for the shortcut as well. If it's not there, the adapter can sync from upstream.

Comment 17

[Marco Bonardo [:mak]]

it looks correct off-hand

Comment 18

[Alan K [:ack]]

Attachment: cloudsync.patch

Supports root folder for each adapter and nested folders in root

Comment 19

[Chris Karlof [:ckarlof]]

I think that CloudSync is a confusing name for this. Why is this more "cloudy" than Firefox Sync? IMO, this is a simplified version of Sync, so I'd prefer something along the lines of SimpleSync.

Comment 20

[Chris Karlof [:ckarlof]]

Comment on attachment 8422114 [details] [diff] [review]
cloudsync.patch

Review of attachment 8422114 [details] [diff] [review]:
-----------------------------------------------------------------

The API functions that interact with the data stores should be asynchronous, using whatever asynchronous methods are available for interacting with the underlying data.

::: services/cloudsync/CloudSync.js
@@ +54,5 @@
> +      os.addObserver(this, "final-ui-startup", true);
> +      break;
> +
> +    case "final-ui-startup":
> +      // Force Weave service to load if it hasn't triggered from overlays

Copy paste comment. This isn't Weave, I assume.

::: services/cloudsync/modules/bookmarks.js
@@ +89,5 @@
> +      }
> +
> +      PlacesUtils.annotations.setItemAnnotation(aItemId, EXCLUDEBACKUP_ANNO, 1, 0,
> +                                                PlacesUtils.annotations.EXPIRE_NEVER);
> +      Utils.nextTick(that.onAdd, that, aGUID);

are these nextTick calls to avoid doing too much work in these notification handlers?

@@ +190,5 @@
> +    let stmt = this._guidToLocalIdStmt;
> +    // guid might be a String object rather than a string.
> +    stmt.params.guid = guid.toString();
> +
> +    let results = Async.querySpinningly(stmt, this._guidToLocalIdCols);

New code and APIs should be asynchronous and not use this event loop spinning stuff.

@@ +214,5 @@
> +    let stmt = this._localIdToGuidStmt;
> +    stmt.params.item_id = id;
> +
> +    // Use the existing GUID if it exists
> +    let result = Async.querySpinningly(stmt, this._localIdToGuidCols)[0];

No event loop spinning please (see above).

@@ +236,5 @@
> +
> +    let stmt = this._setGuidStmt;
> +    stmt.params.guid = guid;
> +    stmt.params.item_id = localId;
> +    Async.querySpinningly(stmt);

No event loop spinning please (see above).

@@ +292,5 @@
> +      this.suspend();
> +    }
> +  },
> +
> +  getLocalBookmarks: function getLocalBookmarks() {

This should be an asynchronous function.

@@ +343,5 @@
> +
> +    return results;
> +  },
> +
> +  getLocalBookmarksByGuid: function getLocalBookmarkByGuid(guids) {

This should be an asynchronous function.

@@ +372,5 @@
> +
> +    return results;
> +  },
> +
> +  mergeRemoteBookmarks: function mergeRemoteBookmarks(items) {

This should be an asynchronous function.

::: services/cloudsync/modules/constants.js
@@ +11,5 @@
> +// Sync Server API version that the client supports.
> +CLOUDSYNC_API_VERSION:                 "1.0",
> +USER_API_VERSION:                      "1.0",
> +MISC_API_VERSION:                      "1.0",
> +

USER_API_VERSION and MISC_API_VERSION are Firefox Sync related constants, I believe. Why here? Likewise for STORAGE_VERSION.

@@ +18,5 @@
> +// the per-engine cleartext formats.
> +STORAGE_VERSION:                       5,
> +PREFS_BRANCH:                          "services.sync.",
> +
> +/*

Why is all this commented out stuff included in the patch? A lot of stuff in this file is Fx Sync specific, which probably isn't relevant to this API.

::: services/cloudsync/modules/tabs.js
@@ +82,5 @@
> +
> +    Observers.notify("cloudsync:tabs:set-remote-tabs", null);
> +  },
> +
> +  getLocalTabs: function getLocalTabs(filter) {

This should be asynchronous.

::: services/cloudsync/services-cloudsync.js
@@ +4,5 @@
> +
> +pref("services.sync.cloudsync.adapters", "{}");
> +pref("services.sync.cloudsync.bookmarks", "{}");
> +
> +/*

Why this large commented out section?

::: services/sync/modules/engines/bookmarks.js
@@ -241,4 @@
>              queryId = PlacesUtils.annotations.getItemAnnotation(
>                id, SMART_BOOKMARKS_ANNO);
>            } catch(ex) {}
> -          

These are all whitespace changes, and IIUC these changes should be removed from the patch.

Comment 21

[Alan K [:ack]]

Comments:

* I'll have a look for other copypasta that I missed (including config stuff from sync/)

* Agree with all the async conversions; Synchronous API was quick and dirty.

* I'm using nextTick to keep the handler light. I'm not actually sure that buys anything though. Thoughts?

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8422114 [details] [diff] [review]
cloudsync.patch

Review of attachment 8422114 [details] [diff] [review]:
-----------------------------------------------------------------

Firstly, huge apologies for taking so long to review this. It warranted a good chunk of time, and there have been a lot of things conspiring to make those in short supply! I appreciate your patience. Please feel free to badger me if I'm holding you up.

Still on my list to review:

 adapters.js
 bookmarks.js
 local.js
 tabs.js

Overview comments so far: in addition to Chris's feedback (many thanks, Chris!), there's a lot of stuff here (particularly around initialization) that's been cargo-culted from old 2008-era Weave code. There's also a lot of stuff that should have been stripped out between f? and r?, like block commented-out prefs.

You should probably get input from a build peer on your build parts, as well as reading <https://developer.mozilla.org/en-US/docs/How_Mozilla%27s_build_system_works#moz.build_Files>.

Extra points if you split this work into two or three parts: setting up the basis of CloudSync, integration hooks, and then the supported datatypes. That will make review passes easier.

I'll find time early next week to do a review of the remaining work; in the mean time, feel free to update this patch with the comments below.

Thanks again for your patience!

::: browser/base/content/sync/aboutSyncTabs.js
@@ +5,4 @@
>  const Cu = Components.utils;
>  
>  Cu.import("resource://services-sync/main.js");
> +Cu.import("resource://services-cloudsync/main.js");

I strongly disagree with jamming this into about:sync-tabs, which needs to die already. It has huge perf and interaction problems, and just about any other solution would be better.

@@ +146,5 @@
> +      this._generateWeaveTabList();
> +    } else {
> +      //XXXzpao We should say something about not being logged in & not having data
> +      //        or tell the appropriate condition. (bug 583344)
> +    }

Ach no.

@@ +222,5 @@
> +        if (appendClient) {
> +          let attrs = {
> +            type: "client",
> +            clientName: remote.client.name,
> +            class: remote.client.isMobile ? "mobile" : "desktop"

There are a lot of things about the Sync ontology that are wrong. This is one of them. A vaguely artificial mobile/desktop division doesn't help too much -- let's do something smarter, like remote.client.deviceType (which can be "phablet" for all we care).

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +1002,5 @@
>    shouldShowTabsFromOtherComputersMenuitem: function() {
>      // If Sync isn't configured yet, then don't show the menuitem.
> +    return (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED &&
> +            Weave.Svc.Prefs.get("firstSync", "") != "notReady") ||
> +           (CloudSync.ready && CloudSync.Adapters.listAllAdapters().length);

IMO PlacesUIUtils is the wrong integration point for this, and this is probably the wrong hook pattern -- Weave.Status is free, CloudSync probably isn't -- but I'd need a little more info from you about how all of this is going to play together before making a recommendation.

Regardless, though: wouldn't you want to just check the CloudSync tabs adapter?

::: browser/confvars.sh
@@ +32,4 @@
>  MOZ_SERVICES_HEALTHREPORT=1
>  MOZ_SERVICES_METRICS=1
>  MOZ_SERVICES_SYNC=1
> +MOZ_SERVICES_CLOUDSYNC=1

Let's pref this on only for Nightly until it's gone through stabilization.

https://wiki.mozilla.org/Platform/Channel-specific_build_defines

::: services/cloudsync/CloudSync.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

You can use the destructuring form here.

  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

@@ +7,5 @@
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FileUtils.jsm");

Not used.

@@ +11,5 @@
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://services-sync/util.js");
> +
> +const SYNC_PREFS_BRANCH = "services.cloudsync.";

You import some Sync code, so you might want to call this CLOUDSYNC_PREFS_BRANCH or something like that.

@@ +25,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
> +                                         Ci.nsISupportsWeakReference]),
> +
> +  ensureLoaded: function () {
> +    Components.utils.import("resource://services-cloudsync/main.js");

Cu

@@ +28,5 @@
> +  ensureLoaded: function () {
> +    Components.utils.import("resource://services-cloudsync/main.js");
> +
> +    // Side-effect of accessing the service is that it is instantiated.
> +    CloudSync.Service;

Is there a reason you're following this pattern from Weave, rather than just trusting to lazymodulegetter?

@@ +60,5 @@
> +      this.timer.initWithCallback({
> +        notify: function() {
> +          this.ensureLoaded();
> +        }.bind(this)
> +      }, 10000, Ci.nsITimer.TYPE_ONE_SHOT);

One of the things we wanted to fix in Sync was initialization -- this kind of delayed init causes unpredictable behavior, such as early changes not being tracked, UI not showing the correct state, and so on.

That's Bug 584771. I'm going to draw a line and say that CloudSync should learn from that mistake and init automatically at a point in the app's lifecycle, not on a timer.

::: services/cloudsync/Makefile.in
@@ +7,5 @@
> +cloudsync_id      := {a133808b-ec8d-4cab-8767-4ef9cf8f66c3}
> +
> +# Preprocess files.
> +CLOUDSYNC_PP := modules/constants.js
> +CLOUDSYNC_PP_FLAGS := \

Please use moz.build for this kind of stuff.

::: services/cloudsync/modules/constants.js
@@ +5,5 @@
> +
> +// Process each item in the "constants hash" to add to "global" and give a name
> +this.EXPORTED_SYMBOLS = [((this[key] = val), key) for ([key, val] in Iterator({
> +
> +CLOUDSYNC_VERSION:                         "@cloudsync_version@",

This looks like a lot of stuff inherited from the Weave add-on. Clean house, plz!

@@ +16,5 @@
> +// Version of the data format this client supports. The data format describes
> +// how records are packaged; this is separate from the Server API version and
> +// the per-engine cleartext formats.
> +STORAGE_VERSION:                       5,
> +PREFS_BRANCH:                          "services.sync.",

That ain't right.

::: services/cloudsync/modules/main.js
@@ +1,5 @@
> +/* 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/. */
> +
> +this.EXPORTED_SYMBOLS = ['CloudSync'];

Double quotes, please.

@@ +4,5 @@
> +
> +this.EXPORTED_SYMBOLS = ['CloudSync'];
> +
> +this.CloudSync = {
> +  ready: false

Trailing comma.

@@ +21,5 @@
> +    Components.utils.import(module, ns);
> +    delete dest[prop];
> +    return dest[prop] = ns[prop];
> +  };
> +  props.forEach(function(prop) dest.__defineGetter__(prop, getter(prop)));

Is there a reason why you're not just using LazyModuleGetter instead of rolling your own?

::: services/cloudsync/modules/service.js
@@ +6,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +const Cu = Components.utils;

Again, destructure.

@@ +12,5 @@
> +Cu.import("resource://services-sync/util.js");
> +Cu.import("resource://services-cloudsync/main.js");
> +
> +function CloudSyncllService() {
> +  this._notify = Utils.notify("cloudsync:service:");

You're not using this everywhere in this file (e.g., line 33), and from experience it makes code archaeology much harder, so I'd prefer for this pattern to die with Weave.

@@ +17,5 @@
> +}
> +
> +CloudSyncllService.prototype = {
> +  onStartup: function onStartup() {
> +    // Send an event now that Weave service is ready.  We don't do this

This is pretty copypasta-ey. Why "CloudSyncllService"? It looks like you're using the Sync 1.1 Service ("Sync11Service") as inspiration.

::: services/common/utils.js
@@ +161,3 @@
>     */
> +  nextTick: function nextTick(callback) {
> +    let args = Array.prototype.slice.call(arguments, 1);

Let's not do this in CommonUtils. You're generating garbage here that most callers of nextTick won't be using.

Alternatively, do an earlier check of args, so not everyone has to pay the cost.

  if (arguments.length > 1) {
    ...

::: services/sync/modules/engines/bookmarks.js
@@ -241,4 @@
>              queryId = PlacesUtils.annotations.getItemAnnotation(
>                id, SMART_BOOKMARKS_ANNO);
>            } catch(ex) {}
> -          

> These are all whitespace changes, and IIUC these changes should be removed from the patch.

Yup, or at least put into a separate Part 0.

::: services/sync/modules/main.js
@@ +11,4 @@
>    "notifications.js":     ["Notifications", "Notification", "NotificationButton"],
>    "service.js":           ["Service"],
>    "status.js":            ["Status"],
> +  "util.js":              ['Utils', 'Svc', "PlacesUtils"]

Fix the single quotes while you're here.

::: xulrunner/confvars.sh
@@ +16,4 @@
>  MOZ_SERVICES_CRYPTO=1
>  MOZ_SERVICES_METRICS=1
>  MOZ_SERVICES_SYNC=1
> +MOZ_SERVICES_CLOUDSYNC=1

Does this belong in xulrunner?

Comment 23

[Alan K [:ack]]

Attachment: cloudsync-service-refactor.patch

Add-on can import CloudSync.jsm and access Bookmarks, Tabs, etc from the singleton object returned by cloudSync(). This replaces all of the Weave service cruft from my previous patch.

Comment 24

[Alan K [:ack]]

Also looking for discussion on about:sync-tabs here. I feel like scrapping it totally and designing something new here might be out of scope for this bug.

Comment 25

[Richard Newman [:rnewman]]

Comment on attachment 8433537 [details] [diff] [review]
cloudsync-service-refactor.patch

Review of attachment 8433537 [details] [diff] [review]:
-----------------------------------------------------------------

Stub-tastic!

::: services/cloudsync/CloudSync.jsm
@@ +1,5 @@
> +/* 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/. */
> +
> +this.EXPORTED_SYMBOLS = ["cloudSync"];

I'm not sure of the style here. Please take a look around, see if there's precedent for lowercase initial on an exported module symbol.

@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +this.CloudSync = function() {
> +}

Nit: trailing ;

@@ +12,5 @@
> +
> +CloudSync.prototype = {
> +
> +  _Adapters: null,
> +  get Adapters() {

None of these should be capitalized.

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 8422114 [details] [diff] [review]
cloudsync.patch

Review of attachment 8422114 [details] [diff] [review]:
-----------------------------------------------------------------

I skipped fairly quickly through bookmarks.js and tabs.js.

I think you need to spend more time on the data model: defining, documenting, determining preconditions, thinking about what happens in various circumstances*, and writing tests for them.

* For example: you don't support microformats, queries, yadda yadda. What happens if you sync a folder containing those things to another device, then that device changes the order of the remaining items and uploads changes? Ruh roh.

There's also a bunch of asyncifying that needs to happen, as well as style and commenting nits. More code reuse, abstraction, and definition of interfaces via prototypes, plz.

I'll wait for the next patch before doing a deeper dive -- I think I've given you enough stuff for now!

Thanks for your patience on the review.

::: services/cloudsync/modules/bookmarks.js
@@ +6,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +const Cu = Components.utils;

Nit: use destructuring here, as you do elsewhere.

@@ +19,5 @@
> +                    .getService(Components.interfaces.nsINavHistoryService);
> +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
> +                                  "resource://gre/modules/NetUtil.jsm");
> +
> +var RESULT_TYPES = [

Nit: prefer `let`, even for top-level declarations. (Throughout.)

But in this case: `const`.

@@ +40,5 @@
> +  "SEPARATOR",
> +];
> +
> +function BookmarkFolder(folderId) {
> +  var that = this;

This is a bad sign.

It means one of three things: you're routinely violating encapsulation, you've got an OO division in the wrong place, or you don't know about .bind(this).

@@ +60,5 @@
> +  let batchBookmarksUpdatePending = false;
> +  let changed = [];
> +  this.ignoreAll = false;
> +  this.suspended = true;
> +  this._observer = {

This is a very heavyweight way to do this. You should be using prototypes here to avoid making a BookmarkFolder incredibly fat.

@@ +64,5 @@
> +  this._observer = {
> +    onBeginUpdateBatch: function onBeginBatchUpdate() {
> +      /*
> +      batchBookmarksUpdatePending = true;
> +      */

There's a lot of commented-out code here, so I look forward to a final patch!

@@ +77,5 @@
> +      }
> +      */
> +    },
> +
> +    onItemAdded: function onItemAdded(aItemId, aParentId, aIndex, aItemType, aURI, aTitle, aDateAdded, aGUID, aParentGUID) {

Style: we don't use `aFoo` in modern code. Nor do we need the method name repeated twice:

  onItemAdded: function (itemID, parentID, ...) {

@@ +78,5 @@
> +      */
> +    },
> +
> +    onItemAdded: function onItemAdded(aItemId, aParentId, aIndex, aItemType, aURI, aTitle, aDateAdded, aGUID, aParentGUID) {
> +      if(that._ignore(aItemId, aParentId, aGUID, aItemType)) {

Nit: space between `if` and `(`.

@@ +126,5 @@
> +      }
> +
> +      if(aOldParent !== aNewParent) {
> +        if(!PlacesUtils.annotations.itemHasAnnotation(aOldParent, this.CLOUDSYNC_BOOKMARKS_ANNO) &&
> +           PlacesUtils.annotations.itemHasAnnotation(aNewParent, this.CLOUDSYNC_BOOKMARKS_ANNO)) {

There's a lot of verbose anno work here, which makes the code hard to read, and what looks like a heavy reliance on annos -- one per folder?

You should talk to mak about whether that's a good idea. I am worried about having thousands of annos unnecessarily.

@@ +151,5 @@
> +    onItemVisited: function onItemVisited(aBookmarkId, aVisitID, time) {
> +      // No action for visited bookmarks
> +    },
> +
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsINavBookmarkObserver])

This goes at the top of the object definition, and every member has a trailing comma (good style in modern JS).

@@ +190,5 @@
> +    let stmt = this._guidToLocalIdStmt;
> +    // guid might be a String object rather than a string.
> +    stmt.params.guid = guid.toString();
> +
> +    let results = Async.querySpinningly(stmt, this._guidToLocalIdCols);

Concur with ckarlof here. I don't want to see a single spinning call in any of this code.

@@ +219,5 @@
> +    if (result && result.guid)
> +      return result.guid;
> +
> +    // Give the uri a GUID if it doesn't have one
> +    return this._setGUID(id);

This encodes outdated assumptions, I think. Shouldn't we be able to trust that all local records have a GUID?

@@ +229,5 @@
> +      "SET guid = :guid " +
> +      "WHERE id = :item_id");
> +  },
> +
> +  _setGuidForLocalId: function _setGuidForLocalId(localId, guid) {

See Bug 1012597.

@@ +248,5 @@
> +
> +    if(!(PlacesUtils.bookmarks.TYPE_BOOKMARK == itemType ||
> +         PlacesUtils.bookmarks.TYPE_FOLDER == itemType ||
> +         PlacesUtils.bookmarks.TYPE_SEPARATOR == itemType)) {
> +      return true;

You can't afford to ignore other types. Your structure will be incorrect.

Even on Android we have to handle the full suite:

http://hg.mozilla.org/mozilla-central/file/default/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java

@@ +276,5 @@
> +    }
> +  },
> +
> +  removeEventListener: function removeEventListener(type, listener) {
> +    if(Object.keys(this.listeners).indexOf(type) < 0) {

What's wrong with `type in this.listeners`?

@@ +286,5 @@
> +    if(listeners.indexOf(listener) < 0) {
> +      return;
> +    }
> +
> +    listeners.splice(i, 1);

Seems like `listeners` should be a Set.

@@ +295,5 @@
> +
> +  getLocalBookmarks: function getLocalBookmarks() {
> +    var that = this;
> +    var options = historyService.getNewQueryOptions();
> +    var query = historyService.getNewQuery();

Aieee var

@@ +356,5 @@
> +      let folderId = PlacesUtils.bookmarks.getFolderIdForItem(id);
> +      let itemType = PlacesUtils.bookmarks.getItemType(id);
> +
> +      let result = {
> +        id: guids[i],

You should abstract out your record definitions. Returning anonymous JSON blobs makes inspection and specification hard.

@@ +361,5 @@
> +        type: ITEM_TYPES[itemType],
> +        index: PlacesUtils.bookmarks.getItemIndex(id),
> +        dateAdded: PlacesUtils.bookmarks.getItemDateAdded(id),
> +        lastModified: PlacesUtils.bookmarks.getItemLastModified(id),
> +        uri: (PlacesUtils.bookmarks.TYPE_BOOKMARK == itemType) ? PlacesUtils.bookmarks.getBookmarkURI(id).spec : null,

This should probably be asciiSpec. Bug 745411.

@@ +385,5 @@
> +    for(let i = 0; i < items.length; ++ i) {
> +      let item = items[i];
> +      let exists = item.id && this._guidToLocalId(item.id);
> +      if(!exists && !item.deleted) {
> +        if("FOLDER" == item.type) {

Caps are sadness.

@@ +398,5 @@
> +        updatedItems.push(item);
> +      }
> +    }
> +
> +    // FIXME: these need to be sorted and created in the right order

Indeed.

::: services/cloudsync/modules/local.js
@@ +6,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +const Cu = Components.utils;

Destructure.

::: services/cloudsync/modules/tabs.js
@@ +6,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +const Cu = Components.utils;

Destructuring.

@@ +26,5 @@
> +}
> +
> +_Tabs.prototype = {
> +  addEventListener: function addEventListener(type, listener) {
> +    if("Change" !== type ||

Magic constant.

@@ +38,5 @@
> +    }
> +  },
> +
> +  removeEventListener: function removeEventListener(type, listener) {
> +    var i = this.listeners.indexOf(listener);

`let`.

@@ +39,5 @@
> +  },
> +
> +  removeEventListener: function removeEventListener(type, listener) {
> +    var i = this.listeners.indexOf(listener);
> +    if("Change" !== type ||

Coding style, and prefer avoiding unnecessary caps.

@@ +44,5 @@
> +       i < 0) {
> +      return;
> +    }
> +
> +    this.listeners.splice(i, 1);

Should be a Set. Also, break out this logic so you can reuse it -- what's shared between Tabs and Bookmarks?

(Which gets horrifyingly close to Sync's concept of an Engine.)

@@ +45,5 @@
> +      return;
> +    }
> +
> +    this.listeners.splice(i, 1);
> +    if(0 == this.listeners.length) {

if (!this.listeners.length) {

@@ +51,5 @@
> +    }
> +  },
> +
> +  // [
> +  //   client: {

Block comment, descriptive text.

@@ +54,5 @@
> +  // [
> +  //   client: {
> +  //     id: client id,
> +  //     name: client name,
> +  //     isMobile: true if client is a mobile device, false otherwise

Don't use Sync's client format. <https://services.etherpad.mozilla.org/9>

1. I can't emphasize this enough: EVERY SINGLE RECORD SHOULD BE VERSIONED.
2. Types are not boolean. Consider: laptop, desktop, Android phone, tablet... phablet... Surface... car...
3. Consider something less opaque than "name".

You also need to address the conflation of clients and tabs: what if a user doesn't want to sync tabs -- do tabs still sync but with an empty tabs array?

@@ +57,5 @@
> +  //     name: client name,
> +  //     isMobile: true if client is a mobile device, false otherwise
> +  //   },
> +  //   timestamp: the time that this info was generated
> +  //   tabs: [ array of tabs, with the following format

Note that this invalidates the entire record whenever a URL changes (less of a problem with the proposed mechanisms of CloudSync, but still a conceptual issue), and doesn't accommodate windows or tab groups.

Firstly, take a look at the Send Tab tab format. https://wiki.mozilla.org/Services/Sync/Push_to_device

Secondly, you should probably partition this according to window, and add tab group metadata.

Thirdly, is this ordered by appearance?

@@ +66,5 @@
> +  //       urlHistory: array of urls
> +  //     }
> +  //   ],
> +  // ]
> +  setRemoteTabs: function setRemoteTabs(tabsInfo) {

Where does tabsInfo come from?

@@ +69,5 @@
> +  // ]
> +  setRemoteTabs: function setRemoteTabs(tabsInfo) {
> +    tabsInfo.forEach(function(info) {
> +      let client = info.client;
> +      if(this.cachedRemotesInfo[client.id]) {

Style.

@@ +72,5 @@
> +      let client = info.client;
> +      if(this.cachedRemotesInfo[client.id]) {
> +        let current = this.cachedRemotesInfo[client.id];
> +        if(current.timestamp <= info.timestamp) {
> +          return;

I see a clock skew risk here.

Comment 27

[Alan K [:ack]]

Attachment: cloudsync.patch

ignore

Comment 28

[Alan K [:ack]]

What's the custom for protecting properties that shouldn't be accessed on returned objects? One option is to define everything in the constructor and keep all private variables in that scope, but that seems like overkill:


function X() {
  let y; // private

  this.a = function() {
    // do something with y
  }
}

Comment 29

[Richard Newman [:rnewman]]

(In reply to Alan K [:ack] from comment #28)
> What's the custom for protecting properties that shouldn't be accessed on
> returned objects?

You can get modification protection with Object.freeze; others can still read it, but modifications are silently discarded.

You can get access protection (modulo BackStagePass) by not exporting a symbol from your module.

The only way to get total encapsulation is the way modules are done in some parts of the wider JS community: use JS scopes and closures, a la:

  let MyModule = (function () {
    let myPrivateThing = 3;
    return Object.freeze({
      foo: 1,
      bar: 2,
      getPrivate: () => myPrivateThing,
    });
  })();

Comment 30

[Alan K [:ack]]

Attachment: CloudSyncPlacesWrapper.jsm

Comment 31

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 32

[Alan K [:ack]]

This is large, but the major chunks are:

1. CloudSync.jsm, moz.build (reviewed already above)
2. CloudSyncEventSource.jsm, CloudSyncFolderCache.jsm, CloudSyncPlacesWrapper.jsm
   * Note that for CloudSyncPlacesWrapper, some of the underlying code is still synchronous but the API
     will make it easy to convert them over at some point.
   * I've added folder id caching instead of relying on annotations, and guid caching to avoid going
     to sqlite all the time.
3. CloudSyncBookmarks.jsm
4. CloudSyncTabs.jsm, PlacesUIUtils.jsm, aboutSyncBookmarks.js
   * The change to PlacesUIUtils makes "tabs from other browsers" always visible. I think if there are
     no tabs to show because a sync service is not configured we should direct users to configure 
     FxAccounts. Thoughts?

Comment 33

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 34

[Richard Newman [:rnewman]]

Comment on attachment 8445231 [details] [diff] [review]
cloudsync.patch

Review of attachment 8445231 [details] [diff] [review]:
-----------------------------------------------------------------

This is a partial review so that you're not blocked waiting on me. Covered:

  Build stuff
  aboutSyncTabs.js
  PlacesUIUtils.jsm
  CloudSync.jsm
  CloudSyncTabs.jsm

My point from Comment 22 still stands: about:sync-tabs needs to die already. You should file a follow-up and push to make sure it gets attention from the desktop team.

::: browser/base/content/sync/aboutSyncTabs.js
@@ +160,5 @@
>          list.removeItemAt(i);
>      }
> +  },
> +
> +  _generateWeaveTabList: function() {

Let's not introduce new uses of terminology that was obsolete five years ago!

"Firefox Sync" is clear enough, I think.

@@ +205,5 @@
> +  _generateCloudSyncTabList: function() {
> +    let updateTabList = function(remoteTabs) {
> +      let list = this._tabsList;
> +
> +      for each(let client in remoteTabs) {

Nit: spaces before (.

@@ +213,5 @@
> +        };
> +        let clientEnt = this.createItem(clientAttrs);
> +        list.appendChild(clientEnt);
> +
> +        for(let tab of client.tabs) {

Likewise.

@@ +224,5 @@
> +          let tabEnt = this.createItem(tabAttrs);
> +          list.appendChild(tabEnt);
> +        }
> +      }
> +    }.bind(this);

Just use fat arrows instead of binding.

@@ +228,5 @@
> +    }.bind(this);
> +
> +    cloudSync.tabs.getRemoteTabs().then(
> +      updateTabList
> +    );

Rejection handling?

@@ +289,4 @@
>          break;
>        case "weave:engine:sync:finish":
>          if (subject == "tabs")
> +          this.buildList(false);

Nit: brace conditionals while you're here.

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +1003,4 @@
>  
>    shouldShowTabsFromOtherComputersMenuitem: function() {
>      // If Sync isn't configured yet, then don't show the menuitem.
> +    return true;

> The change to PlacesUIUtils makes "tabs from other browsers"
> always visible. I think if there are no tabs to show because
> a sync service is not configured we should direct users to
> configure FxAccounts. Thoughts?

That's really a question for rfeeley and the rest of the Firefox UI team. But the comment is definitely wrong :P

::: services/cloudsync/CloudSync.jsm
@@ +11,5 @@
> +Cu.import("resource://gre/modules/CloudSyncBookmarks.jsm");
> +Cu.import("resource://gre/modules/CloudSyncTabs.jsm");
> +
> +let CloudSync = function() {
> +

Nit: kill newline.

@@ +54,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "cloudSync", function() {
> +  let instance = new CloudSync();
> +
> +  return instance;

return new CloudSync();

::: services/cloudsync/CloudSyncTabs.jsm
@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm", this);
> +XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils", "resource://gre/modules/PrivateBrowsingUtils.jsm");
> +XPCOMUtils.defineLazyServiceGetter(this, "Session", "@mozilla.org/browser/sessionstore;1", "nsISessionStore");

Interesting style! Any reason you did this here rather than after imports?

@@ +13,5 @@
> +Cu.import("resource://gre/modules/CloudSyncEventSource.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://services-common/observers.js");
> +
> +const API_VERSION = "0.0.1";

Let's stick to just incrementing integers. v1, v2, etc. is much less error prone and communicates fewer false guarantees.

And does this version the API methods, the records, or what?

@@ +18,5 @@
> +
> +let ClientRecord = function(params) {
> +  this.id = params.id;
> +  this.name = params.name || "?";
> +  this.tabs = new Set();

You need a block comment, and some .rst docs, to explain your data format.

I'm assuming from the code that each client has an unsorted set of tabs. So questions:

* Where did windows go?
* Why are tabs not ordered?
* Can I have two tabs open with the same URL/title/etc.? My browser lets me...

@@ +23,5 @@
> +}
> +
> +ClientRecord.prototype = {
> +
> +  version: API_VERSION,

Why is the record version the same as the API version?

@@ +26,5 @@
> +
> +  version: API_VERSION,
> +
> +  update: function(params) {
> +    this.name = params.name;

Shouldn't you throw if the id doesn't match (which should be a programming error), and update the tabs?

If not, why is this called "update"?

@@ +35,5 @@
> +let TabRecord = function(params) {
> +  this.title = params.title || params.url;
> +  this.url = params.url || "";
> +  this.icon = params.icon || "";
> +  this.lastUsed = Math.floor((params.lastUsed || 0) / 1000);

Definitely needs docs -- lastUsed is in seconds?

@@ +40,5 @@
> +};
> +
> +TabRecord.prototype = {
> +
> +  version: API_VERSION,

Similarly: why does this match the API version?

@@ +43,5 @@
> +
> +  version: API_VERSION,
> +
> +  update: function(params) {
> +    if(this.url !== params.url) {

Nit: spaces before "(", throughout.

@@ +47,5 @@
> +    if(this.url !== params.url) {
> +      return;
> +    }
> +
> +    if(params.lastUsed < this.lastUsed) {

What about clock skew?

@@ +51,5 @@
> +    if(params.lastUsed < this.lastUsed) {
> +      return;
> +    }
> +
> +    this.title = params.title;

update({title: null})

... your constructor explicitly avoids this being possible. Perhaps you need get/set methods here?

@@ +62,5 @@
> +let TabCache = function() {
> +
> +  this.tabs = {};
> +  this.clients = {};
> +

Whitespace pruning plz

@@ +194,5 @@
> +      while(wins.hasMoreElements()) {
> +        unregisterListenersForWindow(wins.getNext());
> +      }
> +    }
> +  }.bind(this);

Fat arrows here and above.

@@ +207,5 @@
> +  let getLocalTabs = function(filter) {
> +    let deferred = Promise.defer();
> +
> +    filter = (undefined === filter) ? true : filter;
> +    let filteredUrls = new RegExp("^(about:.*|chrome://weave/.*|wyciwyg:.*|file:.*)$"); // FIXME: should be a pref

Do or file.

@@ +221,5 @@
> +        if(!tab.entries.length)
> +          return;
> +
> +        // Get only the latest entry
> +        // FIXME: support full history

File.

@@ +225,5 @@
> +        // FIXME: support full history
> +        let entry = tab.entries[tab.index - 1];
> +
> +        if(!entry.url || filter && filteredUrls.test(entry.url))
> +          return;

Brace conditionals (throughout).

@@ +244,5 @@
> +
> +  let isOpenLocally = function(url) {
> +    let deferred = Promise.defer();
> +
> +    deferred.resolve(this.getLocalTabs().some(function(tab) {

This approach is really inefficient. See Bug 841096 for how this was fixed in about:sync-tabs.

Comment 35

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 36

[Alan K [:ack]]

Some questions:

1. Is there anything special that needs to be done for places queries? Initially I thought that they would need embedded ID->GUID conversion, but I'm not sure if that's a real use-case.

2. Handling multiple sync accounts was a goal at the beginning, but it's unclear how to support moving bookmark items between subtrees that are tracked by different adapters (including fxaccounts).

3. Bookmark items may be generated on non-firefox clients, so they'll need a GUID of some sort when they get synced. GUIDs are stored in a text field in the places db, so it looks like anything that can be stored in there should work. We happen to use 9 random bytes. Any pitfalls in mixing GUIDs created on other clients into the places db?

Comment 37

[Richard Newman [:rnewman]]

(In reply to Alan K [:ack] from comment #36)
 
> 1. Is there anything special that needs to be done for places queries?
> Initially I thought that they would need embedded ID->GUID conversion, but
> I'm not sure if that's a real use-case.

That depends on how tightly you define semantics. If you're fine with this being one-directional sync, or records being meaningless on some devices or after certain operations (e.g., wiping the local tree and restoring from the cloud), then great. If you make any claims of the data being intrinsically meaningful, then you must translate.

See Bug 641617 Comment 7, Bug 641074, which might allow you to cut the knot.


> 2. Handling multiple sync accounts was a goal at the beginning, but it's
> unclear how to support moving bookmark items between subtrees that are
> tracked by different adapters (including fxaccounts).

Indeed. This has come up in several next-generation representation discussions. It's isomorphic to "so, how do I atomically move a file between two git repos?" -- you can't do it without some risk (dupes) or without some external transactional primitive.


> 3. Bookmark items may be generated on non-firefox clients, so they'll need a
> GUID of some sort when they get synced. GUIDs are stored in a text field in
> the places db, so it looks like anything that can be stored in there should
> work. We happen to use 9 random bytes. Any pitfalls in mixing GUIDs created
> on other clients into the places db?

This is a fairly firm requirement. For one thing, Sync mandates 12-char GUIDs (almost but not quite to the point of the server API rejecting anything else -- 12-char URL-safe base64 was the spec, and we loosened that to 16-char printable ASCII in Bug 951986, IIRC).

Storing anything else might cause Sync problems, cause database constraint failures at a future date, etc.

The docs around this for Places aren't all that good, alas :/  Those columns are only meant to be accessed by Places itself, and are only modified by now-deprecated APIs (as you've no doubt seen on MDN). CloudSync should reject remote records that don't match the local constraints.

Comment 38

[Richard Newman [:rnewman]]

Comment on attachment 8450536 [details] [diff] [review]
cloudsync.patch

Review of attachment 8450536 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review until comments in Comment 34 are addressed -- e.g., docs, what about windows, etc.

Here are some more skim responses. There's a concern about promise handling during error cases.

::: services/cloudsync/CloudSyncBookmarks.jsm
@@ +35,5 @@
> +
> +const EXCLUDE_BACKUP_ANNO = "places/excludeFromBackup";
> +
> +const API_VERSION = "v1";
> +const DATA_VERSION = "v1";

Similarly. Sorry if I was unclear in my comment.

::: services/cloudsync/CloudSyncLocal.jsm
@@ +11,5 @@
> +Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://services-crypto/utils.js");
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +
> +const API_VERSION = "0.0.1";

Prefer plain integers.

::: services/cloudsync/CloudSyncPlacesWrapper.jsm
@@ +138,5 @@
> +        if(guid) {
> +          this.idGuidCache.update(id, guid);
> +        }
> +        deferred.resolve(guid);
> +      }.bind(this)

Arrows.

@@ +141,5 @@
> +        deferred.resolve(guid);
> +      }.bind(this)
> +    );
> +
> +    return deferred.promise;

This promise will never be rejected if the query fails, right? That can't be good.

@@ +161,5 @@
> +    this.asyncQuery(query).then(
> +      function() {
> +        deferred.resolve();
> +      }
> +    );

Error handling.

@@ +173,5 @@
> +               "FROM moz_bookmarks b " +
> +               "LEFT JOIN moz_places p ON b.fk = p.id " +
> +               "WHERE b.id in (" + ids.join(",") + ") AND b.type in (" + types.join(",") + ")";
> +    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
> +    let query = db.createAsyncStatement(stmt);

Why not use getQuery here?

::: services/cloudsync/CloudSyncTabs.jsm
@@ +15,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils", "resource://gre/modules/PrivateBrowsingUtils.jsm");
> +XPCOMUtils.defineLazyServiceGetter(this, "Session", "@mozilla.org/browser/sessionstore;1", "nsISessionStore");
> +
> +const API_VERSION = "v1";
> +const DATA_VERSION = "v1";

Let's just use integers here. The 'v' gives the impression of lexical ordering, and isn't all that useful!

Comment 39

[Alan K [:ack]]

* docs: will need this, but shouldn't block initial patch

* tabs/windows: not required at this point (consistent with current aboutSyncTabs display). not a big deal to add this later and increment the version number for tabs data.

* versioning: I would prefer dotted version numbers so we can indicate when a change is not backward-compatible (0.1.0 -> 0.2.0 or something), but straightforward integers is fine with me

* I didn't use getQuery here because some query parameters had to be inlined, so a new query is produced for each call. parameter substitution was not suitable here.

* error handling: doing another pass to clean this up for next patch.

Comment 40

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 41

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 42

[Richard Newman [:rnewman]]

Comment on attachment 8460399 [details] [diff] [review]
cloudsync.patch

Review of attachment 8460399 [details] [diff] [review]:
-----------------------------------------------------------------

Before I can finish review of this, I need you to do a self-review and answer your own review comments. That means reading through this patch and making sure that someone who's not you can tell:

* What each file or class is for, goal-wise
* How they are supposed to collaborate to achieve that goal
* How you can know if they're working correctly.

I can try to *guess* the above, but (a) my review will spot more problems if I have more information, and (b) this code is likely to stick around for ten years. Be nice to our successors.

There are currently zero docs and zero tests, and the formatting isn't finished (see nits, and apply them everywhere). Address those things -- as well as reading each of my previous reviews to see if you missed anything -- then let's see another patch.

::: browser/base/content/sync/aboutSyncTabs.js
@@ +8,5 @@
>  Cu.import("resource://services-sync/main.js");
>  Cu.import("resource:///modules/PlacesUIUtils.jsm");
>  Cu.import("resource://gre/modules/PlacesUtils.jsm", this);
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/CloudSync.jsm");

This should be a lazyModuleGetter. Users who aren't using CloudSync shouldn't pay the price.

This might require you to do a little more work in the cloudsync:tabs:update handler, or to have a more decoupled signaling mechanism there.

@@ +140,5 @@
>  
> +  _waitingForBuildList: false,
> +  _buildListRequested: false,
> +  buildList: function(force) {
> +    if(this._waitingForBuildList) {

Nit: space before parens.

@@ +147,5 @@
> +    }
> +
> +    this._waitingForBuildList = true;
> +    this._buildListRequested = false;
> +    

Nit: whitespace.

@@ +159,5 @@
> +    }
> +
> +    function complete() {
> +      this._waitingForBuildList = false;
> +      if(this._buildListRequested) {

Nit: space.

@@ +164,5 @@
> +        CommonUtils.nextTick(this.buildList, this);
> +      }
> +    }
> +
> +    if(cloudSync.Tabs.hasRemoteTabs()) {

Same.

@@ +230,5 @@
> +  _generateCloudSyncTabList: function() {
> +    let updateTabList = function(remoteTabs) {
> +      let list = this._tabsList;
> +
> +      for each(let client in remoteTabs) {

Nit: space before (

@@ +238,5 @@
> +        };
> +        let clientEnt = this.createItem(clientAttrs);
> +        list.appendChild(clientEnt);
> +
> +        for(let tab of client.tabs) {

Same throughout.

@@ +249,5 @@
> +          let tabEnt = this.createItem(tabAttrs);
> +          list.appendChild(tabEnt);
> +        }
> +      }
> +    }.bind(this);

Prefer fat arrows to bind (throughout).

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +1002,4 @@
>    },
>  
>    shouldShowTabsFromOtherComputersMenuitem: function() {
> +    return true;

Firstly, if you're doing this, the page has to be meaningful without Sync set up. You talked in Comment 32 about offering to set up FxA.

A bug has to be on file and work beginning for this to be acceptable; we can't have completely dead-end UI available to all users when this reaches Beta.

Secondly, in Comment 22 and Comment 34 I reiterated that about:sync-tabs needs to die: a bug needs to be on file, and the desktop UI folks need to be starting to look at it.

Link both of those bugs here and in aboutSyncTabs.js.

@@ +1006,4 @@
>    },
>  
>    shouldEnableTabsFromOtherComputersMenuitem: function() {
> +    return true;

This is probably wrong; if the user isn't syncing tabs (either via Sync or via CloudSync), this should be disabled.

::: services/cloudsync/CloudSync.jsm
@@ +5,5 @@
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["cloudSync"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Remove Cc and Ci.

@@ +10,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/CloudSyncAdapters.jsm");
> +Cu.import("resource://gre/modules/CloudSyncLocal.jsm");
> +Cu.import("resource://gre/modules/CloudSyncBookmarks.jsm");
> +Cu.import("resource://gre/modules/CloudSyncTabs.jsm");

Is it necessary to import all of these, rather than making them lazy module getters?

@@ +13,5 @@
> +Cu.import("resource://gre/modules/CloudSyncBookmarks.jsm");
> +Cu.import("resource://gre/modules/CloudSyncTabs.jsm");
> +
> +let CloudSync = function() {
> +

Nit: no newline.

@@ +36,5 @@
> +  },
> +
> +  _local: null,
> +  get local() {
> +    if(!this._local) {

Nit: space before (, throughout.

@@ +51,5 @@
> +    return this._tabs;
> +  },
> +
> +};
> +

Nit: no double newlines (throughout).

::: services/cloudsync/CloudSyncAdapters.jsm
@@ +5,5 @@
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["Adapters"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

You don't use any of these. Kill the line.

@@ +8,5 @@
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +this.Adapters = function() {
> +

Whitespace throughout.

@@ +15,5 @@
> +Adapters.prototype = {
> +
> +  _registeredAdapters: {},
> +
> +  // Should call whenever a sync adapter starts.

Should be called? By whom?

Should call something? What?

@@ +21,5 @@
> +    opts = opts || {};
> +    this._registeredAdapters[name] = opts;
> +  },
> +
> +  // Should call when stopping a sync adapter.

Same.

@@ +27,5 @@
> +    delete this._registeredAdapters[name];
> +  },
> +
> +  getAdapterNames: function() {
> +    return Object.keys(this._registeredAdapters);

Use a Map instead of an object. Saves you doing `delete` on a JS object, which is undesirable.

::: services/cloudsync/CloudSyncBookmarks.jsm
@@ +5,5 @@
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["Bookmarks"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Kill Cc, Ci.

@@ +24,5 @@
> +  "NULL",
> +  "BOOKMARK",
> +  "FOLDER",
> +  "SEPARATOR",
> +  "DYNAMIC_CONTAINER", // deprecated

More verbose explanation, please.

@@ +51,5 @@
> +    this.queue.push(item);
> +    this.schedule();
> +  },
> +
> +  schedule: function() {

Nit: "function (".

Substantive: it's not clear what "schedule" means in this context.

Does it mean "figure out when to do some lengthy operation, and do it then"? If so, this method does not do that.

If it means "process queued items on next tick", then maybe this could have a better name, like "processQueuedItems"?

@@ +52,5 @@
> +    this.schedule();
> +  },
> +
> +  schedule: function() {
> +    if(!this.pending && this.queue.length) {

Nit: whitespace before (, throughout entire patch.

@@ +65,5 @@
> +    let item = this.queue.shift();
> +
> +    item.process().then(
> +      this.schedule.bind(this),
> +      this.schedule.bind(this)

Call `bind` only once. It generates a new function object, which is expensive.

Also, you've set up a needless mutual recursion here:

  schedule -> nextTick -> process -> schedule 

Why not have

  schedule: function () {
    if (this.pending || !this.queue.length) {
      return;
    }
    CommonUtils.nextTick(this.process, this, () => { this.pending = false; });
    this.pending = true;
  },

  process: function (done) {
    this.queue.shift().process().then(done, done);
  },

This makes me think that the mutual recursion isn't expressing what you intend -- this processes a single item, not all remaining items. There are no docstrings or tests to tell me what is *supposed* to happen here. Please change that.

::: services/cloudsync/CloudSyncEventSource.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +this.EXPORTED_SYMBOLS = ["EventSource"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

You can guess.

@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://services-common/utils.js");
> +
> +let EventSource = function(types, suspendFunc, resumeFunc) {

this.EventSource = 

and kill the last line of the file.

@@ +8,5 @@
> +
> +Cu.import("resource://services-common/utils.js");
> +
> +let EventSource = function(types, suspendFunc, resumeFunc) {
> +  this.listeners = {};

Use a Map.

@@ +56,5 @@
> +    }
> +    CommonUtils.nextTick(
> +      function() {
> +        for(let listener of this.listeners[type]) {
> +          listener.call(undefined, arg);

What if a listener throws? You won't finish. Wrap this in an exception handler, and log the shit out of it.

::: services/cloudsync/CloudSyncFolderCache.jsm
@@ +5,5 @@
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["FolderCache"];
> +
> +let FolderCache = function() {

Can our poor unlucky successors get some docs to explain why they shouldn't just delete the file? What kind of folder does it store? What kind of IDs? What are you caching? What's the expiration policy? What does this do that a Map doesn't? Why doesn't this cache have any observers?

@@ +9,5 @@
> +let FolderCache = function() {
> +  this.cache = {};
> +}
> +
> +FolderCache.prototype = {

You're using a map from ID to record. Each record has a parent ID pointer and a set of children.

Each entry, then, burns multiple objects. Each operation apart from child ID -> parent ID involves hitting multiple records.

Is this the best data structure for what you're doing with the cache? It's hard to tell what you intend it to achieve.

@@ +17,5 @@
> +  },
> +
> +  insert: function(id, parentId) {
> +    if(id in this.cache) {
> +      return false;

What if the parent has changed? You're forcing the caller to guess why you're returning false, and switch behaviors.

@@ +21,5 @@
> +      return false;
> +    }
> +
> +    if(parentId && !(parentId in this.cache)) {
> +      return false;

This strikes me as an awful exceptional situation.

@@ +26,5 @@
> +    }
> +
> +    this.cache[id] = {
> +      parent: parentId || null,
> +      children: new Set(),

Why are children not ordered? Explain why in the docstring.

@@ +47,5 @@
> +      this.cache[parentId].children.delete(id);
> +    }
> +
> +    for(let child of this.cache[id].children) {
> +      this.cache[child].parent = null;

I would regard deletion of a parent without first deleting its children as a programming error. I suspect Places agrees with me. Why does this cache allow it? Why `null` rather than a sentinel expressing that the parent has been deleted?

@@ +82,5 @@
> +
> +    return this.cache[id].parent;
> +  },
> +
> +  setChildren: function(id, children) {

This destroys encapsulation -- e.g., it doesn't make sure that that old or new children records are consistent. Elsewhere you do strong validation, like refusing to insert a record if its parent isn't already known.

Is this a standalone cache, or is it a map owned by the caller? You're between two stools right now.

@@ +92,5 @@
> +  },
> +
> +  getChildren: function(id) {
> +    if(!(id in this.cache)) {
> +      return []

Why not just return undefined? This isn't a Set, after all.

@@ +104,5 @@
> +  },
> +
> +};
> +
> +this.FolderCache = FolderCache;
\ No newline at end of file

Similar comment.

::: services/cloudsync/CloudSyncLocal.jsm
@@ +13,5 @@
> +Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://services-crypto/utils.js");
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +
> +const API_VERSION = "0.0.1";

What for?

@@ +23,5 @@
> +
> +this.Str = {};
> +["errors", "sync"].forEach(function(lazy) {
> +  XPCOMUtils.defineLazyGetter(Str, lazy, lazyStrings(lazy));
> +});

This is way more expensive and less readable than just

{"errors": ...,
 "sync": ...}

@@ +29,5 @@
> +function makeGUID() {
> +  return CommonUtils.encodeBase64URL(CryptoUtils.generateRandomBytes(9));
> +}
> +
> +this.Local = function() {

What is a Local? What does it do?

@@ +35,5 @@
> +  let prefs = new Preferences("services.cloudsync.");
> +
> +  this.__defineGetter__("prefs", function() {
> +    return prefs;
> +  });

Does this win you much over

  this.prefs = new Preferences("services.cloudsync.");

?

@@ +43,5 @@
> +Local.prototype = {
> +
> +  get id() {
> +    let clientId = this.prefs.get("client.GUID", "");
> +    return clientId == "" ? this.id = makeGUID(): clientId;

Nit: spaces around ":".

@@ +53,5 @@
> +
> +  get name() {
> +    let clientName = this.prefs.get("client.name", "");
> +    if (clientName != "")
> +      return clientName;

Modern style, please.

Also:

  !!("")
  false

::: services/cloudsync/CloudSyncPlacesWrapper.jsm
@@ +15,5 @@
> +Cu.import("resource://services-common/utils.js");
> +
> +let PlacesQueries = function() {
> +
> +}

Trailing ;.

@@ +32,5 @@
> +  }
> +
> +};
> +
> +/*

Kill it or fix it.

::: services/cloudsync/CloudSyncTabs.jsm
@@ +143,5 @@
> +    return results;
> +  },
> +
> +  isEmpty: function() {
> +    return isEmptyObject(this.clients);

Can't you just directly test for the minimal set of required properties?

Comment 43

[Alan K [:ack]]

Attachment: cloudsync.patch

* I haven't changed the formatting. From what I can tell, it's consistent with the code in services/healthreport. Same with bind() vs =>.

* Added docs, including an example

* Using Maps instead of objects/properties where it makes sense

* Opened bugs for issues and added bug# annotations where relevant

* Removed commented/dead code and lingering debug dumps/logs

* Removed observerQueue from CloudSyncBookmarks.jsm (no more schedule/process)

* Not addressing clock drift for lastUsed in tabs (consistent with current sync behavior). Can open a bug for this if necessary.

* I haven't added mochitests yet, but they are coming.

Comment 44

[Richard Newman [:rnewman]]

Comment on attachment 8463542 [details] [diff] [review]
cloudsync.patch

Review of attachment 8463542 [details] [diff] [review]:
-----------------------------------------------------------------

Summary:

* Style nits
* Typos and minor code improvements
* Lots of .then improvements, and consider using `yield` instead
* Good start on the docs, but lots of open questions.

Looking forward to the tests!

::: browser/base/content/sync/aboutSyncTabs.js
@@ +142,5 @@
>  
> +  _waitingForBuildList: false,
> +  _buildListRequested: false,
> +  buildList: function(force) {
> +    if(this._waitingForBuildList) {

Nit: space before (, throughout. Always always

  if (

@@ +149,5 @@
> +    }
> +
> +    this._waitingForBuildList = true;
> +    this._buildListRequested = false;
> +    

Nit: trailing whitespace.

@@ +166,5 @@
> +        CommonUtils.nextTick(this.buildList, this);
> +      }
> +    }
> +
> +    if(cloudSync.tabs.hasRemoteTabs()) {

This makes your lazy getter a little pointless, no? Needs some mechanism to determine if you have any sync adapters without loading a ton of code.

@@ +232,5 @@
> +  _generateCloudSyncTabList: function() {
> +    let updateTabList = function(remoteTabs) {
> +      let list = this._tabsList;
> +
> +      for each(let client in remoteTabs) {

Nit: space before (.

@@ +240,5 @@
> +        };
> +        let clientEnt = this.createItem(clientAttrs);
> +        list.appendChild(clientEnt);
> +
> +        for(let tab of client.tabs) {

Nit: space before (.

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +1006,5 @@
>  
>    shouldShowTabsFromOtherComputersMenuitem: function() {
> +    let weaveReady = Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED &&
> +                     Weave.Svc.Prefs.get("firstSync", "") != "notReady";
> +    let cloudSyncReady = CloudSync.tabs.hasRemoteTabs();

Shouldn't this be "cloudSync" instead of "CloudSync"? (And below.)

If you've tested this, then that kinda implies that this code never runs.

::: services/cloudsync/CloudSyncBookmarks.jsm
@@ +73,5 @@
> +        this.type = CS_FOLDER;
> +      }
> +      break;
> +    case PlacesUtils.bookmarks.TYPE_BOOKMARK:
> +      if(this.uri.indexOf("place:") == 0) {

.startsWith

@@ +141,5 @@
> +        deferred.resolve(rootFolderId);
> +      },
> +      function(error) {
> +        deferred.reject(error);
> +      }

Just

  deferred.reject

-- no need to wrap it in another function.

@@ +168,5 @@
> +    PlacesWrapper.getLocalIdsWithAnnotation(ROOT_FOLDER_ANNO).then(
> +      checkRootFolder,
> +      function(err) {
> +        deferred.reject(err);
> +      }

Same.

@@ +177,5 @@
> +        deferred.resolve(rootFolderObject);
> +      },
> +      function(err) {
> +        deferred.reject(err);
> +      }

Same.

@@ +221,5 @@
> +        ).then(
> +          deleteFolderDeferred.resolve(),
> +          function(err) {
> +            deleteFolderDeferred.reject(err);
> +          }

Same.

@@ +240,5 @@
> +        deferred.resolve();
> +      },
> +      function(err) {
> +        deferred.reject(err);
> +      }

Same for both.

@@ +287,5 @@
> +  let getCachedFolderIds = function(cache, roots) {
> +    let nodes = [...roots];
> +    let results = [];
> +
> +    while(nodes.length) {

Nit: space before (.

@@ +318,5 @@
> +      return PlacesWrapper.getItemsByParentId(parents, types)
> +    }
> +
> +    function getParentGuids(results) {
> +      results = Array.prototype.concat.apply([], results);

:(

@@ +328,5 @@
> +            return Promise.resolve(result);
> +          },
> +          function(err) {
> +            return Promise.reject(err);
> +          }

Promise.reject

@@ +346,5 @@
> +            return Promise.resolve(result);
> +          },
> +          function(err) {
> +            return Promise.reject(err);
> +          }

Same.

@@ +362,5 @@
> +    Promise.all(promises).then(
> +      getParentGuids
> +    ).then(
> +      getAnnos
> +    ).then(

Gentle preference for

  Foo.bar()
     .then(doFoo)
     .then(doBar)

which matches how we do chaining elsewhere, but nbd if you're strongly in favor of this style.

@@ +370,5 @@
> +        });
> +        deferred.resolve(results);
> +      },
> +      function(err) {
> +        deferred.reject(err);

The usual.

@@ +396,5 @@
> +            result.parent = guidResult;
> +            return Promise.resolve(result);
> +          },
> +          function(err) {
> +            return Promise.reject(err);

...

@@ +414,5 @@
> +        });
> +        deferred.resolve(results);
> +      },
> +      function(err) {
> +        deferred.reject(err);

...

@@ +447,5 @@
> +      }
> +
> +      function fail(err) {
> +        deferred.reject(err);
> +      }

Just use `deferred.reject` directly.

@@ +498,5 @@
> +        deferred.resolve();
> +      },
> +      function(error) {
> +        deferred.reject(error);
> +      }

Usual. Throughout.

::: services/cloudsync/CloudSyncLocal.jsm
@@ +50,5 @@
> +
> +  get name() {
> +    let clientName = this.prefs.get("client.name", "");
> +    if (clientName != "")
> +      return clientName;

Always brace conditionals.

@@ +62,5 @@
> +    try {
> +      let syncStrings = new StringBundle("chrome://browser/locale/sync.properties");
> +      appName = syncStrings.getFormattedString("sync.defaultAccountApplication", [brandName]);
> +    } catch (ex) {}
> +    appName = appName || brandName;

More newlines in this method, please. You can borrow some from lines 30-40 :)

::: services/cloudsync/CloudSyncPlacesWrapper.jsm
@@ +67,5 @@
> +      }
> +    ).then(
> +      function(id) {
> +        deferred.resolve(id);
> +      }.bind(this)

Why does this need to be bound? And same comment as always about function wrapping.

::: services/cloudsync/docs/architecture.rst
@@ +3,5 @@
> +============
> +Architecture
> +============
> +
> +CloudSync offers functionality similar to Firefox sync for data sources. Thirt-party addons

s/sync/Sync
s/Thirt/Third

@@ +32,5 @@
> +CloudSyncLocal.jsm
> +    Provides information about the local device, such as name and a unique id.
> +
> +CloudSyncPlacesWrapper.jsm
> +    Wraps parts of the places API in promises. Some methods are implemented to be asynchronous

s/places/Places

::: services/cloudsync/docs/dataformat.rst
@@ +1,5 @@
> +.. _cloudsync_dataformat:
> +
> +=========
> +Data Format
> +=========

Overall restrictions? Max record size? Required fields for all records?

@@ +10,5 @@
> +Record
> +------
> +
> +id:
> +    unique ID for each bookmark item

You probably mean "unique GUID". Give example and constraints.

@@ +12,5 @@
> +
> +id:
> +    unique ID for each bookmark item
> +
> +parent: 

Nit: trailing whitespace.

@@ +13,5 @@
> +id:
> +    unique ID for each bookmark item
> +
> +parent: 
> +    id of parent folder

GUID or numeric? Example.

@@ +16,5 @@
> +parent: 
> +    id of parent folder
> +
> +index:
> +    item index in parent folder; indices must be unique and contiguous

What happens if they're not?

@@ +19,5 @@
> +index:
> +    item index in parent folder; indices must be unique and contiguous
> +
> +title:
> +    bookmark or folder title; not meaningful for separators

How do I know it's a separator? There's no 'type' field.

@@ +22,5 @@
> +title:
> +    bookmark or folder title; not meaningful for separators
> +
> +dateAdded:
> +    timestamp for item added

In seconds? Milliseconds? Since epoch?

@@ +40,5 @@
> +ClientRecord
> +------------
> +
> +id:
> +	unique ID for each client

Indenting. Same comment as above.

@@ +43,5 @@
> +id:
> +	unique ID for each client
> +
> +name:
> +    name for this client; may not be unique

Not guaranteed to be unique. "May not be unique" implies that the client is responsible for adding a duplicate!

@@ +56,5 @@
> +TabRecord
> +---------
> +
> +title:
> +	name for this tab

Size limitation?

@@ +59,5 @@
> +title:
> +	name for this tab
> +
> +url:
> +	URL for this tab; only one tab for each URL is stored

Indenting.

@@ +62,5 @@
> +url:
> +	URL for this tab; only one tab for each URL is stored
> +
> +icon:
> +    favicon for this tab

URL? Data? What happens if there's no icon?

::: services/cloudsync/docs/example.rst
@@ +7,5 @@
> +.. code-block:: javascript
> +
> +	Cu.import("resource://gre/modules/CloudSync.jsm");
> +
> +	var HelloWorld = {

`let`

@@ +16,5 @@
> +
> +
> +	    cloudSync.tabs.addEventListener("change", function() {
> +	      console.log("tab change");
> +	      cloudSync.tabs.getLocalTabs().then(

Should clients debounce themselves, or will cloudSync do it for them? How often do you send a change notification? What changes trigger one?

@@ +55,5 @@
> +	      }
> +	    );
> +
> +	    cloudSync.bookmarks.getRootFolder("Hello World").then(
> +	      function(rootFolder) {

These examples would benefit tremendously from being rephrased as `yield` inside a task. For that matter, so would the code. You're hitting async callback hell.

::: services/moz.build
@@ +36,1 @@
>  SPHINX_TREES['services'] = 'docs'

I think you need to wire in the docs here, too, or move your docs into services/docs.

Comment 45

[Alan K [:ack]]

Attachment: cloudsync.patch

* Formatting changes, fixed promise error handlers

* Module loading for about:sync-tabs is staged and nothing is initialized until first use

* Rechecked tab menu code, was missed when I rebuilt locally. Both function are executed as expected.

* Added API docs from github gist and updated RST docs

* No debouncing is done here. Not sure if it's needed, but I can open a bug for it if necessary.

* Docs build correctly via build-docs (via services/cloudsync/moz.build)

Comment 46

[Alan K [:ack]]

Attachment: cloudsync.patch

Added a couple of sanity checks.

Comment 47

[Alan K [:ack]]

Attachment: cloudsync.patch

Added a quick mochitest to make sure that getting local tabs from the session store doesn't break.

Comment 48

[Richard Newman [:rnewman]]

Comment on attachment 8465192 [details] [diff] [review]
cloudsync.patch

Time for this to get superreview (although I'm not finished reviewing).

Gavin, would you please redirect as necessary? Starting with you 'cos this is a new Firefox feature.

Comment 49

[Tracy Walker [:tracy]]

:rnewman, :ack, who what when where why and how? :-) Me thinks a brain dump, or at minimum, be pointed to project page is in order for QA to proceed here. Thanks.

Comment 50

[Richard Newman [:rnewman]]

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #49)
> :rnewman, :ack, who what when where why and how? :-) Me thinks a brain dump,
> or at minimum, be pointed to project page is in order for QA to proceed
> here. Thanks.

QA plan is down to Alan; I don't have all the knowledge I'd need to write one, even if I had the time!

Comment 51

[Tracy Walker [:tracy]]

I can create the QA test plan, at least for the client side, if provided with some information about the project and its general functional expectations.

Comment 52

[Alan K [:ack]]

Attachment: cloudsync.patch

Updated CloudSyncTabs.jsm, using original code to fetch local tabs and removed dead code.

Comment 53

[Richard Newman [:rnewman]]

Comment on attachment 8467888 [details] [diff] [review]
cloudsync.patch

Review of attachment 8467888 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag until style nits (and other review comments, I assume) from last review are addressed.

I'd like the next uploaded patch to not require me to type "Nit: ".

::: browser/base/content/browser-places.js
@@ +589,4 @@
>        return;
>  
>      if (!PlacesUIUtils.shouldShowTabsFromOtherComputersMenuitem()) {
> +      dump("hide menu item");

Remove dump statements added in this file.

::: browser/base/content/sync/aboutSyncTabs.js
@@ +12,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "cloudSync",
> +                                  "resource://gre/modules/CloudSync.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "cloudSyncReady",
> +                                  "resource://gre/modules/CloudSync.jsm");

I don't think this will do what you want it to do.

_cloudSyncReady will only be true if cloudSync has been accessed.

You want to see if you *should* access cloudSync -- i.e., that there's something to sync to.

Does this imply that whenever a cloudSync adapter is configured, it's eagerly initialized?

@@ +146,5 @@
> +
> +  _buildListRequested: false,
> +
> +  buildList: function(force) {
> +    if(this._waitingForBuildList) {

Nit: space before (.

@@ +153,5 @@
> +    }
> +
> +    this._waitingForBuildList = true;
> +    this._buildListRequested = false;
> +    

Nit: whitespace.

Comment 54

[Alan K [:ack]]

> Remove dump statements added in this file.

Mistake, fixed.

> Nit: whitespace.

This seems readable to me. Are you expecting an extra blank line here? If not, please be specific.

> You want to see if you *should* access cloudSync -- i.e., that there's something to sync to.

CloudSync adapters are just addons, they initialize themselves. This is meant to test if cloudSync has been accessed in a way that won't cause it to become ready. The symbols here are imported separately because I don't see a way to specify multiple symbols to import when calling defineLazyModuleGetter.

Comment 55

[Brad Lassey [:blassey] (use needinfo?)]

> > Nit: whitespace.
> 
> This seems readable to me. Are you expecting an extra blank line here? If
> not, please be specific.
No, your blank line starts with 4 spaces, delete those.

Comment 56

[Alan K [:ack]]

Ah, gotcha. Fixed.

Comment 57

[Richard Newman [:rnewman]]

What's the ni for?

Comment 58

[Alan K [:ack]]

(copied from above)

> You want to see if you *should* access cloudSync -- i.e., that there's something to sync to.

CloudSync adapters are just addons, they initialize themselves. This is meant to test if cloudSync has been accessed in a way that won't cause it to become ready. The symbols here are imported separately because I don't see a way to specify multiple symbols to import when calling defineLazyModuleGetter. Am I wrong here?

Comment 59

[Richard Newman [:rnewman]]

(In reply to Alan K [:ack] from comment #58)
> Am I wrong here?

If it works, I'm happy!

Comment 60

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 61

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 62

[Alan K [:ack]]

* Removed missed calls to dumps
* Removed trailing whitespace
* Refactored lazy loading and added a test (see test_lazyload.js)

Comment 63

[Richard Newman [:rnewman]]

Comment on attachment 8470638 [details] [diff] [review]
cloudsync.patch

Review of attachment 8470638 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/cloudsync/CloudSync.jsm
@@ +26,5 @@
> +
> +  _adapters: null,
> +
> +  get adapters() {
> +    if(!this._adapters) {

DOESN'T FIX STYLE NITS

░░░░░▄▄▄▄▀▀▀▀▀▀▀▀▄▄▄▄▄▄░░░░░░░
░░░░░█░░░░▒▒▒▒▒▒▒▒▒▒▒▒░░▀▀▄░░░░
░░░░█░░░▒▒▒▒▒▒░░░░░░░░▒▒▒░░█░░░
░░░█░░░░░░▄██▀▄▄░░░░░▄▄▄░░░░█░░
░▄▀▒▄▄▄▒░█▀▀▀▀▄▄█░░░██▄▄█░░░░█░
█░▒█▒▄░▀▄▄▄▀░░░░░░░░█░░░▒▒▒▒▒░█
█░▒█░█▀▄▄░░░░░█▀░░░░▀▄░░▄▀▀▀▄▒█
░█░▀▄░█▄░█▀▄▄░▀░▀▀░▄▄▀░░░░█░░█░
░░█░░░▀▄▀█▄▄░█▀▀▀▄▄▄▄▀▀█▀██░█░░
░░░█░░░░██░░▀█▄▄▄█▄▄█▄████░█░░░
░░░░█░░░░▀▀▄░█░░░█░█▀██████░█░░
░░░░░▀▄░░░░░▀▀▄▄▄█▄█▄█▄█▄▀░░█░░
░░░░░░░▀▄▄░▒▒▒▒░░░░░░░░░░▒░░░█░
░░░░░░░░░░▀▀▄▄░▒▒▒▒▒▒▒▒▒▒░░░░█░
░░░░░░░░░░░░░░▀▄▄▄▄▄░░░░░░░░█░░ 

RE-REQUESTS REVIEW


Let me know if you want me to upload a new patch that follows the correct style, so you have something to work from.

Comment 64

[Alan K [:ack]]

Attachment: cloudsync.patch

Fixed critical style nits.

Comment 65

[Richard Newman [:rnewman]]

Attachment: Interdiff for reading purposes

* Fixed typo (e.g., "rejsolve") that presumably wasn't covered by tests
* Fixed style (e.g., trailing commas, newlines, spacing)
* Removed unnecessary function wrappers
* Inverted some conditionals
* Early returns
* Tracked the in-tree move of PlacesUIUtils.

Alan, give this a skim and make sure it looks right to you.

Comment 66

[Richard Newman [:rnewman]]

Attachment: Updated patch. v1

Here's your patch with my changes applied.

Comment 67

[Richard Newman [:rnewman]]

Attachment: Updated patch. v2

Forgot to hit aboutSyncTabs.js.

Comment 68

[Richard Newman [:rnewman]]

Comment on attachment 8470951 [details] [diff] [review]
Updated patch. v2

Review of attachment 8470951 [details] [diff] [review]:
-----------------------------------------------------------------

That's enough of a review pass for now, I think.

You'll need Gavin or another superreviewer to sign off on this, so re-flagging him.

::: browser/base/content/sync/aboutSyncTabs.js
@@ +264,5 @@
> +      }
> +    }.bind(this);
> +
> +    return CloudSync().tabs.getRemoteTabs()
> +                           .then(updateTabList);

You should handle the rejection here, no?

::: browser/confvars.sh
@@ +30,5 @@
>  MOZ_SERVICES_CRYPTO=1
>  MOZ_SERVICES_HEALTHREPORT=1
>  MOZ_SERVICES_METRICS=1
>  MOZ_SERVICES_SYNC=1
> +MOZ_SERVICES_CLOUDSYNC=1

Move this chunk to Bug 1052009; we can't turn this on until ship blockers are tackled.

::: configure.in
@@ +8306,5 @@
>  if test -n "$MOZ_SERVICES_SYNC"; then
>    AC_DEFINE(MOZ_SERVICES_SYNC)
>  fi
>  
> +dnl Build Sync Services if required

s/Sync Services/CloudSync.

::: services/cloudsync/CloudSyncBookmarks.jsm
@@ +134,5 @@
> +    ).then(
> +      setRootShortcutCloudSyncAnnotation
> +    ).then(
> +      setRootFolderExcludeFromBackupAnnotation
> +    ).then(

Sorry, I missed this on my pass. Pleas rearrange these to be

  foo.bar()
     .then(...)
     .then(...);

@@ +169,5 @@
> +    ).then(
> +      function (rootFolderObject) {
> +        deferred.resolve(rootFolderObject);
> +      },
> +      deferred.reject

Same.

@@ +212,5 @@
> +        }
> +      ).then(
> +        function () {
> +          deleteFolderDeferred.resolve()
> +        },

Replace with deleteFolderDeferred.resolve.

@@ +492,5 @@
> +                folderCache.setParent(localId, parent);
> +              }
> +              return PlacesWrapper.moveItem(localId, parent, index);
> +            }
> +          ).then(deferred.resolve);

This is why I like putting chained .then on their own line.

Should this have a rejection handler?

@@ +650,5 @@
> +
> +    if (rootId == parent || folderCache.has(parent)) {
> +      return false;
> +    }
> +    

Oops, trailing whitespace. My bad.

::: services/cloudsync/CloudSyncLocal.jsm
@@ +73,5 @@
> +      // hostname of the system, usually assigned by the user or admin
> +      Cc["@mozilla.org/system-info;1"].getService(Ci.nsIPropertyBag2).get("host") ||
> +      // fall back on ua info string
> +      Cc["@mozilla.org/network/protocol;1?name=http"].getService(Ci.nsIHttpProtocolHandler).oscpu;
> +      

Trailing whitespace.

::: services/cloudsync/CloudSyncPlacesWrapper.jsm
@@ +52,5 @@
> +    query.params.guid = guid.toString();
> +
> +    this.asyncQuery(query, ["item_id"])
> +        .then(getLocalId, deferred.reject)
> +        .then(deferred.resolve);

Should this have a rejection handler?

@@ +74,5 @@
> +    query.params.item_id = id;
> +
> +    this.asyncQuery(query, ["guid"])
> +        .then(getGuid, deferred.reject)
> +        .then(deferred.resolve);

Likewise.

@@ +91,5 @@
> +    query.params.guid = guid;
> +    query.params.item_id = localId;
> +
> +    this.asyncQuery(query)
> +        .then(deferred.resolve, deferred.reject);

... because this does.

::: services/cloudsync/docs/api.md
@@ +1,5 @@
> +### Importing the JS module
> +
> +````
> +Cu.import("resource://gre/modules/CloudSync.jsm");
> + 

Trailing whitespace throughout this file.

::: services/cloudsync/docs/architecture.rst
@@ +22,5 @@
> +CloudSyncBookmarks.jsm
> +    Provides operations for interacting with bookmarks.
> +
> +CloudSyncBookmarksFolderCache.jsm
> +    Implements a cache used to store folder heirarchy for filtering bookmark events.

hierarchy

::: services/cloudsync/docs/dataformat.rst
@@ +15,5 @@
> +type:
> +    record type; one of CloudSync.bookmarks.{BOOKMARK, FOLDER, SEPARATOR, QUERY, LIVEMARK}
> +
> +id:
> +    GUID for this bookmark item

See each of my comments in Comment 44. I shan't repeat them here.

::: services/cloudsync/tests/mochitest/test_windowEvents.html
@@ +28,5 @@
> +function runTest () {
> +  function handleTabChangeEvent () {
> +    cloudSync.tabs.removeEventListener("change", handleTabChangeEvent);
> +    ok(true, "tab change event");
> +    

Trailing whitespace.

Comment 69

[Alan K [:ack]]

Attachment: cloudsync.patch

Can you elaborate on the indentation issue in architecture.rst? My intention is for that to be a definition list.

I rebased and applied your patch. Examples and tests pass.

Comment 70

[Richard Newman [:rnewman]]

(In reply to Alan K [:ack] from comment #69)

> Can you elaborate on the indentation issue in architecture.rst? My intention
> is for that to be a definition list.

No indentation issue: you misspelled hierarchy.

Comment 71

[Alan K [:ack]]

Attachment: cloudsync.patch

Imports for classes and interfaces missing from CloudSyncLocal.jsm -- fixed.

Comment 72

[Alan K [:ack]]

I've suggested that Gavin review the elements that touch existing code. Is there anything in addition that he should be reviewing?

Comment 73

[Gregory Szorc [:gps]]

Comment on attachment 8473424 [details] [diff] [review]
cloudsync.patch

Review of attachment 8473424 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/cloudsync/tests/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +XPCSHELL_TESTS_MANIFESTS += ['xpcshell/xpcshell.ini']
> +
> +MOCHITEST_CHROME_MANIFESTS += ['mochitest/chrome.ini']

Please merge the contents of this moz.build file into the parent directory's.

Comment 74

[Gregory Szorc [:gps]]

rnewman is on holiday until August 25. Unless you made arrangement to have him review this during his holiday, you might be waiting for a while.

Comment 75

[Alan K [:ack]]

Attachment: cloudsync.patch

* fix events not firing after a bookmarks error
* merged moz.builds

Comment 76

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 77

[Alan K [:ack]]

Attachment: cloudsync.patch

Comment 78

[Alan K [:ack]]

Comment on attachment 8476411 [details] [diff] [review]
cloudsync.patch

Spoke with rnewman. Super review should cover:

(a) this is the right approach, 
(b) it's necessary, and 
(c) it meets all of the requirements we have for landing big new chunks of Firefox

Adding review request for rnewman on this patch since he's not done reviewing.

Comment 79

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8476411 [details] [diff] [review]
cloudsync.patch

(In reply to Alan K [:ack] from comment #78)
> (a) this is the right approach, 

What alternate approaches were considered? Are the constraints laid out somewhere beyond comment 0? This is generally something that should be covered by a code review or up-front planning. If you have specific concerns here, lay them out.

> (b) it's necessary, and 

That answer seems to have mostly been settled.

> (c) it meets all of the requirements we have for landing big new chunks of
> Firefox

rnewman's review should cover this. If there are specific concerns, happy to discuss further.

I'm trying to get out of your way here - I'm asserting this doesn't need super-review. If there are specific concerns or parts of the patch that rnewman doesn't feel comfortable r+ing, please point those out.

Comment 80

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #79)

> I'm trying to get out of your way here - I'm asserting this doesn't need
> super-review. If there are specific concerns or parts of the patch that
> rnewman doesn't feel comfortable r+ing, please point those out.

My primary high-level concern is to make sure that the desktop team is happy supporting this as part of Firefox.

I was asked to review the code and impart lessons from Sync, so I've done that as best I can without being able to run it, or evaluate its performance or correctness empirically. I've asked Alan to make sure there are at least some basic tests and docs, and I've requested that a bunch of follow-ups be filed and block enabling the feature in Nightly.

But we're under no illusions: this is code that is not well-exercised by any non-partner code; after it's turned on there will be no people assigned to work on it; and even the reviewer (me) will not be -- as is our custom -- fixing any bugs that arise. It's approaching landing only because I trust Alan (and Brad and Andreas) when they say it's important to land it and they're confident that it works well enough to ship.

Landing this is effectively approving the creation of a new submodule in Firefox for the desktop team to maintain, even though they have had ~0 say in its design or implementation. It doesn't feel quite right for me to do so without involving you.

Bearing in mind the existing list of follow-ups in the dependencies, are you happy with this bundle of code/tests/docs becoming part of the API surface area of Firefox (which I understand to be a decision that benefits from sr), and moreover having the desktop team maintain it for as long as necessary?

Comment 81

[Richard Newman [:rnewman]]

Comment on attachment 8476411 [details] [diff] [review]
cloudsync.patch

Review of attachment 8476411 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix the inline comments below (e.g., seconds/microseconds, input validation, making tests exercise Unicode…) before landing, and check any questions and fix any problems that the test changes reveal.

One non-trivial point that's not mentioned below: URI encoding safety.

Thoroughly read Bug 586082. Here you're directly querying Places via SQL, so you're getting back "raw" URI strings, as best I can tell. I don't see anywhere that you encode these in a consistent fashion. That can cause unexpected consequences.

Please file a follow-up and block Bug 1052009 if cloudSync suffers from this. The fix should be trivial, so it's worth making sure encoding problems are dead before launching.

Beyond those things: if it works, make sure tests pass and land it!

::: services/cloudsync/CloudSyncLocal.jsm
@@ +16,5 @@
> +Cu.import("resource://services-crypto/utils.js");
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +
> +function lazyStrings(name) {
> +  let bundle = "chrome://weave/locale/services/" + name + ".properties";

It occurs to me: this might not be packaged if MOZ_SERVICES_SYNC isn't set. That shouldn't occur, but be aware that this could fail if cloudSync is enabled and Sync is not.

@@ +17,5 @@
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +
> +function lazyStrings(name) {
> +  let bundle = "chrome://weave/locale/services/" + name + ".properties";
> +  return function () new StringBundle(bundle);

Either () => new StringBundle(bundle), or add {}.

::: services/cloudsync/CloudSyncPlacesWrapper.jsm
@@ +52,5 @@
> +    query.params.guid = guid.toString();
> +
> +    this.asyncQuery(query, ["item_id"])
> +        .then(getLocalId, deferred.reject)
> +        .then(deferred.resolve, deferred.rejec);

s/rejec/reject.

@@ +246,5 @@
> +      return Promise.reject("unable to parse URI '" + uri + "': " + e);
> +    }
> +
> +    try {
> +      let id = PlacesUtils.bookmarks.insertBookmark(parent, parsedURI, index, title, guid);

Does insertBookmark adequately validate invalid/null/empty parent/index/title/guid?

::: services/cloudsync/docs/api.md
@@ +66,5 @@
> +     lastUsed: 1400799296192390},
> +    {title: "Reddit",
> +     url: "http://www.reddit.com",
> +     icon: "http://www.reddit.com/favicon.ico",
> +     lastUsed: 1400799296192390

That looks like a value in *microseconds*. The doc says it should be in seconds. Is the code wrong, or is this example wrong?

It's probably worth putting a one-liner range check somewhere -- either log or throw if you get obviously-wrong values.

@@ +125,5 @@
> +
> +Bookmark type. Used in results objects.
> +
> +````
> +var bookmarkType = rootFolder.BOOKMARK;

Use `let` not `var`, even in docs.

::: services/cloudsync/docs/dataformat.rst
@@ +45,5 @@
> +ClientRecord
> +------------
> +
> +id:
> +	GUID for this client

There's mixed indenting in this file. Please fix.

@@ +67,5 @@
> +url:
> +	URL for this tab; only one tab for each URL is stored
> +
> +icon:
> +    favicon for this tab; optional

"favicon URL", I presume.

::: services/cloudsync/tests/mochitest/test_windowEvents.html
@@ +49,5 @@
> +
> +  cloudSync.tabs.addEventListener("change", handleTabChangeEvent);
> +
> +  window.open("other_window.html");
> +  window.open("other_window.html");

Can we make one of these include URL parameters, escaping, and non-ASCII?

::: services/cloudsync/tests/xpcshell/test_tabs.js
@@ +18,5 @@
> +      id: "001",
> +      name: "FakeClient",
> +    },[
> +      {url:"https://www.google.ca",title:"Google Canada",icon:"https://www.google.ca/favicon.ico",lastUsed:0},
> +      {url:"http://www.reddit.com",title:"Reddit",icon:"http://www.reddit.com/favicon.ico",lastUsed:1},

Throw some Unicode into these.

Comment 82

[:Gavin Sharp [email: [e-mail]]]

(In reply to Richard Newman [:rnewman] from comment #80)
> Bearing in mind the existing list of follow-ups in the dependencies, are you
> happy with this bundle of code/tests/docs becoming part of the API surface
> area of Firefox (which I understand to be a decision that benefits from sr),
> and moreover having the desktop team maintain it for as long as necessary?

Yes. "As long as necessary" isn't a known timeline, and neither is the amount of maintenance necessary - when those things are known the story might change, but we can have that conversation then.

Comment 83

[Alan K [:ack]]

Attachment: cloudsync.patch

* Converted to microseconds everywhere, updated docs accordingly
* Fixed formatting/typos
* Added unicode/escapes to URLs in tests

Comment 84

[Alan K [:ack]]

Attachment: 0001-Bug-993584-Initial-landing-for-CloudSync.patch

Comment 85

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Can we get a Try run for this and the other cloudsync bugs?

Comment 86

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/2df073cb512c

Comment 87

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX browser_946320_tabs_from_other_computers.js failures.
https://hg.mozilla.org/integration/fx-team/rev/afb1867dba70

https://tbpl.mozilla.org/php/getParsedLog.php?id=47102998&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=47102437&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=47103241&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=47103297&tree=Fx-Team

Comment 88

[Ryan VanderMeulen [:RyanVM]]

(In reply to Wes Kocher (:KWierso) from comment #85)
> Can we get a Try run for this and the other cloudsync bugs?

Might have helped.

Comment 89

[Ryan VanderMeulen [:RyanVM]]

Was perma-fail on Windows as well. Linux was green because the test is skipped there.

Comment 90

[Alan K [:ack]]

One of my tests was missing a fix. I'm still getting test timeouts for an unrelated test: https://tbpl.mozilla.org/?tree=Try&rev=acf7a6a5b02d.

Comment 91

[Alan K [:ack]]

Attachment: 0001-Bug-993584-Initial-landing-for-CloudSync.patch

Comment 92

[Alan K [:ack]]

https://tbpl.mozilla.org/?tree=Try&rev=f0128f81350f

Comment 93

[:Gavin Sharp [email: [e-mail]]]

This shouldn't land with sr=gavin, because it didn't get sr+.

Comment 94

[Alan K [:ack]]

Attachment: 0001-Bug-993584-Initial-landing-for-CloudSync.patch

Remove sr=.

Comment 95

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/c4c2474a4f75

Comment 96

[Carsten Book [:Tomcat]]

sorry had to backout in https://tbpl.mozilla.org/?tree=Fx-Team&rev=48b63c04b664 since i guess that one of this changesets caused https://tbpl.mozilla.org/php/getParsedLog.php?id=47317116&tree=Fx-Team

Comment 97

[Alan K [:ack]]

I'm doing a try build with cloudsync disabled, maybe that's the problem?

Comment 98

[Alan K [:ack]]

https://tbpl.mozilla.org/?tree=Try&rev=7138259914e6 (disabled)
https://tbpl.mozilla.org/?tree=Try&rev=7c69d5131a39 (enabled)

The tabs mochitest has a failure. Could be a timing issue. I'm refactoring the test to make it less fragile.

Comment 99

[Alan K [:ack]]

I'm pretty sure the intermittent test failure is caused by the delay in the test itself. It needs to flush the tab data instead of using a delay. Will fix and attach a new patch.

The other test failures are caused by cloudsync being disabled. I need to add checks for this in existing code.

Comment 100

[Alan K [:ack]]

Attachment: 0001-Bug-993584-Initial-landing-for-CloudSync.patch

Comment 101

[Alan K [:ack]]

https://tbpl.mozilla.org/?tree=Try&rev=f9527e78253c

Comment 102

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/bb965f10472c

Comment 103

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb965f10472c

Comment 104

[Tracy Walker [:tracy]]

Is this pref'ed off on Nightly? I can't get it to work with latest Nightly (20140911) and the partner extension. Clicking their Sync icon does not open the accounts page as expected.

Comment 105

[Alan K [:ack]]

Should be pref'd on in nightly. I just checked m-c and all my patches are there. I think it was merged this morning though so check tonight's build.

Comment 106

[Tracy Walker [:tracy]]

This is just as broken with the try build from comment #101.  Now I'm starting to suspect their extension.  Alan, do you have a fully working setup?

Comment 107

[Tracy Walker [:tracy]]

ah, bug 1052009 enabling this on trunk just landed and will be available tomorrow. Sanity check of the try-build there shows this is ok. I will sign-off with testing against Nightly tomorrow.

Comment 108

[Alan K [:ack]]

Comment on attachment 8484764 [details] [diff] [review]
0001-Bug-993584-Initial-landing-for-CloudSync.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:required for partner project
[Describe test coverage new/current, TBPL]:sanity tests
[Risks and why]:low impact, mostly new code
[String/UUID change made/needed]:

Comment 109

[Alan K [:ack]]

There are several patches that need to be uplifted. I'll add them in a single file shortly.

Comment 110

[Lawrence Mandel [:lmandel] (use needinfo)]

I spoke with Alan about this feature. Some notes:

- No string changes
- Can be disabled with the build time flag MOZ_SERVICES_CLOUDSYNC
- For a partner project that we have committed to delivering in 34
- Light on automated tests - I would prefer to see more because (see next point)
- Unlikely to receive any input from our pre-release user populations as this requires a separate add-on that is not yet ready

ni on GMC for their awareness of this feature and agreement that we should proceed with this in desktop.
ni on Paul to inform pr.

Comment 111

[Alan K [:ack]]

Attachment: 0001-CloudSync-all.patch

Comment 112

[Chad Weiner [:chadw]]

On behalf of GMC, I acknowledge awareness and willingness.

Comment 113

[Alan K [:ack]]

Additional automated tests to cover bookmarks: https://bugzilla.mozilla.org/show_bug.cgi?id=1074640

Once these land, I'll update the composite patch in comment#111.

Comment 114

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Alan K [:ack] from comment #113)
> Additional automated tests to cover bookmarks:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1074640
> 
> Once these land, I'll update the composite patch in comment#111.

And then the plan is to get this on Aurora.

Comment 115

[Alan K [:ack]]

Attachment: 0001-CloudSync-all.patch

Updated patch includes additional bookmarks tests from bug#1074640.

Comment 116

[Alan K [:ack]]

Comment on attachment 8499603 [details] [diff] [review]
0001-CloudSync-all.patch

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]:For a partner project that we have committed to delivering in 34
[Describe test coverage new/current, TBPL]: Basic test coverage for API, some mochitest coverage for UI events
[Risks and why]: Low impact, mostly new code; Unlikely to receive any input from our pre-release user populations as this requires a separate add-on that is not yet ready
[String/UUID change made/needed]: No string changes

Comment 117

[Alan K [:ack]]

I'm pushing a try build for latest patch. Will link when available.

Comment 118

[Alan K [:ack]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0f672a152187

Comment 119

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8484764 [details] [diff] [review]
0001-Bug-993584-Initial-landing-for-CloudSync.patch

Clearing the Aurora nom on this patch. The "all" patch has everything needed for uplift.

Comment 120

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8499603 [details] [diff] [review]
0001-CloudSync-all.patch

As noted in comment 116, we're taking this largely isolated feature in 34 for a partner opportunity. Alan is on the hook for handling any fallout. Note that this feature is unlikely to see any feedback from user testing on beta and will need a more dedicated test plan in order to receive coverage. Aurora+

Comment 121

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34c81d98d96a