Closed Bug 1374501 Opened 7 years ago Closed 7 years ago

Change fxa webchannel handshake and enable sync and credit-card engines if necessary

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: markh, Assigned: eoger)

References

()

Details

Attachments

(1 file)

The FxA signup step will start to offer addresses (and at some stage credit-cards) to new sync accounts on desktop. In bug 1374500 we are adding these new engines, which will be disabled by default. This bug is to detect when those engines were offered but not declined and to enable the relevant engines.

The FxA issue to support this is at https://github.com/mozilla/fxa-content-server/issues/5087
WIP content-server code PR at https://github.com/mozilla/fxa-content-server/pull/5158.

:markh, :eoger - I'm copying Mark's comments over from the content server issue that I used as a basis for the implementation in PR 5158.

> So as a straw-man: desktop asserts it supports the "addresses" and "creditcards" capabilities - all 
> other engines remain implied.
> When FxA sends the "login" message, it should also list the engines it offered.

> When FxA sends a "fxaccounts:fxa_status" message, desktop currently replies with 
> {signedInUser: {email: "...", ...}. We extend that so it also includes a top-level "capabilities" 
> object, which looks something like, say, {capabilities: {engines: ["creditcards", "addresses"]}}. 
> As mentioned above, other engines remain implied.

> If the client supplied capabilities["engines"], the "fxaccounts:login" message sent back to the client 
> will include, say "enginesOffered" - in this case it would just return exactly what was passed in 
> capabilities. This is the signal to desktop that the engines were definitely offered, so it can be 
> assumed that the user left them selected if they don't appear in declined. 
> All other engines remain implied here too.


> I was wrong about the fact we can hard-code everything - the entire CC feature has some risk of not making 56. Addresses seems fine - can we maybe hard-code that to get the ball rolling and handle CC later?

I expect the response to fxaccounts:fxa_status to be of the form:

>  capabilities: {
>    engines: ['creditcards', 'addresses']
>  },
>  signedInUser: null


I'll send back an `enginesOffered` array in the `fxaccounts:login` message, of the same format as `engines`.
> I'll send back an `enginesOffered` array in the `fxaccounts:login` message, of the same format as `engines`.

:mhammond, :eoger - we already send a `declinedSyncEngines` field. Instead of sending `enginesOffered`, I'd like
to send `displayedSyncEngines`, the name is a bit clearer, and it's consistent with what already exists. Does that work for you?
Flags: needinfo?(markh)
Flags: needinfo?(eoger)
I thought "offered" was a nice match for the existing "declined", but I'm not bothered by the spelling :)
Flags: needinfo?(markh)
I'd also be fine with "accepted" (which would be a semantic change - ie, accepted + declined would be the full set) - but whatever...
Thanks markh,

offeredSyncEngines WFM, my primary concern is to re-use the `SyncEngines` suffix. 

Would it be painful if I sent the full set of engines that are offered instead of only the subset that is conditional, e.g.:
> offeredSyncEngines: ['addons', 'tabs', 'bookmarks', 'passwords', 'history', 'prefs', 'addresses']
Flags: needinfo?(markh)
Flags: needinfo?(eoger)
> Would it be painful if I sent the full set of engines that are offered instead of only the subset that is conditional

As far as my implementation goes, it doesn't matter.
Comment on attachment 8880055 [details]
Bug 1374501 - Allow extra sync engines to be activited using FxA Web Channels.

https://reviewboard.mozilla.org/r/151380/#review156508

Looks great! Please ensure Shane signs off on how you are communicating those "capabilities" (I think it's what we agreed in the bug, but an explicit OK from him is worthwhile)

::: services/fxaccounts/FxAccountsWebChannel.jsm:41
(Diff revision 1)
>  const COMMAND_CHANGE_PASSWORD      = "fxaccounts:change_password";
>  const COMMAND_FXA_STATUS           = "fxaccounts:fxa_status";
>  
>  const PREF_LAST_FXA_USER           = "identity.fxaccounts.lastSignedInUserHash";
>  
> +const EXTRA_ENGINES = ["addresses", "creditcards"];

I think a brief comment for what's special about these engines would be nice.
Attachment #8880055 - Flags: review?(markh) → review+
(In reply to Shane Tomlinson [:stomlinson] from comment #5)
> Would it be painful if I sent the full set of engines that are offered
> instead of only the subset that is conditional, e.g.:
> > offeredSyncEngines: ['addons', 'tabs', 'bookmarks', 'passwords', 'history', 'prefs', 'addresses']

Sounds great!
Flags: needinfo?(markh)
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8880055 [details]
Bug 1374501 - Allow extra sync engines to be activited using FxA Web Channels.

https://reviewboard.mozilla.org/r/151380/#review156670

r+ Works great for me when testing locally! If I set services.sync.engine.creditcards.available to true, the `creditcards` engine is displayed in CWTS.
Attachment #8880055 - Flags: review+
Thank, should I land this or wait a little bit?
I think we might as well wait a week or so, unless Shane would like it committed (for testing etc reasons), in which case I'd be fine with it landing now.
(In reply to Mark Hammond [:markh] from comment #13)
> I think we might as well wait a week or so, unless Shane would like it
> committed (for testing etc reasons), in which case I'd be fine with it
> landing now.

I've already applied the patch locally, so no rush.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a0365361a9f
Allow extra sync engines to be activited using FxA Web Channels. r=markh,stomlinson
https://hg.mozilla.org/mozilla-central/rev/1a0365361a9f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.