957460 - Firefox Accounts widget for customization panel

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

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

Attachment: Mockup of when no user is logged in

Current fxa/sync mockups for desktop call for a "Firefox Accounts" widget in the customization panel.  I'm attaching mockups of what this might look like both when the no user is logged in and when one is.

In the first instance, I believe that in both cases, clicking this would open about:accounts (the main UI entry-point for Firefox Accounts), from where they could either login, logout, setup sync, and in the future, manage other services which use Firefox Accounts.

Comment 1

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

Attachment: Mockup of when a user is logged in

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch

We're still missing an icon for the button but other than that it should be functional.

When the user is logged into their Firefox Account, we show the button that redirects to the sync prefpane.

When the user isn't logged in and has no sync set up, we show a button that will open about:accounts when clicked.

When the user isn't logged in and has sync set up it must be an old sync user and we don't show any new buttons.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8358557 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch

Adding an Australis team member to the feedback list as I'm touching the PanelUI.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8358557 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch

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

Hrm. Is this the way this has been designed? Now that there are 10-11 buttons in by default, adding another row to the footer makes the menu very big, which is even worse on Linux... 

Other than that, have you thought about how this should look in Customize Mode (maybe something like "Firefox Account Status" or whatever as a label), and have you ensured it doesn't do anything when in customize mode? (It doesn't look like it.)

(clearing feedback for now because too many questions)

::: browser/base/content/browser-fxaccounts.js
@@ +33,5 @@
> +        signedoff.hidden = true;
> +
> +        // Update visible user name.
> +        let user = document.getElementById("PanelUI-signedin-user");
> +        user.setAttribute("value", data.email);

This is going to be quite wide, depending on the address. I get that the crop will 'solve' this, but are we working towards having a nicer identifier here, that is less likely to be cropped (first name or nickname or something)? Maybe we should also provide a tooltip text that shows the full email address.

@@ +50,5 @@
> +  },
> +
> +  signIn: function () {
> +    switchToTabHavingURI("about:accounts", true);
> +    PanelUI.hide();

Why are you calling this manually? That shouldn't be necessary, IIRC.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +14,5 @@
>          <vbox id="PanelUI-contents"/>
>        </vbox>
>  
>        <footer id="PanelUI-footer">
> +        <toolbarbutton id="PanelUI-signedoff" label="Sign In to Firefox"

This should be localizable.

@@ +18,5 @@
> +        <toolbarbutton id="PanelUI-signedoff" label="Sign In to Firefox"
> +                       tabindex="0" observes="fxa-signedoff-state"
> +                       oncommand="gFxAccounts.signIn();"/>
> +
> +        <button id="PanelUI-signedin" tabindex="0" observes="fxa-signedin-state"

Why is this a button and not a toolbarbutton?

@@ +22,5 @@
> +        <button id="PanelUI-signedin" tabindex="0" observes="fxa-signedin-state"
> +                oncommand="gFxAccounts.openPrefPane();" flex="1">
> +          <image class="toolbarbutton-icon"/>
> +          <label id="PanelUI-signedin-label" class="toolbarbutton-text"
> +                 value="Signed In as "/>

This should be localizable, and should indicate what it's a label for, and really, it'd be better to have it set programmatically (from a .properties rather than a .dtd file) to include the user in a single label.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +210,5 @@
>  #PanelUI-customize,
>  #PanelUI-quit {
>    margin: -1px 0 0;
>    padding: 10px 0;
> +  min-height: 4em;

This is probably not the right way to do this; have you checked what this looks like on Linux (where the default font size is close to 15px) and how big that makes the rows?

@@ +223,5 @@
>  }
>  
> +#PanelUI-signedin,
> +#PanelUI-signedoff {
> +  width: calc(@menuPanelWidth@ + 20px);

This doesn't make sense to me; how is this equal to the menu panel width, and then some?

Comment 5

[u428464]

Possibly of interest : 
http://people.mozilla.org/~shorlander/drops/Australis-i02-DesignSpec-%28Win8%29-SignIn-01-Sheet.jpg
http://people.mozilla.org/~shorlander/drops/Australis-i02-DesignSpec-%28Win8%29-SignIn-02-Sheet.jpg
http://people.mozilla.org/~shorlander/drops/Australis-i02-DesignSpec-%28Win8%29-SignIn-03-Sheet.jpg

Comment 6

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

Comment on attachment 8358557 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch

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

Notwithstanding Gij's comments, this looks reasonable to me - a number of these bugs are going to need to agree on some of these states, but it's a little too early to decide exactly what that looks like.

::: browser/base/content/browser-fxaccounts.js
@@ +25,5 @@
> +  updateUI: function () {
> +    let signedin = document.getElementById("fxa-signedin-state");
> +    let signedoff = document.getElementById("fxa-signedoff-state");
> +
> +    fxAccounts.getSignedInUser().then(data => {

It seems we should hide both elements before we call getSignedInUser(), to ensure that both elements are hidden before the promise resolves - while they default to hidden, I guess we should still reset them incase they logout after we've first initialized things).

Later, we might also need something special to happen if they are logged in but haven't yet verified their email (as sync isn't going to work in that case).

FYI, it's likely that there will be a preference which tells sync whether to use Fxa or a legacy provider which will be used by the sync options preference panel - it's probably not guaranteed that just because someone is logged into Fxa means Sync must be using Fxa (but that will depend on when we can actually remove the "legacy" sync provider and what other Fxa-based features might appear in the meantime).  I wonder if we should create a .jsm which we can use to determine some of these states, and which can be used by both this browser code and by the sync preferences panel?

@@ +42,5 @@
> +        signedoff.hidden = false;
> +      } else {
> +        // The user has the old sync set up. Don't bother them with Firefox
> +        // accounts for now.
> +        signedin.hidden = false;

wouldn't they both be .hidden = true in this case?  If so, then as above, we could consider hiding them before calling getSignedInUser, so this block could be removed (but a comment would remain)

Comment 7

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #6)
> @@ +42,5 @@
> > +        signedoff.hidden = false;
> > +      } else {
> > +        // The user has the old sync set up. Don't bother them with Firefox
> > +        // accounts for now.
> > +        signedin.hidden = false;
> 
> wouldn't they both be .hidden = true in this case?  If so, then as above, we
> could consider hiding them before calling getSignedInUser, so this block
> could be removed (but a comment would remain)

Quick comment now that I see this in bugmail and so I don't forget: note that the panel contents are hidden by default, which means no XBL binding, which might mean these settings are problematic.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v2

(In reply to :Gijs Kruitbosch from comment #4)
> Hrm. Is this the way this has been designed?

Yup :)

> Other than that, have you thought about how this should look in Customize
> Mode (maybe something like "Firefox Account Status" or whatever as a label),
> and have you ensured it doesn't do anything when in customize mode? (It
> doesn't look like it.)

No, forgot that part. The button is now disabled in customize mode.

> > +        // Update visible user name.
> > +        let user = document.getElementById("PanelUI-signedin-user");
> > +        user.setAttribute("value", data.email);
> 
> This is going to be quite wide, depending on the address. I get that the
> crop will 'solve' this, but are we working towards having a nicer identifier
> here, that is less likely to be cropped (first name or nickname or
> something)? Maybe we should also provide a tooltip text that shows the full
> email address.

Added a tooltip to the button. Fixing the cropping might be something we decide to do later if it gets too ugly...

> > +  signIn: function () {
> > +    switchToTabHavingURI("about:accounts", true);
> > +    PanelUI.hide();
> 
> Why are you calling this manually? That shouldn't be necessary, IIRC.

I added this only after the panel stuck around.

> >        <footer id="PanelUI-footer">
> > +        <toolbarbutton id="PanelUI-signedoff" label="Sign In to Firefox"
> 
> This should be localizable.

Indeed, I forgot that, too. Done.

> > +        <toolbarbutton id="PanelUI-signedoff" label="Sign In to Firefox"
> > +                       tabindex="0" observes="fxa-signedoff-state"
> > +                       oncommand="gFxAccounts.signIn();"/>
> > +
> > +        <button id="PanelUI-signedin" tabindex="0" observes="fxa-signedin-state"
> 
> Why is this a button and not a toolbarbutton?

toolbarbuttons put their <children> even before the icon and the text label so I didn't see a better way than to manually rebuild a toolbarbutton to put a second label in there.
> > +        <button id="PanelUI-signedin" tabindex="0" observes="fxa-signedin-state"
> > +                oncommand="gFxAccounts.openPrefPane();" flex="1">
> > +          <image class="toolbarbutton-icon"/>
> > +          <label id="PanelUI-signedin-label" class="toolbarbutton-text"
> > +                 value="Signed In as "/>
> 
> This should be localizable, and should indicate what it's a label for, and
> really, it'd be better to have it set programmatically (from a .properties
> rather than a .dtd file) to include the user in a single label.

Localized. The second label is because the email address is bold. Or is there a way to do that in a single label?

> >  #PanelUI-customize,
> >  #PanelUI-quit {
> >    margin: -1px 0 0;
> >    padding: 10px 0;
> > +  min-height: 4em;
> 
> This is probably not the right way to do this; have you checked what this
> looks like on Linux (where the default font size is close to 15px) and how
> big that makes the rows?

Reduced to 2em including 10px top and bottom padding to account for huge font sizes on Linux and elsewhere.

> > +#PanelUI-signedin,
> > +#PanelUI-signedoff {
> > +  width: calc(@menuPanelWidth@ + 20px);
> 
> This doesn't make sense to me; how is this equal to the menu panel width,
> and then some?

It is the panel width plus padding. I needed to specify the width otherwise the label would not crop the email address.

(In reply to Mark Hammond [:markh] from comment #6)
> > +  updateUI: function () {
> > +    let signedin = document.getElementById("fxa-signedin-state");
> > +    let signedoff = document.getElementById("fxa-signedoff-state");
> > +
> > +    fxAccounts.getSignedInUser().then(data => {
> 
> It seems we should hide both elements before we call getSignedInUser(), to
> ensure that both elements are hidden before the promise resolves - while
> they default to hidden, I guess we should still reset them incase they
> logout after we've first initialized things).

Both of the elements are hidden by default and the UI is updated when logging in and out.

> Later, we might also need something special to happen if they are logged in
> but haven't yet verified their email (as sync isn't going to work in that
> case).

Could this maybe be shown in the pref dialog? Something that says "You're not verified, yet. Please check your mail or click here to request another one."?

> FYI, it's likely that there will be a preference which tells sync whether to
> use Fxa or a legacy provider which will be used by the sync options
> preference panel - it's probably not guaranteed that just because someone is
> logged into Fxa means Sync must be using Fxa (but that will depend on when
> we can actually remove the "legacy" sync provider and what other Fxa-based
> features might appear in the meantime).  I wonder if we should create a .jsm
> which we can use to determine some of these states, and which can be used by
> both this browser code and by the sync preferences panel?

Yeah, sharing that code sounds good to me.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8359834 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v2

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

This looks good to me; has any thought been put into what we do if Australis doesn't ship on 29? I guess the observer makes it possible to put the UI elsewhere for the moment if necessary?

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +19,5 @@
> +                       tabindex="0" observes="fxa-signedoff-state"
> +                       oncommand="gFxAccounts.signIn();"/>
> +
> +        <button id="PanelUI-signedin" tabindex="0" observes="fxa-signedin-state"
> +                oncommand="gFxAccounts.openPrefPane();" flex="1">

You can use a toolbarbutton, and style the additional label with -moz-box-ordinal-group: 2 (or any value that's high enough); Somewhat hacky, but I'd probably prefer it over this purely because it's simpler code and you may get some other toolbarbutton stuff for free...

At that point, you could probably even get away with a single toolbar button. Hide the email address by default and use CSS to show it in the signed-in case. Same for picking which icon to show, and the label you can update with setAttribute on the observer when the state changes.

@@ +24,5 @@
> +          <image class="toolbarbutton-icon"/>
> +          <label id="PanelUI-signedin-label" class="toolbarbutton-text"
> +                 value="&fxaSignedInAs.label;"/>
> +          <label id="PanelUI-signedin-user" class="toolbarbutton-text"
> +                 crop="end" flex="1"/>

Re: single labels: no, unless you don't care about cropping (but you do, so nevermind that)

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> the label you can
> update with setAttribute on the observer when the state changes.

on the broadcaster, obviously...

Comment 11

[:Gijs (he/him)]

Comment on attachment 8359834 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v2

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

::: browser/base/content/browser-fxaccounts.js
@@ +39,5 @@
> +
> +    fxAccounts.getSignedInUser().then(data => {
> +      if (data) {
> +        // The user is signed into their Firefox account.
> +        signedin.hidden = false;

Again, are you sure these are OK and you don't need .setAttribute? Try what happens when session-restoring customize mode in particular.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +181,5 @@
>  
>        window.gNavToolbox.addEventListener("toolbarvisibilitychange", this);
>  
> +      document.getElementById("fxa-signedin-state").setAttribute("disabled", true);
> +      document.getElementById("fxa-signedoff-state").setAttribute("disabled", true);

I missed this. This is sort of not the best place to do this; it'd be better if browser-fxaccounts.js listened for the relevant events on the toolbox. Ditto for re-enabling these.

Comment 12

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

Comment on attachment 8359834 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v2

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

Cancelling feedback based on Gij's comments and the fact my previous feedback hasn't been addressed or commented on

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mark Hammond [:markh] from comment #12)
> Cancelling feedback based on Gij's comments and the fact my previous
> feedback hasn't been addressed or commented on

Did you read my whole comment? I responded to what you said.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v3

I should have addressed all comments with this patch. The only thing still missing is the right icon but we think it's better to iterate on this and get the UI landed sooner. We wouldn't want to block on UX to deliver icons but rather get strings and elements landed and fix styling later.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8361670 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v3

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

f+ because there seem to be some issues that need resolving, like the observer/observe (does that break stuff?), the extra label and combining it with flex, and your email address. ;-)

::: browser/base/content/browser-fxaccounts.js
@@ +31,5 @@
> +
> +    this.updateUI();
> +  },
> +
> +  observer: function () {

Err, you mean observe: ?

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +22,5 @@
> +                 value="&fxaSignIn.label;" flex="1"/>
> +          <label id="PanelUI-fxa-label-signedin" class="toolbarbutton-text"
> +                 value="&fxaSignedInAs.label;"/>
> +          <label id="PanelUI-fxa-username" class="toolbarbutton-text"
> +                 crop="end" flex="1" value="[e-mail]"/>

As we discussed at some point, but sadly not in the bug, this needs another label for after the signed-in email address. In which case maybe the flex="1" on the email label should be reconsidered.

Also, should we really have your email address in here? :-)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v4

(In reply to :Gijs Kruitbosch from comment #15)
> > +  observer: function () {
> 
> Err, you mean observe: ?

Yes, sorry for not smoke testing. I ensured that signin in works now.

> As we discussed at some point, but sadly not in the bug, this needs another
> label for after the signed-in email address. In which case maybe the
> flex="1" on the email label should be reconsidered.

So I talked to jgruen and rfeeley on IRC about the fact the font size in the customize panel will increase and there isn't enough space to really show an email address. We agreed to just show the email address which makes implementing this widget a tad easier.

> Also, should we really have your email address in here? :-)

Probably not ;)

Comment 17

[:Gijs (he/him)]

Comment on attachment 8362918 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v4

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

I looked over this quickly; generally it seems fine. I'm relying on Mark to actually review the accounts bit.

Also, while you can have an r+, I would be like 100 times more comfortable with this if there were tests. Please. :-)

::: browser/base/content/browser-fxaccounts.js
@@ +51,5 @@
> +
> +  handleEvent: function (event) {
> +    let button = document.getElementById("PanelUI-fxa-status");
> +
> +    if (event.type == "customizationstarting") {

You might also want to reset the label to something more generic than the actual sign in state here.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +192,5 @@
>  }
>  
>  #PanelUI-footer {
>    display: flex;
> +  flex-direction: column;

This is new flexbox, but you set flex="1" on the toolbarbutton you added, which is old flexbox. Either of these can go, I'm pretty sure...

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v5

(In reply to :Gijs Kruitbosch from comment #17)
> Also, while you can have an r+, I would be like 100 times more comfortable
> with this if there were tests. Please. :-)

Agreed, I will think about what tests we want here.

> > +    if (event.type == "customizationstarting") {
> 
> You might also want to reset the label to something more generic than the
> actual sign in state here.

Good idea.

> >  #PanelUI-footer {
> >    display: flex;
> > +  flex-direction: column;
> 
> This is new flexbox, but you set flex="1" on the toolbarbutton you added,
> which is old flexbox. Either of these can go, I'm pretty sure...

Looks like both can.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v6

Same widget, using .isEnabled() from bug 959548.

Comment 20

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

Comment on attachment 8363864 [details] [diff] [review]
0003-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v6

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

LGTM, although I didn't look too much at the Australis-specific stuff/styling (but Gijs has, so that's fine)

::: browser/base/content/browser-fxaccounts.js
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +let gFxAccounts = {
> +
> +  OBSERVES: ["fxaccounts:onlogin", "fxaccounts:onlogout"],

Bug 952063 recently landed, which means you now want fxaccounts:onverified instead of :onlogin (or, given you just update the UI on these notifications, having all 3 probably makes more sense)  Note also that FxAccountsCommon.js now has constants for these observer names (although a case could be made to not import this for startup performance reasons - your call (but a comment with the symbol name exported by FxAccountsCommon would be good, so a mxr/grep for the symbol name still finds this code)

@@ +49,5 @@
> +    } else {
> +      button.removeAttribute("disabled");
> +    }
> +
> +    fxAccounts.isEnabled().then(([enabled, userData]) => {

as discussed, this will need to change back to getting fxAccountsEnabled and checking getSignedInUser()

@@ +64,5 @@
> +        button.setAttribute("hidden", "true");
> +      }
> +
> +      // Show the user's email address if they are logged
> +      // in we currently are not in customization mode.

nit: "in and we ..."

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v7

(In reply to Mark Hammond [:markh] from comment #20)
> LGTM, although I didn't look too much at the Australis-specific
> stuff/styling (but Gijs has, so that's fine)

Had to rebase a little to incorporate the new separators.

> Bug 952063 recently landed, which means you now want fxaccounts:onverified
> instead of :onlogin (or, given you just update the UI on these
> notifications, having all 3 probably makes more sense)  Note also that
> FxAccountsCommon.js now has constants for these observer names (although a
> case could be made to not import this for startup performance reasons - your
> call (but a comment with the symbol name exported by FxAccountsCommon would
> be good, so a mxr/grep for the symbol name still finds this code)

Updating the UI on verification doesn't make sense as there is no new information to display. Bug 957436 will add the onverified observer to show the doorhanger. Using FxAccountsCommon is a good idea, I added that with a lazy getter to load it when we need it.

> as discussed, this will need to change back to getting fxAccountsEnabled and
> checking getSignedInUser()

Done.

I also simplified .updateUI() a little due to init() bailing out early when FxA is disabled. Weave.fxAccountsEnabled doesn't change until a restart so there is no need for us to support that here, esp. if it makes the code easier to read.

Comment 22

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

Comment on attachment 8364605 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v7

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

r+ with the fix to allow fxAccountsEnabled to toggle from false to true (which doesn't look too hard to fix, so I'm happy for r+ to be carried forward once those changes are made)

::: browser/base/content/browser-fxaccounts.js
@@ +33,5 @@
> +                  .getService(Ci.nsISupports)
> +                  .wrappedJSObject;
> +
> +    // Bail out if we're initialized already or FxA isn't available.
> +    if (this._initialized || !weave.fxAccountsEnabled) {

Sadly, fxAccountsEnabled *can* change - for an existing Sync user it will be false until they do an "unlink device", when it will go true.  It will never go from true to false though, but it looks like we do need to handle that first case.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mark Hammond [:markh] from comment #22)
> > +    // Bail out if we're initialized already or FxA isn't available.
> > +    if (this._initialized || !weave.fxAccountsEnabled) {
> 
> Sadly, fxAccountsEnabled *can* change - for an existing Sync user it will be
> false until they do an "unlink device", when it will go true.  It will never
> go from true to false though, but it looks like we do need to handle that
> first case.

This doesn't seem to be how it's currently implemented though? When starting with a legacy sync account we set .fxAccountsEnabled to false and override the getter.

Comment 24

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

(In reply to Tim Taubert [:ttaubert] from comment #23)
> (In reply to Mark Hammond [:markh] from comment #22)
> > > +    // Bail out if we're initialized already or FxA isn't available.
> > > +    if (this._initialized || !weave.fxAccountsEnabled) {
> > 
> > Sadly, fxAccountsEnabled *can* change - for an existing Sync user it will be
> > false until they do an "unlink device", when it will go true.  It will never
> > go from true to false though, but it looks like we do need to handle that
> > first case.
> 
> This doesn't seem to be how it's currently implemented though? When starting
> with a legacy sync account we set .fxAccountsEnabled to false and override
> the getter.

But 959222's "part 2" patch changes this so the getter is no longer overridden and the pref is reset.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v8

(In reply to Mark Hammond [:markh] from comment #24)
> > > Sadly, fxAccountsEnabled *can* change - for an existing Sync user it will be
> > > false until they do an "unlink device", when it will go true.  It will never
> > > go from true to false though, but it looks like we do need to handle that
> > > first case.
> > 
> > This doesn't seem to be how it's currently implemented though? When starting
> > with a legacy sync account we set .fxAccountsEnabled to false and override
> > the getter.
> 
> But 959222's "part 2" patch changes this so the getter is no longer
> overridden and the pref is reset.

Fixed. I'll make the changes that choose whether to use the doorhanger or the modal dialog in bug 963384. For now it would be great to get this landed with doorhanger support, only.

Comment 26

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

Comment on attachment 8366055 [details] [diff] [review]
0001-Bug-957460-Add-Firefox-Accounts-widget-for-customiza.patch, v8

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

::: browser/base/content/browser-fxaccounts.js
@@ +12,5 @@
> +  _originalLabel: null,
> +  _inCustomizationMode: false,
> +
> +  get weave() {
> +    delete this.weave;

A bit of a nit, but "weave" probably isn't the best name here - sync's naming kinda sucks - most other sync-related code where "Weave" is used is a different object.  "weave" doesn't hurt here though - it's more an FYI

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 28

[Ed Morley [:emorley]]

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

Comment 29

[Tracy Walker [:tracy]]

Verified with latest builds of Nightly and Aurora