1183917 - Move "choose what to sync" to the Web

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Chris Karlof [:ckarlof]]

https://github.com/mozilla/fxa/issues/18

At the least, this will involve:

* sending a signal to FxA Web content that is supports this flow
* processing the selections made in Web content and seeding the Sync engine

Comment 1

[Vlad Filippov (:vladikoff)]

Attachment: Bug-1183917-Move-choose-what-to-sync-to-the-Web.patch

When you have time could you provide some feedback if this is the right approach to take to set the engine list and avoid the popup?

Comment 2

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

Comment on attachment 8635449 [details] [diff] [review]
Bug-1183917-Move-choose-what-to-sync-to-the-Web.patch

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

LGTM - it's going to be difficult to test this which sucks :( Another consideration is what to do if the user disables all available engines - I guess "nothing" might be an option, but that would suck a little. If the content-server UI prevents them from disabling all checkboxes we would probably be OK except in pathological cases.

I'm also wondering how the content server will know it is dealing with a client that has support for this - UA sniffing, or something else?

::: services/sync/modules/browserid_identity.js
@@ +235,5 @@
> +            }
> +
> +            this._log.info("Received declined engines");
> +            this._log.info(JSON.stringify(declinedSyncEngines));
> +            Weave.Service.engineManager.setDeclined(declinedSyncEngines);

I haven't looked deeply, but I don't think we need to set the declined here - just disabling the engine should be fine and the existing declineDisabled() call below should do the right thing.

Comment 3

[Vlad Filippov (:vladikoff)]

Planning to have this in Firefox 43 Nightly on August 11...

Comment 4

[Vlad Filippov (:vladikoff)]

Thanks for the feedback Mark!

> it's going to be difficult to test this which sucks :(

Testing will include incoming messages that have `.declinedSyncEngines`. as part of the login message

> Another consideration is what to do if the user disables all available engines - I guess "nothing" might be an option, but that would suck a little. 

Same behaviour as we have now, everything can be deselected in the native confirm box, which results in all engines being unchecked in the preference ux: http://v14d.com/i/55b80f917e8cb.png Figuring the "all unchecked" is out of scope of this PR.

> I'm also wondering how the content server will know it is dealing with a client that has support for this - UA sniffing, or something else?

In the future a client can send a message using a WebChannel or something and include that it supports this flow. I'm gonna update my PR to add some kind of a signal that FF43 supports this flow via about:accounts.

Comment 5

[Vlad Filippov (:vladikoff)]

Attachment: Bug-1183917-Add-ability-to-use-choose-what-to-sync-f.patch

Comment 6

[Vlad Filippov (:vladikoff)]

Comment on attachment 8641286 [details] [diff] [review]
Bug-1183917-Add-ability-to-use-choose-what-to-sync-f.patch

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

::: services/sync/modules/browserid_identity.js
@@ +235,5 @@
> +            }
> +
> +            this._log.info("Received declined engines");
> +            this._log.info(JSON.stringify(declinedSyncEngines));
> +            Weave.Service.engineManager.setDeclined(declinedSyncEngines);

This line is gone from the latest version

Comment 7

[Vlad Filippov (:vladikoff)]

Ryan & Mark, can I get some feedback. We are adding a "capabilities" event on the content server that will ask the client about what it supports (https://github.com/mozilla/fxa-content-server/pull/2757)

Here is a GIF of before and after: http://v14d.com/g/bug1183917.gif

Comment 8

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

Comment on attachment 8641286 [details] [diff] [review]
Bug-1183917-Add-ability-to-use-choose-what-to-sync-f.patch

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +154,5 @@
> +        Services.prefs.setCharPref(PREF_SYNC_DECLINED_ENGINES, JSON.stringify(accountData.declinedSyncEngines));
> +
> +        delete accountData.declinedSyncEngines;
> +      }
> +      delete accountData.customizeSync;

This was outside the block because I was seeing accountData.customizeSync=false - has that now changed on the server side?

::: services/fxaccounts/FxAccounts.jsm
@@ +1303,5 @@
>          return currentState.reject(error);
>        }
>      ).catch(err => Promise.reject(this._errorToErrorClass(err)));
>    },
> +  getCapabilities: function () {

A random thought - would it be possible to change the identity.fxaccounts.remote.signup.uri preference to include a modified query portion that carries the fact the client is capable? Or even just UA sniffing? This just seems a little overkill (although maybe it dovetails into some future ideas?)

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +199,5 @@
>    setShowCustomizeSyncPref(showCustomizeSyncPref) {
>      Services.prefs.setBoolPref(PREF_SYNC_SHOW_CUSTOMIZATION, showCustomizeSyncPref);
>    },
>  
> +  getShowCustomizeSyncPref() {

nice catch, thanks!

::: services/sync/modules/browserid_identity.js
@@ +230,5 @@
> +          if (declinedSyncEngines) {
> +            try {
> +              declinedSyncEngines = JSON.parse(declinedSyncEngines);
> +            } catch (e) {
> +              this._log.error("Failed to parse declined sync engines", e);

I think we need to take some other action here, else declinedEngines will remain the invalid string that couldn't be parsed. I guess we'd want to arrange to enter the offerSyncOptions block below.

@@ +233,5 @@
> +            } catch (e) {
> +              this._log.error("Failed to parse declined sync engines", e);
> +            }
> +
> +            this._log.info("Received declined engines");

Note that |this._log.info("Received declined engines", declinedSyncEngines);| should magically do the right thing and log the stringified data.

@@ +254,3 @@
>  
>              // Mark any non-selected engines as declined.
>              Weave.Service.engineManager.declineDisabled();

I may have mislead you in the previous feedback, but I think we need to call declineDisabled() in both branches.

Comment 9

[Ryan Kelly [:rfkelly]]

Comment on attachment 8641286 [details] [diff] [review]
Bug-1183917-Add-ability-to-use-choose-what-to-sync-f.patch

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

I generally like how this is shaping up.  The "capabilities" handshake is something we talked about as an alternative to user-agenet sniffing, and it does feed into some future ideas.  More commentary on that in https://github.com/mozilla/fxa-content-server/pull/2757

Comment 10

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

(In reply to Ryan Kelly [:rfkelly] from comment #9)

> I generally like how this is shaping up.  The "capabilities" handshake is
> something we talked about as an alternative to user-agenet sniffing, and it
> does feed into some future ideas.  More commentary on that in
> https://github.com/mozilla/fxa-content-server/pull/2757

That link doesn't really give me much more context and I've still got a number of issues with the implementation.

The way I see it, we are adding an over-generically-named function to FxAccounts.jsm - |getCapabilities()|, which returns a constant value that has nothing to do with the account itself. This constant value is stored in a pref, but I can't see a scenario where we'd ever change the preference value - A/B testing and the pulling of a kill-switch would happen at the server rather than the client doing a chemspill or hotfix to adjust the pref.

So the server knows exactly what capabilities the client has based on the UA string. Given we support self-hosters, ISTM it's the client that doesn't know the capabilities of the server, so it also seems backwards! In this scenario we are working around the lack of server capability information by ensuring the data exchanged is consistent with older versions of the servers, which is great - and I'd expect we'd come up with similar solutions in the future (until we can't, at which time the client would need to determine the server capabilities)

So while I genuinely don't see what this is solving, if you really feel it is necessary, I'd prefer to have the patch do simply:

> + onCapabilities: function () {
> +   error("Received: 'capabilities'.");
> +   this.injectData("message", { status: "capabilities", data: {
> +     chooseWhatToSync: {
> +       name: 'choose_what_to_sync',
> +       version: 'web_v1'
> +     }
> +   });
> + }

(ie, encapsulated entirely in about:accounts using hard-coded values) and be done with it.

Comment 11

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

I also meant to say: consider a scenario where desktop and (say) android both advertised this capability in 42. But by the time 42 hits release we found desktop was subtly buggy in such a way that we decide it is prudent for desktop to fall back to the "old" way. Android works fine, so we'd prefer to keep using it there. We know that 43 will have a fix that allows us to reliably use it on desktop.

Would we then declare "web_v1" not working (therefore preventing Android using it) and adjust both Desktop and Android to declare they support "web_v2"? That seems suboptimal given web_v1 isn't actually the problem. Or would we do something else?

Comment 12

[Ryan Kelly [:rfkelly]]

> So the server knows exactly what capabilities the client has based on the UA string

Someone was warning us off doing UA-sniffing for this; TBH I thought it was you Mark, but I must be mis-remembering :-).  Perhaps it was :nalexander.

One downside of UA-sniffing is that users can mess with that string without realizing the consequences, and there are e.g. extensions designed specifically to achieve this for a variety of unrelated-to-firefox-accounts reasons.

> This constant value is stored in a pref, but I can't see a scenario where we'd ever change
> the preference value

Agreed, this is a property of the client itself and doesn't feel like it should be in a pref.

Zooming out from the specifics of naming and implementation, ISTM there's value in having the device and the server do an introductory handshake to coordinate their state.  The server can send a message like "hi, I'm fxa content server version XYZ, I support this set of functionality, etc" and then the device replying with "I support this intersecting set of functionality, I have a user logged in with this identity, etc".

Does it seem more or less sensible in that broader context?

Comment 13

[Ryan Kelly [:rfkelly]]

> Given we support self-hosters, ISTM it's the client that doesn't know the capabilities of the server

Also, thanks for raising this explicitly.  It's equally relevant for us if we ever need to roll back to a previous version of the server code.

Comment 14

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

(In reply to Ryan Kelly [:rfkelly] from comment #12)
> Zooming out from the specifics of naming and implementation, ISTM there's
> value in having the device and the server do an introductory handshake to
> coordinate their state.  The server can send a message like "hi, I'm fxa
> content server version XYZ, I support this set of functionality, etc" and
> then the device replying with "I support this intersecting set of
> functionality, I have a user logged in with this identity, etc".
> 
> Does it seem more or less sensible in that broader context?

Yes, this makes far more sense IMO (and I'll avoid proposing Firefox just report its version number here where UA-changing addons aren't able to touch ;)

Comment 15

[Ryan Kelly [:rfkelly]]

> and I'll avoid proposing Firefox just report its version number here where UA-changing addons aren't able to touch

Nice reverse psychology; I think that will be worth doing as a fallback for scenarios like Comment 11.

Comment 17

[Chris Karlof [:ckarlof]]

Mark, good point that this should be a two way affair.

I encouraged Vlad to go with the capabilities approach. The motivation for that is similar to the "feature detection" vs "UA sniffing" argument for Web development. The UA string is often too fragile to rely on and isn't scalable. I acknowledge this is a slightly different context, but we do want to avoid some giant switch statement handling two dozen UA strings in our server code. Our capability matching will also be non-trivial, but seems less gross to me. Also, we do have third-parties re-packaging Firefox or Firefox Sync with "interesting" UA strings (e.g., https://bugzilla.mozilla.org/show_bug.cgi?id=1189406#c7), so I'm not sure we can count on anything there.

Comment 18

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #14)
> (In reply to Ryan Kelly [:rfkelly] from comment #12)
> > Does it seem more or less sensible in that broader context?
> 
> Yes, this makes far more sense IMO (and I'll avoid proposing Firefox just
> report its version number here where UA-changing addons aren't able to touch
> ;)


The capabilities scheme allows the client side code to make fewer assumptions about what features the UA supports. It's a complete inversion of control to the current scheme. 

The UA would state explicitly, "this is what I can do." The client code would not need to maintain tables of UA version=>capabilities, which is the situation we currently find ourselves in. The notion of capabilities could be extended to include user flow control. 

Some examples might help make the desired goal more concrete. Firefox desktop wants to prevent FxA from polling for user verification? It could say so. Firefox for Android still wants FxA to poll? It could say so. Fx on iOS wants a user who signs in to redirect to the /settings page but about:accounts on FxDesktop does not? It could say so. Fx Desktop wants to ditch about:accounts and move everything to about:preferences#sync, with slightly different behaviors for the FxA flow? The browser would update its capabilities for the new context and the folks on the content-server would not need to be involved.

Comment 19

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

Note I'm not pushing back on the concept, but I think we need to keep things in perspective.

(In reply to Shane Tomlinson [:stomlinson] from comment #18)
> Firefox desktop wants to prevent FxA from polling for user verification? It
> could say so.

I'd restate this as "as of version XX, Firefox Desktop does <something>". It isn't like Nightly or any future Firefox will simply say "please don't offer Sync prefs via the web". Ditto "polling for verification". IOW, in a very short time, 100% of desktop Firefoxes we care about will do these things, so the value of these capabilities tends negative (ie, it's dead code once everything does it)

IMO, capabilities are more interesting for things where you don't already know the answer to (eg, a user preference or device configuration).

> The browser would update its
> capabilities for the new context and the folks on the content-server would
> not need to be involved.

That's very true - it does have alot of value in the introduction of new features. That's something the server doesn't already know.

> It's a complete inversion of control to the current scheme.

Which is a problem in the case of comment 11 - very soon, the behaviour of Firefox 42 will be fixed - so it has no control. All control is with the servers and it will save our bacon - witness the recent key rotation :)

tl;dr - I'm not objecting to this being a capability, but if Firefox ever unconditionally says "true", we should explore not asking it in the first place ;)

Comment 20

[Chris Karlof [:ckarlof]]

Some data on how many users choose what to sync: Bug 1130044

Comment 21

[Nick Alexander :nalexander [he/him]]

zaach: can I get a status update on where this is fxa-content-server side?

I think we need Bug 1191068 on day one of Fennec FxA web sign in.  It would be best to just do this on the web, all the time; but if we need to build a native UI, like we do on Desktop, we can arrange that.  It will take time and some Android UX work, though.

Comment 22

[Chris Karlof [:ckarlof]]

Descheduling this until we decide when we're going to actually coordinate to do it. Current thinking is 44.2.

Comment 23

[Chris Karlof [:ckarlof]]

The decision to delay to 44.2 assumed that timing would be ok. Nick can you confirm that?

Comment 24

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Karlof [:ckarlof] from comment #23)
> The decision to delay to 44.2 assumed that timing would be ok. Nick can you
> confirm that?

Yessir.  The intention is to land Fennec strings in the 43 cycle (this week :)), flip the build switch for Nightly at the start of the Fennec 44 cycle, and then flip the build switch for Aurora 43 cycle half way through that cycle.  (This gives us some bake time on Nightly.  Many pieces are landed but it hasn't had time to bake.)

So we expect to go live in Nightly and Aurora in about 3 weeks, which is halfway through 44.2.

We can have a slightly-worse Nightly experience (no choose what to Sync at all), or build the client before the server is ready, without much issue.  As long as this is ready for Aurora 43 going to Beta 43 -- that is, in 6 weeks -- Fennec will be happy.

Comment 25

[Nick Alexander :nalexander [he/him]]

vladikoff: you've done most of the work on the fxa-content-server side of things.  It would be convenient for clients if the 'login' message that fxa-content-server provides includes an "enginesToSync: {'bookmarks': true, 'history': false}" entry, or "enginesToSync: ['bookmarks']", or similar.  Have you already set a format for this addition?  If you have, or if we can agree on one, I can implement (and hand test) the client side code.

Comment 26

[Ryan Kelly [:rfkelly]]

We could potentially ship choose-what-to-sync on the web for Fennec without solving the Desktop piece represented in this bug.  In particular, since *all* fennec web logins would support the web-based chooser, we don't have to solve the capabilities questions raised above.

Given that Fennec (and iOS, which will also need this when they enable sign-ups) are the main short-term drivers, I suggest we move the scheduling discussion to Bug 1191068 where we can nail down a plan and a minimal set of requirements.

Comment 27

[Vlad Filippov (:vladikoff)]

> We could potentially ship choose-what-to-sync on the web for Fennec without solving the Desktop piece represented in this bug. 

+1

> It would be convenient for clients if the 'login' message that fxa-content-server provides includes an "enginesToSync: {'bookmarks': true, 'history': false}" entry, or "enginesToSync: ['bookmarks']", or similar.  Have you already set a format for this addition? 

The format uses the "declined engine", that is what Firefox Desktop uses. Please see https://github.com/mozilla/fxa-content-server/pull/3121/files#diff-c1a8b541d222ace921c5e34f1f6850e2R63 for more information. Let us know in that PR if that format is good or bad idea for Fennec.

Ref for Desktop code: http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/service.js#1333

Comment 28

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

(In reply to Ryan Kelly [:rfkelly] from comment #26)
> We could potentially ship choose-what-to-sync on the web for Fennec without
> solving the Desktop piece represented in this bug.  In particular, since
> *all* fennec web logins would support the web-based chooser, we don't have
> to solve the capabilities questions raised above.

OTOH though, if the only blocker for desktop is that capabilities support, I think we should "just do it" - something similar to the earlier proposal looks simple enough and I'm fine with it.

Comment 29

[Shane Tomlinson [:stomlinson]]

(In reply to Vlad Filippov :vladikoff from comment #27)
> > We could potentially ship choose-what-to-sync on the web for Fennec without solving the Desktop piece represented in this bug. 
> 
> +1
> 
> > It would be convenient for clients if the 'login' message that fxa-content-server provides includes an "enginesToSync: {'bookmarks': true, 'history': false}" entry, or "enginesToSync: ['bookmarks']", or similar.  Have you already set a format for this addition? 
> 
> The format uses the "declined engine", that is what Firefox Desktop uses.
> Please see
> https://github.com/mozilla/fxa-content-server/pull/3121/files#diff-
> c1a8b541d222ace921c5e34f1f6850e2R63 for more information. Let us know in
> that PR if that format is good or bad idea for Fennec.
> 
> Ref for Desktop code:
> http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/service.
> js#1333


I admit that I find the "declined engines" paradigm confusing, and wonder it's easier to understand if FxA sends an "accepted engines" data package which the browser then uses to determine which engines to disable. I generally prefer opt-in to opt-out, but I was not around for the discussion for why "declined" was used instead of "accepted". Can somebody point me in the right direction?

Comment 30

[Nick Alexander :nalexander [he/him]]

(In reply to Shane Tomlinson [:stomlinson] from comment #29)
> (In reply to Vlad Filippov :vladikoff from comment #27)
> > > We could potentially ship choose-what-to-sync on the web for Fennec without solving the Desktop piece represented in this bug. 
> > 
> > +1
> > 
> > > It would be convenient for clients if the 'login' message that fxa-content-server provides includes an "enginesToSync: {'bookmarks': true, 'history': false}" entry, or "enginesToSync: ['bookmarks']", or similar.  Have you already set a format for this addition? 
> > 
> > The format uses the "declined engine", that is what Firefox Desktop uses.
> > Please see
> > https://github.com/mozilla/fxa-content-server/pull/3121/files#diff-
> > c1a8b541d222ace921c5e34f1f6850e2R63 for more information. Let us know in
> > that PR if that format is good or bad idea for Fennec.
> > 
> > Ref for Desktop code:
> > http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/service.
> > js#1333
> 
> 
> I admit that I find the "declined engines" paradigm confusing, and wonder
> it's easier to understand if FxA sends an "accepted engines" data package
> which the browser then uses to determine which engines to disable. I
> generally prefer opt-in to opt-out, but I was not around for the discussion
> for why "declined" was used instead of "accepted". Can somebody point me in
> the right direction?

The Sync storage server can store collections that no current client knows about -- for example, a new add-on can take a new collection name and start using it.  The protocol on top of the storage server uses meta/global to co-ordinate such additional collections.  If we include the "accepted" list, then we have a closed universe of available collections.  If we include the "declined" list, we have an open universe.  The latter is better for unexpected additional collections.

In the future, I hope to stop supporting unknown collections entirely; and to stop using (a clinet managed) meta/global entirely.  At that point, we have fewer complications.

Comment 31

[Vlad Filippov (:vladikoff)]

Fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1218022

Comment 32

[Ryan Kelly [:rfkelly]]

It's not obvious what work remains in this bug, if any - :vladikoff can you please triage or update as appropriate?

Comment 33

[Vlad Filippov (:vladikoff)]

This is done, the pending bug is to enable this for the firstrun flow, tracked here: https://github.com/mozilla/fxa-content-server/issues/3365

Let me know if we need a bugzilla bug for that or not.