Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
944563002:
Fix fast user switcher appearance when empty. (Closed)
DescriptionRemove one pixel line in the fast user switcher appearance when there are no profiles to display.
BUG=458355
TEST=
Make sure the new avatar menu flag is enabled.
1. Open a Chrome instance with 0 or 1 available profiles.
2. Right click on the User Menu button.
3. Nothing should be displayed.
4. Open the User Manager and add some profiles until there are at least 2.
5. Right click on the User Menu button
6. The fast user switcher should display a list of profiles that can be switched to.
Committed: https://crrev.com/3803191395efc35ca1dc288e16d47987a2bf4176
Cr-Commit-Position: refs/heads/master@{#318063}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Histograms and cleanup #
Total comments: 3
Patch Set 3 : Revert instrumentation changes. #
Messages
Total messages: 20 (4 generated)
anthonyvd@chromium.org changed reviewers: + asvitkine@chromium.org, mlerman@chromium.org, sky@chromium.org
Hi guys, could you please take a look at this: asvitkine@: chrome/browser/ui/cocoa/* mlerman@: profile_metrics sky@: profile_chooser_view.cc Much appreciated!
LGTM
https://codereview.chromium.org/944563002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/944563002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.h:77: SWITCH_PROFILE_FAST_SWITCHER, Please update histograms.xml' definition of this enum. https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/profile_chooser_view.cc:497: Nit: No empty line in start of function body.
https://codereview.chromium.org/944563002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/944563002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.h:76: // User switches profiles from the Avatar Menu fast user switcher. s/Avatar Menu/User Menu https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/profile_chooser_view.cc:811: ProfileMetrics::SWITCH_PROFILE_FAST_SWITCHER); Don't we want to send SWITCH_PROFILE_FAST_SWITCHER only if the view_mode == BUBBLE_BIEW_MODE_FAST_PROFILE_CHOOSER? Otherwise we can just keep using SWITCH_PROFILE_ICON, there's no value in moving to a new bucket; the two buckets will have the same meaning. (an update to the description in histograms.xml may be warranted, but that's it)
New patchsets have been uploaded after l-g-t-m from sky@chromium.org
https://codereview.chromium.org/944563002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/944563002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.h:76: // User switches profiles from the Avatar Menu fast user switcher. On 2015/02/20 14:40:57, Mike Lerman wrote: > s/Avatar Menu/User Menu Done. https://codereview.chromium.org/944563002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.h:77: SWITCH_PROFILE_FAST_SWITCHER, On 2015/02/19 22:24:28, Alexei Svitkine wrote: > Please update histograms.xml' definition of this enum. Done. https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/profile_chooser_view.cc:497: On 2015/02/19 22:24:28, Alexei Svitkine wrote: > Nit: No empty line in start of function body. Done. https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/profile_chooser_view.cc:811: ProfileMetrics::SWITCH_PROFILE_FAST_SWITCHER); On 2015/02/20 14:40:57, Mike Lerman wrote: > Don't we want to send SWITCH_PROFILE_FAST_SWITCHER only if the view_mode == > BUBBLE_BIEW_MODE_FAST_PROFILE_CHOOSER? Otherwise we can just keep using > SWITCH_PROFILE_ICON, there's no value in moving to a new bucket; the two buckets > will have the same meaning. (an update to the description in histograms.xml may > be warranted, but that's it) You're absolutely right but SWITCH_PROFILE_ICON is still used when the New Avatar Menu flag is disabled (it logs switches from the old bubble). I'm separating it into a different bucket to be able to separate the two at least until the new avatar menu is the only one available. Does that make sense?
https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/profile_chooser_view.cc:811: ProfileMetrics::SWITCH_PROFILE_FAST_SWITCHER); We can always determine whether someone's in the New Avatar world vs. the Old Avatar world by looking at which finch experiment they're reporting in, and separate out the histograms reporting by Finch Experiment (this can be done either on the histograms page as a filter or the variations page on uma.googleplex). Keeping it together also allows us to compare usage over time, especially comparing back to the old avatar world of yester-year. https://codereview.chromium.org/944563002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/944563002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:501: g_browser_process->profile_manager()->GetNumberOfProfiles() <= 1) { The ProfileManager only shows you the number of Loaded profiles (and also can include guest!). We want all profiles. This can be gotten from the ProfileInfoCache in general or perhaps from the avatar_menu_ object for this class.
https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/944563002/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/profile_chooser_view.cc:811: ProfileMetrics::SWITCH_PROFILE_FAST_SWITCHER); On 2015/02/20 15:33:59, Mike Lerman wrote: > We can always determine whether someone's in the New Avatar world vs. the Old > Avatar world by looking at which finch experiment they're reporting in, and > separate out the histograms reporting by Finch Experiment (this can be done > either on the histograms page as a filter or the variations page on > uma.googleplex). > > Keeping it together also allows us to compare usage over time, especially > comparing back to the old avatar world of yester-year. Ah! I didn't think about separating by experiment. Very good point I'll change it back. https://codereview.chromium.org/944563002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/944563002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:501: g_browser_process->profile_manager()->GetNumberOfProfiles() <= 1) { On 2015/02/20 15:34:00, Mike Lerman wrote: > The ProfileManager only shows you the number of Loaded profiles (and also can > include guest!). We want all profiles. This can be gotten from the > ProfileInfoCache in general or perhaps from the avatar_menu_ object for this > class. Hm, this function is static and the avatar_menu_ object hasn't been created yet. However, I see that ProfileManager::GetNumberOfProfiles just calls ProfileInfoCache::GetNumberOfProfiles directly. What is the latter supposed to return? On my machine I can just fire up Chrome and it returns the total number of profiles, not just the opened ones (let's say I have 3 profiles on my machine it'll return 3 even though I haven't switched to any profile). Thoughts?
https://codereview.chromium.org/944563002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/944563002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:501: g_browser_process->profile_manager()->GetNumberOfProfiles() <= 1) { On 2015/02/20 19:06:05, anthonyvd wrote: > On 2015/02/20 15:34:00, Mike Lerman wrote: > > The ProfileManager only shows you the number of Loaded profiles (and also can > > include guest!). We want all profiles. This can be gotten from the > > ProfileInfoCache in general or perhaps from the avatar_menu_ object for this > > class. > > Hm, this function is static and the avatar_menu_ object hasn't been created yet. > However, I see that ProfileManager::GetNumberOfProfiles just calls > ProfileInfoCache::GetNumberOfProfiles directly. What is the latter supposed to > return? On my machine I can just fire up Chrome and it returns the total number > of profiles, not just the opened ones (let's say I have 3 profiles on my machine > it'll return 3 even though I haven't switched to any profile). Thoughts? Ah - I didn't realize the ProfileManager just calls through to the ProfileInfoCache. Nothing to do here, then :)
histograms.xml LGTM I'm a little bit sad that we're duplicating profiles logic in two UI implementations - ideally it would be in one place. But it's not the worst.
Reverted the instrumentation changes in light of the discussion with mlerman@
On 2015/02/24 21:15:46, anthonyvd wrote: > Reverted the instrumentation changes in light of the discussion with mlerman@ Please modify the CL's title, description and referenced bugs to reflect this.
On 2015/02/25 14:42:10, Mike Lerman wrote: > On 2015/02/24 21:15:46, anthonyvd wrote: > > Reverted the instrumentation changes in light of the discussion with mlerman@ > > Please modify the CL's title, description and referenced bugs to reflect this. Done.
LGTM Thanks!
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/944563002/#ps40001 (title: "Revert instrumentation changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/944563002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3803191395efc35ca1dc288e16d47987a2bf4176 Cr-Commit-Position: refs/heads/master@{#318063} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
