1139698 - Display the user's FxA profile image in the hamburger menu

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Zachary Carter [:zaach]]

Let's spice up the hamburger menu for signed-in users by showing their profile image in place of the Sync icon.

Comment 1

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

(In reply to Zachary Carter [:zaach] from comment #0)
> Let's spice up the hamburger menu for signed-in users by showing their
> profile image in place of the Sync icon.

Keep in mind that the sync icon "spins" while syncing, which is useful.

Comment 2

[Zachary Carter [:zaach]]

Ryan suggests that we use circular icons here, too. [https://bugzilla.mozilla.org/show_bug.cgi?id=1139677#c11].

Comment 3

[Zachary Carter [:zaach]]

Attachment: Display the user's FxA profile image in the hamburger menu

This was the work-in-progress I began a while ago. It's out of date but does
show the general approach that should work.

Comment 4

[Edouard Oger [:eoger]]

Attachment: 0001-Bug-1139698-Added-Firefox-Account-avatar-in-Hamburge.patch

Okay here's an update of zaach's patch with rounded avatars in the Hamburger menu. Would like some feedback!

Comment 5

[Edouard Oger [:eoger]]

Attachment: Screen Shot 2015-06-02 at 10.48.35 AM.png

Screenshot of the patched hamburger menu.

Comment 6

[Edouard Oger [:eoger]]

zaach says we should align the avatar with the power button, I agree with him.

Comment 7

[Ryan Feeley [:rfeeley]]

I'm hoping we can have it on the left like this:
http://people.mozilla.org/~rfeeley/images/avatar-circle-padding.gif

When other sync-related icons (spinning, reconnect to sync) are needed, we would show them instead of the avatar.

Comment 8

[Edouard Oger [:eoger]]

Attachment: syncinprogress.png

Comment 9

[Edouard Oger [:eoger]]

Attachment: withoutavatar.png

Comment 10

[Edouard Oger [:eoger]]

Attachment: withavatar.png

Comment 11

[Edouard Oger [:eoger]]

UI updated, what do you think?

Comment 12

[Ryan Feeley [:rfeeley]]

Great quick work! I appreciate the screenshots too. Next step for me: I will redesign it with better transitions between the different states (no account, has account with and without avatar, mid-sync, sync unverified). Do you want to work on it? I'll create another bug and assign you so it doesn't get lost.

Comment 13

[Edouard Oger [:eoger]]

Alright hit me up!

Comment 14

[Edouard Oger [:eoger]]

Attachment: 0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

Comment 15

[Edouard Oger [:eoger]]

Attachment: hamburger_menu_fxa_wip.zip

Comment 16

[Edouard Oger [:eoger]]

Here's the updated hamburger menu with rfeely's redesign. We will need to file bug afterwards to remove the sync error browser bottom bar.

Comment 17

[Ryan Feeley [:rfeeley]]

Amazingly quick work Eduoard!!!

Comment 18

[Ryan Feeley [:rfeeley]]

On hover, I would extend the darkened background to be under the avatar (noavatar_signedin_prefs_tooltip.png) as that area also opens Sync preferences.

Comment 19

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

Comment on attachment 8617635 [details] [diff] [review]
0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

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

I ran out of time, but I'm still posting the partial review anyway in-case there are things that can be addressed before I get back to it. Note also it doesn't actually work for me - I see the avatar in Prefs but not in the hamburger menu. The code appears to be seeing the correct URL but I didn't dig any deeper.  I'm also hoping we can simplify browser-fxaccounts, but it's not yet clear if that will be possible.

::: browser/base/content/browser-fxaccounts.js
@@ +7,4 @@
>    PREF_SYNC_START_DOORHANGER: "services.sync.ui.showSyncStartDoorhanger",
>    DOORHANGER_ACTIVATE_DELAY_MS: 5000,
>    SYNC_MIGRATION_NOTIFICATION_TITLE: "fxa-migration",
> +  DEFAULT_AVATAR: "chrome://browser/skin/preferences/in-content/default-profile-image.svg",

I think we need to move this default image, so we end up with something like chrome://browser/skin/fxa/default-profile-image.svg. This obviously means the preferences.inc.css will need a similar trivial change.

::: browser/base/content/browser-syncui.js
@@ +167,5 @@
>  
>      let syncButton = document.getElementById("sync-button");
> +    let statusButton = document.getElementById("PanelUI-fxa-sync-status");
> +    if (needsSetup) {
> +      if (syncButton)

The style of this file is that single lines are still bracketed, so that should continue.

::: browser/base/content/browser.xul
@@ +972,5 @@
> +          <vbox align="center"
> +                mousethrough="always"
> +                left="25" top="-5">
> +            <image id="PanelUI-button-warning"
> +                    src="https://shannonbunzey.files.wordpress.com/2015/02/mozilla-firefox.png"

I doubt this is the correct URL ;) Also, the URL will need to be specified in a .css file using the "background" attribute, and will probably need a different image to be used on retina displays (we tend to use png files with the exact size rather than relying on scaling) - see other <image> elements in this file for examples.

In general though, I'm not yet convinced this is the way to achieve this - eg, there's also the "restart to update" overlay that appears. It might even make sense to split this from this bug - have this bug be just about the opened menu and address this in a different bug. I also can't see this change in any of the mockups - what bug did Ryan OK these changes in?

Comment 20

[Edouard Oger [:eoger]]

Attachment: 0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

Updated with markh first impressions, thank you.
I removed the warning icon on the popup menu, let's do that in a separate patch.

Don't forget to set identity.fxaccounts.profile_image.enabled to true in about:config to see the avatar!

Comment 21

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

Comment on attachment 8621746 [details] [diff] [review]
0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

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

::: browser/base/content/browser-fxaccounts.js
@@ +40,4 @@
>      ];
>    },
>  
> +  get container() {

this file is used to handle various parts of the FxA UI for the browser, so I think the property "button" was an original mistake (it caught me out - I assumed it was the actual sync button!). I think we should not take this mistake further, so can we name them a bit better, and ideally to something close to their ID, so someone reading the XUL can make a good guess.  Something like panelUIContainer, panelUIAvatar and panelUIStatus seems to make sense.

You could even consider renaming .button, but I don't care too much about that.

(and I think the attempt to override the getter with the instance is premature optimization, but I understand that already existed too - but this would just be a whole lot less messy with something like:

get container() document.getElementById("PanelUI-footer-fxa"),
get button() document.getElementById("PanelUI-fxa-status"),
etc

especially given that could would actually fail in "strict" mode (you can't set a property that has a getter but no setter)

@@ +242,5 @@
>        // that case, hide the button.  We'll get another notification with a null
>        // state once migration is complete.
>        this.button.hidden = true;
> +      this.status.hidden = true;
> +      this.avatar.hidden = true;

as mentioned below, I don't think this is necessary given the css changes should set display: none once you remove the attribute from the container

@@ +278,5 @@
>        this.button.removeAttribute("tooltiptext");
> +      this.avatar.removeAttribute("tooltiptext");
> +      this.container.removeAttribute("fxastatus");
> +      this.container.removeAttribute("fxaprofileimage");
> +      this.status.hidden = false;

Is this.status.hidden and this.avatar.hidden necessary given the new css rules that set display: none when there's no fxastatus attribute on the container?

@@ +311,4 @@
>          }
>        }
>      }
> +    fxAccounts.getSignedInUserProfile().then(result => {

There's a little confusion here. Later, I expect that getSignedInUser() will return an object with a "profile" attribute, that itself has .avatar, .displayName etc, whereas getSignedInUserProfile() will return just the profile object.

IOW, code would look like either:
  getSignedInUser().then(data => { ..use data.profile.avatar, data.profile.displayName, etc})
or:
  getSignedInUserProfile().then(profile => { ..use profile.avatar, profile.displayName, etc})

(but note that I believe getSignedInUser() will currently never return a .profile element, but probably will after bug 1157529 lands, which will allow us to persist the profile data)

I assume, but I'm not sure, that the profile data doesn't include the user's email, so this is going to need more work to do the right thing (ie, as written above, userData will probably *never* have both .email and .avatar, which probably explains why I'm regularly not seeing the avatar show up)

@@ +416,4 @@
>      Weave.Notifications.replaceTitle(note);
>    }),
>  
> +  doSync: function() {

The fact browser-syncui and browser-fxaccounts are separate files is a mistake we need to fix at some point, but for now I think it's probably better to just call gSyncUI.doSync() instead of duplicating it here.

@@ +449,5 @@
>  
> +  openChangeProfileImage: function(event) {
> +    if (this.container.getAttribute("fxastatus") === "error") {
> +      this.onMenuPanelCommand(event);
> +    }

nit: please cuddle else (ie, |} else {| without a newline), or alternatively early-return and drop the else.  See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style if you are interested :)

@@ +461,5 @@
> +    }
> +  },
> +
> +  openContentInBrowser: function(url, options) {
> +    let win = Services.wm.getMostRecentWindow("navigator:browser");

note this code is running in a browser window, so I think you will find |switchToTabHavingURI()| will work directly (ie, window.switchToTabHavingURI() will exist). So you can probably drop this function completely.

::: browser/base/content/browser-syncui.js
@@ +191,4 @@
>        if (button) {
>          button.setAttribute("status", "active");
>        }
> +      button = document.getElementById("PanelUI-footer-fxa");

This is no longer a button (ie, I think you want the variable to be named "container")

Comment 22

[Edouard Oger [:eoger]]

Attachment: 0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

Thank for the throughout review, here's the updated patch. I can confirm that email is in the profile object

Comment 23

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

Comment on attachment 8622670 [details] [diff] [review]
0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

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

An aside - it would be great if you could upload patches with 8 lines of context (I can't find where that's documented, but it is somewhere ;) - https://bholley.wordpress.com/2010/10/23/using-git-with-mozilla/ is close.

But this looks great! I pushed a try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=969c100af804

::: browser/base/content/browser-fxaccounts.js
@@ +226,4 @@
>        return;
>      }
>  
> +    let profileImageEnabled = false;

it might be clearer for future readers to name the variable something like "profileInfoEnabled" and a comment reflecting what zaach mentioned about the preference being misnamed.

@@ +303,5 @@
> +    .then(profile => {
> +      doUpdate(profile);
> +    })
> +    .catch(error => {
> +      this.FxAccountsCommon.log.error(error);

I think this log would be better as:
 this.FXAccountsCommon.log.error("Failed to update profile info", error);

the logging module will do the right thing appending the error.

@@ +306,5 @@
> +    .catch(error => {
> +      this.FxAccountsCommon.log.error(error);
> +
> +      // If we can't fetch a profile try using the account data.
> +      return fxAccounts.getSignedInUser()

I'm not sure it is worth bothering with this, as IIUC, this will never do the right thing. I know I mentioned that it might in the future, but in that future I can't imagine a scenario where getSignedInUserProfile() would fail but get getSignedInUser() would successfully return the profile info.

This would mean you can drop one the .catch() handler above and just rely on the one below.

@@ +312,5 @@
> +        doUpdate(user.profile);
> +      });
> +    })
> +    .catch(error => {
> +      this.FxAccountsCommon.log.error(error);

ditto here re log message.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +371,4 @@
>  <!ENTITY appMenuHistory.restoreSession.label "Restore Previous Session">
>  <!ENTITY appMenuHistory.viewSidebar.label "View History Sidebar">
>  <!ENTITY appMenuHelp.tooltip "Open Help Menu">
> +<!ENTITY appMenuFxaAvatar.tooltip "Change Firefox Account avatar">

The capitalization of this string seems strange - I'll needinfo rfeeley for this - please wait for his OK before landing.

Comment 24

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

Ryan, see the bottom of the above comment - the capitalization seems inconsistent with other tooltips (ie, I was expecting "avatar" to be capitalized.) I'm not at all bothered if you think it is correct as it stands, but thought it worth confirming.

Comment 25

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

Comment on attachment 8622670 [details] [diff] [review]
0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

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

::: browser/base/content/browser-fxaccounts.js
@@ +299,4 @@
>          }
>        }
>      }
> +    fxAccounts.getSignedInUserProfile()

another side-effect of this is that we get an error thrown when no such account, whereas getSignedInUser just returns null.

We could check if error.message == this.FxAccountsCommon.ERROR_NO_ACCOUNT, but this ends up making log noise which might lead people to think there's an actual problem.  I'll attach a patch that I think improves things and should work unchanged if getUserAccountData() ends up returning the profile in the future.

Comment 26

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

Attachment: 0003-fix-browser-fxaccounts.patch

Try failed for reasons unrelated to this patch, but as per the last comment, the log noise from here made me think this patch was to blame :)

I suggest the following patch on-top of yours - if you think that looks good, please just fold it in and carry my r+ forward.

New try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1009464b2696

Comment 27

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

(In reply to Mark Hammond [:markh] from comment #26)
> New try at
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1009464b2696

Which failed :( This time it was this patch, but for mysterious reasons.

>      let avatarIcon = document.getAnonymousElementByAttribute(
>        this.panelUIAvatar, "class", "toolbarbutton-icon");

is returning null in all mochi tests. this.panelUIAvatar (and best I can tell all other "real" elements) are correct, so my only guess would be that the xbl bindings haven't been created yet - but that seems unlikely as the browser window should have been alive for a long time.

I pushed to try with a hacky workaround - https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8547bfdeb0c - but I'm not particularly comfortable with it - it might be worth trying to at-least understand it ;)

Comment 28

[Ryan Feeley [:rfeeley]]

Well spotted on the capitalization, although clicking the avatar should open sync preferences, not change the avatar (three actions in one row is too many). This is also why the background hover should be the same. So that tooltip shouldn't even be needed at all.

Comment 29

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

Comment on attachment 8622670 [details] [diff] [review]
0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

The patch is in good shape but there's going to be an update that I should probably look over again before landing.

Comment 30

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

Comment on attachment 8623479 [details] [diff] [review]
0003-fix-browser-fxaccounts.patch

eoger is going to take a bit of this, but I'll mark it obsolete now so it doesn't confused.

Comment 31

[Edouard Oger [:eoger]]

Attachment: 0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

Failing tests (on both tries) corrected, we're not using the XUL anonymous element anymore which is a lot better.

Comment 32

[Edouard Oger [:eoger]]

Attachment: 0001-Bug-1171253-enable-FxA-profile-image-in-Sync-prefere.patch

Sorry that last patch was a git patch

Comment 33

[Edouard Oger [:eoger]]

Attachment: 0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

And the last one wasn't even the correct patch, I feel ashamed now. I'm really sorry about the spam.

Comment 34

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c062e1d24278

Comment 35

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

Comment on attachment 8624462 [details] [diff] [review]
0001-Bug-1139698-Add-Firefox-Account-avatar-in-Hamburger-.patch

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

Sadly it doesn't work for me. I haven't the time to dig deeper now, but best I can tell the code is working correctly (ie, the backgroundImage attribute is set correctly) but I see nothing at all in the button. The browser inspector also sees things setup correctly and *does* show me the (correct) image being used for that attribute - it's just not visible.

Also:
(In reply to Ryan Feeley from comment #28)
> This is also why the background hover should be the same.

I don't think what we have that - or to put it another way, I don't think the profile image should actually be a toolbarbutton. Although I don't see an image, I *do* see a clearly defined button outline when I hover over where the image should be - which doesn't make sense to me when clicking anywhere in that area has the exact same effect as clicking the button.  I'll attach an image to demonstrate what I mean.

::: browser/base/content/browser-fxaccounts.js
@@ +305,5 @@
>      fxAccounts.getSignedInUser().then(userData => {
> +      if (!userData) {
> +        return null;
> +      }
> +      // We've a logged-in user but no profile information in it, so

this comment is now stale - I'd say just delete it.

Comment 36

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

Attachment: hamburger.png

This is how it looks for me when the mouse is hovering over the area where the profile image should be shown.

Ryan, can you please clarify if you actually want this hover effect to make the image look like a button even though clicking on the button or clicking on the email address take the exact same action?

Comment 37

[Edouard Oger [:eoger]]

Not intended at all, problem is it looks fine on mac os x, I'll have to try it with a Windows VM or something..

Comment 38

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

(In reply to Edouard Oger [:eoger] from comment #37)
> Not intended at all, problem is it looks fine on mac os x, I'll have to try
> it with a Windows VM or something..

ok, cool - I'll clear ni for Ryan.

Comment 39

[Edouard Oger [:eoger]]

Attachment: Add Firefox Avatar in the Hamburger menu

This patch uses <image> instead of <toolbarbutton> to display the avatar.
I verified and it works well on Mac OS X, Windows and Ubuntu (the separator looked weird, fixed in this patch).

Trybuild looks good too : https://treeherder.mozilla.org/#/jobs?repo=try&revision=9376b68f4939

Comment 40

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

Comment on attachment 8625561 [details] [diff] [review]
Add Firefox Avatar in the Hamburger menu

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

looking good, but sadly there's still an edge-case in the "needs reauthentication" state I missed until I actually tested that state :(

::: browser/base/content/browser-fxaccounts.js
@@ +269,5 @@
> +      this.panelUILabel.removeAttribute("tooltiptext");
> +      this.panelUIAvatar.removeAttribute("tooltiptext");
> +      this.panelUIFooter.removeAttribute("fxastatus");
> +      this.panelUIFooter.removeAttribute("fxaprofileimage");
> +      this.panelUIAvatar.style.backgroundImage = "";

I should have picked this up before, but the idea here is to remove a specific user's profile image we previously set, right? If so, I think you should use .style.removeProperty("background-image"); rather than setting to an empty string.

@@ +274,4 @@
>  
>        if (!this._inCustomizationMode) {
>          if (this.loginFailed) {
> +          let tooltipDescription = this.strings.formatStringFromName("reconnectDescription", [profile.email], 1);

another problem I just noticed during testing is that profile is likely to be null here - we have an auth error, so can't fetch the profile. So this is a case where the data from getSignedInUser would be preferred - we *do* have the email in that blob.

So I think the code below needs to change so the result of getSignedInUser() *and* getSignedInUserProfile() is passed to this function, and this branch needs to use the email from the former rather than the latter.

@@ +279,5 @@
> +          this.panelUILabel.setAttribute("label", errorLabel);
> +          this.panelUIStatus.setAttribute("tooltiptext", tooltipDescription);
> +          this.panelUIAvatar.setAttribute("tooltiptext", tooltipDescription);
> +        } else if (profile) {
> +          let label = profile.displayName ? profile.displayName : profile.email;

I think we have a similar concern in this branch - if for some reason getSignedInUser works (which doesn't hit the network) but getSignedInUserProfile does not (as that *does* hit the network), we still want to enter this branch and say "signed in as foo@bar" - it will mean we don't have a displayName, but that's probably OK.

Comment 41

[Edouard Oger [:eoger]]

Attachment: Add Firefox Avatar in the Hamburger menu

Patch corrected, waiting on L1 access to try push.

Comment 42

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

Comment on attachment 8626354 [details] [diff] [review]
Add Firefox Avatar in the Hamburger menu

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

\o/

Comment 43

[Pulsebot]

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

Comment 44

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/bd60210362fe for bustage in JP, all the browser-chrome chunks, and some devtools chunks, like https://treeherder.mozilla.org/logviewer.html#?job_id=3570824&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=3571658&repo=fx-team

Comment 45

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

I haven't looked into the failures, but some further testing on poor wifi did give a bad experience - when the profile server request ended up timing out, the hamburger menu looked bad until the request completed. I'm starting to think we may need to do this in 2 phases - as soon as getUserAccountData() returns we update the elements with the data we have, then kick off the profile request and update when that completes.

Comment 46

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d3fbd9bf65f

Comment 47

[Edouard Oger [:eoger]]

Attachment: Add Firefox Avatar in the Hamburger menu

Updated, let's hope this one is the one!

Comment 48

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

Comment on attachment 8626769 [details] [diff] [review]
Add Firefox Avatar in the Hamburger menu

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

::: browser/base/content/browser-fxaccounts.js
@@ +310,5 @@
> +      updateWithUserData(userData);
> +      return fxAccounts.getSignedInUserProfile();
> +    }).then(profile => {
> +      if (!profile) {
> +        return null;

no need to return null here (ie, just |return|)

Comment 49

[Edouard Oger [:eoger]]

Attachment: Add Firefox Avatar in the Hamburger menu

Nitpick fixed, trypush to come.

Comment 50

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67ad48eea6c1

Comment 51

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b47cb21551c

Comment 52

[Edouard Oger [:eoger]]

Attachment: Add Firefox Avatar in the Hamburger menu

^ WIP with memoization for getters

Comment 53

[Edouard Oger [:eoger]]

Comment on attachment 8627498 [details] [diff] [review]
Add Firefox Avatar in the Hamburger menu

Tests looks OK.

Comment 54

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

Comment on attachment 8627498 [details] [diff] [review]
Add Firefox Avatar in the Hamburger menu

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

I'm almost tempted to say we should see how to avoid the evil getters by fixing the tests (which presumably just means making an early return somewhere when the window is being torn down) but I'm resisting as it's not something we are introducing and there's not a real lot of value in doing that at this point, so LGTM

Comment 55

[Pulsebot]

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

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/67a4133bc5db

Comment 57

[Hossain Al Ikram [:ikram] (QA Contact)]

I have seen this bug by signing into my sync account on Nightly 35.0a1 (2014-09-29) on Windows 7 64 Bit!

This bug (or feature request) is now fixed on Latest Nightly 42.0a1 (2015-07-07)! I just loved this implementation :D

Build ID : 20150707030206
User Agent : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0

Comment 58

[Ashickur Rahman]

I have seen this bug by signing into my sync account on Firefox 39.0 (20150630154324) on Mac OS X 10.10.4!

The fixed works for me on Latest Nightly 42.0a1 , Build ID 20150708030204 , User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0 .

Based on Comment 57 and my verification, I am marking the bug as verified.

Comment 59

[Chris Karlof [:ckarlof]]

Markh, can we uplift this to Fx41 (and the fix for long emails: https://bugzilla.mozilla.org/show_bug.cgi?id=1181684), so it ships with the profile images in the Sync pref pane in Fx41 (https://bugzilla.mozilla.org/show_bug.cgi?id=1171253)?

Comment 60

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

Edouard, you said you were making a list of followups to this - can you please mark then as blocking this bug? Then when we go through the uplift request process we can make sure all bugs blocking it are also managed.

Comment 61

[Edouard Oger [:eoger]]

I think this is everything related to this patch.

Comment 62

[Edwin Wong [:edwong]]

[Tracking Requested - why for this release]: Please uplift to Fx41 DevEdition. This patch is critical in shipping FxAccts Profile images in browser chrome for Fx41. It's been in nightly for a few weeks and no issues to report.

Comment 63

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

Bugger - this patch added a string, which in almost all cases, prevents an uplift. We could consider asking for an exception but IMO it's hard to make the case that this warrants one.

Comment 64

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

(In reply to Mark Hammond [:markh] from comment #63)
> Bugger - this patch added a string, which in almost all cases, prevents an
> uplift. We could consider asking for an exception but IMO it's hard to make
> the case that this warrants one.

Although I guess we could consider making an Aurora specific patch without that string and just live without a tooltip in 41. Chris, what's your take?

Comment 65

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

Chris said in IRC he's happy with a version sans-tooltip. Edouard, can you please make such a patch?

Comment 66

[Edouard Oger [:eoger]]

Attachment: bug-1139698-aurora.patch

Here it is.

Comment 67

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

Comment on attachment 8634253 [details] [diff] [review]
bug-1139698-aurora.patch

Approval Request Comment
[Feature/regressing bug #]: Displaying Firefox Accounts profile in the browser.
[User impact if declined]: This 1/2 landed in 41 but without polish and without the information (eg, profile image) in the hamburger menu.
[Describe test coverage new/current, TreeHerder]: Been on central for some time, all existing and new tests pass.
[Risks and why]: Risk is mainly limited to the "Firefox Accounts" area of the hamburger menu, but given the time left on 41 being Aurora we are confident we can manage any unlikely regressions.
[String/UUID change made/needed]: None (but note that the version of this patch that landed on central in 42 did have a new string - this patch is Aurora-specific and removed it)

Note I'm also going to be requesting uplift on most dependents (ie, followups) of this bug.

Comment 68

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8634253 [details] [diff] [review]
bug-1139698-aurora.patch

Approving for uplift. Mark thank you for keeping the quality bar high and also conforming to the "no string changes in aurora" guideline. This is awesome!

Comment 69

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Mark, is this mini-feature worth adding to FF41 release-notes?

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ade4dc95c00

Comment 71

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

(In reply to Ritu Kothari (:ritu) from comment #69)
> Mark, is this mini-feature worth adding to FF41 release-notes?

Yeah, I think it is - although it should be inclusive of the fact this info is also shown in the "Preference -> Sync" page. So I guess it would be something like "Sync users can now personalize the look of their browser by choosing a profile image and a display name, both of which will be shown in the hamburger menu and Sync preferences."

needinfo rfeeley to help craft a better set of words to use
needinfo ritu because I'm not sure of the process to make this actually happen and not be forgotten.

Comment 72

[Ryan Feeley [:rfeeley]]

No display name yet :(

“Sync users can now add their own account picture which will appear in the toolbar menu, Sync preferences, and when managing the account on the web.”

Comment 73

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

As discussed with Ritu:
- setting the feature keyword so QA can track and test accordingly
- setting 41 as the Target Milestone since that's where we want to release this

Comment 74

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Release Note Request (optional, but appreciated)
[Why is this notable]: This is a cool new mini-feature that is part of FF41 release. 
[Suggested wording]: “Sync users can now add their own account picture which will appear in the toolbar menu, Sync preferences, and when managing the account on the web.”
[Links (documentation, blog post, etc)]: None

Comment 75

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to FF41 release notes in Nucleus.

Comment 76

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 41
Build Id: 20150810004008
OS: Win 7 x64, Mac OS X 10.10.4, Ubuntu 12.04 x86