Refactored signin/sync error controllers for the avatar button

chrome/browser/ui/avatar_button_error_controller.cc

Index: chrome/browser/ui/avatar_button_error_controller.cc
diff --git a/chrome/browser/ui/avatar_button_error_controller.cc b/chrome/browser/ui/avatar_button_error_controller.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d39f76b2b3a50e079f8dbcb49776fd56dafea883
--- /dev/null
+++ b/chrome/browser/ui/avatar_button_error_controller.cc
@@ -0,0 +1,102 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/avatar_button_error_controller.h"
+
+#include "chrome/browser/profiles/profiles_state.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "components/browser_sync/browser/profile_sync_service.h"
+#include "components/signin/core/browser/signin_error_controller.h"
+#include "components/signin/core/common/profile_management_switches.h"
+
+AvatarButtonErrorController::AvatarButtonErrorController(
+    AvatarButtonErrorControllerDelegate* delegate,
+    Profile* profile)
+    : delegate_(delegate),
+      avatar_signin_error_controller_(profile, this),
+      avatar_sync_error_controller_(profile, this),
+      has_signin_error_(false),
+      has_sync_error_(false) {}
+
+void AvatarButtonErrorController::GetAvatarErrorUpdate() {

[sky, 2016/08/01 22:59:40]

Can you elaborate on why you need this function? AFAICT you only call this once,
and shortly after the constructor. Can't the constructor do this?

I suspect you have this function to delay calling to the delegate from the
constructor. I would definitely avoid calling the delegate from the constructor,
but having an explicit GetAvatarErrorUpdate() is easy to miss, resulting in an
awkward api.

My recommendation is to have the constructor update has_*_error_, but not call
the delegate. The delegate is only called on changes after the constructor.
NewAvatarButton checks HasAvatarError() from the constructor and does what ever
it needs to do.

[Jane, 2016/08/02 14:23:18]

Done. Makes sense! I refactored the actual error detection code into helper
functions HasSigninError() and HasSyncError(), call them from
AvatarButtonErrorController ctor, and have NewAvatarButton() check
HasAvatarError() in its ctor.

+  avatar_signin_error_controller_.OnErrorChanged();
+  avatar_sync_error_controller_.OnErrorChanged();
+}
+
+void AvatarButtonErrorController::UpdateSigninError(bool has_signin_error) {
+  has_signin_error_ = has_signin_error;
+  delegate_->OnAvatarErrorChanged();

[sky, 2016/08/01 22:59:40]

The public api for this class is effectively HasAvatarError(), you should call
OnAvatarErrorChanged() only if HasAvatarError() has changed.

[Jane, 2016/08/02 14:23:18]

Done. I added a check for HasAvatarError().

+}
+
+void AvatarButtonErrorController::UpdateSyncError(bool has_sync_error) {
+  has_sync_error_ = has_sync_error;
+  delegate_->OnAvatarErrorChanged();
+}
+
+AvatarButtonErrorController::SigninErrorObserver::SigninErrorObserver(
+    Profile* profile,
+    AvatarButtonErrorController* avatar_button_error_controller)
+    : profile_(profile),
+      avatar_button_error_controller_(avatar_button_error_controller) {
+  SigninErrorController* signin_error_controller =
+      profiles::GetSigninErrorController(profile_);
+  if (signin_error_controller)
+    signin_error_controller->AddObserver(this);
+}
+
+AvatarButtonErrorController::SigninErrorObserver::~SigninErrorObserver() {
+  SigninErrorController* signin_error_controller =
+      profiles::GetSigninErrorController(profile_);
+  if (signin_error_controller)
+    signin_error_controller->RemoveObserver(this);
+}
+
+void AvatarButtonErrorController::SigninErrorObserver::OnErrorChanged() {
+  const SigninErrorController* signin_error_controller =
+      profiles::GetSigninErrorController(profile_);
+  avatar_button_error_controller_->UpdateSigninError(
+      signin_error_controller && signin_error_controller->HasError());
+}
+
+AvatarButtonErrorController::SyncErrorObserver::SyncErrorObserver(
+    Profile* profile,
+    AvatarButtonErrorController* avatar_button_error_controller)
+    : profile_(profile),
+      avatar_button_error_controller_(avatar_button_error_controller) {
+  SyncErrorController* sync_error_controller = GetSyncErrorControllerIfNeeded();
+  if (sync_error_controller)
+    sync_error_controller->AddObserver(this);
+}
+
+AvatarButtonErrorController::SyncErrorObserver::~SyncErrorObserver() {
+  SyncErrorController* sync_error_controller = GetSyncErrorControllerIfNeeded();
+  if (sync_error_controller)
+    sync_error_controller->RemoveObserver(this);
+}
+
+void AvatarButtonErrorController::SyncErrorObserver::OnErrorChanged() {
+  ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (switches::IsMaterialDesignUserMenu() && sync_service) {
+    SyncErrorController* sync_error_controller =
+        sync_service->sync_error_controller();
+    ProfileSyncService::Status status;
+    sync_service->QueryDetailedSyncStatus(&status);
+    avatar_button_error_controller_->UpdateSyncError(
+        sync_service->HasUnrecoverableError() ||
+        status.sync_protocol_error.action == syncer::UPGRADE_CLIENT ||
+        (sync_error_controller && sync_error_controller->HasError()));
+  } else {
+    avatar_button_error_controller_->UpdateSyncError(false);
+  }
+}
+
+SyncErrorController* AvatarButtonErrorController::SyncErrorObserver::
+    GetSyncErrorControllerIfNeeded() {
+  if (!switches::IsMaterialDesignUserMenu())
+    return nullptr;
+  ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  return sync_service ? sync_service->sync_error_controller() : nullptr;
+}