1139677 - Display the user's FxA profile image in the Sync Pref pane

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Zachary Carter [:zaach]]

If the signed in user has a FxA profile image we should display it on the preferences pane.

Comment 1

[Zachary Carter [:zaach]]

Attachment: Display the user's FxA profile image in the Sync Pref pane

Comment 2

[Zachary Carter [:zaach]]

Attachment: Signed in but no profile image

Comment 3

[Zachary Carter [:zaach]]

Attachment: Signed in with a profile image

Comment 4

[Zachary Carter [:zaach]]

I should note that this is pref-ed off. "identity.fxaccounts.profile_image.enabled" must be set to true for the UI to reflect the changes.

Comment 5

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

Comment on attachment 8572955 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

LGTM, but note:

* sadly there are 2 versions of sync.js and sync.xul that need to be updated (starting with just 1 is fine while we nut things out, but both will need to change)
* like my comments in bug 1139657, I wonder if this is to focused on the avatar? eg, promiseAccountsChangeProfileImageURI() could just be promiseAccountsChangeProfile(), which is a generic page used to change *any* part of the profile, including the avatar (even if the avatar is the only thing that can be changed now)
* I don't think the pref is needed.

Comment 6

[Zachary Carter [:zaach]]

(In reply to Mark Hammond [:markh] from comment #5)
> Comment on attachment 8572955 [details] [diff] [review]
> Display the user's FxA profile image in the Sync Pref pane
> 
> Review of attachment 8572955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, but note:
> 
> * sadly there are 2 versions of sync.js and sync.xul that need to be updated
> (starting with just 1 is fine while we nut things out, but both will need to
> change)
> * like my comments in bug 1139657, I wonder if this is to focused on the
> avatar? eg, promiseAccountsChangeProfileImageURI() could just be
> promiseAccountsChangeProfile(), which is a generic page used to change *any*
> part of the profile, including the avatar (even if the avatar is the only
> thing that can be changed now)

I think we'd still want some distinction in the URL so that the settings page could highlight the field that the user intends to edit (or show a different page, as is the current case). There is a unified settings page planned in the future, but avatars might still be on there own page for the first version.

> * I don't think the pref is needed.

Let's keep it in for now, at least until the support for WebChannel makes it to production on the content server.

Comment 7

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

(In reply to Zachary Carter [:zaach] from comment #6)

> I think we'd still want some distinction in the URL so that the settings
> page could highlight the field that the user intends to edit (or show a
> different page, as is the current case).

The profile field name in the query string might work?

> > * I don't think the pref is needed.
> 
> Let's keep it in for now, at least until the support for WebChannel makes it
> to production on the content server.

No worries - although maybe you can still remove it from firefox.js (which just means catching an exception when you get the value).  But whatevs :)

Comment 8

[Zachary Carter [:zaach]]

Attachment: Display the user's FxA profile image in the Sync Pref pane

Comment 9

[Zachary Carter [:zaach]]

Attachment: Display the user's FxA profile image in the Sync Pref pane

Comment 10

[Ryan Feeley [:rfeeley]]

It works! Can the profile picture in the menu be encircled with adjusted padding like this? http://people.mozilla.org/~rfeeley/images/avatar-circle-padding.gif

Comment 11

[Ryan Feeley [:rfeeley]]

Comment on attachment 8574396 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

Circle crop for a plus.

Comment 12

[Zachary Carter [:zaach]]

Comment on attachment 8574396 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

The hamburger menu has its own issue here: https://bugzilla.mozilla.org/show_bug.cgi?id=1139698. I'll make a note that we should use circles there, too.

As far as the pref pane is concerned, what do you think, Ryan?

Comment 13

[Ryan Feeley [:rfeeley]]

Pref pane looks great!

Comment 14

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

Comment on attachment 8574396 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

This looks pretty good, but the "old" style prefs aren't handled.  Personally I can live with that, but there is a (small IIUC) risk that in-content prefs will not make the next train, so we'd want approval from someone that we can live without the profile image for the entire next release train if that happens, or we'd need similar treatment to that dialog-based code.  So only f+ until we've a plan there.

(I also added ui-review+ given comment 13 - rfeeley is still learning his way around these flags ;)

::: browser/components/preferences/in-content/sync.js
@@ +294,5 @@
> +        document.getElementById("fxaProfileImage").style.backgroundImage = "";
> +
> +        if (data.avatar) {
> +          let img = new Image();
> +          // Make sure the image is available before displaying it

please extend this comment with something like ", as we don't want to overwrite the default profile image with a broken/unavailable image"

@@ +303,5 @@
> +          img.src = data.avatar;
> +        }
> +      };
> +
> +      fxAccounts.getSignedInUserProfile().then(data => {

getSignedInUserProfile() isn't part of this patch - I recall it flying past before, but the name seems unfortunate given its use in this context - it *sounds* like it will just return profile data but it seems to be returning the user data *and* the profile data - any chance of renaming that to something like getSignedInUserDataWithProfile() or similar?

@@ +305,5 @@
> +      };
> +
> +      fxAccounts.getSignedInUserProfile().then(data => {
> +        doUpdate(data);
> +      },

I think this should ready:
fxAccounts.getSignedInUserProfile().then(data => {
  doUpdate(data);
}.catch(err) {
  log.error(err); // (actually, make this log.error("Failed to update profile", err);
});

As IIUC, the way you have written it will only catch errors in getSignedInUserProfile, not in doUpdate, whereas the above will catch both.

::: services/fxaccounts/FxAccounts.jsm
@@ +972,5 @@
>    },
>  
> +  // Returns a promise that resolves with the URL to use to change
> +  // the current account's profile image.
> +  promiseAccountsChangeProfileURI: function(editSetting) {

it looks like you expect editSetting to be not specified (maybe just in the future) - if so, you might as well add a default (eg, editSetting = null) - and also add a comment for what this is for.  If you don't expect it to ever be called without editSetting then I wouldn't bother checking it is truthy below - just let it blow up if someone calls it incorrectly.

Comment 15

[Zachary Carter [:zaach]]

Since this is more experimental I think we'd be okay if in-content prefs don't make it, but let's ask rfk.

Comment 16

[Zachary Carter [:zaach]]

(In reply to Mark Hammond [:markh] from comment #14)
> Comment on attachment 8574396 [details] [diff] [review]
> Display the user's FxA profile image in the Sync Pref pane
> 
> Review of attachment 8574396 [details] [diff] [review]:
> -----------------------------------------------------------------
...
> 
> @@ +303,5 @@
> > +          img.src = data.avatar;
> > +        }
> > +      };
> > +
> > +      fxAccounts.getSignedInUserProfile().then(data => {
> 
> getSignedInUserProfile() isn't part of this patch - I recall it flying past
> before, but the name seems unfortunate given its use in this context - it
> *sounds* like it will just return profile data but it seems to be returning
> the user data *and* the profile data - any chance of renaming that to
> something like getSignedInUserDataWithProfile() or similar?

Yeah, "verified" is the odd attribute out– "uid" and "email" appear in both profile and account data (although  the value of "email" could potentially diverge once/if we support multiple email addresses). I feel like we should perhaps refactor so that getSignedInUserProfile rejects with the appropriate error (NO_ACCOUNT or UNVERIFIED_ACCOUNT) and let the consumer handle it (the Sync pref UI code, in this case) instead of relying on the account data being null or verified being false. If the account is unverified, the consumer can a separate call to retrieve the account data and display that instead.

Comment 17

[Chris Karlof [:ckarlof]]

> This looks pretty good, but the "old" style prefs aren't handled.  Personally I can live with that, but there is a (small IIUC) risk that in-content prefs will not make the next train, so we'd want approval from someone that we can live without the profile image for the entire next release train if that happens, or we'd need similar treatment to that dialog-based code.

What release is in-content prefs targeting? I heard 38.

This avatar stuff would probably be in 39, correct?

Comment 18

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

(In reply to Chris Karlof [:ckarlof] from comment #17)
> What release is in-content prefs targeting? I heard 38.
> 
> This avatar stuff would probably be in 39, correct?

That's my understanding too, but I've been told that for stuff like readinglist I must keep the old prefs current "just incase".  We probably don't have to be quite as strict for this given it's just fluff.

Comment 19

[Ryan Kelly [:rfkelly]]

As long as we're not *breaking* the old-style prefs pane, I don't see a problem with shipping it avatar-less for a cycle if we need to.

Comment 20

[Zachary Carter [:zaach]]

Attachment: Display the user's FxA profile image in the Sync Pref pane

This version refactors getSignedInUserProfile to only return profile data. It will
reject with an error that the UI code handles when there's no account
or it's unverified.

Comment 21

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

Comment on attachment 8582815 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

This looks good, apart from the sync.js changes which have crossed a (vague, subjective) complexity threshold and I think my suggestion will make it simpler to reason about (but let me know if I've missed something there)

::: browser/components/preferences/in-content/sync.js
@@ +284,5 @@
>            checkbox.disabled = enginesListDisabled;
>          }
> +
> +        // Clear the profile image (if any) of the previously logged in account.
> +        document.getElementById("fxaProfileImage").style.backgroundImage = "";

I think document.getElementById("fxaProfileImage").style.removeProperty("background-image") is the preferred way of doing that.

@@ +304,5 @@
> +        doUpdate(data);
> +      }).catch(err => {
> +        FxAccountsCommon.log.error(err);
> +
> +        if (err.message === FxAccountsCommon.ERROR_NO_ACCOUNT) {

TBH I'm not that keen on these changes and the "retry" of doUpdate.  Can we rejig this to something like:

      fxAccounts.getSignedInUser().then(data => {
        ... all the existing code.
      }).then(() => {
        return fxAccounts.getSignedInUserProfile();
      }).then(profileData => {
        ... just do the profile thing
      }, err => {
         ... handle getSignedInUserProfile() failing (ie, no account, not verified)
      }).catch(err => {
        ... just log the error - it should be "impossible" to get here - it could even be left off as the existing code has no such .catch (but I think it makes sense to have it - maybe just Cu.reportError() rather than logging, as the error may not be FxA related at all.
      });

Comment 22

[Chris Karlof [:ckarlof]]

> We probably don't have to be quite as strict for this given it's just fluff.

I'm just going to assume that "fluff" is Australian for "really fantastic engineering".

Comment 23

[Ryan Kelly [:rfkelly]]

> I'm just going to assume that "fluff" is Australian for "really fantastic engineering".

Am Australian, can confirm...

Comment 24

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

(In reply to Chris Karlof [:ckarlof] from comment #22)

> I'm just going to assume that "fluff" is Australian for "really fantastic
> engineering".

:) Seriously though, I hope I haven't offended Zaach here - I meant "visual awesomeness rather than new required functionality" :)

Comment 25

[Chris Karlof [:ckarlof]]

> :) Seriously though, I hope I haven't offended Zaach here - I meant "visual awesomeness rather than new required functionality" :)

I doubt it.

I was just trying to de-escalate to avoid anyone going all "Mark Hammond" on the bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1139166#c27). :)

Comment 26

[Zachary Carter [:zaach]]

No worries, Mark. I knew what you meant :)

(In reply to Mark Hammond [:markh] from comment #21)
> Comment on attachment 8582815 [details] [diff] [review]
> Display the user's FxA profile image in the Sync Pref pane
> 
> Review of attachment 8582815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, apart from the sync.js changes which have crossed a (vague,
> subjective) complexity threshold and I think my suggestion will make it
> simpler to reason about (but let me know if I've missed something there)
> 
> ::: browser/components/preferences/in-content/sync.js
> @@ +284,5 @@
> >            checkbox.disabled = enginesListDisabled;
> >          }
> > +
> > +        // Clear the profile image (if any) of the previously logged in account.
> > +        document.getElementById("fxaProfileImage").style.backgroundImage = "";
> 
> I think
> document.getElementById("fxaProfileImage").style.removeProperty("background-
> image") is the preferred way of doing that.
> 
> @@ +304,5 @@
> > +        doUpdate(data);
> > +      }).catch(err => {
> > +        FxAccountsCommon.log.error(err);
> > +
> > +        if (err.message === FxAccountsCommon.ERROR_NO_ACCOUNT) {
> 
> TBH I'm not that keen on these changes and the "retry" of doUpdate.  Can we
> rejig this to something like:
> 
>       fxAccounts.getSignedInUser().then(data => {
>         ... all the existing code.

Hm, which existing code do you mean?

>       }).then(() => {
>         return fxAccounts.getSignedInUserProfile();
>       }).then(profileData => {
>         ... just do the profile thing
>       }, err => {
>          ... handle getSignedInUserProfile() failing (ie, no account, not
> verified)

getSignedInUser above would tell us whether the account was absent or unverified, so if we handle those states there this could just log whatever error occured. Or we can defer that to the handler below.

>       }).catch(err => {
>         ... just log the error - it should be "impossible" to get here - it
> could even be left off as the existing code has no such .catch (but I think
> it makes sense to have it - maybe just Cu.reportError() rather than logging,
> as the error may not be FxA related at all.
>       });

Comment 27

[Zachary Carter [:zaach]]

Attachment: Display the user's FxA profile image in the Sync Pref pane

Implemented markh's suggestions

Comment 28

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

Comment on attachment 8583683 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

Yeah, I think that's much better - thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Richard Marti (:Paenglab)]

The default-profile-image.svg isn't copied to skin/classic/aero/browser/preferences/in-content/