1524665 - FxA Toolbar Menu - Browser Feature for Release 67

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, enhancement, P1)

Description

[Grace Xu [:gxu]]

https://docs.google.com/document/d/129HzVoufID4S2HRKMivXNW6BvlR6lYMa7Nyg-D-ZenA/edit

Comment 1

[Vijay Budhram]

As part of the next phases for the FxA Discoverability work, we plan on implementing the feature entirely in the browser code. This bug will be used to track that development.

The native implementation should have feature parity with the V1 version of FxA Discoverability addon.

• Feature doc
  • https://docs.google.com/document/d/129HzVoufID4S2HRKMivXNW6BvlR6lYMa7Nyg-D-ZenA/edit
• UX doc
  • https://mozilla.invisionapp.com/share/85PSXVZAKGY#/screens/339003076_Artboard

Comment 2

[Vijay Budhram]

Attachment: Bug 1524665 - Add FxA avatar toolbar menu

Comment 3

[Mike Kaply [:mkaply]]

Just an FYI we want to make sure this doesn't appear if sync is disable via enterprise policy.

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Looking at the latest Try run[1], there are quite a few Talos regressions[2]:

platform	test	regression (%)
windows10-64	sessionrestore_no_auto_restore opt e10s stylo	2.76%
windows10-64	tart opt e10s stylo	3.51%
windows10-64	tpaint opt e10s stylo	4.86%
windows10-64	tresize opt e10s stylo	11.99%
windows10-64	tsvg_static opt e10s stylo	4.73%
windows10-64	tsvgx opt e10s stylo	2.65%
windows7-32	tp5o_scroll opt e10s stylo	2.42%

Couple things to note about this particular Try run:

1. There are a number of linux64-qr regressions, but I'm not sure if that platform blocks landing code right now, so I haven't listed them here.
2. The osx tests didn't run, due to a build error on Try (looks like an intermittent Try issue, bug 1411358, to me). I've retriggered this build, but we don't have a successful build or Talos data yet.

Another recent Try run[3] (with slightly different code) did build osx successfully, and found it regressed sessionrestore (by 3%), tpaint (by 11.25%), and ts_paint (by 3%). I'm not sure these numbers are applicable here, since the code has changed, but they do suggest we should get osx Talos data before landing.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6b9330dbcfcd904394ed003f785a4a924a37fb3
[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=d6b9330dbcfcd904394ed003f785a4a924a37fb3&framework=1&selectedTimeRange=172800
[3] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=9d1868a79fe0fe593bd6a4b1bfcc02039bca6a06&framework=1&selectedTimeRange=172800

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: sessionrestore_no_auto_restore opt e10s stylo windows10-64 (mozilla-central).png

I don't think the windows10-64 sessionrestore regression of 2.76% is significant.

The last 4 results on mozilla-central had absolute values of 319, 321, 325, and 326 (the rightmost four purple points on the graph in the attached screenshot).

By comparison, our average absolute value is 328. It looks like a worse regression because the 319 result happened to be chosen as a base, but is very close to what's already in master.

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: tresize opt e10s stylo windows10-64 (mozilla-central).png

It appears the ~12% win10 tresize regression is also not significant.

Looking at the graph (attached screenshot), the results for this test are bimodal, and two recent builds had high values of 7.87 and 7.85. The parent commit for this patch is one of the two recent builds (2e02ffd), so this isn't our regression.

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: tart opt e10s stylo windows10-64 (mozilla-central).png

The tart regression of 3.51% also appears to be an artifact of a bimodal distribution.

Our absolute value is 2.35, and our base commit (highlighted in the screenshot) is actually the highest value of the recent four builds (2.36). Not our regression.

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: tpaint opt e10s stylo windows10-64 (mozilla-central).png

The tpaint regression of 4.86% might be significant. I'm not sure; we should gather some more data.

It looks like the top of the recent build range is 124.22 and 123.96; our absolute value is 125.79. This is less than a 1% regression from the top range of recent builds.

Looking at our base commit, its result here was 121.26. Relative to this value, our result is about 3.7% relatively worse.

I'm not sure if tpaint numbers are commit-dependent or if it's more an issue of random noise for any given build. We can do another tpaint run to see if the regression is repeatable.

Comment 9

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: tsvg_static opt e10s stylo windows10-64 (mozilla-central).png

The win10 tsvg_static regression also looks like it falls comfortably within the range of recent m-c pushes.

Our value was 37.97, and the graph indicates a bimodal distribution with high values from 38 - 38.5 in the last day.

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: tsvgx opt e10s stylo windows10-64 (mozilla-central).png

Same happy story with the win10 tsvgx regression.

Our value (130.14) is within 1% of the 3 of 5 recent builds that fell on the high side of the bimodal distribution (128.98, 129.86, 129.04). I also see a build with a value above 132 in the last day.

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: tp5o_scroll opt e10s stylo windows7-32 (mozilla-central).png

Rounding out our windows regression extravaganza, once again we see a non-significant regression in the windows7-32 tp5o_scroll regression.

Our value was ~1.48 against a base of ~1.45, but the 5 most recent builds are clustered between 1.47-1.49.

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

A reminder regarding Talos results: note that this feature's UI will be preffed off when it lands. We'll rely on other teams to flip the pref for limited populations.

In other words, beyond the lack of Talos regressions, the performance risk posed by this patch is low.

Comment 13

[:Gijs (he/him)]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please) from comment #12)

A reminder regarding Talos results: note that this feature's UI will be preffed off when it lands. We'll rely on other teams to flip the pref for limited populations.

What is the actual plan here? Because bug 1529464 implies that we intend to roll this out for 99% of release population, so that makes the performance issues pretty important (also with the pref on). (needinfo for this)

In other words, beyond the lack of Talos regressions, the performance risk posed by this patch is low.

I'm sorry but the Talos comparisons here don't make sense. When you hover over the results, you can see the individual runs (there are 6 on the trypush, and the compare page seems to have picked 15-18 from m-c). Yes, there is always noise, but going from e.g. 15 runs of tpaint that range from 114 to 124 (avg 119) to one where every single run is >=122 is a regression. Ditto for something that is bimodal but 70% hits the low case, and now you hit the high case 100% of the time (tresize).

I don't really understand what's happened here because an earlier comparison didn't flag up nearly as many regressions. We should figure out what changed.

Comment 14

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Vijay repushed using pgo builds, including a baseline. Maybe that will help with interpreting the results.

The plan is apparently now to not uplift to 66, but to land in 67. I don't know of the details of flipping this pref for rollout. Maybe Grace knows.

Comment 15

[Phyllis McDermott]

Hi, we plan to have the prefs default to on.

Comment 16

[paulo8624]

I don't know if this is being treated on this bug or separately, but it would be INCREDIBLY useful for the avatar icon to allow switching profiles, like Chrome's does.

Comment 17

[lolipopplus]

I have filed similar things literally one year ago. Glad to see progress.
https://bugzilla.mozilla.org/show_bug.cgi?id=1419236

can someone add this to see also?

Comment 18

[:Gijs (he/him)]

(In reply to paulo8624 from comment #16)

I don't know if this is being treated on this bug or separately, but it would be INCREDIBLY useful for the avatar icon to allow switching profiles, like Chrome's does.

(In reply to lolipopplus from comment #17)

I have filed similar things literally one year ago. Glad to see progress.
https://bugzilla.mozilla.org/show_bug.cgi?id=1419236

can someone add this to see also?

They're not really even related, because sync accounts aren't "profiles". In Chrome, I'm assuming user profiles and sync are related and 1:1. That is not the case in Firefox, and this menu only covers a connection to sync/fxa, not local user profiles. Disconnecting from a sync/fxa account does not wipe all your local data, and so if you "switched profiles" in fxa in this way, it wouldn't actually do what you expect, it'd just copy your data to the new sync/fxa account, which is probably the last thing you want if you're trying to keep e.g. work/private stuff separate.

What you want is switching local user profiles (which could each be associated to a separate sync account). There are no plans to add more UI for that, though about:profiles might be relevant to your interests.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a075843a5cc
add strings for FxA avatar toolbar menu, r=gijs

Comment 20

[:Gijs (he/him)]

(We use critical for crash fixes etc, so downgrading to "normal", P1 given this is high prio.)

I'm landing the strings for this on inbound, so marking leave-open so that when the strings hit central that doesn't close the bug automatically.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a075843a5ccddc55d2d04f5bc810ae35b0f6c09

Comment 21

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/3a075843a5cc

Comment 22

[Vijay Budhram]

Attachment: Bug 1524665 - Add send tab to device button

Depends on D20433

Comment 23

[:Gijs (he/him)]

Attachment: Bug 1524665 - add string for 'Sync Now' for when sync is pending, r?flod

Comment 24

[:Gijs (he/him)]

Attachment: Bug 1524665 - update sync now item when syncing, r?vbudhram

Depends on D23694

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a163024784b
add string for 'Sync Now' for when sync is pending, r=flod

Comment 26

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4a163024784b

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d28922f30e
Add FxA avatar toolbar menu, r=gijs,eoger
https://hg.mozilla.org/integration/mozilla-inbound/rev/465af3e56127
Add send tab to device button, r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc47fddd9a63
update sync now item when syncing, r=vbudhram

Comment 28

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a3d28922f30e
https://hg.mozilla.org/mozilla-central/rev/465af3e56127
https://hg.mozilla.org/mozilla-central/rev/dc47fddd9a63

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/51e5fcce16d5
https://hg.mozilla.org/releases/mozilla-beta/rev/fbad8125c402
https://hg.mozilla.org/releases/mozilla-beta/rev/1e5265ef2db3

Comment 30

[Pascal Chevrel:pascalc (PTO until April 26)]

Added to release notes with 67 beta 7, thanks.

We’ve added a toolbar menu for your Firefox Account to provide more transparency for when you are synced, sharing data across devices and with Firefox. Personalize the appearance of the menu with your own avatar.

Comment 31

[Phyllis McDermott]

The name of the feature has been changed from 'FxA + Sync Discoverability w/Avatar' to 'FxA Toolbar Menu' so making sure all the documentation is updated with the new name. Thank you.

Comment 32

[Vlad Lucaci, Desktop QA (:vlucaci)]

Please disregard bug flags.
Reverted to previous changes.