Closed Bug 1139698 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
42.2 - Jul 27
Tracking Status
firefox41 + verified
firefox42 --- verified
relnote-firefox --- 41+

People

(Reporter: zaach, Assigned: eoger)

References

Details

(Keywords: feature, Whiteboard: [fxsync])

Attachments

(4 files, 17 obsolete files)

871.27 KB, application/zip
Details
5.83 KB, image/png
Details
46.14 KB, patch
markh
: review+
Details | Diff | Splinter Review
44.72 KB, patch
markh
: review+
Details | Diff | Splinter Review
Let's spice up the hamburger menu for signed-in users by showing their profile image in place of the Sync icon.
(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.
Ryan suggests that we use circular icons here, too. [https://bugzilla.mozilla.org/show_bug.cgi?id=1139677#c11].
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.
Assignee: zack.carter → edouard.oger
Okay here's an update of zaach's patch with rounded avatars in the Hamburger menu. Would like some feedback!
Attachment #8614216 - Flags: feedback?(zack.carter)
Screenshot of the patched hamburger menu.
Flags: needinfo?(rfeeley)
zaach says we should align the avatar with the power button, I agree with him.
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.
Flags: needinfo?(rfeeley)
Attached image syncinprogress.png (obsolete) —
Attached image withoutavatar.png (obsolete) —
Attached image withavatar.png (obsolete) —
UI updated, what do you think?
Flags: needinfo?(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.
Flags: needinfo?(rfeeley)
Alright hit me up!
Attachment #8614216 - Attachment is obsolete: true
Attachment #8614216 - Flags: feedback?(zack.carter)
Attachment #8614275 - Attachment is obsolete: true
Attachment #8616309 - Attachment is obsolete: true
Attachment #8616310 - Attachment is obsolete: true
Attachment #8616311 - Attachment is obsolete: true
Flags: needinfo?(rfeeley)
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.
Depends on: 1173419
Flags: needinfo?(rfeeley)
Amazingly quick work Eduoard!!!
Attachment #8617635 - Flags: review?(markh)
Depends on: 1171253
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 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?
Attachment #8617635 - Flags: review?(markh)
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!
Attachment #8617635 - Attachment is obsolete: true
Attachment #8621746 - Flags: review?(markh)
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")
Attachment #8621746 - Flags: review?(markh) → feedback+
Thank for the throughout review, here's the updated patch. I can confirm that email is in the profile object
Attachment #8621746 - Attachment is obsolete: true
Attachment #8622670 - Flags: review?(markh)
Priority: -- → P1
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.
Attachment #8622670 - Flags: review?(markh) → review+
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.
Flags: needinfo?(rfeeley)
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.
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
(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 ;)
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.
Flags: needinfo?(rfeeley)
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.
Attachment #8622670 - Flags: review+
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.
Attachment #8623479 - Attachment is obsolete: true
Failing tests (on both tries) corrected, we're not using the XUL anonymous element anymore which is a lot better.
Attachment #8622670 - Attachment is obsolete: true
Attachment #8624371 - Flags: review?(markh)
Sorry that last patch was a git patch
Attachment #8624371 - Attachment is obsolete: true
Attachment #8624371 - Flags: review?(markh)
Attachment #8624457 - Flags: review?(markh)
And the last one wasn't even the correct patch, I feel ashamed now. I'm really sorry about the spam.
Attachment #8624457 - Attachment is obsolete: true
Attachment #8624457 - Flags: review?(markh)
Attachment #8624462 - Flags: review?(markh)
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.
Attachment #8624462 - Flags: review?(markh) → review-
Attached image 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?
Flags: needinfo?(rfeeley)
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..
(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.
Flags: needinfo?(rfeeley)
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
Attachment #8613118 - Attachment is obsolete: true
Attachment #8624462 - Attachment is obsolete: true
Attachment #8625561 - Flags: review?(markh)
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.
Attachment #8625561 - Flags: review?(markh) → review-
Patch corrected, waiting on L1 access to try push.
Attachment #8625561 - Attachment is obsolete: true
Attachment #8626354 - Flags: review?(markh)
Comment on attachment 8626354 [details] [diff] [review]
Add Firefox Avatar in the Hamburger menu

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

\o/
Attachment #8626354 - Flags: review?(markh) → review+
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.
Updated, let's hope this one is the one!
Attachment #8626354 - Attachment is obsolete: true
Attachment #8626769 - Flags: review?(markh)
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|)
Attachment #8626769 - Flags: review?(markh) → review+
Nitpick fixed, trypush to come.
Attachment #8626769 - Attachment is obsolete: true
Attachment #8627228 - Flags: review?(markh)
Attachment #8627228 - Flags: review?(markh)
^ WIP with memoization for getters
Attachment #8627228 - Attachment is obsolete: true
Comment on attachment 8627498 [details] [diff] [review]
Add Firefox Avatar in the Hamburger menu

Tests looks OK.
Attachment #8627498 - Flags: review?(markh)
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
Attachment #8627498 - Flags: review?(markh) → review+
Keywords: checkin-needed
No longer blocks: 1127540
https://hg.mozilla.org/mozilla-central/rev/67a4133bc5db
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1180106
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
QA Whiteboard: [bugday-20150708]
Whiteboard: [bugday-20150708]
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.
Status: RESOLVED → VERIFIED
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)?
Flags: needinfo?(markh)
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.
Flags: needinfo?(edouard.oger)
Depends on: 1181684
Depends on: 1181352
Depends on: 1179998
I think this is everything related to this patch.
Flags: needinfo?(edouard.oger)
Depends on: 1183730
[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.
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.
Flags: needinfo?(markh)
(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?
Flags: needinfo?(ckarlof)
Chris said in IRC he's happy with a version sans-tooltip. Edouard, can you please make such a patch?
Flags: needinfo?(ckarlof) → needinfo?(edouard.oger)
Here it is.
Flags: needinfo?(edouard.oger)
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.
Attachment #8634253 - Flags: review+
Attachment #8634253 - Flags: approval-mozilla-aurora?
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!
Attachment #8634253 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mark, is this mini-feature worth adding to FF41 release-notes?
Flags: needinfo?(markh)
(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.
Flags: needinfo?(rkothari)
Flags: needinfo?(rfeeley)
Flags: needinfo?(markh)
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.”
Flags: needinfo?(rfeeley)
QA Contact: catalin.varga
Blocks: 971266
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
Keywords: feature
Target Milestone: Firefox 42 → Firefox 41
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
relnote-firefox: --- → ?
Flags: needinfo?(rkothari)
Iteration: --- → 42.2 - Jul 27
QA Whiteboard: [bugday-20150708] → [bugday-20150708], [fxsync]
QA Whiteboard: [bugday-20150708], [fxsync] → [fxsync],[bugday-20150708]
QA Whiteboard: [fxsync],[bugday-20150708] → [bugday-20150708]
Whiteboard: [bugday-20150708] → [bugday-20150708],[fxsync]
Whiteboard: [bugday-20150708],[fxsync] → [fxsync]
Depends on: 1182009
Depends on: 1190908
Added to FF41 release notes in Nucleus.
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
Depends on: 1193246
You need to log in before you can comment on or make changes to this bug.