Revamped signin/sync error surfacing on desktop user menu

chrome/browser/ui/views/profiles/profile_chooser_view.cc

Index: chrome/browser/ui/views/profiles/profile_chooser_view.cc
diff --git a/chrome/browser/ui/views/profiles/profile_chooser_view.cc b/chrome/browser/ui/views/profiles/profile_chooser_view.cc
index cbae94c49150976daea87beb1a9ca1efbfd16644..670d5d98ba37045b7c83c5b02a26cf153b344354 100644
--- a/chrome/browser/ui/views/profiles/profile_chooser_view.cc
+++ b/chrome/browser/ui/views/profiles/profile_chooser_view.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/signin_promo.h"
 #include "chrome/browser/signin/signin_ui_util.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_dialogs.h"
@@ -36,12 +37,14 @@
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_error_controller.h"
 #include "components/signin/core/browser/signin_header_helper.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/signin/core/common/profile_management_switches.h"
+#include "components/sync_driver/sync_error_controller.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "grit/theme_resources.h"
@@ -788,6 +791,11 @@ void ProfileChooserView::ResetView() {
   tutorial_see_whats_new_button_ = nullptr;
   tutorial_not_you_link_ = nullptr;
   tutorial_learn_more_link_ = nullptr;
+  sync_error_signin_button_ = nullptr;

[msw, 2016/07/13 22:31:38]

nit: order to match the decl of all these members.

[Jane, 2016/07/14 00:18:58]

Done.

+  sync_error_passphrase_button_ = nullptr;
+  sync_error_upgrade_button_ = nullptr;
+  sync_error_signin_again_button_ = nullptr;
+  sync_error_signout_button_ = nullptr;
 }
 
 void ProfileChooserView::Init() {
@@ -872,7 +880,11 @@ void ProfileChooserView::ShowView(profiles::BubbleViewMode view_to_display,
     DCHECK(switches::IsEnableAccountConsistency());
     const AvatarMenu::Item& active_item = avatar_menu->GetItemAt(
         avatar_menu->GetActiveProfileIndex());
-    DCHECK(active_item.signed_in);
+    if (!active_item.signed_in)

[msw, 2016/07/13 22:31:38]

Add curlies here or move the comment above the if block.

[Jane, 2016/07/14 00:18:58]

Done.

+      // This is the case when the user selects the sign out option in the user
+      // menu upon encountering unrecoverable error. Then following the action,

[msw, 2016/07/13 22:31:37]

nit: "errors"

[Jane, 2016/07/14 00:18:57]

Done.

+      // profile chooser view would be shown instead of account management view.

[msw, 2016/07/13 22:31:38]

nit: "Afterwards, the profile chooser view is shown instead of the account
management view." or similar.

[Jane, 2016/07/14 00:18:58]

Done.

+      view_to_display = profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER;
   }
 
   if (browser_->profile()->IsSupervised() &&
@@ -1000,8 +1012,22 @@ void ProfileChooserView::ButtonPressed(views::Button* sender,
     PostActionPerformed(ProfileMetrics::PROFILE_DESKTOP_MENU_LOCK);
   } else if (sender == close_all_windows_button_) {
     profiles::CloseProfileWindows(browser_->profile());
-  } else if (sender == auth_error_email_button_) {
+  } else if (sender == auth_error_email_button_ ||
+             sender == sync_error_signin_button_) {
     ShowViewFromMode(profiles::BUBBLE_VIEW_MODE_GAIA_REAUTH);
+  } else if (sender == sync_error_passphrase_button_) {
+    chrome::ShowSettingsSubPage(browser_, chrome::kSyncSetupSubPage);
+  } else if (sender == sync_error_upgrade_button_) {
+    chrome::OpenUpdateChromeDialog(browser_);
+  } else if (sender == sync_error_signin_again_button_) {
+    if (ProfileSyncServiceFactory::GetForProfile(browser_->profile()))
+      ProfileSyncService::SyncEvent(ProfileSyncService::STOP_FROM_OPTIONS);
+    SigninManagerFactory::GetForProfile(browser_->profile())
+        ->SignOut(signin_metrics::USER_CLICKED_SIGNOUT_SETTINGS,
+                  signin_metrics::SignoutDelete::IGNORE_METRIC);
+    ShowViewFromMode(profiles::BUBBLE_VIEW_MODE_GAIA_SIGNIN);
+  } else if (sender == sync_error_signout_button_) {
+    chrome::ShowSettingsSubPage(browser_, chrome::kSignOutSubPage);
   } else if (sender == tutorial_sync_settings_ok_button_) {
     LoginUIServiceFactory::GetForProfile(browser_->profile())->
         SyncConfirmationUIClosed(LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
@@ -1175,6 +1201,7 @@ void ProfileChooserView::PopulateCompleteProfileChooserView(
   // Separate items into active and alternatives.
   Indexes other_profiles;
   views::View* tutorial_view = NULL;
+  views::View* sync_error_view = NULL;
   views::View* current_profile_view = NULL;
   views::View* current_profile_accounts = NULL;
   views::View* option_buttons_view = NULL;
@@ -1189,10 +1216,14 @@ void ProfileChooserView::PopulateCompleteProfileChooserView(
               ? CreateMaterialDesignCurrentProfileView(item, false)
               : CreateCurrentProfileView(item, false);
       if (IsProfileChooser(view_mode_)) {
-        tutorial_view = CreateTutorialViewIfNeeded(item);
+        if (!switches::IsMaterialDesignUserMenu())

[msw, 2016/07/13 22:31:38]

Do we really not want tutorial_view in MD? Call this out in the CL desc?

[Jane, 2016/07/14 00:18:58]

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!

[msw, 2016/07/14 02:23:33]

Acknowledged.

+          tutorial_view = CreateTutorialViewIfNeeded(item);
       } else {
         current_profile_accounts = CreateCurrentProfileAccountsView(item);
       }
+      if (switches::IsMaterialDesignUserMenu()) {

[msw, 2016/07/13 22:31:38]

nit: curlies not needed

[Jane, 2016/07/14 00:18:57]

Done.

+        sync_error_view = CreateSyncErrorViewIfNeeded();
+      }
     } else {
       other_profiles.push_back(i);
     }
@@ -1206,6 +1237,13 @@ void ProfileChooserView::PopulateCompleteProfileChooserView(
     tutorial_mode_ = profiles::TUTORIAL_MODE_NONE;
   }
 
+  if (sync_error_view) {
+    layout->StartRow(1, 0);
+    layout->AddView(sync_error_view);
+    layout->StartRow(0, 0);
+    layout->AddView(new views::Separator(views::Separator::HORIZONTAL));
+  }
+
   if (!current_profile_view) {
     // Guest windows don't have an active profile.
     current_profile_view = CreateGuestProfileView();
@@ -1434,6 +1472,137 @@ views::View* ProfileChooserView::CreateTutorialView(
   return view;
 }
 
+views::View* ProfileChooserView::CreateSyncErrorViewIfNeeded() {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(browser_->profile());
+
+  // The order or priority is going to be: 1. Unrecoverable errors.
+  // 2. Auth errors. 3. Protocol errors. 4. Passphrase errors.
+  if (service && service->HasUnrecoverableError()) {
+    // Unrecoverable error is sometimes accompanied by actionable error.

[msw, 2016/07/13 22:31:38]

nit: "An unrecoverable" and "an actionable"

[Jane, 2016/07/14 00:18:58]

Done.

+    // If actionable error is not set to be UPGRADE_CLIENT, then show generic

[msw, 2016/07/13 22:31:38]

nit: "an actionable" and "show a generic"

[Jane, 2016/07/14 00:18:58]

Done.

+    // unrecoverable error message.
+    ProfileSyncService::Status status;
+    service->QueryDetailedSyncStatus(&status);
+    if (status.sync_protocol_error.action != syncer::UPGRADE_CLIENT) {
+        // Display different error messages and buttons depending on whether it

[msw, 2016/07/13 22:31:38]

nit: "Display different messages and buttons for managed accounts."

[Jane, 2016/07/14 00:18:58]

Done.

+        // is a managed account.
+        if (SigninManagerFactory::GetForProfile(browser_->profile())
+            ->IsSignoutProhibited()) {
+          return CreateSyncErrorView(
+              l10n_util::GetStringUTF16(
+                  IDS_SYNC_ERROR_USER_MENU_SIGNOUT_MESSAGE),

[msw, 2016/07/13 22:31:38]

Did you flip the intended values here? You're using SIGNOUT strings when
IsSignoutProhibited returns true.

[Jane, 2016/07/14 00:18:58]

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!

[msw, 2016/07/14 02:23:33]

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.

[Jane, 2016/07/14 15:34:37]

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!

+              l10n_util::GetStringUTF16(
+                  IDS_SYNC_ERROR_USER_MENU_SIGNOUT_BUTTON),
+              &sync_error_signout_button_);
+        } else {

[msw, 2016/07/13 22:31:38]

nit: no else after return.

[Jane, 2016/07/14 00:18:57]

Done.

+          return CreateSyncErrorView(
+              l10n_util::GetStringUTF16(
+                  IDS_SYNC_ERROR_USER_MENU_SIGNIN_AGAIN_MESSAGE),
+              l10n_util::GetStringUTF16(
+                  IDS_SYNC_ERROR_USER_MENU_SIGNIN_AGAIN_BUTTON),
+              &sync_error_signin_again_button_);
+        }
+    }
+  }
+
+  // Check for an auth error.
+  if (HasAuthError(browser_->profile())) {
+    return CreateSyncErrorView(
+        l10n_util::GetStringUTF16(IDS_SYNC_ERROR_USER_MENU_LOGIN_MESSAGE),
+        l10n_util::GetStringUTF16(IDS_SYNC_ERROR_USER_MENU_LOGIN_BUTTON),
+        &sync_error_signin_button_);
+  }
+
+  // Check for sync errors if the sync service is enabled.
+  if (service) {
+    // Check for an actionable error UPGRADE_CLIENT.

[msw, 2016/07/13 22:31:38]

nit: "actionable UPGRADE_CLIENT error"?

[Jane, 2016/07/14 00:18:58]

Done.

+    ProfileSyncService::Status status;
+    service->QueryDetailedSyncStatus(&status);
+    if (status.sync_protocol_error.action == syncer::UPGRADE_CLIENT) {
+      return CreateSyncErrorView(
+          l10n_util::GetStringUTF16(IDS_SYNC_ERROR_USER_MENU_UPGRADE_MESSAGE),
+          l10n_util::GetStringUTF16(IDS_SYNC_ERROR_USER_MENU_UPGRADE_BUTTON),
+          &sync_error_upgrade_button_);
+    }
+
+    // Check for a sync passphrase error.
+    SyncErrorController* sync_error_controller =
+        service->sync_error_controller();
+    if (sync_error_controller && sync_error_controller->HasError()) {
+      return CreateSyncErrorView(
+          l10n_util::GetStringUTF16(
+              IDS_SYNC_ERROR_USER_MENU_PASSPHRASE_MESSAGE),
+          l10n_util::GetStringUTF16(IDS_SYNC_ERROR_USER_MENU_PASSPHRASE_BUTTON),
+          &sync_error_passphrase_button_);
+    }
+  }
+
+  // There is no error.
+  return nullptr;
+}
+
+views::View* ProfileChooserView::CreateSyncErrorView(
+    const base::string16& content_text,

[msw, 2016/07/13 22:31:38]

nit: take a resource ID here instead of the string; ditto for button_text.

[Jane, 2016/07/14 00:18:58]

Good suggestion! Changed it.

+    const base::string16& button_text,
+    views::LabelButton** button) {

[msw, 2016/07/13 22:31:38]

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.

[Jane, 2016/07/14 00:18:58]

Done.

+  // Sets an overall horizontal layout.
+  views::View* view = new views::View();
+  views::BoxLayout* layout = new views::BoxLayout(
+      views::BoxLayout::kHorizontal, kMaterialMenuEdgeMargin,
+      kMaterialMenuEdgeMargin, views::kUnrelatedControlHorizontalSpacing);
+  layout->set_cross_axis_alignment(
+      views::BoxLayout::CROSS_AXIS_ALIGNMENT_START);
+  view->SetLayoutManager(layout);
+
+  // Adds the sync problem icon.
+  views::ImageView* sync_problem_icon = new views::ImageView();
+  sync_problem_icon->SetImage(gfx::CreateVectorIcon(
+      gfx::VectorIconId::SYNC_PROBLEM, 20, gfx::kGoogleRed700));
+  view->AddChildView(sync_problem_icon);
+
+  // Adds a vertical view to organize the error title, message, and button.

[msw, 2016/07/13 22:31:38]

nit: GridLayout *might* be simpler than nesting BoxLayout, but maybe not.

[Jane, 2016/07/14 00:18:58]

Oh, right, maybe, but I thought this was pretty straightforward in itself. I'll
keep it this way for now.

+  views::View* vertical_view = new views::View();
+  views::BoxLayout* vertical_layout =
+      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
+                           views::kRelatedControlSmallVerticalSpacing);
+  vertical_layout->set_cross_axis_alignment(
+      views::BoxLayout::CROSS_AXIS_ALIGNMENT_START);
+  vertical_view->SetLayoutManager(vertical_layout);
+
+  // Adds the title.
+  views::Label* title_label = new views::Label(
+      l10n_util::GetStringUTF16(IDS_SYNC_ERROR_USER_MENU_TITLE));
+  title_label->SetFontList(gfx::FontList().DeriveWithSizeDelta(1));

[msw, 2016/07/13 22:31:38]

I doubt that a 1px Font size difference matters here; skip this.

[Jane, 2016/07/14 00:18:57]

Done.

+  title_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  title_label->SetEnabledColor(gfx::kGoogleRed700);
+  vertical_view->AddChildView(title_label);
+
+  // Adds body content.
+  if (!content_text.empty()) {

[msw, 2016/07/13 22:31:38]

This text should never be empty; DCHECK instead or just assume it's valid?

[Jane, 2016/07/14 00:18:58]

Done.

+    views::Label* content_label = new views::Label(content_text);
+    content_label->SetMultiLine(true);
+    content_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    vertical_view->AddChildView(content_label);
+  }
+
+  // Adds a padding row between error title/content and the button.
+  SizedContainer* padding = new SizedContainer(
+      gfx::Size(GetFixedMenuWidth() - 2 * kMaterialMenuEdgeMargin,

[msw, 2016/07/13 22:31:38]

nit: I bet a 0 width would be okay here...

[Jane, 2016/07/14 00:18:58]

Done.

+                views::kRelatedControlVerticalSpacing));
+  vertical_view->AddChildView(padding);
+
+  // Adds action_button.

[msw, 2016/07/13 22:31:38]

nit: "an action button"; there is no |action_button| identifier.

[Jane, 2016/07/14 00:18:57]

Done.

+  *button = views::MdTextButton::CreateSecondaryUiBlueButton(this, button_text);
+  vertical_view->AddChildView(*button);
+
+  view->AddChildView(vertical_view);
+  view->SetBorder(views::Border::CreateEmptyBorder(
+      0, 0, views::kRelatedControlSmallVerticalSpacing, 0));
+
+  return view;
+}
+
 views::View* ProfileChooserView::CreateCurrentProfileView(
     const AvatarMenu::Item& avatar_item,
     bool is_guest) {