1218022 - Enable context=fx_desktop_v2 WebChannel communication for about:accounts

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Comment 1

[Vlad Filippov (:vladikoff)]

Blocks: https://bugzilla.mozilla.org/show_bug.cgi?id=1183917
Depends on: https://github.com/mozilla/fxa-content-server/pull/3214

Comment 2

[Vlad Filippov (:vladikoff)]

The goal of this effort is to use WebChannels (that use already for the First Run flow) in about:accounts.
In addition the browser should expect to get a list of declinedEngines as part of the "Choose what to sync" flow on the web with fx_desktop_v2.

The patch for this will also involve changing the default login and registration uris to fx_desktop_v2:

```
line 1478 -- pref("identity.fxaccounts.remote.signup.uri", "https://accounts.firefox.com/signup?service=sync&context=fx_desktop_v1");
line 1482 -- pref("identity.fxaccounts.remote.force_auth.uri", "https://accounts.firefox.com/force_auth?service=sync&context=fx_desktop_v1");
line 1485 -- pref("identity.fxaccounts.remote.signin.uri", "https://accounts.firefox.com/signin?service=sync&context=fx_desktop_v1");
```

Mark, fx_desktop_v2 will also indicate that Desktop supports "Choose What to Sync". Do you have any concerns about this?

Comment 3

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

(In reply to Vlad Filippov :vladikoff from comment #2)
> Mark, fx_desktop_v2 will also indicate that Desktop supports "Choose What to
> Sync". Do you have any concerns about this?

SGTM!

Comment 4

[Vlad Filippov (:vladikoff)]

Attachment: 0554-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

Comment 5

[Vlad Filippov (:vladikoff)]

I'm thinking we will need a test for the declinedEngines logic in `browserid_identity`.
Anything else?

When testing locally (against LOCAL + LATEST) with and without this change I always get "Please sign in to reconnect [email]" in Firefox (Nightly + Stable). Need to figure out what's going on there ... :\

Comment 6

[Vlad Filippov (:vladikoff)]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3d13bd212d0 for https://bugzilla.mozilla.org/attachment.cgi?id=8683832&action=diff

Comment 7

[Ryan Kelly [:rfkelly][PTO April 2nd - May 16]]

Comment on attachment 8683832 [details] [diff] [review]
0554-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

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

I had a couple of possibly-naive questions but overall I'm pleased to see the small size of this diff!

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +217,5 @@
>     * @param accountData the user's account data and credentials
>     */
>    login(accountData) {
>      if (accountData.customizeSync) {
>        this.setShowCustomizeSyncPref(true);

Does this "show the customize dialog" bit get ignored under the desktop_v2, and if so, should we avoid setting it at all in order to keep the code clear?

::: services/sync/modules/browserid_identity.js
@@ +47,5 @@
>    fxAccountsCommon.ONLOGOUT_NOTIFICATION,
>  ];
>  
>  const PREF_SYNC_SHOW_CUSTOMIZATION = "services.sync-setup.ui.showCustomizationDialog";
> +const PREF_SYNC_DECLINED_ENGINES = "services.sync-setup.ui.declinedSyncEngines";

It seems weird to me that these consts and duplicated in multiple files, but I guess it's inline with existing practice so *shrug*

@@ +254,5 @@
> +              Weave.Service.engineManager.declineDisabled();
> +            } else {
> +              // Log out if the user canceled the dialog.
> +              return this._fxaService.signOut();
> +            }

Is there a chance that this code will still be used with fx_desktop_v1 context, e.g. in existing installs for which we can't override the signup URL pref?

Comment 8

[Vlad Filippov (:vladikoff)]

Comment on attachment 8683832 [details] [diff] [review]
0554-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

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

::: services/sync/modules/browserid_identity.js
@@ +254,5 @@
> +              Weave.Service.engineManager.declineDisabled();
> +            } else {
> +              // Log out if the user canceled the dialog.
> +              return this._fxaService.signOut();
> +            }

Yes, this code path *will* be used in older clients. Do you see a problem with this flow?

Comment 9

[Vlad Filippov (:vladikoff)]

(In reply to Ryan Kelly [:rfkelly] from comment #7)
> Comment on attachment 8683832 [details] [diff] [review]
> 0554-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch
> 
> Review of attachment 8683832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I had a couple of possibly-naive questions but overall I'm pleased to see
> the small size of this diff!
> 
> ::: services/fxaccounts/FxAccountsWebChannel.jsm
> @@ +217,5 @@
> >     * @param accountData the user's account data and credentials
> >     */
> >    login(accountData) {
> >      if (accountData.customizeSync) {
> >        this.setShowCustomizeSyncPref(true);
> 
> Does this "show the customize dialog" bit get ignored under the desktop_v2,
> and if so, should we avoid setting it at all in order to keep the code clear?
> 
> ::: services/sync/modules/browserid_identity.js
> @@ +47,5 @@
> >    fxAccountsCommon.ONLOGOUT_NOTIFICATION,
> >  ];
> >  
> >  const PREF_SYNC_SHOW_CUSTOMIZATION = "services.sync-setup.ui.showCustomizationDialog";
> > +const PREF_SYNC_DECLINED_ENGINES = "services.sync-setup.ui.declinedSyncEngines";
> 
> It seems weird to me that these consts and duplicated in multiple files, but
> I guess it's inline with existing practice so *shrug*
> 
> @@ +254,5 @@
> > +              Weave.Service.engineManager.declineDisabled();
> > +            } else {
> > +              // Log out if the user canceled the dialog.
> > +              return this._fxaService.signOut();
> > +            }
> 
> Is there a chance that this code will still be used with fx_desktop_v1
> context, e.g. in existing installs for which we can't override the signup
> URL pref?

Tried to reply inline but that doesn't seem to work, see https://bugzilla.mozilla.org/show_bug.cgi?id=1218022#c8

Comment 10

[Ryan Kelly [:rfkelly][PTO April 2nd - May 16]]

> Do you see a problem with this flow?

No, I just like to make sure we're not missing out on opportunities to delete code :-)

Comment 11

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

Comment on attachment 8683832 [details] [diff] [review]
0554-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

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

Does the client still need to support _v1 at all? ie, can't we nuke the login handling code from about:accounts at the same time as we flip the pref (or very soon after to handle a window between the client being updated and the server deploy that leverages it)?

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +30,5 @@
>  const COMMAND_DELETE               = "fxaccounts:delete";
>  
>  const PREF_LAST_FXA_USER           = "identity.fxaccounts.lastSignedInUserHash";
>  const PREF_SYNC_SHOW_CUSTOMIZATION = "services.sync-setup.ui.showCustomizationDialog";
> +const PREF_SYNC_DECLINED_ENGINES   = "services.sync-setup.ui.declinedSyncEngines";

As Ryan implied I'd be inclined to move all these prefs to FxAccountsCommon - it seems everywhere they are duplicated already import FxAccountsCommon

@@ +219,5 @@
>    login(accountData) {
>      if (accountData.customizeSync) {
>        this.setShowCustomizeSyncPref(true);
> +      // if we received a list of engines that we need
> +      if (accountData.declinedSyncEngines) {

Again, like Ryan said :) I don't see a need to check for customizeSync here at all (so the server could not even send it) and there's no need to set that pref. There's no b/w compat issue here as we've never handled that message on desktop before.

Even for the declined engines, it is probably even possible to move the logs for the pref setting from browserid_identity to here (or possibly to a helper in services/sync which is called from here). The only reason browserid_identity does it that way was because it explicitly shows a UI and we only wanted that UI to appear after verification. But because this new-world doesn't require UI, I can't see any reason not to just set those prefs immediately (ie, possibly before verification) - if the user disconnects those prefs get reset, and they will not be acted on until verification anyway. That will help remove the "split" between this code and browserid_identity. Assuming the customize pref isn't set, bid_identity should then get away with no changes and these declined engines never get set to a transient pref.

::: services/sync/modules/browserid_identity.js
@@ +112,5 @@
>        return false;
>      }
>    },
>  
> +  get declinedSyncEngines() {

I think we should rename this (or even just inline it) - it may give the casual reader the impression that it should return the currently declined engines, but in reality it returns a transient value used to set the actual list of declined engines.

(But see the other comments - I can't see why we don't handle everything in the channel module - possibly with a new helper in services/sync that both this module and the channel module can call so the internal pref names used by sync remains abstracted away)

@@ +259,1 @@
>            }

I'm not quite clear on "this will be used in older clients"? Older clients will be using the older version of this code. IIUC, this code would only be hit by new clients that somehow still have fx_desktop_v1 as the pref value?

Comment 12

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

I wrote:

> it is probably even possible to move the logs for the pref

I think I meant to write "logic" instead of "logs" :)

Comment 13

[Vlad Filippov (:vladikoff)]

(In reply to Mark Hammond [:markh] from comment #11)
> Comment on attachment 8683832 [details] [diff] [review]
> 0554-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch
> 
> Review of attachment 8683832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does the client still need to support _v1 at all? ie, can't we nuke the
> login handling code from about:accounts at the same time as we flip the pref
> (or very soon after to handle a window between the client being updated and
> the server deploy that leverages it)?

I thought we wanted to keep it in for now in cases where:

1. We want to release a minor FF Desktop release with _v1 if there are issues with _v2?
1. Other reasons?

I'm all for removing all the old code though, I would suggest doing that as you say "very soon after to handle a window between the client being updated". 
We can file a follow up bug to keep patches simpler and easier to manage.

> 
> ::: services/fxaccounts/FxAccountsWebChannel.jsm
> @@ +30,5 @@
> >  const COMMAND_DELETE               = "fxaccounts:delete";
> >  
> >  const PREF_LAST_FXA_USER           = "identity.fxaccounts.lastSignedInUserHash";
> >  const PREF_SYNC_SHOW_CUSTOMIZATION = "services.sync-setup.ui.showCustomizationDialog";
> > +const PREF_SYNC_DECLINED_ENGINES   = "services.sync-setup.ui.declinedSyncEngines";
> 
> As Ryan implied I'd be inclined to move all these prefs to FxAccountsCommon
> - it seems everywhere they are duplicated already import FxAccountsCommon

Will do that, thanks!

> 
> @@ +219,5 @@
> >    login(accountData) {
> >      if (accountData.customizeSync) {
> >        this.setShowCustomizeSyncPref(true);
> > +      // if we received a list of engines that we need
> > +      if (accountData.declinedSyncEngines) {
> 
> Again, like Ryan said :) I don't see a need to check for customizeSync here
> at all (so the server could not even send it) and there's no need to set
> that pref. There's no b/w compat issue here as we've never handled that
> message on desktop before.

Okay will remove it, thanks! I think Firstrun flow already uses this login message, so when you say "as we've never handled that message on desktop before" it confuses me.

> 
> Even for the declined engines, it is probably even possible to move the logs
> for the pref setting from browserid_identity to here (or possibly to a
> helper in services/sync which is called from here). The only reason
> browserid_identity does it that way was because it explicitly shows a UI and
> we only wanted that UI to appear after verification. But because this
> new-world doesn't require UI, I can't see any reason not to just set those
> prefs immediately (ie, possibly before verification) - if the user
> disconnects those prefs get reset, and they will not be acted on until
> verification anyway. That will help remove the "split" between this code and
> browserid_identity. Assuming the customize pref isn't set, bid_identity
> should then get away with no changes and these declined engines never get
> set to a transient pref.
> 
> ::: services/sync/modules/browserid_identity.js
> @@ +112,5 @@
> >        return false;
> >      }
> >    },
> >  
> > +  get declinedSyncEngines() {
> 
> I think we should rename this (or even just inline it) - it may give the
> casual reader the impression that it should return the currently declined
> engines, but in reality it returns a transient value used to set the actual
> list of declined engines.
> 
> (But see the other comments - I can't see why we don't handle everything in
> the channel module - possibly with a new helper in services/sync that both
> this module and the channel module can call so the internal pref names used
> by sync remains abstracted away)
> 
> @@ +259,1 @@
> >            }
> 
> I'm not quite clear on "this will be used in older clients"? Older clients
> will be using the older version of this code. IIUC, this code would only be
> hit by new clients that somehow still have fx_desktop_v1 as the pref value?

"code would only be hit by new clients that somehow still have fx_desktop_v1 as the pref value?"

Exactly. Self-hosters, https://accounts.firefox.com.cn/signup, other cases where the Content Server is not updated and Firefox Desktop is updated and has the pref forced to "fx_desktop_v1" will not have a code path if we remove it. Thoughts, am I missing something?

Comment 14

[Ryan Kelly [:rfkelly][PTO April 2nd - May 16]]

> other cases where the Content Server is not updated and Firefox Desktop is
> updated and has the pref forced to "fx_desktop_v1"

This is a good point to talk about explicitly - what will happen if Firefox tries to use context=fx_desktop_v2 against an old version of the content-server that doesn't recognize that context?  Will it error out cleanly?

Comment 15

[Shane Tomlinson [:stomlinson]]

(In reply to Ryan Kelly [:rfkelly] from comment #14)
> > other cases where the Content Server is not updated and Firefox Desktop is
> > updated and has the pref forced to "fx_desktop_v1"
> 
> This is a good point to talk about explicitly - what will happen if Firefox
> tries to use context=fx_desktop_v2 against an old version of the
> content-server that doesn't recognize that context?  Will it error out
> cleanly?

Are you thinking of Mozilla China in particular? 

The content server will fall back to the implicit "no context specified" context, which acts like a user who directly accessed the site by typing "accounts.firefox.com" into the URL bar.

Comment 16

[Ryan Kelly [:rfkelly][PTO April 2nd - May 16]]

> Are you thinking of Mozilla China in particular? 

Yes, although they're pretty responsive about updates if we give them a heads-up on breaking changes.  They're *probably* OK because the change also changes the URLs in about:config, so they'll probably keep the old context param until they explicitly update it.

Comment 17

[Vlad Filippov (:vladikoff)]

(In reply to Shane Tomlinson [:stomlinson] from comment #15)
> (In reply to Ryan Kelly [:rfkelly] from comment #14)
> > > other cases where the Content Server is not updated and Firefox Desktop is
> > > updated and has the pref forced to "fx_desktop_v1"
> > 
> > This is a good point to talk about explicitly - what will happen if Firefox
> > tries to use context=fx_desktop_v2 against an old version of the
> > content-server that doesn't recognize that context?  Will it error out
> > cleanly?
> 
> Are you thinking of Mozilla China in particular? 
> 
> The content server will fall back to the implicit "no context specified"
> context, which acts like a user who directly accessed the site by typing
> "accounts.firefox.com" into the URL bar.

(In reply to Shane Tomlinson [:stomlinson] from comment #15)
> (In reply to Ryan Kelly [:rfkelly] from comment #14)
> > > other cases where the Content Server is not updated and Firefox Desktop is
> > > updated and has the pref forced to "fx_desktop_v1"
> > 
> > This is a good point to talk about explicitly - what will happen if Firefox
> > tries to use context=fx_desktop_v2 against an old version of the
> > content-server that doesn't recognize that context?  Will it error out
> > cleanly?
> 
> Are you thinking of Mozilla China in particular? 
> 
> The content server will fall back to the implicit "no context specified"
> context, which acts like a user who directly accessed the site by typing
> "accounts.firefox.com" into the URL bar.

Train 41.1 (train before merging context_v2) fell back to context_v1 for me and worked. 

screenshot: http://v14d.com/i/5643b23ead8c1.jpg

Comment 18

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

(In reply to Vlad Filippov :vladikoff from comment #13)
> Okay will remove it, thanks! I think Firstrun flow already uses this login
> message, so when you say "as we've never handled that message on desktop
> before" it confuses me.

Sorry for the confusion - I meant we haven't handled a .declinedEngines property, so having the check for that within the check for .customizeSync shouldn't be necessary.

> Exactly. Self-hosters, https://accounts.firefox.com.cn/signup, other cases
> where the Content Server is not updated and Firefox Desktop is updated and
> has the pref forced to "fx_desktop_v1" will not have a code path if we
> remove it. Thoughts, am I missing something?

That makes sense.

Comment 19

[Vlad Filippov (:vladikoff)]

Attachment: 2520-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

Comment 20

[Vlad Filippov (:vladikoff)]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4dd6983f02b

Comment 21

[Ryan Kelly [:rfkelly][PTO April 2nd - May 16]]

Comment on attachment 8689931 [details] [diff] [review]
2520-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

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

This looks OK to me, but of course I don't have real r+ power over here, so I switched to to an f+ :-)

Was there something device-registration-related that the v2 protocol will also opt you in to, and do we need to account for it in this bug?

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +223,2 @@
>        delete accountData.customizeSync;
>      }

I was naively expecting to get *either* an old-style `customizeSync` flag or a new-style `declinedSyncEngines` object, but not both.  Having both seems OK, but we'll need to note the protocol on the fxa-content-server side, so that future maintainers don't accidentally refactor away the `customizeSync` because we already showed that UI ourselves.

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

Will this leave an invalid value in the declinedSyncEngines variable?  ISTM we may need to error out, or explicitly clear it somehow.

@@ +232,5 @@
> +            this._log.info(JSON.stringify(declinedSyncEngines));
> +            Weave.Service.engineManager.setDeclined(declinedSyncEngines);
> +            declinedSyncEngines.forEach(engine => {
> +              Svc.Prefs.set("engine." + engine, false);
> +            });

It seems weird to me that we have to set these prefs in addition to calling setDeclined; can it be folded into setDeclined itself?

Comment 22

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

Comment on attachment 8689931 [details] [diff] [review]
2520-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

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

Looks OK, but I'd like to see if we can avoid that new pref, or understand why we can't.

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +223,2 @@
>        delete accountData.customizeSync;
>      }

IIRC, in the last feedback round I suggested not checking for .customizeSync and to just decline the engines directly here - there should be no need to wait for browserid_identity to find the user verified. That should allow us to avoid the new pref completely.

::: services/fxaccounts/tests/xpcshell/test_web_channel.js
@@ +249,5 @@
> +
> +        // customizeSync should be stripped in the data.
> +        do_check_false('customizeSync' in accountData);
> +        do_check_false('declinedSyncEngines' in accountData);
> +        //do_check_eq(Services.prefs.getCharPref("services.sync-setup.ui.declinedSyncEngines"), '["addons", "prefs"]');

this commented line doesn't look right

Comment 23

[Vlad Filippov (:vladikoff)]

> Was there something device-registration-related that the v2 protocol will also opt you in to, and do we need to account for it in this bug?

We should keep an eye on that yeap. However from what I've seen in others patches we are still working in different files for these 2 features.

**One important** thing to mention again the `customizeSync` is staying for several reasons:
- Older Clients (as we talked before)
- Slow roll out of choose what to sync, First run will still use the old flow while we test this new flow: see https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/models/auth_brokers/first-run.js#L29
- In some cases clients, such as iOS (or FXOS?) were not capable of choosing data types, so we would disable customization. 

> Having both seems OK, but we'll need to note the protocol on the fxa-content-server side, so that future maintainers don't accidentally refactor away the `customizeSync` because we already showed that UI ourselves.

We can certainly do the "keep both" option for now and go from there. I will update the PR...

Comment 24

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

(In reply to Vlad Filippov :vladikoff from comment #23)
> **One important** thing to mention again the `customizeSync` is staying for
> several reasons:

I'm happy to treat that as a server-side implementation detail - from the client's POV, I think they are mutually exclusive. customizeSync should only be acted on by about:accounts, declinedEngines by the channel. customizeSync is effectively deprecated, so browserid_identity can grow a comment indicating that, plus mentioning the implementation is that way due to the requirement of showing modal UI after verification and a pointer to the webchannel code for the new way. That code can then die when about:accounts dies - presumably some point in the future where we say "screw you" to old servers (and after our own login process doesn't still rely on it - v3? ;)

Comment 25

[Vlad Filippov (:vladikoff)]

With latest rebase I keep getting this error on a fresh profile / new sync user:
```
INFO: 1448386474585	FirefoxAccounts	DEBUG	setSignedInUser - aborting any existing flows
INFO: 1448386474585	FirefoxAccounts	INFO	An accountState promise was resolved, but was actually rejected due to a different user being signed in. Originally resolved with: null
INFO: 1448386474586	FirefoxAccounts	INFO	An accountState promise was resolved, but was actually rejected due to a different user being signed in. Originally resolved with: null
INFO: 1448386474586	FirefoxAccounts	INFO	An accountState promise was resolved, but was actually rejected due to a different user being signed in. Originally resolved with: null
INFO: 1448386474586	FirefoxAccounts	ERROR	Error updating FxA account info: Error: A different user signed in (resource://gre/modules/FxAccounts.jsm:157:29) JS Stack trace: [e-mail]:157:29 < getUserAccountData/<@FxAccounts.jsm:141:14 < [e-mail]:934:23 < [e-mail]:813:7 < Promise*[e-mail]:744:11 < [e-mail]:776:7 < [e-mail]:711:7 < [e-mail]:94:7 < [e-mail]:93:5 < [e-mail]:409:7
INFO: 1448386474587	browserwindow.syncui	ERROR	updateUI failed: Error: A different user signed in (resource://gre/modules/FxAccounts.jsm:157:29) JS Stack trace: [e-mail]:157:29 < getUserAccountData/<@FxAccounts.jsm:141:14 < [e-mail]:934:23 < [e-mail]:813:7 < Promise*[e-mail]:744:11 < [e-mail]:776:7 < [e-mail]:711:7 < [e-mail]:94:7 < [e-mail]:93:5 < [e-mail]:409:7
INFO: 1448386474587	browserwindow.syncui	ERROR	updateUI failed: Error: A different user signed in (resource://gre/modules/FxAccounts.jsm:157:29) JS Stack trace: [e-mail]:157:29 < getUserAccountData/<@FxAccounts.jsm:141:14 < [e-mail]:934:23 < [e-mail]:813:7 < Promise*[e-mail]:744:11 < [e-mail]:776:7 < [e-mail]:711:7 < [e-mail]:94:7 < [e-mail]:93:5 < [e-mail]:409:7
```

blocked on this for now, not sure what is causing this exactly. Full log: https://gist.github.com/vladikoff/e85c9a78321b75a94555

Comment 26

[Vlad Filippov (:vladikoff)]

Never mind I resolved that issue..

Comment 27

[Vlad Filippov (:vladikoff)]

Attachment: 0001-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

Updated, patch is a lot smaller now. hopefully addresses all the issues. Thanks again for all the feedback.

This patch should work out of the box with our prod FxA environment, no need to change anything in config.

Comment 28

[Vlad Filippov (:vladikoff)]

Try build started: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ffb615a0d35

Comment 29

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

Comment on attachment 8691519 [details] [diff] [review]
0001-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

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

Awesome, LGTM

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +217,5 @@
>     *
>     * @param accountData the user's account data and credentials
>     */
>    login(accountData) {
>      if (accountData.customizeSync) {

I'm actually still confused why this block is here at all - IIUC, desktop will never hit it - but maybe FxOS does? Either way, it's fine to leave it.

@@ +224,5 @@
>      }
>  
> +    if (accountData.declinedSyncEngines) {
> +      let declinedSyncEngines = accountData.declinedSyncEngines;
> +      log.debug("Received declined engines");

change these 2 lines to |log.debug("Received declined engines", declinedSyncEngines);| and we automagically get the stringify done by the logging module.

Comment 30

[Ryan Kelly [:rfkelly][PTO April 2nd - May 16]]

Comment on attachment 8691519 [details] [diff] [review]
0001-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

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

LGTM!

Comment 31

[Vlad Filippov (:vladikoff)]

Attachment: 0001-Bug-1218022-switch-to-fx_desktop_v2-context-for-Fire.patch

Comment 32

[Pulsebot]

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

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8233c8116679