969669 - Display data type selection UI whenever meta/global indicates that another client could not offer one of our supported datatypes

Status: RESOLVED WONTFIX

(Firefox :: Sync, enhancement)

Description

[Richard Newman [:rnewman]]

Brief summary: Android Sync won't offer to sync your prefs etc. (because it can't), so if you set up Sync on your phone, your desktop will never sync add-ons or prefs.

Fix this -- and future extensions -- by re-offering to choose what to sync when this situation is detected.



From an old email:

----
Here's the issue. Android syncs some stuff. Desktop syncs more: add-ons, prefs. (It only syncs these to other desktops.)

The Sync protocol was never designed to handle a growing set of engines. It has no built-in mechanism for adding to an existing set, or for clients to have different capabilities but still share datatype elections.

This is OK right now: only desktop can create Sync accounts. Android simply ignores the engines it doesn't know about when it syncs.

But now Android can create a new FxA+Sync account.

We have a number of options. *dons ideation hat*


1. Include prefs and add-ons in the Android datatype elections. This has the potential to be confusing, even if we call them out specially as only affecting desktop, but it's the least complex option. We are still screwed if we add an engine in the future (as we were when we added add-on sync).

2. Don't include prefs and add-ons. Anyone who sets up an account on Android won't be syncing prefs or add-ons. This is dodgy.

3. Include prefs and add-ons in the defaults, but don't show them in the UI. Users who don't want to be syncing prefs or add-ons between their desktops *will be*, without their knowledge.

4. Try to figure out a smarter thing to do in the protocol altogether, in the vein of "voting" with explicit capabilities. This is difficult, and I don't think in scope for 29 (or Sync 1.5 in general).

5. Do a limited, smarter thing on desktop clients, the beginnings of 'growth handling':
1. If this is the first meta/global we've seen, and
2. It omits engines we know about ("addons" and "prefs"), then
3. Pop up a pre-filled datatype election dialog, just as if you asked to do so during setup, or prompt them to take a look in settings to turn some stuff on.

6. Some combination of #5 and #3.

7. #6 but better: try to figure out a dumb thing we can do in the protocol, whereby meta/global entries are tri-state: on, off, unknown. Do option #5, but smarter.

----



ibarlow's opinion:


----
- not show users stuff that isn’t of use on that particular platform. In other words, don’t show desktop sync options on Android.

- For the small subset of users who opt to customize their sync options on their android device first, and then sign in on their desktop browser second, show the datatype elections UI on both platforms. First on Android with the basic options (bookmarks / passwords / history / tabs), and then again on Desktop with the additional Prefs / Add-ons options in the list. 
----



Bending this a little bit to address our goals, the conclusion is somewhere between #5 and #7 in my original email -- either *guess* that if "prefs" and "addons" are missing from meta/global, we should show a dialog, or *explicitly* track "declined" engines in meta/global. The latter requires client support and a spec bump.

Because this involves a spec bump, we should do it before 29 reaches Beta.

Comment 1

[Richard Newman [:rnewman]]

Marking dev-doc-needed to update the object formats doc.

Comment 2

[Richard Newman [:rnewman]]

Nobody who sets up Sync on Android will be syncing prefs or add-ons from desktop. Tracking 29?

Comment 3

[Benjamin Kerensa [:bkerensa]]

(In reply to Richard Newman [:rnewman] from comment #2)
> Nobody who sets up Sync on Android will be syncing prefs or add-ons from
> desktop. Tracking 29?

Richard, I'm unsure this is something we will want to track since its going to have a low impact on our users can you give more information on why you need us to track this and why we would block release on this?

Comment 4

[Richard Newman [:rnewman]]

I quote the first line of the enormous wall of text in Comment 0:

---
Brief summary: Android Sync won't offer to sync your prefs etc. (because it can't), so if you set up Sync on your phone, your desktop will never sync add-ons or prefs.
---

I'd add to that "... and the user won't know unless they go poking around in the Settings UI".

As far as I know, "tracking" doesn't strictly imply "blocking release"; it just means "tracking for this release".

Work that's not tracking won't get done.

This falls into the same bucket as the other work that needs to get done in order to ship Firefox Accounts-based Sync in 29; it's just as user-affecting as the rest of it. It might slip to 30, but that'll be a decision for some weeks from now.

Hope that clarifies.

Comment 5

[Richard Newman [:rnewman]]

Note also Comment 0's observation that this requires a spec bump, and so has to be done before 29 reaches GA.

Comment 6

[Richard Newman [:rnewman]]

Attachment: WIP. v1

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8378788 [details] [diff] [review]
WIP. v1

Greg, if you have time to skim this, please do.

Core concept: separate "not present in m/g:engines" from "user decided not to sync this".

An engine that's not present in meta/global, but is available locally, is eligible to be presented to the user, because they haven't yet turned it off.

This applies in two scenarios:

* It's their first sync on this device, and they created their account on Android, where prefs and add-ons aren't available for syncing.

* We added a new sync engine to the version they just installed, and so they have never opted in to syncing it.

Next up: hooking in the engine selection UI to mark unchecked engines as declined, manual testing, etc. etc.

Comment 8

[Richard Newman [:rnewman]]

Observation: startOver should clean up the engineManager.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8378788 [details] [diff] [review]
WIP. v1

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

Nifty approach.

Before I grant r+, I'd like more tests (which I'm sure you would write):

1) Start with an old style metaglobal and verify the client stores the appropriate new value
2) Start with a metaglobal with declines, sync, verify everything that should occur does
3) Ensure a sync doesn't update the metaglobal record if declines haven't changed

::: services/sync/modules/engines.js
@@ +504,5 @@
> +    for (let e of engines) {
> +      this._declined.add(e);
> +    }
> +  },
> +    

Nit: WS

::: services/sync/modules/stages/declined.js
@@ +27,5 @@
> +  for (let x of b) {
> +    out.add(x);
> +  }
> +  return out;
> +}

It baffles me that JS doesn't offer these on Set.

@@ +66,5 @@
> +      return false;
> +    }
> +  }
> +  return true;
> +}

Shouldn't these all be in util.js if not toolkit/modules?

@@ +112,5 @@
> +        undecided: undecided,
> +      };
> +      CommonUtils.nextTick(function () {
> +        Observers.notify("weave:engines:notdeclined", subject);
> +      });

I'm surprised to see that you are a fan of the spread operator but aren't using fat arrows to reduce typing here :)

::: services/sync/modules/stages/enginesync.js
@@ +223,5 @@
>  
>      this.service._ignorePrefObserver = true;
>  
>      let enabled = this.service.enabledEngineNames;
> +

Nit: WS

::: services/sync/tests/unit/test_declined.js
@@ +64,5 @@
> +
> +  let declined = manager.getDeclined();
> +  _("Declined: " + JSON.stringify(declined));
> +  // This engine is disabled locally, but that doesn't mean it was declined.
> +  do_check_eq(-1, declined.indexOf("actual"));

I'd use Assert.equal and friends in new tests.

Comment 11

[Richard Newman [:rnewman]]

Attachment: Part 0: add set utilities to CommonUtils. v1

Let's put them here for now, and move them later.

Comment 12

[Richard Newman [:rnewman]]

Attachment: Part 1: display data type selection UI whenever meta/global indicates that another client could not offer one of our supported datatypes. v2

Comments addressed:

* Moved set operations to part 0.
* Use Assert.
* More tests.
* Refactor EngineSynchronizer stage to allow testing.

Comment 13

[Richard Newman [:rnewman]]

I split out the non-UI parts of this into Bug 978876, because those can be uplifted to 29, and I want them to bake for as long as possible while I work on the UI.

Comment 14

[Richard Newman [:rnewman]]

QA steps for this:

* Create a new account. During creation, customize datatypes.
* Wait for a sync to complete.
* Open a Browser Console (after enabling devtools.chrome.enabled).
* Evaluate the following:

  let service = Weave.service;
  let meta = service.recordManager.get(service.metaURL);
  meta.payload.declined = [];
  service.uploadMetaGlobal(meta);

* Quit the browser.

The server now has a meta/global that hasn't declined some engines that a client supports. This simulates a client that has a new engine, never-before-seen.

* Launch a new profile.
* Sign in to Sync.
* Observe that a grey bar pops up, offering new data type choices.
* Make data type elections.
* Sync both profiles, restart both profiles. Observe that no new grey bar appears.

Comment 15

[Richard Newman [:rnewman]]

Correction:

  let service = Weave.Service;
  let meta = service.recordManager.get(service.metaURL);
  meta.payload.declined = [];
  service.uploadMetaGlobal(meta);

Comment 16

[Richard Newman [:rnewman]]

Attachment: Hook into pref changes to mark engines as declined.

This builds on top of Bug 978876. Not too much here. UI will need a little work, so consider this a POC to demonstrate the flow; I'll get UI input before landing.

Comment 17

[Richard Newman [:rnewman]]

Ryan, John: can we chat briefly Thur or Fri about how we might present some kind of affordance here in 30 or 31? I've sketched out a shitty programmer UI based on Sync's "grey bar", but I have no doubt that you can do better.

Comment 18

[John Gruen [:jgruen]]

Richard, my calendar is up to date. Definitely grab some time for us to talk about this.

Comment 19

[Richard Newman [:rnewman]]

Note to self: some add-ons insert themselves as engines in the engine list. If they're disabled in that add-on's UI, it'll probably not appear to be correctly opted-out. Must handle this case.

Comment 20

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

We discussed this in triage on Wednesday (desktop UX/Eng/PM and Identity/services UX/Eng/QA in attendance). The conclusion we reached was that "option 2" from comment 0 (i.e. do nothing) is acceptable for Firefox 29.

Our understanding is that most of the negative impact is limited to the case where you set up your account on Firefox for Android, and then also link it to two desktop accounts (in that case the two desktops won't sync prefs/add-ons). It also affects the single-desktop/single-Android case, in that we won't be "backing up" your prefs/add-ons, but that's not a primary use case for Sync (at least not yet).

Given the relative user populations, the percentage of users creating Sync accounts on Android will be low. So while far from optimal and we should fix it, we're not going to call this a 29 blocker. We could still take the patches if ready, but at this point it seems like targeting 30 is a better bet.

Comment 21

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #20)
> We discussed this in triage on Wednesday (desktop UX/Eng/PM and
> Identity/services UX/Eng/QA in attendance). The conclusion we reached was
> that "option 2" from comment 0 (i.e. do nothing) is acceptable for Firefox
> 29.

I have no intention of addressing the UI portion of this in 29 -- it would need string changes, and we're only starting to talk about the UI today -- so I'm surprised this was discussed.

The object format change (Bug 978876 for desktop, Bug 969672 for Android) has to land, of course; all deployed clients need to do the right thing from day 1.


> It also affects the single-desktop/single-Android case, in
> that we won't be "backing up" your prefs/add-ons, but that's not a primary
> use case for Sync (at least not yet).

That case is unaffected. Android has no prefs or add-ons to back up, and desktop would be backing them up unless you chose otherwise.

(There is a single-device case that's affected, but you can only hit it if you go from 1A -> 0 -> 1D devices.)

Comment 22

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

(In reply to Richard Newman [:rnewman] from comment #21)
> The object format change (Bug 978876 for desktop, Bug 969672 for Android)
> has to land, of course; all deployed clients need to do the right thing from
> day 1.

We're not tracking it for 29 at the moment. Can you expand on the reasoning for "needs to land" in that bug, and make it block bug 969593?

> That case is unaffected. Android has no prefs or add-ons to back up, and
> desktop would be backing them up unless you chose otherwise.

That's not what you said in comment 0?

"Anyone who sets up an account on Android won't be syncing prefs or add-ons."

Comment 23

[Richard Newman [:rnewman]]

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

> We're not tracking it for 29 at the moment. Can you expand on the reasoning
> for "needs to land" in that bug, and make it block bug 969593?

I'll add a comment transferring the tracking from here to there.

> That's not what you said in comment 0?
> 
> "Anyone who sets up an account on Android won't be syncing prefs or add-ons."

Android doesn't sync prefs or add-ons at all. If you're a single-device user, and that device is Android, you don't care. If you're a single-device user, and that device is a desktop, you'll be "backing up" your prefs and add-ons.

Perhaps you meant the two-device case where you created the account on Android, rather than the single-* cases?

Comment 24

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

(In reply to Richard Newman [:rnewman] from comment #23)
> Perhaps you meant the two-device case where you created the account on
> Android, rather than the single-* cases?

Yes.

"single-desktop/single-Android case" -> "single-desktop+single-Android case", if that helps clarify!

Comment 25

[Richard Newman [:rnewman]]

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

> "single-desktop/single-Android case" -> "single-desktop+single-Android
> case", if that helps clarify!

Gotcha, thanks. Yes, those users will be affected if they signed up on Android first.

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8382570 [details] [diff] [review]
Part 1: display data type selection UI whenever meta/global indicates that another client could not offer one of our supported datatypes. v2

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

There's room for improvement. But nothing that should stand in the way of r+.

::: services/sync/modules/service.js
@@ +1255,5 @@
> +      }
> +
> +      // Upload the new meta/global.
> +      this._log.info("XXX uploading meta/global: " + JSON.stringify(meta));
> +      // TODO: X-I-U-S.

You really want to leave this as TODO? File a bug and update comment, please.

@@ +1260,5 @@
> +      let res = this.resource(this.metaURL);
> +      let resp = res.put(meta);
> +      if (!resp.success) {
> +        throw resp;
> +      }

This looks like copied code from _freshStart(). Should we create uploadMeta()?

::: services/sync/modules/stages/declined.js
@@ +1,3 @@
> +/* 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/. */

"file," is supposed to be on line 3.

@@ +45,5 @@
> +    // it as changed.
> +    let declinedChanged = !CommonUtils.setEqual(newDeclined, remoteDeclined);
> +    if (declinedChanged) {
> +      meta.changed = true;
> +      meta.payload.declined = [...newDeclined];

Do you want to sort this for consistency? I suppose it doesn't matter if everyone is treating it as a set...

::: services/sync/modules/stages/enginesync.js
@@ +70,5 @@
>        infoURL += "?v=" + WEAVE_VERSION;
>        Svc.Prefs.set("lastPing", now);
>      }
>  
> +    let engineManager = this.service.engineManager;

There was once a time when I would get a part 0 with all these minor refactors...

@@ +143,5 @@
>        return;
>      }
>  
>      try {
> +      for each (let engine in engineManager.getEnabled()) {

for (let engine of ...)

@@ +274,5 @@
>      Svc.Prefs.resetBranch("engineStatusChanged.");
>      this.service._ignorePrefObserver = false;
>    },
>  
> +  _updateEnabledEngines: function _updateEnabledEngines() {

Nit: kill the function name.

::: services/sync/tests/unit/test_declined.js
@@ +8,5 @@
> +Cu.import("resource://services-common/observers.js");
> +
> +function run_test() {
> +  run_next_test();
> +}

I just filed bug 982852 to make this boilerplate optional.

@@ +74,5 @@
> +  let declinedEngines = new DeclinedEngines(Service);
> +
> +  function onNotDeclined(subject, topic, data) {
> +    Observers.remove("weave:engines:notdeclined", onNotDeclined);
> +    Assert.ok(subject.undecided.has("actual"), "EngineManager observed that 'actual' was undecided.");

Shouldn't you verify the other properties of subject?

@@ +131,5 @@
> +
> +    Assert.equal(declined.indexOf("clients"), -1, "'clients' is enabled and not remotely declined.");
> +    Assert.equal(declined.indexOf("petrol"), -1, "'petrol' is enabled and not remotely declined.");
> +    Assert.equal(declined.indexOf("diesel"), -1, "'diesel' is enabled and not remotely declined.");
> +    Assert.equal(declined.indexOf("dummy"), -1, "'dummy' is enabled and not remotely declined.");

All these indexOf make me wonder if a Set should be involved.

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8385880 [details] [diff] [review]
Hook into pref changes to mark engines as declined.

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

Not getting r+ because no l10n.

::: browser/base/content/browser-syncui.js
@@ +136,5 @@
>    },
>  
> +  onNotDeclinedEnginesDiscovered: function () {
> +    let title = "Choose what to sync";
> +    let description = "Your Sync options have changed.";

Where's the l10n? See onSyncDelay() for how I think this is supposed to be implemented.

Comment 28

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #27)

> Where's the l10n? See onSyncDelay() for how I think this is supposed to be
> implemented.

Yeah, this is programmer UI. John's gonna throw up some wireframes at some point.

Comment 29

[Richard Newman [:rnewman]]

Requesting to punt this to 31, now that the more urgent stuff moved to Bug 978876 (and landed for 29).

Comment 30

[Richard Newman [:rnewman]]

jgruen sketched out how this would work, but I won't have time to do it.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Stop tracking this bug. No activity for the past month and low priority.

Comment 32

[Mark Hammond [:markh] [:mhammond]]

We have work-arounds for this and will probably end up with something completely different anyway