Identity API: Pop-up a sign-in dialog if gaia credentials are bad

chrome/browser/extensions/api/identity/identity_signin_flow.cc

+// Copyright 2013 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/extensions/api/identity/identity_signin_flow.h"
+
+#include "chrome/browser/signin/token_service.h"
+#include "chrome/browser/signin/token_service_factory.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/notification_details.h"
+#include "google_apis/gaia/gaia_constants.h"
+
+namespace extensions {
+
+IdentitySigninFlow::IdentitySigninFlow(Delegate* delegate, Profile* profile)
+    : delegate_(delegate),
+      profile_(profile) {
+}
+
+IdentitySigninFlow::~IdentitySigninFlow() {
+  delegate_ = NULL;

[miket_OOO, 2013/03/29 22:06:22]

This is a naked pointer. It's OK to just let it die.

Alternatively, if there used to be an ordering constraint in the destructor that
got refactored out, please clean this up.

[Michael Courage, 2013/03/30 00:18:30]

Done. Forgot to clean this up after removing the reentrancy issue that
originally necessitated it.

+}
+
+void IdentitySigninFlow::Start() {
+#if !defined(OS_CHROMEOS)
+  DCHECK(delegate_);
+  TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_TOKEN_AVAILABLE,
+                 content::Source<TokenService>(token_service));
+
+  LoginUIService* login_ui_service =
+      LoginUIServiceFactory::GetForProfile(profile_);
+  login_ui_service->ShowLoginPopup();
+#else
+  // On Chrome OS, the user has to log out to re-establish credentials. Let the
+  // global error popup handle everything.
+  delegate_->SigninFailed();
+  delegate_ = NULL;
+#endif
+}
+
+void IdentitySigninFlow::Observe(int type,
+                            const content::NotificationSource& source,

[miket_OOO, 2013/03/29 22:06:22]

indent messed up

[Michael Courage, 2013/03/30 00:18:30]

Done.

+                            const content::NotificationDetails& details) {
+  if (type == chrome::NOTIFICATION_TOKEN_AVAILABLE) {
+    TokenService::TokenAvailableDetails* token_details =
+        content::Details<TokenService::TokenAvailableDetails>(details).ptr();
+    if (token_details->service() == GaiaConstants::kGaiaOAuth2LoginRefreshToken)
+      ReportSigninSuccess(token_details->token());
+  } else {

[miket_OOO, 2013/03/29 22:06:22]

same comment as earlier about unusual block structure (and as before, OK if you
have a reason for it)

[Michael Courage, 2013/03/30 00:18:30]

Done.

+    NOTREACHED();
+  }
+}
+
+void IdentitySigninFlow::ReportSigninSuccess(const std::string& token) {
+  if (delegate_) {
+    delegate_->SigninSuccess(token);
+    delegate_ = NULL;

[miket_OOO, 2013/03/29 22:06:22]

These nulls make me nervous. Have you made a promise somewhere that the delegate
methods will be called only once? I worry that if these turned out to be
necessary to prevent multiple calls, maybe they're symptomatic of a deeper bug,
like maybe the token service is not well.

Alternatively, if you're just doing it for cleanliness, then the downside is
that they might mask buggy behavior like what I've described above.

[Michael Courage, 2013/03/30 00:18:30]

Done.

+  }
+}
+
+}  // namespace extensions