Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2052473003:
Reflow of user menu's active profile card into a button (Closed)
DescriptionReflow of user menu's active profile card into a button
Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile.
See screenshots (marked with 4.*:
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8
Mockup:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20Sign%20In/user_menu#%2Fspec-2.png
Upstream CL (landed):
https://codereview.chromium.org/2023093002/
BUG=615893
Committed: https://crrev.com/478d83e5b642df214eea6a01d8a3a46b6b44cf1c
Cr-Commit-Position: refs/heads/master@{#403919}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebased after upstream CL landed #
Total comments: 15
Patch Set 3 : Addressed comments from sky #
Messages
Total messages: 33 (19 generated)
Description was changed from ========== Reflow of user menu's active profile card into a button BUG= ========== to ========== Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. Most of the changes in CreateMaterialDesignCurrentProfileView was a result of moving conditional logic around to accommodate the new layout. See screenshot: https://drive.google.com/open?id=0B7Fvv7JszRyGaUptUDhPcTh6UVk Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... This CL is dependent on https://codereview.chromium.org/2023093002/. BUG=615893 ==========
janeliulwq@google.com changed reviewers: + rogerta@chromium.org
janeliulwq@google.com changed required reviewers: + rogerta@chromium.org
lgtm https://codereview.chromium.org/2052473003/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2052473003/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1495: const int content_width = |content_width| seems to be used only at line 1582 below. Maybe move this declaration closer to there.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. Most of the changes in CreateMaterialDesignCurrentProfileView was a result of moving conditional logic around to accommodate the new layout. See screenshot: https://drive.google.com/open?id=0B7Fvv7JszRyGaUptUDhPcTh6UVk Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... This CL is dependent on https://codereview.chromium.org/2023093002/. BUG=615893 ========== to ========== Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. Most of the changes in CreateMaterialDesignCurrentProfileView was a result of moving conditional logic around to accommodate the new layout. See screenshot (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... This CL is dependent on https://codereview.chromium.org/2023093002/. BUG=615893 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. Most of the changes in CreateMaterialDesignCurrentProfileView was a result of moving conditional logic around to accommodate the new layout. See screenshot (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... This CL is dependent on https://codereview.chromium.org/2023093002/. BUG=615893 ========== to ========== Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. Most of the changes in CreateMaterialDesignCurrentProfileView was a result of moving conditional logic around to accommodate the new layout. See screenshot (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... BUG=615893 ==========
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Description was changed from ========== Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. Most of the changes in CreateMaterialDesignCurrentProfileView was a result of moving conditional logic around to accommodate the new layout. See screenshot (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... BUG=615893 ========== to ========== Reflow of user menu's active profile card into a button Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. See screenshots (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... Upstream CL (landed): https://codereview.chromium.org/2023093002/ BUG=615893 ==========
janeliulwq@google.com changed reviewers: + sky@chromium.org
janeliulwq@google.com changed required reviewers: + sky@chromium.org
Thanks!
https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:201: SetMinSize(gfx::Size(0, Are you sure you don't want to include the size of the icon in the min size? Not doing that means the image could be clipped. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:243: bool CanProcessEventsWithinSubtree() const override { return false; } nit: generally sections that override functions have a comment to indicate that. See line 209 (new) for what I mean. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:244: }; Good practice is to have private: DISALLOW_... (see line 219 for what I mean). https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1038: PostActionPerformed(ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_NAME); Are you intentionally calling two PostActionPerformed? If so, what is the effects of resetting gaia_service_type_ when PostActionPerformed is called? https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1559: (&ui::ResourceBundle::GetSharedInstance()) Remove the (&...)-> and replace with ., eg ui::ResourceBundle::GetSharedInstance().GetFont... https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1580: gfx::Size(0, current_profile_photo->GetPreferredSize().height() + Similar comment about width sizing. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1599: manage_accounts_button_->SetMinSize(gfx::Size(0, kButtonHeight)); And here.
Thanks! https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:201: SetMinSize(gfx::Size(0, On 2016/06/30 22:00:40, sky wrote: > Are you sure you don't want to include the size of the icon in the min size? Not > doing that means the image could be clipped. Done. This line is from line 191 so I thought it's ok to do so, but it's probably safer with a defined width. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:243: bool CanProcessEventsWithinSubtree() const override { return false; } On 2016/06/30 22:00:41, sky wrote: > nit: generally sections that override functions have a comment to indicate that. > See line 209 (new) for what I mean. Done. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:244: }; On 2016/06/30 22:00:41, sky wrote: > Good practice is to have private: DISALLOW_... (see line 219 for what I mean). Done. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1038: PostActionPerformed(ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_NAME); On 2016/06/30 22:00:40, sky wrote: > Are you intentionally calling two PostActionPerformed? If so, what is the > effects of resetting gaia_service_type_ when PostActionPerformed is called? Yes, I'm intentionally calling them. My understanding was that since current_profile_card_ will now be the entry point to editing profile image and name, these actions should be logged just like in line 1034 and line 1161 (this patch). I took a closer look at gaia_service_type_, and it looks like to me that it's not really used in this class at all. If you think it's safe to do so, I can just remove this member variable. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1559: (&ui::ResourceBundle::GetSharedInstance()) On 2016/06/30 22:00:40, sky wrote: > Remove the (&...)-> and replace with ., eg > ui::ResourceBundle::GetSharedInstance().GetFont... Done. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1580: gfx::Size(0, current_profile_photo->GetPreferredSize().height() + On 2016/06/30 22:00:41, sky wrote: > Similar comment about width sizing. Done. https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1599: manage_accounts_button_->SetMinSize(gfx::Size(0, kButtonHeight)); On 2016/06/30 22:00:41, sky wrote: > And here. Done.
https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2052473003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1038: PostActionPerformed(ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_NAME); On 2016/07/01 03:22:54, Jane wrote: > On 2016/06/30 22:00:40, sky wrote: > > Are you intentionally calling two PostActionPerformed? If so, what is the > > effects of resetting gaia_service_type_ when PostActionPerformed is called? > > Yes, I'm intentionally calling them. My understanding was that since > current_profile_card_ will now be the entry point to editing profile image and > name, these actions should be logged just like in line 1034 and line 1161 (this > patch). > I took a closer look at gaia_service_type_, and it looks like to me that it's > not really used in this class at all. If you think it's safe to do so, I can > just remove this member variable. As gaia_service_type_ is reset on line 2159 new the effect of calling PostActionPerformed twice is: ProfileMetrics::LogProfileDesktopMenu(signin::GAIAServiceType supplied to constructor) ProfileMetrics::LogProfileDesktopMenu(signin::GAIA_SERVICE_TYPE_NONE) LogProfileDesktopMenu uses the service type. I'm not familiar with why gaia_service_type_ is reset on line 2159, that seems wrong. Can you ask Roger about this? Seems to me line 2159 should be removed, but I could certainly be missing something.
Hi Mike, Scott's previous comment raised a question about why gaia_service_type_ is reset on line 2159 in ProfileChooserView::PostActionPerformed(ProfileMetrics::ProfileDesktopMenu action_performed). I'm wondering if you have an answer for us since you added that function originally. The question was raised because we need to know whether it's ok to call this function twice in a roll, now that we are consolidating profile icon editing and name editing into one button. Alternatively, would it be better practice to just create a new action enum? Thanks, Jane
On 2016/07/01 19:56:51, Jane wrote: > Hi Mike, > Scott's previous comment raised a question about why gaia_service_type_ is reset > on line 2159 in > ProfileChooserView::PostActionPerformed(ProfileMetrics::ProfileDesktopMenu > action_performed). I'm wondering if you have an answer for us since you added > that function originally. > The question was raised because we need to know whether it's ok to call this > function twice in a roll, now that we are consolidating profile icon editing and > name editing into one button. Alternatively, would it be better practice to just > create a new action enum? > Thanks, > Jane The idea is that we want to track whether the user performed the action GAIA thought they were going to perform as their first action. e.g. If GAIA has triggered the browser to open the User Menu because the user indicated in the content area that they wanted to Add a User, vs. Go Incognito, vs. Sign Out... did they actually perform that as their first action? The best way to keep to that original intent would be to leave the CL as you've written it: That multiple calls to PostActionPerformed will not all have the same GaiaServiceType. An interesting sidepoint is that it doesn't matter much, since there's no platform where the ProfileChooserView is used where AccountConsistency is actually enabled. This means that, although you'd want to chat with Roger (about the plans for Account Consistency on desktop) and Ganggui (who's currently doing all the things metrics) about just removing the GaiaServiceType parameter here; I expect it's basically always NO_SERVICE_TYPE.
On 2016/07/04 12:38:20, Mike Lerman wrote: > On 2016/07/01 19:56:51, Jane wrote: > > Hi Mike, > > Scott's previous comment raised a question about why gaia_service_type_ is > reset > > on line 2159 in > > ProfileChooserView::PostActionPerformed(ProfileMetrics::ProfileDesktopMenu > > action_performed). I'm wondering if you have an answer for us since you added > > that function originally. > > The question was raised because we need to know whether it's ok to call this > > function twice in a roll, now that we are consolidating profile icon editing > and > > name editing into one button. Alternatively, would it be better practice to > just > > create a new action enum? > > Thanks, > > Jane > > The idea is that we want to track whether the user performed the action GAIA > thought they were going to perform as their first action. e.g. If GAIA has > triggered the browser to open the User Menu because the user indicated in the > content area that they wanted to Add a User, vs. Go Incognito, vs. Sign Out... > did they actually perform that as their first action? > > The best way to keep to that original intent would be to leave the CL as you've > written it: That multiple calls to PostActionPerformed will not all have the > same GaiaServiceType. > > An interesting sidepoint is that it doesn't matter much, since there's no > platform where the ProfileChooserView is used where AccountConsistency is > actually enabled. This means that, although you'd want to chat with Roger (about > the plans for Account Consistency on desktop) and Ganggui (who's currently doing > all the things metrics) about just removing the GaiaServiceType parameter here; > I expect it's basically always NO_SERVICE_TYPE. Thanks for the detailed explanation Mike! Roger previously told me that there is a plan to enable account consistency on desktop starting from enterprise users, so I'm guessing we shouldn't remove the GaiaServiceType parameter right now. I will talk to Roger later and make sure that is the case. Meanwhile, Scott, would you agree that I should leave the CL as is then? Thanks!
Yes, LGTM
The CQ bit was checked by janeliulwq@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2052473003/#ps180001 (title: "Addressed comments from sky")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Reflow of user menu's active profile card into a button Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. See screenshots (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... Upstream CL (landed): https://codereview.chromium.org/2023093002/ BUG=615893 ========== to ========== Reflow of user menu's active profile card into a button Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. See screenshots (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... Upstream CL (landed): https://codereview.chromium.org/2023093002/ BUG=615893 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Reflow of user menu's active profile card into a button Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. See screenshots (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... Upstream CL (landed): https://codereview.chromium.org/2023093002/ BUG=615893 ========== to ========== Reflow of user menu's active profile card into a button Reflowed the top section of the user menu (i.e. where the profile icon and avatar name are displayed) into a button that leads to chrome://settings/manageProfile. See screenshots (marked with 4.*: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... Upstream CL (landed): https://codereview.chromium.org/2023093002/ BUG=615893 Committed: https://crrev.com/478d83e5b642df214eea6a01d8a3a46b6b44cf1c Cr-Commit-Position: refs/heads/master@{#403919} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/478d83e5b642df214eea6a01d8a3a46b6b44cf1c Cr-Commit-Position: refs/heads/master@{#403919} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
