From 7529916efbda5763f6b5ddd9256ea46e29065281 Mon Sep 17 00:00:00 2001 From: ricea Date: Wed, 19 Aug 2015 02:32:03 -0700 Subject: [PATCH] Revert of Delete dead signin code (SigninGlobalError) (patchset #8 id:140001 of https://codereview.chromium.org/1299543002/ ) Reason for revert: Broke the Mac10.9 Tests (dbg) bot: http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/10278 See the stack trace in the logs. Original issue's description: > Delete dead signin code (SigninGlobalError) > > BUG=none > > Committed: https://crrev.com/c91b178b07b0d9fb0c7a437df3a1da3db1887160 > Cr-Commit-Position: refs/heads/master@{#344176} TBR=anthonyvd@chromium.org,sky@chromium.org,rogerta@chromium.org,estade@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=none Review URL: https://codereview.chromium.org/1301583004 Cr-Commit-Position: refs/heads/master@{#344188} --- chrome/app/chrome_command_ids.h | 4 + chrome/browser/BUILD.gn | 5 + chrome/browser/app_controller_mac.h | 6 + chrome/browser/app_controller_mac.mm | 58 ++++++ chrome/browser/app_controller_mac_unittest.mm | 132 ++++++++++++++ chrome/browser/signin/signin_global_error.cc | 196 +++++++++++++++++++++ chrome/browser/signin/signin_global_error.h | 71 ++++++++ .../browser/signin/signin_global_error_factory.cc | 52 ++++++ .../browser/signin/signin_global_error_factory.h | 39 ++++ .../browser/signin/signin_global_error_unittest.cc | 170 ++++++++++++++++++ chrome/browser/signin/signin_ui_util.cc | 67 +++++++ chrome/browser/signin/signin_ui_util.h | 11 ++ chrome/browser/ui/browser_command_controller.cc | 27 +++ chrome/browser/ui/browser_command_controller.h | 7 + .../ui/browser_command_controller_unittest.cc | 35 ++++ .../browser/ui/cocoa/browser_window_controller.mm | 8 + .../ui/cocoa/panels/panel_cocoa_unittest.mm | 7 + chrome/browser/ui/toolbar/wrench_menu_model.cc | 71 +++++++- chrome/browser/ui/toolbar/wrench_menu_model.h | 1 + chrome/browser/ui/views/toolbar/toolbar_view.cc | 8 +- chrome/chrome_browser.gypi | 9 + chrome/chrome_tests_unit.gypi | 2 + chrome/test/BUILD.gn | 5 +- 23 files changed, 986 insertions(+), 5 deletions(-) create mode 100644 chrome/browser/signin/signin_global_error.cc create mode 100644 chrome/browser/signin/signin_global_error.h create mode 100644 chrome/browser/signin/signin_global_error_factory.cc create mode 100644 chrome/browser/signin/signin_global_error_factory.h create mode 100644 chrome/browser/signin/signin_global_error_unittest.cc diff --git a/chrome/app/chrome_command_ids.h b/chrome/app/chrome_command_ids.h index 0513df0dbd77..6ca7b939340a 100644 --- a/chrome/app/chrome_command_ids.h +++ b/chrome/app/chrome_command_ids.h @@ -190,6 +190,10 @@ #define IDC_SHOW_KEYBOARD_OVERLAY 40027 #define IDC_PROFILING_ENABLED 40028 #define IDC_BOOKMARKS_MENU 40029 +// TODO(atwilson): Remove IDC_SHOW_SYNC_SETUP when we officially allow signin +// when sync is disabled. +#define IDC_SHOW_SYNC_SETUP 40030 +#define IDC_SHOW_SIGNIN 40030 #define IDC_EXTENSION_ERRORS 40031 #define IDC_SHOW_SIGNIN_ERROR 40032 #define IDC_SHOW_SETTINGS_CHANGE_FIRST 40033 diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn index 6e4b32e266bf..a88e8871c655 100644 --- a/chrome/browser/BUILD.gn +++ b/chrome/browser/BUILD.gn @@ -507,6 +507,11 @@ source_set("browser") { ".", "//chrome") deps += [ "//chrome/browser/chromeos" ] + } else { + # Non-ChromeOS. + sources += rebase_path(gypi_values.chrome_browser_non_chromeos_sources, + ".", + "//chrome") } if (is_ios) { diff --git a/chrome/browser/app_controller_mac.h b/chrome/browser/app_controller_mac.h index 421e998fc962..9b640f2980b4 100644 --- a/chrome/browser/app_controller_mac.h +++ b/chrome/browser/app_controller_mac.h @@ -112,6 +112,12 @@ class WorkAreaWatcherObserver; @property(readonly, nonatomic) BOOL startupComplete; @property(readonly, nonatomic) Profile* lastProfile; +// Helper method used to update the "Signin" menu item in the main menu and the +// wrench menu to reflect the current signed in state. ++ (void)updateSigninItem:(id)signinItem + shouldShow:(BOOL)showSigninMenuItem + currentProfile:(Profile*)profile; + - (void)didEndMainMessageLoop; // Try to close all browser windows, and if that succeeds then quit. diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm index e1f5a42245b7..137a0605b44a 100644 --- a/chrome/browser/app_controller_mac.mm +++ b/chrome/browser/app_controller_mac.mm @@ -44,6 +44,7 @@ #include "chrome/browser/sessions/tab_restore_service_factory.h" #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.h" #include "chrome/browser/sync/sync_ui_util.h" #include "chrome/browser/ui/browser.h" @@ -302,6 +303,33 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { @synthesize startupComplete = startupComplete_; ++ (void)updateSigninItem:(id)signinItem + shouldShow:(BOOL)showSigninMenuItem + currentProfile:(Profile*)profile { + DCHECK([signinItem isKindOfClass:[NSMenuItem class]]); + NSMenuItem* signinMenuItem = static_cast(signinItem); + + // Look for a separator immediately after the menu item so it can be hidden + // or shown appropriately along with the signin menu item. + NSMenuItem* followingSeparator = nil; + NSMenu* menu = [signinItem menu]; + if (menu) { + NSInteger signinItemIndex = [menu indexOfItem:signinMenuItem]; + DCHECK_NE(signinItemIndex, -1); + if ((signinItemIndex + 1) < [menu numberOfItems]) { + NSMenuItem* menuItem = [menu itemAtIndex:(signinItemIndex + 1)]; + if ([menuItem isSeparatorItem]) { + followingSeparator = menuItem; + } + } + } + + base::string16 label = signin_ui_util::GetSigninMenuLabel(profile); + [signinMenuItem setTitle:l10n_util::FixUpWindowsStyleLabel(label)]; + [signinMenuItem setHidden:!showSigninMenuItem]; + [followingSeparator setHidden:!showSigninMenuItem]; +} + - (void)dealloc { [[closeTabMenuItem_ menu] setDelegate:nil]; [super dealloc]; @@ -929,6 +957,27 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { // dialog. enable = ![self keyWindowIsModal]; break; + case IDC_SHOW_SYNC_SETUP: { + Profile* lastProfile = [self lastProfile]; + // The profile may be NULL during shutdown -- see + // http://code.google.com/p/chromium/issues/detail?id=43048 . + // + // TODO(akalin,viettrungluu): Figure out whether this method + // can be prevented from being called if lastProfile is + // NULL. + if (!lastProfile) { + LOG(WARNING) + << "NULL lastProfile detected -- not doing anything"; + break; + } + SigninManager* signin = SigninManagerFactory::GetForProfile( + lastProfile->GetOriginalProfile()); + enable = signin->IsSigninAllowed() && ![self keyWindowIsModal]; + [AppController updateSigninItem:item + shouldShow:enable + currentProfile:lastProfile]; + break; + } #if defined(GOOGLE_CHROME_BUILD) case IDC_FEEDBACK: enable = NO; @@ -1080,6 +1129,14 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { else chrome::OpenHelpWindow(lastProfile, chrome::HELP_SOURCE_MENU); break; + case IDC_SHOW_SYNC_SETUP: + if (Browser* browser = ActivateBrowser(lastProfile)) { + chrome::ShowBrowserSigninOrSettings(browser, + signin_metrics::SOURCE_MENU); + } else { + chrome::OpenSyncSetupWindow(lastProfile, signin_metrics::SOURCE_MENU); + } + break; case IDC_TASK_MANAGER: chrome::OpenTaskManager(NULL); break; @@ -1233,6 +1290,7 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { #if defined(GOOGLE_CHROME_BUILD) menuState_->UpdateCommandEnabled(IDC_FEEDBACK, true); #endif + menuState_->UpdateCommandEnabled(IDC_SHOW_SYNC_SETUP, true); menuState_->UpdateCommandEnabled(IDC_TASK_MANAGER, true); } diff --git a/chrome/browser/app_controller_mac_unittest.mm b/chrome/browser/app_controller_mac_unittest.mm index eb046193e8f6..5cf952c86f3a 100644 --- a/chrome/browser/app_controller_mac_unittest.mm +++ b/chrome/browser/app_controller_mac_unittest.mm @@ -91,3 +91,135 @@ TEST_F(AppControllerTest, LastProfile) { EXPECT_EQ(dest_path2, [ac lastProfile]->GetPath()); } + +TEST_F(AppControllerTest, TestSigninMenuItemNoErrors) { + base::scoped_nsobject syncMenuItem( + [[NSMenuItem alloc] initWithTitle:@"" + action:@selector(commandDispatch) + keyEquivalent:@""]); + [syncMenuItem setTag:IDC_SHOW_SYNC_SETUP]; + + NSString* startSignin = l10n_util::GetNSStringFWithFixup( + IDS_SYNC_MENU_PRE_SYNCED_LABEL, + l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME)); + + // Make sure shouldShow parameter is obeyed, and we get the default + // label if not signed in. + [AppController updateSigninItem:syncMenuItem + shouldShow:YES + currentProfile:profile_]; + + EXPECT_TRUE([[syncMenuItem title] isEqualTo:startSignin]); + EXPECT_FALSE([syncMenuItem isHidden]); + + [AppController updateSigninItem:syncMenuItem + shouldShow:NO + currentProfile:profile_]; + EXPECT_TRUE([[syncMenuItem title] isEqualTo:startSignin]); + EXPECT_TRUE([syncMenuItem isHidden]); + + // Now sign in. + std::string username = "foo@example.com"; + NSString* alreadySignedIn = l10n_util::GetNSStringFWithFixup( + IDS_SYNC_MENU_SYNCED_LABEL, base::UTF8ToUTF16(username)); + SigninManager* signin = SigninManagerFactory::GetForProfile(profile_); + signin->SetAuthenticatedAccountInfo(username, username); + ProfileSyncService* sync = ProfileSyncServiceFactory::GetForProfile(profile_); + sync->SetSyncSetupCompleted(); + [AppController updateSigninItem:syncMenuItem + shouldShow:YES + currentProfile:profile_]; + EXPECT_TRUE([[syncMenuItem title] isEqualTo:alreadySignedIn]); + EXPECT_FALSE([syncMenuItem isHidden]); +} + +TEST_F(AppControllerTest, TestSigninMenuItemAuthError) { + base::scoped_nsobject syncMenuItem( + [[NSMenuItem alloc] initWithTitle:@"" + action:@selector(commandDispatch) + keyEquivalent:@""]); + [syncMenuItem setTag:IDC_SHOW_SYNC_SETUP]; + + // Now sign in. + std::string username = "foo@example.com"; + SigninManager* signin = SigninManagerFactory::GetForProfile(profile_); + signin->SetAuthenticatedAccountInfo(username, username); + ProfileSyncService* sync = ProfileSyncServiceFactory::GetForProfile(profile_); + sync->SetSyncSetupCompleted(); + // Force an auth error. + FakeAuthStatusProvider provider( + SigninErrorControllerFactory::GetForProfile(profile_)); + GoogleServiceAuthError error( + GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS); + provider.SetAuthError("user@gmail.com", error); + [AppController updateSigninItem:syncMenuItem + shouldShow:YES + currentProfile:profile_]; + NSString* authError = + l10n_util::GetNSStringWithFixup(IDS_SYNC_SIGN_IN_ERROR_WRENCH_MENU_ITEM); + EXPECT_TRUE([[syncMenuItem title] isEqualTo:authError]); + EXPECT_FALSE([syncMenuItem isHidden]); +} + +// If there's a separator after the signin menu item, make sure it is hidden/ +// shown when the signin menu item is. +TEST_F(AppControllerTest, TestSigninMenuItemWithSeparator) { + base::scoped_nsobject menu([[NSMenu alloc] initWithTitle:@""]); + NSMenuItem* signinMenuItem = [menu addItemWithTitle:@"" + action:@selector(commandDispatch) + keyEquivalent:@""]; + [signinMenuItem setTag:IDC_SHOW_SYNC_SETUP]; + NSMenuItem* followingSeparator = [NSMenuItem separatorItem]; + [menu addItem:followingSeparator]; + [signinMenuItem setHidden:NO]; + [followingSeparator setHidden:NO]; + + [AppController updateSigninItem:signinMenuItem + shouldShow:NO + currentProfile:profile_]; + + EXPECT_FALSE([followingSeparator isEnabled]); + EXPECT_TRUE([signinMenuItem isHidden]); + EXPECT_TRUE([followingSeparator isHidden]); + + [AppController updateSigninItem:signinMenuItem + shouldShow:YES + currentProfile:profile_]; + + EXPECT_FALSE([followingSeparator isEnabled]); + EXPECT_FALSE([signinMenuItem isHidden]); + EXPECT_FALSE([followingSeparator isHidden]); +} + +// If there's a non-separator item after the signin menu item, it should not +// change state when the signin menu item is hidden/shown. +TEST_F(AppControllerTest, TestSigninMenuItemWithNonSeparator) { + base::scoped_nsobject menu([[NSMenu alloc] initWithTitle:@""]); + NSMenuItem* signinMenuItem = [menu addItemWithTitle:@"" + action:@selector(commandDispatch) + keyEquivalent:@""]; + [signinMenuItem setTag:IDC_SHOW_SYNC_SETUP]; + NSMenuItem* followingNonSeparator = + [menu addItemWithTitle:@"" + action:@selector(commandDispatch) + keyEquivalent:@""]; + [signinMenuItem setHidden:NO]; + [followingNonSeparator setHidden:NO]; + + [AppController updateSigninItem:signinMenuItem + shouldShow:NO + currentProfile:profile_]; + + EXPECT_TRUE([followingNonSeparator isEnabled]); + EXPECT_TRUE([signinMenuItem isHidden]); + EXPECT_FALSE([followingNonSeparator isHidden]); + + [followingNonSeparator setHidden:YES]; + [AppController updateSigninItem:signinMenuItem + shouldShow:YES + currentProfile:profile_]; + + EXPECT_TRUE([followingNonSeparator isEnabled]); + EXPECT_FALSE([signinMenuItem isHidden]); + EXPECT_TRUE([followingNonSeparator isHidden]); +} diff --git a/chrome/browser/signin/signin_global_error.cc b/chrome/browser/signin/signin_global_error.cc new file mode 100644 index 000000000000..b2e8f86e6889 --- /dev/null +++ b/chrome/browser/signin/signin_global_error.cc @@ -0,0 +1,196 @@ +// Copyright (c) 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/signin/signin_global_error.h" + +#include "base/logging.h" +#include "base/metrics/histogram_macros.h" +#include "chrome/app/chrome_command_ids.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/signin/signin_manager_factory.h" +#include "chrome/browser/ui/browser_commands.h" +#include "chrome/browser/ui/browser_window.h" +#include "chrome/browser/ui/chrome_pages.h" +#include "chrome/browser/ui/global_error/global_error_service.h" +#include "chrome/browser/ui/global_error/global_error_service_factory.h" +#include "chrome/browser/ui/singleton_tabs.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/url_constants.h" +#include "chrome/grit/chromium_strings.h" +#include "chrome/grit/generated_resources.h" +#include "components/signin/core/browser/signin_header_helper.h" +#include "components/signin/core/browser/signin_manager.h" +#include "components/signin/core/browser/signin_metrics.h" +#include "components/signin/core/common/profile_management_switches.h" +#include "net/base/url_util.h" +#include "ui/base/l10n/l10n_util.h" + +#if !defined(OS_ANDROID) && !defined(OS_IOS) +#include "chrome/browser/signin/signin_promo.h" +#endif + +SigninGlobalError::SigninGlobalError( + SigninErrorController* error_controller, + Profile* profile) + : profile_(profile), + error_controller_(error_controller), + is_added_to_global_error_service_(false) { + error_controller_->AddObserver(this); + is_added_to_global_error_service_ = !switches::IsNewAvatarMenu(); + if (is_added_to_global_error_service_) + GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(this); +} + +SigninGlobalError::~SigninGlobalError() { + DCHECK(!error_controller_) + << "SigninGlobalError::Shutdown() was not called"; +} + +bool SigninGlobalError::HasError() { + return HasMenuItem(); +} + +void SigninGlobalError::AttemptToFixError(Browser* browser) { + if (!HasError()) + return; + + ExecuteMenuItem(browser); +} + +void SigninGlobalError::Shutdown() { + if (is_added_to_global_error_service_) { + GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveGlobalError(this); + is_added_to_global_error_service_ = false; + } + + error_controller_->RemoveObserver(this); + error_controller_ = NULL; +} + +bool SigninGlobalError::HasMenuItem() { + return error_controller_->HasError(); +} + +int SigninGlobalError::MenuItemCommandID() { + return IDC_SHOW_SIGNIN_ERROR; +} + +base::string16 SigninGlobalError::MenuItemLabel() { + // Notify the user if there's an auth error the user should know about. + if (error_controller_->HasError()) + return l10n_util::GetStringUTF16(IDS_SYNC_SIGN_IN_ERROR_WRENCH_MENU_ITEM); + return base::string16(); +} + +void SigninGlobalError::ExecuteMenuItem(Browser* browser) { +#if defined(OS_CHROMEOS) + if (error_controller_->auth_error().state() != + GoogleServiceAuthError::NONE) { + DVLOG(1) << "Signing out the user to fix a sync error."; + // TODO(beng): seems like this could just call chrome::AttemptUserExit(). + chrome::ExecuteCommand(browser, IDC_EXIT); + return; + } +#endif + + // Global errors don't show up in the wrench menu on mobile. +#if !defined(OS_ANDROID) && !defined(OS_IOS) + LoginUIService* login_ui = LoginUIServiceFactory::GetForProfile(profile_); + if (login_ui->current_login_ui()) { + login_ui->current_login_ui()->FocusUI(); + return; + } + + UMA_HISTOGRAM_ENUMERATION("Signin.Reauth", + signin_metrics::HISTOGRAM_REAUTH_SHOWN, + signin_metrics::HISTOGRAM_REAUTH_MAX); + if (switches::IsNewAvatarMenu()) { + browser->window()->ShowAvatarBubbleFromAvatarButton( + BrowserWindow::AVATAR_BUBBLE_MODE_REAUTH, + signin::ManageAccountsParams()); + } else { + chrome::ShowSingletonTab( + browser, + signin::GetReauthURL(profile_, error_controller_->error_account_id())); + } +#endif +} + +bool SigninGlobalError::HasBubbleView() { + return !GetBubbleViewMessages().empty(); +} + +base::string16 SigninGlobalError::GetBubbleViewTitle() { + return l10n_util::GetStringUTF16(IDS_SIGNIN_ERROR_BUBBLE_VIEW_TITLE); +} + +std::vector SigninGlobalError::GetBubbleViewMessages() { + std::vector messages; + + // If the user isn't signed in, no need to display an error bubble. + SigninManagerBase* signin_manager = + SigninManagerFactory::GetForProfileIfExists(profile_); + if (signin_manager && !signin_manager->IsAuthenticated()) + return messages; + + if (!error_controller_->HasError()) + return messages; + + switch (error_controller_->auth_error().state()) { + // TODO(rogerta): use account id in error messages. + + // User credentials are invalid (bad acct, etc). + case GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS: + case GoogleServiceAuthError::SERVICE_ERROR: + case GoogleServiceAuthError::ACCOUNT_DELETED: + case GoogleServiceAuthError::ACCOUNT_DISABLED: + messages.push_back(l10n_util::GetStringUTF16( + IDS_SYNC_SIGN_IN_ERROR_BUBBLE_VIEW_MESSAGE)); + break; + + // Sync service is not available for this account's domain. + case GoogleServiceAuthError::SERVICE_UNAVAILABLE: + messages.push_back(l10n_util::GetStringUTF16( + IDS_SYNC_UNAVAILABLE_ERROR_BUBBLE_VIEW_MESSAGE)); + break; + + // Generic message for "other" errors. + default: + messages.push_back(l10n_util::GetStringUTF16( + IDS_SYNC_OTHER_SIGN_IN_ERROR_BUBBLE_VIEW_MESSAGE)); + } + return messages; +} + +base::string16 SigninGlobalError::GetBubbleViewAcceptButtonLabel() { + // If the auth service is unavailable, don't give the user the option to try + // signing in again. + if (error_controller_->auth_error().state() == + GoogleServiceAuthError::SERVICE_UNAVAILABLE) { + return l10n_util::GetStringUTF16( + IDS_SYNC_UNAVAILABLE_ERROR_BUBBLE_VIEW_ACCEPT); + } else { + return l10n_util::GetStringUTF16(IDS_SYNC_SIGN_IN_ERROR_BUBBLE_VIEW_ACCEPT); + } +} + +base::string16 SigninGlobalError::GetBubbleViewCancelButtonLabel() { + return base::string16(); +} + +void SigninGlobalError::OnBubbleViewDidClose(Browser* browser) { +} + +void SigninGlobalError::BubbleViewAcceptButtonPressed(Browser* browser) { + ExecuteMenuItem(browser); +} + +void SigninGlobalError::BubbleViewCancelButtonPressed(Browser* browser) { + NOTREACHED(); +} + +void SigninGlobalError::OnErrorChanged() { + GlobalErrorServiceFactory::GetForProfile(profile_)->NotifyErrorsChanged(this); +} diff --git a/chrome/browser/signin/signin_global_error.h b/chrome/browser/signin/signin_global_error.h new file mode 100644 index 000000000000..86846c804378 --- /dev/null +++ b/chrome/browser/signin/signin_global_error.h @@ -0,0 +1,71 @@ +// Copyright (c) 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. + +#ifndef CHROME_BROWSER_SIGNIN_SIGNIN_GLOBAL_ERROR_H_ +#define CHROME_BROWSER_SIGNIN_SIGNIN_GLOBAL_ERROR_H_ + +#include +#include "base/basictypes.h" +#include "base/compiler_specific.h" +#include "base/gtest_prod_util.h" +#include "chrome/browser/ui/global_error/global_error.h" +#include "components/keyed_service/core/keyed_service.h" +#include "components/signin/core/browser/signin_error_controller.h" + +class Profile; + +// Shows auth errors on the wrench menu using a bubble view and a menu item. +class SigninGlobalError : public GlobalErrorWithStandardBubble, + public SigninErrorController::Observer, + public KeyedService { + public: + SigninGlobalError(SigninErrorController* error_controller, + Profile* profile); + ~SigninGlobalError() override; + + // Returns true if there is an authentication error. + bool HasError(); + + // Shows re-authentication UI to the user in an attempt to fix the error. + // The re-authentication UI will be shown in |browser|. + void AttemptToFixError(Browser* browser); + + private: + FRIEND_TEST_ALL_PREFIXES(SigninGlobalErrorTest, NoErrorAuthStatusProviders); + FRIEND_TEST_ALL_PREFIXES(SigninGlobalErrorTest, ErrorAuthStatusProvider); + FRIEND_TEST_ALL_PREFIXES(SigninGlobalErrorTest, AuthStatusEnumerateAllErrors); + + // KeyedService: + void Shutdown() override; + + // GlobalErrorWithStandardBubble: + bool HasMenuItem() override; + int MenuItemCommandID() override; + base::string16 MenuItemLabel() override; + void ExecuteMenuItem(Browser* browser) override; + bool HasBubbleView() override; + base::string16 GetBubbleViewTitle() override; + std::vector GetBubbleViewMessages() override; + base::string16 GetBubbleViewAcceptButtonLabel() override; + base::string16 GetBubbleViewCancelButtonLabel() override; + void OnBubbleViewDidClose(Browser* browser) override; + void BubbleViewAcceptButtonPressed(Browser* browser) override; + void BubbleViewCancelButtonPressed(Browser* browser) override; + + // SigninErrorController::Observer: + void OnErrorChanged() override; + + // The Profile this service belongs to. + Profile* profile_; + + // The SigninErrorController that provides auth status. + SigninErrorController* error_controller_; + + // True if signin global error was added to the global error service. + bool is_added_to_global_error_service_; + + DISALLOW_COPY_AND_ASSIGN(SigninGlobalError); +}; + +#endif // CHROME_BROWSER_SIGNIN_SIGNIN_GLOBAL_ERROR_H_ diff --git a/chrome/browser/signin/signin_global_error_factory.cc b/chrome/browser/signin/signin_global_error_factory.cc new file mode 100644 index 000000000000..a1900a7dbef1 --- /dev/null +++ b/chrome/browser/signin/signin_global_error_factory.cc @@ -0,0 +1,52 @@ +// Copyright 2014 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/signin/signin_global_error_factory.h" + +#include "chrome/browser/browser_process.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/signin/signin_error_controller_factory.h" +#include "chrome/browser/signin/signin_global_error.h" +#include "chrome/browser/ui/global_error/global_error_service_factory.h" +#include "components/keyed_service/content/browser_context_dependency_manager.h" +#include "components/signin/core/browser/profile_oauth2_token_service.h" + +#if defined(USE_ASH) +#include "ash/shell.h" +#endif + +SigninGlobalErrorFactory::SigninGlobalErrorFactory() + : BrowserContextKeyedServiceFactory( + "SigninGlobalError", + BrowserContextDependencyManager::GetInstance()) { + DependsOn(SigninErrorControllerFactory::GetInstance()); + DependsOn(GlobalErrorServiceFactory::GetInstance()); +} + +SigninGlobalErrorFactory::~SigninGlobalErrorFactory() {} + +// static +SigninGlobalError* SigninGlobalErrorFactory::GetForProfile( + Profile* profile) { + return static_cast( + GetInstance()->GetServiceForBrowserContext(profile, true)); +} + +// static +SigninGlobalErrorFactory* SigninGlobalErrorFactory::GetInstance() { + return Singleton::get(); +} + +KeyedService* SigninGlobalErrorFactory::BuildServiceInstanceFor( + content::BrowserContext* context) const { +#if defined(USE_ASH) + if (ash::Shell::HasInstance()) + return NULL; +#endif + + Profile* profile = static_cast(context); + + return new SigninGlobalError( + SigninErrorControllerFactory::GetForProfile(profile), profile); +} diff --git a/chrome/browser/signin/signin_global_error_factory.h b/chrome/browser/signin/signin_global_error_factory.h new file mode 100644 index 000000000000..36bb94c57133 --- /dev/null +++ b/chrome/browser/signin/signin_global_error_factory.h @@ -0,0 +1,39 @@ +// Copyright 2014 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. + +#ifndef CHROME_BROWSER_SIGNIN_SIGNIN_GLOBAL_ERROR_FACTORY_H_ +#define CHROME_BROWSER_SIGNIN_SIGNIN_GLOBAL_ERROR_FACTORY_H_ + +#include "base/memory/singleton.h" +#include "components/keyed_service/content/browser_context_keyed_service_factory.h" + +class SigninGlobalError; +class Profile; + +// Singleton that owns all SigninGlobalErrors and associates them with +// Profiles. Listens for the Profile's destruction notification and cleans up +// the associated SigninGlobalError. +class SigninGlobalErrorFactory : public BrowserContextKeyedServiceFactory { + public: + // Returns the instance of SigninGlobalError associated with this + // profile, creating one if none exists. In Ash, this will return NULL. + static SigninGlobalError* GetForProfile(Profile* profile); + + // Returns an instance of the SigninGlobalErrorFactory singleton. + static SigninGlobalErrorFactory* GetInstance(); + + private: + friend struct DefaultSingletonTraits; + + SigninGlobalErrorFactory(); + ~SigninGlobalErrorFactory() override; + + // BrowserContextKeyedServiceFactory: + KeyedService* BuildServiceInstanceFor( + content::BrowserContext* profile) const override; + + DISALLOW_COPY_AND_ASSIGN(SigninGlobalErrorFactory); +}; + +#endif // CHROME_BROWSER_SIGNIN_SIGNIN_GLOBAL_ERROR_FACTORY_H_ diff --git a/chrome/browser/signin/signin_global_error_unittest.cc b/chrome/browser/signin/signin_global_error_unittest.cc new file mode 100644 index 000000000000..783aa3111d9f --- /dev/null +++ b/chrome/browser/signin/signin_global_error_unittest.cc @@ -0,0 +1,170 @@ +// Copyright (c) 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/signin/signin_global_error.h" + +#include "base/memory/scoped_ptr.h" +#include "base/prefs/pref_service.h" +#include "base/strings/utf_string_conversions.h" +#include "base/test/histogram_tester.h" +#include "chrome/browser/prefs/pref_service_syncable.h" +#include "chrome/browser/profiles/profile_info_cache.h" +#include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/profiles/profile_metrics.h" +#include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h" +#include "chrome/browser/signin/fake_signin_manager_builder.h" +#include "chrome/browser/signin/profile_oauth2_token_service_factory.h" +#include "chrome/browser/signin/signin_error_controller_factory.h" +#include "chrome/browser/signin/signin_global_error_factory.h" +#include "chrome/browser/signin/signin_manager_factory.h" +#include "chrome/browser/ui/global_error/global_error_service.h" +#include "chrome/browser/ui/global_error/global_error_service_factory.h" +#include "chrome/common/pref_names.h" +#include "chrome/test/base/testing_browser_process.h" +#include "chrome/test/base/testing_profile.h" +#include "chrome/test/base/testing_profile_manager.h" +#include "components/signin/core/browser/fake_auth_status_provider.h" +#include "components/signin/core/browser/signin_error_controller.h" +#include "components/signin/core/browser/signin_manager.h" +#include "content/public/test/test_browser_thread_bundle.h" +#include "testing/gtest/include/gtest/gtest.h" + +static const char kTestAccountId[] = "id-testuser@test.com"; +static const char kTestGaiaId[] = "gaiaid-testuser@test.com"; +static const char kTestUsername[] = "testuser@test.com"; + +class SigninGlobalErrorTest : public testing::Test { + public: + SigninGlobalErrorTest() : + profile_manager_(TestingBrowserProcess::GetGlobal()) {} + + void SetUp() override { + ASSERT_TRUE(profile_manager_.SetUp()); + + // Create a signed-in profile. + TestingProfile::TestingFactories testing_factories; + testing_factories.push_back(std::make_pair( + ProfileOAuth2TokenServiceFactory::GetInstance(), + BuildFakeProfileOAuth2TokenService)); + testing_factories.push_back(std::make_pair( + SigninManagerFactory::GetInstance(), BuildFakeSigninManagerBase)); + profile_ = profile_manager_.CreateTestingProfile( + "Person 1", scoped_ptr(), + base::UTF8ToUTF16("Person 1"), 0, std::string(), testing_factories); + + SigninManagerFactory::GetForProfile(profile()) + ->SetAuthenticatedAccountInfo(kTestAccountId, kTestUsername); + ProfileInfoCache& cache = + profile_manager_.profile_manager()->GetProfileInfoCache(); + cache.SetAuthInfoOfProfileAtIndex( + cache.GetIndexOfProfileWithPath(profile()->GetPath()), + kTestGaiaId, base::UTF8ToUTF16(kTestUsername)); + + global_error_ = SigninGlobalErrorFactory::GetForProfile(profile()); + error_controller_ = SigninErrorControllerFactory::GetForProfile(profile()); + } + + TestingProfile* profile() { return profile_; } + TestingProfileManager* testing_profile_manager() { + return &profile_manager_; + } + SigninGlobalError* global_error() { return global_error_; } + SigninErrorController* error_controller() { return error_controller_; } + + private: + content::TestBrowserThreadBundle thread_bundle_; + TestingProfileManager profile_manager_; + TestingProfile* profile_; + SigninGlobalError* global_error_; + SigninErrorController* error_controller_; +}; + +TEST_F(SigninGlobalErrorTest, NoErrorAuthStatusProviders) { + scoped_ptr provider; + + ASSERT_FALSE(global_error()->HasMenuItem()); + + // Add a provider. + provider.reset(new FakeAuthStatusProvider(error_controller())); + ASSERT_FALSE(global_error()->HasMenuItem()); + + // Remove the provider. + provider.reset(); + ASSERT_FALSE(global_error()->HasMenuItem()); +} + +TEST_F(SigninGlobalErrorTest, ErrorAuthStatusProvider) { + scoped_ptr provider; + scoped_ptr error_provider; + + provider.reset(new FakeAuthStatusProvider(error_controller())); + ASSERT_FALSE(global_error()->HasMenuItem()); + + error_provider.reset(new FakeAuthStatusProvider(error_controller())); + error_provider->SetAuthError( + kTestAccountId, + GoogleServiceAuthError( + GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS)); + ASSERT_TRUE(global_error()->HasMenuItem()); + + error_provider.reset(); + ASSERT_FALSE(global_error()->HasMenuItem()); + + provider.reset(); + error_provider.reset(); + ASSERT_FALSE(global_error()->HasMenuItem()); +} + +// Verify that SigninGlobalError ignores certain errors. +TEST_F(SigninGlobalErrorTest, AuthStatusEnumerateAllErrors) { + typedef struct { + GoogleServiceAuthError::State error_state; + bool is_error; + } ErrorTableEntry; + + ErrorTableEntry table[] = { + { GoogleServiceAuthError::NONE, false }, + { GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS, true }, + { GoogleServiceAuthError::USER_NOT_SIGNED_UP, true }, + { GoogleServiceAuthError::CONNECTION_FAILED, false }, + { GoogleServiceAuthError::CAPTCHA_REQUIRED, true }, + { GoogleServiceAuthError::ACCOUNT_DELETED, true }, + { GoogleServiceAuthError::ACCOUNT_DISABLED, true }, + { GoogleServiceAuthError::SERVICE_UNAVAILABLE, true }, + { GoogleServiceAuthError::TWO_FACTOR, true }, + { GoogleServiceAuthError::REQUEST_CANCELED, true }, + { GoogleServiceAuthError::HOSTED_NOT_ALLOWED, true }, + { GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE, true }, + { GoogleServiceAuthError::SERVICE_ERROR, true }, + { GoogleServiceAuthError::WEB_LOGIN_REQUIRED, true }, + }; + static_assert(arraysize(table) == GoogleServiceAuthError::NUM_STATES, + "table size should match number of auth error types"); + + // Mark the profile with an active timestamp so profile_metrics logs it. + testing_profile_manager()->UpdateLastUser(profile()); + + for (size_t i = 0; i < arraysize(table); ++i) { + base::HistogramTester histogram_tester; + FakeAuthStatusProvider provider(error_controller()); + provider.SetAuthError(kTestAccountId, + GoogleServiceAuthError(table[i].error_state)); + + EXPECT_EQ(global_error()->HasMenuItem(), table[i].is_error); + EXPECT_EQ(global_error()->MenuItemLabel().empty(), !table[i].is_error); + EXPECT_EQ(global_error()->GetBubbleViewMessages().empty(), + !table[i].is_error); + EXPECT_FALSE(global_error()->GetBubbleViewTitle().empty()); + EXPECT_FALSE(global_error()->GetBubbleViewAcceptButtonLabel().empty()); + EXPECT_TRUE(global_error()->GetBubbleViewCancelButtonLabel().empty()); + + ProfileMetrics::LogNumberOfProfiles( + testing_profile_manager()->profile_manager()); + + if (table[i].is_error) + histogram_tester.ExpectBucketCount("Signin.AuthError", i, 1); + histogram_tester.ExpectBucketCount( + "Profile.NumberOfProfilesWithAuthErrors", table[i].is_error, 1); + } +} diff --git a/chrome/browser/signin/signin_ui_util.cc b/chrome/browser/signin/signin_ui_util.cc index 8710548fe0bd..a5ed1122b1c5 100644 --- a/chrome/browser/signin/signin_ui_util.cc +++ b/chrome/browser/signin/signin_ui_util.cc @@ -10,6 +10,8 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/signin/account_tracker_service_factory.h" #include "chrome/browser/signin/signin_error_controller_factory.h" +#include "chrome/browser/signin/signin_global_error.h" +#include "chrome/browser/signin/signin_global_error_factory.h" #include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/profile_sync_service_factory.h" @@ -26,8 +28,73 @@ #include "ui/gfx/font_list.h" #include "ui/gfx/text_elider.h" +namespace { +// Maximum width of a username - we trim emails that are wider than this so +// the wrench menu doesn't get ridiculously wide. +const int kUsernameMaxWidth = 200; +} // namespace + namespace signin_ui_util { +GlobalError* GetSignedInServiceError(Profile* profile) { + std::vector errors = GetSignedInServiceErrors(profile); + if (errors.empty()) + return NULL; + return errors[0]; +} + +std::vector GetSignedInServiceErrors(Profile* profile) { + std::vector errors; + // Chrome OS doesn't use SigninGlobalError or SyncGlobalError. Other platforms + // may use these services to show auth and sync errors in the toolbar menu. +#if !defined(OS_CHROMEOS) + // Auth errors have the highest priority - after that, individual service + // errors. + SigninGlobalError* signin_error = + SigninGlobalErrorFactory::GetForProfile(profile); + if (signin_error && signin_error->HasError()) + errors.push_back(signin_error); + + // No auth error - now try other services. Currently the list is just hard- + // coded but in the future if we add more we can create some kind of + // registration framework. + if (profile->IsSyncAllowed()) { + SyncGlobalError* error = SyncGlobalErrorFactory::GetForProfile(profile); + if (error && error->HasMenuItem()) + errors.push_back(error); + } +#endif + + return errors; +} + +base::string16 GetSigninMenuLabel(Profile* profile) { + GlobalError* error = signin_ui_util::GetSignedInServiceError(profile); + if (error) + return error->MenuItemLabel(); + + // No errors, so just display the signed in user, if any. + ProfileSyncService* service = profile->IsSyncAllowed() ? + ProfileSyncServiceFactory::GetForProfile(profile) : NULL; + + // Even if the user is signed in, don't display the "signed in as..." + // label if we're still setting up sync. + if (!service || !service->FirstSetupInProgress()) { + std::string username; + SigninManagerBase* signin_manager = + SigninManagerFactory::GetForProfileIfExists(profile); + if (signin_manager) + username = signin_manager->GetAuthenticatedUsername(); + if (!username.empty() && !signin_manager->AuthInProgress()) { + const base::string16 elided = gfx::ElideText(base::UTF8ToUTF16(username), + gfx::FontList(), kUsernameMaxWidth, gfx::ELIDE_EMAIL); + return l10n_util::GetStringFUTF16(IDS_SYNC_MENU_SYNCED_LABEL, elided); + } + } + return l10n_util::GetStringFUTF16(IDS_SYNC_MENU_PRE_SYNCED_LABEL, + l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME)); +} + // Given an authentication state this helper function returns various labels // that can be used to display information about the state. void GetStatusLabelsForAuthError(Profile* profile, diff --git a/chrome/browser/signin/signin_ui_util.h b/chrome/browser/signin/signin_ui_util.h index 7e1a5dc54565..943573801cea 100644 --- a/chrome/browser/signin/signin_ui_util.h +++ b/chrome/browser/signin/signin_ui_util.h @@ -20,6 +20,17 @@ namespace signin_ui_util { // The maximum number of times to show the welcome tutorial for an upgrade user. const int kUpgradeWelcomeTutorialShowMax = 1; +// If a signed in service is reporting an error, returns the GlobalError +// object associated with that service, or NULL if no errors are reported. +GlobalError* GetSignedInServiceError(Profile* profile); + +// Returns all errors reported by signed in services. +std::vector GetSignedInServiceErrors(Profile* profile); + +// Returns the label that should be displayed in the signin menu (i.e. +// "Sign in to Chromium", "Signin Error...", etc). +base::string16 GetSigninMenuLabel(Profile* profile); + void GetStatusLabelsForAuthError(Profile* profile, const SigninManagerBase& signin_manager, base::string16* status_label, diff --git a/chrome/browser/ui/browser_command_controller.cc b/chrome/browser/ui/browser_command_controller.cc index c007a8ea1eea..abb123388f0c 100644 --- a/chrome/browser/ui/browser_command_controller.cc +++ b/chrome/browser/ui/browser_command_controller.cc @@ -229,6 +229,11 @@ BrowserCommandController::BrowserCommandController(Browser* browser) base::Bind(&BrowserCommandController::UpdateCommandsForFullscreenMode, base::Unretained(this))); #endif + pref_signin_allowed_.Init( + prefs::kSigninAllowed, + profile()->GetOriginalProfile()->GetPrefs(), + base::Bind(&BrowserCommandController::OnSigninAllowedPrefChange, + base::Unretained(this))); InitCommandState(); @@ -759,6 +764,9 @@ void BrowserCommandController::ExecuteCommandWithDisposition( case IDC_HELP_PAGE_VIA_MENU: ShowHelp(browser_, HELP_SOURCE_MENU); break; + case IDC_SHOW_SIGNIN: + ShowBrowserSigninOrSettings(browser_, signin_metrics::SOURCE_MENU); + break; case IDC_TOGGLE_SPEECH_INPUT: ToggleSpeechInput(browser_); break; @@ -777,6 +785,16 @@ void BrowserCommandController::ExecuteCommandWithDisposition( } } +//////////////////////////////////////////////////////////////////////////////// +// BrowserCommandController, SigninPrefObserver implementation: + +void BrowserCommandController::OnSigninAllowedPrefChange() { + // For unit tests, we don't have a window. + if (!window()) + return; + UpdateShowSyncState(IsShowingMainUI()); +} + // BrowserCommandController, TabStripModelObserver implementation: void BrowserCommandController::TabInsertedAt(WebContents* contents, @@ -966,6 +984,8 @@ void BrowserCommandController::InitCommandState() { } #endif + UpdateShowSyncState(true); + // Navigation commands command_updater_.UpdateCommandEnabled( IDC_HOME, @@ -1050,6 +1070,7 @@ void BrowserCommandController::UpdateSharedCommandsForIncognitoAvailability( command_updater->UpdateCommandEnabled(IDC_IMPORT_SETTINGS, !forced_incognito); command_updater->UpdateCommandEnabled(IDC_OPTIONS, !forced_incognito || guest_session); + command_updater->UpdateCommandEnabled(IDC_SHOW_SIGNIN, !forced_incognito); } void BrowserCommandController::UpdateCommandsForIncognitoAvailability() { @@ -1228,6 +1249,7 @@ void BrowserCommandController::UpdateCommandsForFullscreenMode() { #if defined(GOOGLE_CHROME_BUILD) command_updater_.UpdateCommandEnabled(IDC_FEEDBACK, show_main_ui); #endif + UpdateShowSyncState(show_main_ui); // Settings page/subpages are forced to open in normal mode. We disable these // commands for guest sessions and when incognito is forced. @@ -1277,6 +1299,11 @@ void BrowserCommandController::UpdateSaveAsState() { command_updater_.UpdateCommandEnabled(IDC_SAVE_PAGE, CanSavePage(browser_)); } +void BrowserCommandController::UpdateShowSyncState(bool show_main_ui) { + command_updater_.UpdateCommandEnabled( + IDC_SHOW_SYNC_SETUP, show_main_ui && pref_signin_allowed_.GetValue()); +} + // static void BrowserCommandController::UpdateOpenFileState( CommandUpdater* command_updater) { diff --git a/chrome/browser/ui/browser_command_controller.h b/chrome/browser/ui/browser_command_controller.h index 0e564c0b9acf..05ec35a47e0d 100644 --- a/chrome/browser/ui/browser_command_controller.h +++ b/chrome/browser/ui/browser_command_controller.h @@ -140,9 +140,15 @@ class BrowserCommandController : public CommandUpdaterDelegate, // Updates the printing command state. void UpdatePrintingState(); + // Updates the SHOW_SYNC_SETUP menu entry. + void OnSigninAllowedPrefChange(); + // Updates the save-page-as command state. void UpdateSaveAsState(); + // Updates the show-sync command state. + void UpdateShowSyncState(bool show_main_ui); + // Ask the Reload/Stop button to change its icon, and update the Stop command // state. |is_loading| is true if the current WebContents is loading. // |force| is true if the button should change its icon immediately. @@ -179,6 +185,7 @@ class BrowserCommandController : public CommandUpdaterDelegate, PrefChangeRegistrar profile_pref_registrar_; PrefChangeRegistrar local_pref_registrar_; + BooleanPrefMember pref_signin_allowed_; DISALLOW_COPY_AND_ASSIGN(BrowserCommandController); }; diff --git a/chrome/browser/ui/browser_command_controller_unittest.cc b/chrome/browser/ui/browser_command_controller_unittest.cc index 4629624bc050..a80c7580269c 100644 --- a/chrome/browser/ui/browser_command_controller_unittest.cc +++ b/chrome/browser/ui/browser_command_controller_unittest.cc @@ -139,6 +139,7 @@ TEST_F(BrowserCommandControllerTest, IsReservedCommandOrKeyIsApp) { TEST_F(BrowserCommandControllerTest, IncognitoCommands) { EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS)); EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS)); + EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_SHOW_SIGNIN)); TestingProfile* testprofile = browser()->profile()->AsTestingProfile(); EXPECT_TRUE(testprofile); @@ -148,6 +149,7 @@ TEST_F(BrowserCommandControllerTest, IncognitoCommands) { browser()->command_controller()->command_updater(), testprofile); EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS)); EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS)); + EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_SIGNIN)); testprofile->SetGuestSession(false); IncognitoModePrefs::SetAvailability(browser()->profile()->GetPrefs(), @@ -157,6 +159,7 @@ TEST_F(BrowserCommandControllerTest, IncognitoCommands) { browser()->command_controller()->command_updater(), testprofile); EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS)); EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS)); + EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_SIGNIN)); } TEST_F(BrowserCommandControllerTest, AppFullScreen) { @@ -418,3 +421,35 @@ TEST_F(BrowserCommandControllerFullscreenTest, EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS)); EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS)); } + +TEST_F(BrowserCommandControllerTest, IncognitoModeOnSigninAllowedPrefChange) { + // Set up a profile with an off the record profile. + scoped_ptr profile1 = TestingProfile::Builder().Build(); + Profile* profile2 = profile1->GetOffTheRecordProfile(); + + EXPECT_EQ(profile2->GetOriginalProfile(), profile1.get()); + + // Create a new browser based on the off the record profile. + Browser::CreateParams profile_params(profile1->GetOffTheRecordProfile(), + chrome::GetActiveDesktop()); + scoped_ptr browser2( + chrome::CreateBrowserWithTestWindowForParams(&profile_params)); + + chrome::BrowserCommandController command_controller(browser2.get()); + const CommandUpdater* command_updater = command_controller.command_updater(); + + // Check that the SYNC_SETUP command is updated on preference change. + EXPECT_TRUE(command_updater->IsCommandEnabled(IDC_SHOW_SYNC_SETUP)); + profile1->GetPrefs()->SetBoolean(prefs::kSigninAllowed, false); + EXPECT_FALSE(command_updater->IsCommandEnabled(IDC_SHOW_SYNC_SETUP)); +} + +TEST_F(BrowserCommandControllerTest, OnSigninAllowedPrefChange) { + chrome::BrowserCommandController command_controller(browser()); + const CommandUpdater* command_updater = command_controller.command_updater(); + + // Check that the SYNC_SETUP command is updated on preference change. + EXPECT_TRUE(command_updater->IsCommandEnabled(IDC_SHOW_SYNC_SETUP)); + profile()->GetPrefs()->SetBoolean(prefs::kSigninAllowed, false); + EXPECT_FALSE(command_updater->IsCommandEnabled(IDC_SHOW_SYNC_SETUP)); +} diff --git a/chrome/browser/ui/cocoa/browser_window_controller.mm b/chrome/browser/ui/cocoa/browser_window_controller.mm index 4598c967ac0c..b86492f79da7 100644 --- a/chrome/browser/ui/cocoa/browser_window_controller.mm +++ b/chrome/browser/ui/cocoa/browser_window_controller.mm @@ -1148,6 +1148,14 @@ using content::WebContents; } break; } + case IDC_SHOW_SIGNIN: { + Profile* original_profile = + browser_->profile()->GetOriginalProfile(); + [AppController updateSigninItem:item + shouldShow:enable + currentProfile:original_profile]; + break; + } case IDC_BOOKMARK_PAGE: { // Extensions have the ability to hide the bookmark page menu item. // This only affects the bookmark page menu item under the main menu. diff --git a/chrome/browser/ui/cocoa/panels/panel_cocoa_unittest.mm b/chrome/browser/ui/cocoa/panels/panel_cocoa_unittest.mm index a11e498b9663..1e3f17fe9334 100644 --- a/chrome/browser/ui/cocoa/panels/panel_cocoa_unittest.mm +++ b/chrome/browser/ui/cocoa/panels/panel_cocoa_unittest.mm @@ -308,6 +308,7 @@ TEST_F(PanelCocoaTest, MenuItems) { NSMenuItem* fullscreen_menu_item = CreateMenuItem(menu, IDC_FULLSCREEN); NSMenuItem* presentation_menu_item = CreateMenuItem(menu, IDC_PRESENTATION_MODE); + NSMenuItem* sync_menu_item = CreateMenuItem(menu, IDC_SHOW_SYNC_SETUP); NSMenuItem* dev_tools_item = CreateMenuItem(menu, IDC_DEV_TOOLS); NSMenuItem* dev_tools_console_item = CreateMenuItem(menu, IDC_DEV_TOOLS_CONSOLE); @@ -326,6 +327,7 @@ TEST_F(PanelCocoaTest, MenuItems) { EXPECT_FALSE([find_next_menu_item isEnabled]); EXPECT_FALSE([fullscreen_menu_item isEnabled]); EXPECT_FALSE([presentation_menu_item isEnabled]); + EXPECT_FALSE([sync_menu_item isEnabled]); // These are not enabled by Panel, so they are expected to be disabled for // this unit_test. In real Chrome app, they are enabled by Chrome NSApp // controller. PanelCocoaBrowsertest.MenuItems verifies that. @@ -336,6 +338,11 @@ TEST_F(PanelCocoaTest, MenuItems) { EXPECT_TRUE([dev_tools_item isEnabled]); EXPECT_TRUE([dev_tools_console_item isEnabled]); + // Verify that commandDispatch on an invalid menu item does not crash. + [NSApp sendAction:[sync_menu_item action] + to:[sync_menu_item target] + from:sync_menu_item]; + ClosePanelAndWait(panel); } diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.cc b/chrome/browser/ui/toolbar/wrench_menu_model.cc index b3ffec09a6b2..5f9ca12ae7d2 100644 --- a/chrome/browser/ui/toolbar/wrench_menu_model.cc +++ b/chrome/browser/ui/toolbar/wrench_menu_model.cc @@ -366,7 +366,8 @@ bool WrenchMenuModel::IsItemForCommandIdDynamic(int command_id) const { #elif defined(OS_WIN) command_id == IDC_PIN_TO_START_SCREEN || #endif - command_id == IDC_UPGRADE_DIALOG; + command_id == IDC_UPGRADE_DIALOG || + (!switches::IsNewAvatarMenu() && command_id == IDC_SHOW_SIGNIN); } base::string16 WrenchMenuModel::GetLabelForCommandId(int command_id) const { @@ -396,6 +397,10 @@ base::string16 WrenchMenuModel::GetLabelForCommandId(int command_id) const { #endif case IDC_UPGRADE_DIALOG: return GetUpgradeDialogMenuItemName(); + case IDC_SHOW_SIGNIN: + DCHECK(!switches::IsNewAvatarMenu()); + return signin_ui_util::GetSigninMenuLabel( + browser_->profile()->GetOriginalProfile()); default: NOTREACHED(); return base::string16(); @@ -414,6 +419,19 @@ bool WrenchMenuModel::GetIconForCommandId(int command_id, } return false; } + case IDC_SHOW_SIGNIN: { + DCHECK(!switches::IsNewAvatarMenu()); + GlobalError* error = signin_ui_util::GetSignedInServiceError( + browser_->profile()->GetOriginalProfile()); + if (error) { + int icon_id = error->MenuItemIconResourceID(); + if (icon_id) { + *icon = rb.GetNativeImageNamed(icon_id); + return true; + } + } + return false; + } default: break; } @@ -428,6 +446,16 @@ void WrenchMenuModel::ExecuteCommand(int command_id, int event_flags) { return; } + if (!switches::IsNewAvatarMenu() && command_id == IDC_SHOW_SIGNIN) { + // If a custom error message is being shown, handle it. + GlobalError* error = signin_ui_util::GetSignedInServiceError( + browser_->profile()->GetOriginalProfile()); + if (error) { + error->ExecuteMenuItem(browser_); + return; + } + } + LogMenuMetrics(command_id); chrome::ExecuteCommand(browser_, command_id); } @@ -672,6 +700,13 @@ void WrenchMenuModel::LogMenuMetrics(int command_id) { } LogMenuAction(MENU_ACTION_SHOW_DOWNLOADS); break; + case IDC_SHOW_SYNC_SETUP: + if (!uma_action_recorded_) { + UMA_HISTOGRAM_MEDIUM_TIMES("WrenchMenu.TimeToAction.ShowSyncSetup", + delta); + } + LogMenuAction(MENU_ACTION_SHOW_SYNC_SETUP); + break; case IDC_OPTIONS: if (!uma_action_recorded_) UMA_HISTOGRAM_MEDIUM_TIMES("WrenchMenu.TimeToAction.Settings", delta); @@ -895,7 +930,21 @@ void WrenchMenuModel::Build() { CreateCutCopyPasteMenu(); AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS); - +#if !defined(OS_CHROMEOS) + if (!switches::IsNewAvatarMenu()) { + // No "Sign in to Chromium..." menu item on ChromeOS. + SigninManager* signin = SigninManagerFactory::GetForProfile( + browser_->profile()->GetOriginalProfile()); + if (signin && signin->IsSigninAllowed() && + signin_ui_util::GetSignedInServiceErrors( + browser_->profile()->GetOriginalProfile()).empty()) { + AddItem(IDC_SHOW_SYNC_SETUP, + l10n_util::GetStringFUTF16( + IDS_SYNC_MENU_PRE_SYNCED_LABEL, + l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME))); + } + } +#endif // The help submenu is only displayed on official Chrome builds. As the // 'About' item has been moved to this submenu, it's reinstated here for // Chromium builds. @@ -941,6 +990,11 @@ bool WrenchMenuModel::AddGlobalErrorMenuItems() { // it won't show in the existing wrench menu. To fix this we need to some // how update the menu if new errors are added. ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); + // GetSignedInServiceErrors() can modify the global error list, so call it + // before iterating through that list below. + std::vector signin_errors; + signin_errors = signin_ui_util::GetSignedInServiceErrors( + browser_->profile()->GetOriginalProfile()); const GlobalErrorService::GlobalErrorList& errors = GlobalErrorServiceFactory::GetForProfile(browser_->profile())->errors(); bool menu_items_added = false; @@ -949,6 +1003,19 @@ bool WrenchMenuModel::AddGlobalErrorMenuItems() { GlobalError* error = *it; DCHECK(error); if (error->HasMenuItem()) { +#if !defined(OS_CHROMEOS) + // Don't add a signin error if it's already being displayed elsewhere. + if (std::find(signin_errors.begin(), signin_errors.end(), error) != + signin_errors.end()) { + MenuModel* model = this; + int index = 0; + if (MenuModel::GetModelAndIndexForCommandId( + IDC_SHOW_SIGNIN, &model, &index)) { + continue; + } + } +#endif + AddItem(error->MenuItemCommandID(), error->MenuItemLabel()); int icon_id = error->MenuItemIconResourceID(); if (icon_id) { diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.h b/chrome/browser/ui/toolbar/wrench_menu_model.h index 1753e6640967..525f47dfe48f 100644 --- a/chrome/browser/ui/toolbar/wrench_menu_model.h +++ b/chrome/browser/ui/toolbar/wrench_menu_model.h @@ -62,6 +62,7 @@ enum WrenchMenuAction { MENU_ACTION_FULLSCREEN, MENU_ACTION_SHOW_HISTORY, MENU_ACTION_SHOW_DOWNLOADS, + MENU_ACTION_SHOW_SYNC_SETUP, MENU_ACTION_OPTIONS, MENU_ACTION_ABOUT, MENU_ACTION_HELP_PAGE_VIA_MENU, diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.cc b/chrome/browser/ui/views/toolbar/toolbar_view.cc index 14fa65220d89..5f1b2a608f0a 100644 --- a/chrome/browser/ui/views/toolbar/toolbar_view.cc +++ b/chrome/browser/ui/views/toolbar/toolbar_view.cc @@ -79,6 +79,7 @@ #endif #if !defined(OS_CHROMEOS) +#include "chrome/browser/signin/signin_global_error_factory.h" #include "chrome/browser/sync/sync_global_error_factory.h" #endif @@ -219,15 +220,18 @@ void ToolbarView::Init() { LoadImages(); // Start global error services now so we badge the menu correctly. -#if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) +#if !defined(OS_CHROMEOS) if (!HasAshShell()) { + SigninGlobalErrorFactory::GetForProfile(browser_->profile()); +#if !defined(OS_ANDROID) SyncGlobalErrorFactory::GetForProfile(browser_->profile()); - } #endif + } #if defined(OS_WIN) RecoveryInstallGlobalErrorFactory::GetForProfile(browser_->profile()); #endif +#endif // OS_CHROMEOS // Add any necessary badges to the menu item based on the system state. // Do this after |app_menu_| has been added as a bubble may be shown that diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 24464125d25a..167a9026bcd1 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -915,6 +915,13 @@ 'browser/renderer_host/pepper/pepper_platform_verification_message_filter.cc', 'browser/renderer_host/pepper/pepper_platform_verification_message_filter.h', ], + # Used everywhere but ChromeOS. + 'chrome_browser_non_chromeos_sources': [ + 'browser/signin/signin_global_error.cc', + 'browser/signin/signin_global_error.h', + 'browser/signin/signin_global_error_factory.cc', + 'browser/signin/signin_global_error_factory.h', + ], # Everything but Android, iOS, and CrOS. 'chrome_browser_desktop_sources': [ 'browser/platform_util.cc', @@ -3528,6 +3535,8 @@ '../components/components.gyp:user_manager', '../ui/chromeos/ui_chromeos.gyp:ui_chromeos_resources', ], + }, { # Non-ChromeOS. + 'sources': [ '<@(chrome_browser_non_chromeos_sources)' ], }], ['use_cups==1', { 'dependencies': [ diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 25ac1214bcc5..76aa216db74e 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -222,6 +222,7 @@ 'browser/signin/account_reconcilor_unittest.cc', 'browser/signin/chrome_signin_client_unittest.cc', 'browser/signin/local_auth_unittest.cc', + 'browser/signin/signin_global_error_unittest.cc', 'browser/signin/signin_manager_unittest.cc', 'browser/signin/signin_tracker_unittest.cc', 'browser/signin/test_signin_client_builder.cc', @@ -2450,6 +2451,7 @@ '../ui/chromeos/ui_chromeos.gyp:ui_chromeos_resources', ], 'sources!': [ + 'browser/signin/signin_global_error_unittest.cc', 'browser/signin/signin_manager_unittest.cc', 'browser/signin/signin_names_io_thread_unittest.cc', ], diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index b9ef7cc10bb7..0cad4ab68f73 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -1676,7 +1676,10 @@ if (!is_android) { } if (is_chromeos) { deps += [ "//chrome/browser/chromeos:unit_tests" ] - sources -= [ "../browser/signin/signin_manager_unittest.cc" ] + sources -= [ + "../browser/signin/signin_global_error_unittest.cc", + "../browser/signin/signin_manager_unittest.cc", + ] } if (use_x11) { deps += [ "//ui/events/devices" ] -- 2.11.4.GIT