Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151513002:
Revamped signin/sync error surfacing on desktop user menu (Closed)
DescriptionRevamped signin/sync error surfacing on desktop user menu
This is for material design user menu on windows/linux only.
Specifically:
1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMkLc/edit#heading=h.o6ftotuznu1h
3. Removed the blue tutorial headers for the user menu because the existing tutorial modes will be either no longer applicable in MD or migrated into another UI, and we are not planning to add new modes.
Other useful links:
- The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=1468394226&l=151
- Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view
- Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20Sign%20In/user_menu#%2Fpreview-3.png (Don't refer to the strings there)
- Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnqik/edit?ts=57445a70#heading=h.6xrfx2rucezz
BUG=615893
Committed: https://crrev.com/8377f1c3a69342cdedf56368e25387d3fcaad2ec
Cr-Commit-Position: refs/heads/master@{#406476}
Patch Set 1 #
Total comments: 73
Patch Set 2 : Rebased and addressed comments #
Total comments: 16
Patch Set 3 : More comments and removed multiple inheritance #Patch Set 4 : Moved a comment around #
Total comments: 14
Patch Set 5 : Nits #
Total comments: 1
Patch Set 6 : Addressed rogerta's last comment #
Messages
Total messages: 40 (23 generated)
Description was changed from ========== Revamped signin/sync error surfacing on desktop user menu This is for material design user menu on windows/linux only. Specifically: 1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI); 2. Funnel all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... Other useful links: - The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=... - Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Don't refer to the strings there) - Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG=615893 ========== to ========== Revamped signin/sync error surfacing on desktop user menu This is for material design user menu on windows/linux only. Specifically: 1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI); 2. Funneled all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... Other useful links: - The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=... - Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view - Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Don't refer to the strings there) - Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG=615893 ==========
janeliulwq@google.com changed reviewers: + msw@chromium.org
janeliulwq@google.com changed required reviewers: + msw@chromium.org
Hi Mike, PTAL. Thanks!
janeliulwq@google.com changed reviewers: - msw@chromium.org
janeliulwq@google.com changed required reviewers: - msw@chromium.org
janeliulwq@google.com changed reviewers: + msw@chromium.org
janeliulwq@google.com changed required reviewers: + msw@chromium.org
Hi Mike, sorry about the confusion. False alarm - patch #1 is still good, PTAL. Thanks!
Mostly minor comments. https://codereview.chromium.org/2151513002/diff/1/chrome/app/chromium_strings... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/2151513002/diff/1/chrome/app/chromium_strings... chrome/app/chromium_strings.grd:1115: Chromium is out of date. It's unfortunate that we are adding a fourth "Chromium is out of date" string and a second "Update Chromium" string; it'd be nice if they were shared. https://codereview.chromium.org/2151513002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2151513002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11517: + <message name="IDS_SYNC_ERROR_USER_MENU_LOGIN_MESSAGE" desc="Message of the out-of-date login info error in the header of desktop user menu."> nit: replace 'LOGIN' with 'SIGNIN' in ID and comment; ditto below. https://codereview.chromium.org/2151513002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11520: + <message name="IDS_SYNC_ERROR_USER_MENU_LOGIN_BUTTON" desc="Button in the header of desktop user menu that prompts the user to sign in again to fix the out-of-date login info error"> nit: trailing period for desc string https://codereview.chromium.org/2151513002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11532: + <message name="IDS_SYNC_ERROR_USER_MENU_SIGNIN_AGAIN_BUTTON" desc="Button in the header of desktop user menu that signs out and signs back in in an attempt to resolve unrecoverable error."> nit: "resolve an" or "errors"; ditto below. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:113: SyncErrorController* sync_error_controller = NULL; nit: nullptr https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:114: ProfileSyncService* sync_service = Add a file-local GetSyncErrorController helper instead of repeating this 3x. (I guess the third time will duplicate the code to get the ProfileSyncService) https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:116: if (switches::IsMaterialDesignUserMenu() && sync_service) { nit: curlies not needed https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:121: if (signin_error_controller) nit: move these two conditionals above the either-or conditional block. (optionally keep each conditional AddObserver with the respective init) https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:201: // If there is an signin error, show an warning icon. nit: "a signin" and "a warning" https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:207: // Also show warning icon for sync error for material design user menu. nit: "a warning", "sync errors", and "for the material" https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:215: if (sync_service->HasUnrecoverableError() || nit: has_sync_error_ |= ... https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:48: // SigninErrorController::Observer and SyncErrorController::Observer This is bad practice... maybe rename one or both functions? https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:794: sync_error_signin_button_ = nullptr; nit: order to match the decl of all these members. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:883: if (!active_item.signed_in) Add curlies here or move the comment above the if block. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:885: // menu upon encountering unrecoverable error. Then following the action, nit: "errors" https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:886: // profile chooser view would be shown instead of account management view. nit: "Afterwards, the profile chooser view is shown instead of the account management view." or similar. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1219: if (!switches::IsMaterialDesignUserMenu()) Do we really not want tutorial_view in MD? Call this out in the CL desc? https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1224: if (switches::IsMaterialDesignUserMenu()) { nit: curlies not needed https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1482: // Unrecoverable error is sometimes accompanied by actionable error. nit: "An unrecoverable" and "an actionable" https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1483: // If actionable error is not set to be UPGRADE_CLIENT, then show generic nit: "an actionable" and "show a generic" https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1488: // Display different error messages and buttons depending on whether it nit: "Display different messages and buttons for managed accounts." https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1494: IDS_SYNC_ERROR_USER_MENU_SIGNOUT_MESSAGE), Did you flip the intended values here? You're using SIGNOUT strings when IsSignoutProhibited returns true. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1498: } else { nit: no else after return. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1519: // Check for an actionable error UPGRADE_CLIENT. nit: "actionable UPGRADE_CLIENT error"? https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1546: const base::string16& content_text, nit: take a resource ID here instead of the string; ditto for button_text. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1548: views::LabelButton** button) { nit: button_out... it'd be nice if we used |views::View::id_| values instead of testing for pointer equality, but that's outside the scope of this CL. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1564: // Adds a vertical view to organize the error title, message, and button. nit: GridLayout *might* be simpler than nesting BoxLayout, but maybe not. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1576: title_label->SetFontList(gfx::FontList().DeriveWithSizeDelta(1)); I doubt that a 1px Font size difference matters here; skip this. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1582: if (!content_text.empty()) { This text should never be empty; DCHECK instead or just assume it's valid? https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1591: gfx::Size(GetFixedMenuWidth() - 2 * kMaterialMenuEdgeMargin, nit: I bet a 0 width would be okay here... https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1595: // Adds action_button. nit: "an action button"; there is no |action_button| identifier. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.h:218: views::LabelButton** button); nit: |button_out|? https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.h:247: // Buttons in the signin/sync error header on top of the desktop user menu. optional nit: move these below auth_error_email_button_
Description was changed from ========== Revamped signin/sync error surfacing on desktop user menu This is for material design user menu on windows/linux only. Specifically: 1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI); 2. Funneled all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... Other useful links: - The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=... - Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view - Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Don't refer to the strings there) - Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG=615893 ========== to ========== Revamped signin/sync error surfacing on desktop user menu This is for material design user menu on windows/linux only. Specifically: 1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI); 2. Funneled all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... 3. Removed the blue tutorial headers for the user menu because the existing tutorial modes will be either no longer applicable in MD or migrated into another UI, and we are not planning to add new modes. Other useful links: - The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=... - Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view - Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Don't refer to the strings there) - Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG=615893 ==========
Thanks for the super detailed review! I definitely need to pay more attention to details. PTAL, thanks! https://codereview.chromium.org/2151513002/diff/1/chrome/app/chromium_strings... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/2151513002/diff/1/chrome/app/chromium_strings... chrome/app/chromium_strings.grd:1115: Chromium is out of date. On 2016/07/13 22:31:37, msw wrote: > It's unfortunate that we are adding a fourth "Chromium is out of date" string > and a second "Update Chromium" string; it'd be nice if they were shared. I noticed that too, but it seemed that different UI components keep a different set of the strings. Let me know if you think it's better to just use the upgrade recovery bubble strings for the user menu. https://codereview.chromium.org/2151513002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2151513002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11517: + <message name="IDS_SYNC_ERROR_USER_MENU_LOGIN_MESSAGE" desc="Message of the out-of-date login info error in the header of desktop user menu."> On 2016/07/13 22:31:37, msw wrote: > nit: replace 'LOGIN' with 'SIGNIN' in ID and comment; ditto below. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11520: + <message name="IDS_SYNC_ERROR_USER_MENU_LOGIN_BUTTON" desc="Button in the header of desktop user menu that prompts the user to sign in again to fix the out-of-date login info error"> On 2016/07/13 22:31:37, msw wrote: > nit: trailing period for desc string Done. https://codereview.chromium.org/2151513002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11532: + <message name="IDS_SYNC_ERROR_USER_MENU_SIGNIN_AGAIN_BUTTON" desc="Button in the header of desktop user menu that signs out and signs back in in an attempt to resolve unrecoverable error."> On 2016/07/13 22:31:37, msw wrote: > nit: "resolve an" or "errors"; ditto below. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:113: SyncErrorController* sync_error_controller = NULL; On 2016/07/13 22:31:37, msw wrote: > nit: nullptr Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:114: ProfileSyncService* sync_service = On 2016/07/13 22:31:37, msw wrote: > Add a file-local GetSyncErrorController helper instead of repeating this 3x. > (I guess the third time will duplicate the code to get the ProfileSyncService) Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:116: if (switches::IsMaterialDesignUserMenu() && sync_service) { On 2016/07/13 22:31:37, msw wrote: > nit: curlies not needed Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:121: if (signin_error_controller) On 2016/07/13 22:31:37, msw wrote: > nit: move these two conditionals above the either-or conditional block. > (optionally keep each conditional AddObserver with the respective init) Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:201: // If there is an signin error, show an warning icon. On 2016/07/13 22:31:37, msw wrote: > nit: "a signin" and "a warning" Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:207: // Also show warning icon for sync error for material design user menu. On 2016/07/13 22:31:37, msw wrote: > nit: "a warning", "sync errors", and "for the material" Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:215: if (sync_service->HasUnrecoverableError() || On 2016/07/13 22:31:37, msw wrote: > nit: has_sync_error_ |= ... Done. Is this what you mean? https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:48: // SigninErrorController::Observer and SyncErrorController::Observer On 2016/07/13 22:31:37, msw wrote: > This is bad practice... maybe rename one or both functions? Do you mean changing the function name "OnErrorChanged()" in SigninErrorController and/or SyncErrorController? I thought they are by convention named this way, and also, changing the name would involve changes in all the observers, right? Do you think it's worth the change? https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:794: sync_error_signin_button_ = nullptr; On 2016/07/13 22:31:38, msw wrote: > nit: order to match the decl of all these members. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:883: if (!active_item.signed_in) On 2016/07/13 22:31:38, msw wrote: > Add curlies here or move the comment above the if block. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:885: // menu upon encountering unrecoverable error. Then following the action, On 2016/07/13 22:31:37, msw wrote: > nit: "errors" Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:886: // profile chooser view would be shown instead of account management view. On 2016/07/13 22:31:38, msw wrote: > nit: "Afterwards, the profile chooser view is shown instead of the account > management view." or similar. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1219: if (!switches::IsMaterialDesignUserMenu()) On 2016/07/13 22:31:38, msw wrote: > Do we really not want tutorial_view in MD? Call this out in the CL desc? Yep, this is an explicit decision and is briefly mentioned in the design doc. This is because the existing tutorial modes will be either no longer applicable in MD or migrated to another UI. I updated the desc for this! https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1224: if (switches::IsMaterialDesignUserMenu()) { On 2016/07/13 22:31:38, msw wrote: > nit: curlies not needed Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1482: // Unrecoverable error is sometimes accompanied by actionable error. On 2016/07/13 22:31:38, msw wrote: > nit: "An unrecoverable" and "an actionable" Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1483: // If actionable error is not set to be UPGRADE_CLIENT, then show generic On 2016/07/13 22:31:38, msw wrote: > nit: "an actionable" and "show a generic" Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1488: // Display different error messages and buttons depending on whether it On 2016/07/13 22:31:38, msw wrote: > nit: "Display different messages and buttons for managed accounts." Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1494: IDS_SYNC_ERROR_USER_MENU_SIGNOUT_MESSAGE), On 2016/07/13 22:31:38, msw wrote: > Did you flip the intended values here? You're using SIGNOUT strings when > IsSignoutProhibited returns true. The values are not flipped. AFAIU, IsSignoutProhibited is just an indication of whether the account is managed (see https://cs.chromium.org/chromium/src/chrome/browser/policy/cloud/user_policy_...). Here, if a managed user wants to signout, we will bring out the sign out dialogue in the settings page to confirm that they want their stored data wiped; whereas for non-managed user, we will sign out on the user's behalf and bring up the signin modal. I know the message names are not too clear, but I couldn't think of anything that's not super long. Let me know if you think this could be improved somehow! https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1498: } else { On 2016/07/13 22:31:38, msw wrote: > nit: no else after return. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1519: // Check for an actionable error UPGRADE_CLIENT. On 2016/07/13 22:31:38, msw wrote: > nit: "actionable UPGRADE_CLIENT error"? Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1546: const base::string16& content_text, On 2016/07/13 22:31:38, msw wrote: > nit: take a resource ID here instead of the string; ditto for button_text. Good suggestion! Changed it. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1548: views::LabelButton** button) { On 2016/07/13 22:31:38, msw wrote: > nit: button_out... it'd be nice if we used |views::View::id_| values instead of > testing for pointer equality, but that's outside the scope of this CL. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1564: // Adds a vertical view to organize the error title, message, and button. On 2016/07/13 22:31:38, msw wrote: > nit: GridLayout *might* be simpler than nesting BoxLayout, but maybe not. Oh, right, maybe, but I thought this was pretty straightforward in itself. I'll keep it this way for now. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1576: title_label->SetFontList(gfx::FontList().DeriveWithSizeDelta(1)); On 2016/07/13 22:31:38, msw wrote: > I doubt that a 1px Font size difference matters here; skip this. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1582: if (!content_text.empty()) { On 2016/07/13 22:31:38, msw wrote: > This text should never be empty; DCHECK instead or just assume it's valid? Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1591: gfx::Size(GetFixedMenuWidth() - 2 * kMaterialMenuEdgeMargin, On 2016/07/13 22:31:38, msw wrote: > nit: I bet a 0 width would be okay here... Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1595: // Adds action_button. On 2016/07/13 22:31:38, msw wrote: > nit: "an action button"; there is no |action_button| identifier. Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.h:218: views::LabelButton** button); On 2016/07/13 22:31:39, msw wrote: > nit: |button_out|? Done. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.h:247: // Buttons in the signin/sync error header on top of the desktop user menu. On 2016/07/13 22:31:39, msw wrote: > optional nit: move these below auth_error_email_button_ I put them here because this follows the flow in the code as well as the layout (i.e. header sits above the profile card) so I think I will leave them here!
msw@chromium.org changed reviewers: + rogerta@chromium.org
msw@chromium.org changed required reviewers: - msw@chromium.org
+Roger for managed account question and related account/sync review. (Roger, please pass this along to someone else if that's appropriate) https://codereview.chromium.org/2151513002/diff/1/chrome/app/chromium_strings... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/2151513002/diff/1/chrome/app/chromium_strings... chrome/app/chromium_strings.grd:1115: Chromium is out of date. On 2016/07/14 00:18:57, Jane wrote: > On 2016/07/13 22:31:37, msw wrote: > > It's unfortunate that we are adding a fourth "Chromium is out of date" string > > and a second "Update Chromium" string; it'd be nice if they were shared. > > I noticed that too, but it seemed that different UI components keep a different > set of the strings. Let me know if you think it's better to just use the upgrade > recovery bubble strings for the user menu. No, if anything, we'd want to consolidate them to IDS_OUT_OF_DATE and IDS_UPDATE or similar, and that's outside the scope of this CL; this is fine for now, I just like to complain sometimes. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:48: // SigninErrorController::Observer and SyncErrorController::Observer On 2016/07/14 00:18:57, Jane wrote: > On 2016/07/13 22:31:37, msw wrote: > > This is bad practice... maybe rename one or both functions? > > Do you mean changing the function name "OnErrorChanged()" in > SigninErrorController and/or SyncErrorController? I thought they are by > convention named this way, and also, changing the name would involve changes in > all the observers, right? Do you think it's worth the change? Yes, renaming the interfaces' OnErrorChanged was my suggestion. I think naming collisions (like this) is one of the reasons we discourage multiple inheritance in our style guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... The Google C++ style guide is even more strict about inheritance. I don't see guidance on this particular practice, but I believe it's highly discouraged. That said, I can think of two other ways forward: 1) Get a second opinion from someone knowledgable saying this is okay as-is. 2) Use composition instead of inheritance here. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1219: if (!switches::IsMaterialDesignUserMenu()) On 2016/07/14 00:18:58, Jane wrote: > On 2016/07/13 22:31:38, msw wrote: > > Do we really not want tutorial_view in MD? Call this out in the CL desc? > > Yep, this is an explicit decision and is briefly mentioned in the design doc. > This is because the existing tutorial modes will be either no longer applicable > in MD or migrated to another UI. > I updated the desc for this! Acknowledged. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1494: IDS_SYNC_ERROR_USER_MENU_SIGNOUT_MESSAGE), On 2016/07/14 00:18:58, Jane wrote: > On 2016/07/13 22:31:38, msw wrote: > > Did you flip the intended values here? You're using SIGNOUT strings when > > IsSignoutProhibited returns true. > > The values are not flipped. AFAIU, IsSignoutProhibited is just an indication of > whether the account is managed (see > https://cs.chromium.org/chromium/src/chrome/browser/policy/cloud/user_policy_...). > Here, if a managed user wants to signout, we will bring out the sign out > dialogue in the settings page to confirm that they want their stored data wiped; > whereas for non-managed user, we will sign out on the user's behalf and bring up > the signin modal. > I know the message names are not too clear, but I couldn't think of anything > that's not super long. Let me know if you think this could be improved somehow! This seems highly counter-intuitive, and deserves a comment at the least. Otherwise, it looks like "if signout is prohibited, suggest that the user sign out...". There's likely a better way to check for a managed account (UserManagerBase::IsEnterpriseManaged(), User::has_gaia_account(), etc...), if that's really the intent and what's appropriate here. This CL needs review from someone more knowledgable about sync/managed accounts... Hopefully Roger can do that review, or point us at a good reviewer. https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.h:247: // Buttons in the signin/sync error header on top of the desktop user menu. On 2016/07/14 00:18:58, Jane wrote: > On 2016/07/13 22:31:39, msw wrote: > > optional nit: move these below auth_error_email_button_ > > I put them here because this follows the flow in the code as well as the layout > (i.e. header sits above the profile card) so I think I will leave them here! Acknowledged. https://codereview.chromium.org/2151513002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:11501: + <message name="IDS_SYNC_ERROR_USER_MENU_SIGNIN_BUTTON" desc="Button in the header of desktop user menu that prompts the user to sign in again to fix the out-of-date signinin info error."> nit: fix 'signinin' https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:61: return nullptr; nit: instead consider this (to avoid service lookup when not needed): if (!switches::IsMaterialDesignUserMenu()) return nullptr ProfileSyncService* sync_service = ProfileSyncServiceFactory::GetForProfile(profile); return sync_service ? sync_service->sync_error_controller() : nullptr; https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:218: has_sync_error_ |= true; Sorry, what I meant was remove the if and unconditionally do: has_sync_error_ |= (sync_service->HasUnrecoverableError() || status.sync_protocol_error.action == syncer::UPGRADE_CLIENT || (sync_error_controller && sync_error_controller->HasError())); Otherwise, restore this to has_sync_error_ |= true; and just add curly braces. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1491: ->IsSignoutProhibited()) { nit: fix closing curly (missing?) https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.h:216: views::View* CreateSyncErrorView(const int content_msg_id, nit: *_string_id here and below (and with the definition)
Looks good Jane. A few questions below. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:60: bool has_sync_error_; Should this be renamed to |has_error| instead? I say this because this code sets this value to true for signin/auth error as well as sync error. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1024: } else if (sender == sync_error_signin_again_button_) { For my own info, what sync error conditions are so bad that the only way to fix is to force the user to be signed out of chrome? This just seems a very drastic measure to fix a problem so just curious. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1490: if (SigninManagerFactory::GetForProfile(browser_->profile()) I'm not sure I understand the check for IsSignoutProhibited(). As the code is written, whether this returns true or false leads to a sign out, either manual or automatic. Is the closing } brace at the wrong line? Move from 1499 to 1495?
https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1490: if (SigninManagerFactory::GetForProfile(browser_->profile()) On 2016/07/14 14:14:00, Roger Tawa wrote: > I'm not sure I understand the check for IsSignoutProhibited(). As the code is > written, whether this returns true or false leads to a sign out, either manual > or automatic. > > Is the closing } brace at the wrong line? Move from 1499 to 1495? Jane and I discussed off thread and I get it now. Code is good.
Patchset #3 (id:40001) has been deleted
Thanks! https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:48: // SigninErrorController::Observer and SyncErrorController::Observer On 2016/07/14 02:23:33, msw wrote: > On 2016/07/14 00:18:57, Jane wrote: > > On 2016/07/13 22:31:37, msw wrote: > > > This is bad practice... maybe rename one or both functions? > > > > Do you mean changing the function name "OnErrorChanged()" in > > SigninErrorController and/or SyncErrorController? I thought they are by > > convention named this way, and also, changing the name would involve changes > in > > all the observers, right? Do you think it's worth the change? > > Yes, renaming the interfaces' OnErrorChanged was my suggestion. > I think naming collisions (like this) is one of the reasons we discourage > multiple inheritance in our style guide: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > The Google C++ style guide is even more strict about inheritance. > I don't see guidance on this particular practice, but I believe it's highly > discouraged. > > That said, I can think of two other ways forward: > 1) Get a second opinion from someone knowledgable saying this is okay as-is. > 2) Use composition instead of inheritance here. I discussed with Roger about this, and he suggested adding two subclasses to each take one observer, and call the OnErrorChanged() function of the parent class when there is an error. This is a smaller code change than renaming functions and solves the multiple inheritance problem. Let me know if you agree that this is ok! https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2151513002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1494: IDS_SYNC_ERROR_USER_MENU_SIGNOUT_MESSAGE), On 2016/07/14 02:23:33, msw wrote: > On 2016/07/14 00:18:58, Jane wrote: > > On 2016/07/13 22:31:38, msw wrote: > > > Did you flip the intended values here? You're using SIGNOUT strings when > > > IsSignoutProhibited returns true. > > > > The values are not flipped. AFAIU, IsSignoutProhibited is just an indication > of > > whether the account is managed (see > > > https://cs.chromium.org/chromium/src/chrome/browser/policy/cloud/user_policy_...). > > Here, if a managed user wants to signout, we will bring out the sign out > > dialogue in the settings page to confirm that they want their stored data > wiped; > > whereas for non-managed user, we will sign out on the user's behalf and bring > up > > the signin modal. > > I know the message names are not too clear, but I couldn't think of anything > > that's not super long. Let me know if you think this could be improved > somehow! > > This seems highly counter-intuitive, and deserves a comment at the least. > Otherwise, it looks like "if signout is prohibited, suggest that the user sign > out...". There's likely a better way to check for a managed account > (UserManagerBase::IsEnterpriseManaged(), User::has_gaia_account(), etc...), if > that's really the intent and what's appropriate here. This CL needs review from > someone more knowledgable about sync/managed accounts... Hopefully Roger can do > that review, or point us at a good reviewer. So Roger said this is the right function to use. I added some comments here explaining the behavior, let me know if this is good! https://codereview.chromium.org/2151513002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:11501: + <message name="IDS_SYNC_ERROR_USER_MENU_SIGNIN_BUTTON" desc="Button in the header of desktop user menu that prompts the user to sign in again to fix the out-of-date signinin info error."> On 2016/07/14 02:23:34, msw wrote: > nit: fix 'signinin' Done. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:61: return nullptr; On 2016/07/14 02:23:34, msw wrote: > nit: instead consider this (to avoid service lookup when not needed): > if (!switches::IsMaterialDesignUserMenu()) > return nullptr > ProfileSyncService* sync_service = > ProfileSyncServiceFactory::GetForProfile(profile); > return sync_service ? sync_service->sync_error_controller() : nullptr; Done. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:218: has_sync_error_ |= true; On 2016/07/14 02:23:34, msw wrote: > Sorry, what I meant was remove the if and unconditionally do: > has_sync_error_ |= (sync_service->HasUnrecoverableError() || > status.sync_protocol_error.action == syncer::UPGRADE_CLIENT || > (sync_error_controller && sync_error_controller->HasError())); > Otherwise, restore this to has_sync_error_ |= true; and just add curly braces. Done. Ah, I see. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:60: bool has_sync_error_; On 2016/07/14 14:14:00, Roger Tawa wrote: > Should this be renamed to |has_error| instead? I say this because this code > sets this value to true for signin/auth error as well as sync error. Done. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1024: } else if (sender == sync_error_signin_again_button_) { On 2016/07/14 14:14:00, Roger Tawa wrote: > For my own info, what sync error conditions are so bad that the only way to fix > is to force the user to be signed out of chrome? This just seems a very drastic > measure to fix a problem so just curious. We talked off the thread about this. The doc is here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1491: ->IsSignoutProhibited()) { On 2016/07/14 02:23:34, msw wrote: > nit: fix closing curly (missing?) Done. https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/2151513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.h:216: views::View* CreateSyncErrorView(const int content_msg_id, On 2016/07/14 02:23:34, msw wrote: > nit: *_string_id here and below (and with the definition) Done.
Okay, thanks for reviewing, Roger! Just some minor nits left on my side. https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:170: OnErrorChanged(); // This calls Update() nit: Just always call OnErrorChanged here and remove the checks and alternative call to Update. (OnErrorChanged does the right thing either way). https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:28: // SigninErrorController::Observer nit: trailing colon https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:33: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:41: // SyncErrorController::Observer nit: trailing colon https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:46: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:78: SigninErrorObserver* signin_error_observer_; nit: std::unique_ptr https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:79: SyncErrorObserver* sync_error_observer_; nit: std::unique_ptr
Thanks! https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:170: OnErrorChanged(); // This calls Update() On 2016/07/14 17:47:52, msw wrote: > nit: Just always call OnErrorChanged here and remove the checks and alternative > call to Update. (OnErrorChanged does the right thing either way). Done. https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:28: // SigninErrorController::Observer On 2016/07/14 17:47:52, msw wrote: > nit: trailing colon Done. https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:33: }; On 2016/07/14 17:47:53, msw wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:41: // SyncErrorController::Observer On 2016/07/14 17:47:52, msw wrote: > nit: trailing colon Done. https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:46: }; On 2016/07/14 17:47:53, msw wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:78: SigninErrorObserver* signin_error_observer_; On 2016/07/14 17:47:53, msw wrote: > nit: std::unique_ptr Done. https://codereview.chromium.org/2151513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:79: SyncErrorObserver* sync_error_observer_; On 2016/07/14 17:47:53, msw wrote: > nit: std::unique_ptr Done.
lgtm
The CQ bit was checked by janeliulwq@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm, one comment below. https://codereview.chromium.org/2151513002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2151513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/new_avatar_button.h:83: std::unique_ptr<SyncErrorObserver> sync_error_observer_; I think you can just write: SigninErrorObserver signin_error_observer_; SyncErrorObserver sync_error_observer_; no need for memory allocation.
The CQ bit was checked by janeliulwq@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by janeliulwq@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2151513002/#ps120001 (title: "Addressed rogerta's last comment")
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 ========== Revamped signin/sync error surfacing on desktop user menu This is for material design user menu on windows/linux only. Specifically: 1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI); 2. Funneled all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... 3. Removed the blue tutorial headers for the user menu because the existing tutorial modes will be either no longer applicable in MD or migrated into another UI, and we are not planning to add new modes. Other useful links: - The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=... - Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view - Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Don't refer to the strings there) - Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG=615893 ========== to ========== Revamped signin/sync error surfacing on desktop user menu This is for material design user menu on windows/linux only. Specifically: 1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI); 2. Funneled all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... 3. Removed the blue tutorial headers for the user menu because the existing tutorial modes will be either no longer applicable in MD or migrated into another UI, and we are not planning to add new modes. Other useful links: - The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=... - Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view - Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Don't refer to the strings there) - Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG=615893 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Revamped signin/sync error surfacing on desktop user menu This is for material design user menu on windows/linux only. Specifically: 1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI); 2. Funneled all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... 3. Removed the blue tutorial headers for the user menu because the existing tutorial modes will be either no longer applicable in MD or migrated into another UI, and we are not planning to add new modes. Other useful links: - The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=... - Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view - Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Don't refer to the strings there) - Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG=615893 ========== to ========== Revamped signin/sync error surfacing on desktop user menu This is for material design user menu on windows/linux only. Specifically: 1. Created a new UI for error surfacing that sits above the user menu (instead of the current email badging UI); 2. Funneled all signin- and sync-related errors through this UI: unrecoverable errors, auth errors, out-of-date-client error, and passphrase error. See the detailed list of errors and their designed behavior here: https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk... 3. Removed the blue tutorial headers for the user menu because the existing tutorial modes will be either no longer applicable in MD or migrated into another UI, and we are not planning to add new modes. Other useful links: - The error detection logic roughly follows the error surfacing mechanism on the settings page in this function: https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=... - Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view - Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Don't refer to the strings there) - Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG=615893 Committed: https://crrev.com/8377f1c3a69342cdedf56368e25387d3fcaad2ec Cr-Commit-Position: refs/heads/master@{#406476} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8377f1c3a69342cdedf56368e25387d3fcaad2ec Cr-Commit-Position: refs/heads/master@{#406476} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
