[signin] Signin interception uses the color of the target profile

The color for the new profile is now created earlier (before the bubble
is shown), and passed to the bubble in the BubbleParameters structure.
This required moving the color out of loadTimeData.

Fixed: 1126238
Change-Id: Ia544b51370011addebd1ec1ba786db23cdfe6ce3
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2398511Reviewed-by: Monica Basta <msalama@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805309}

chrome/browser/resources/signin/dice_web_signin_intercept/dice_web_signin_intercept_app.js

@@ -34,7 +34,6 @@ Polymer({
 
   /** @override */
   attached() {
-    this.setColors_();
     this.diceWebSigninInterceptBrowserProxy_ =
         DiceWebSigninInterceptBrowserProxyImpl.getInstance();
     this.addWebUIListener(
@@ -54,15 +53,6 @@ Polymer({
     this.diceWebSigninInterceptBrowserProxy_.cancel();
   },
 
-  /** @private */
-  setColors_() {
-    this.style.setProperty(
-        '--header-background-color',
-        loadTimeData.getString('headerBackgroundColor'));
-    this.style.setProperty(
-        '--header-text-color', loadTimeData.getString('headerTextColor'));
-  },
-
   /**
    * Called when the interception parameters are updated.
    * @param {!InterceptionParameters} parameters
@@ -70,6 +60,9 @@ Polymer({
    */
   handleParametersChanged_(parameters) {
     this.interceptionParameters_ = parameters;
+    this.style.setProperty(
+        '--header-background-color', parameters.headerBackgroundColor);
+    this.style.setProperty('--header-text-color', parameters.headerTextColor);
     this.notifyPath('interceptionParameters_.interceptedAccount.isManaged');
   },
 });

chrome/browser/resources/signin/dice_web_signin_intercept/dice_web_signin_intercept_browser_proxy.js

@@ -22,6 +22,8 @@ export let AccountInfo;
  *   headerText: string,
  *   bodyTitle: string,
  *   bodyText: string,
+ *   headerTextColor: string,
+ *   headerBackgroundColor: string,
  *   interceptedAccount: AccountInfo,
  * }}
  */

chrome/browser/signin/dice_web_signin_interceptor.cc

@@ -28,6 +28,7 @@
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/signin/profile_colors_util.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/themes/autogenerated_theme_util.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
@@ -72,13 +73,6 @@ AccountInfo GetPrimaryAccountInfo(signin::IdentityManager* manager) {
 
 }  // namespace
 
-bool DiceWebSigninInterceptor::Delegate::BubbleParameters::operator==(
-    const DiceWebSigninInterceptor::Delegate::BubbleParameters& rhs) const {
-  return interception_type == rhs.interception_type &&
-         intercepted_account == rhs.intercepted_account &&
-         primary_account == rhs.primary_account;
-}
-
 DiceWebSigninInterceptor::DiceWebSigninInterceptor(
     Profile* profile,
     std::unique_ptr<Delegate> delegate)
@@ -137,7 +131,8 @@ void DiceWebSigninInterceptor::MaybeInterceptWebSignin(
                                          ->GetProfileAttributesStorage())) {
     Delegate::BubbleParameters bubble_parameters{
         SigninInterceptionType::kProfileSwitch, *account_info,
-        GetPrimaryAccountInfo(identity_manager_)};
+        GetPrimaryAccountInfo(identity_manager_),
+        SkColor()};  // TODO: use the color of the target profile.
     delegate_->ShowSigninInterceptionBubble(
         web_contents, bubble_parameters,
         base::BindOnce(&DiceWebSigninInterceptor::OnProfileSwitchChoice,
@@ -298,12 +293,14 @@ void DiceWebSigninInterceptor::OnExtendedAccountInfoUpdated(
     return;
   }
 
+  SkColor profile_color = GenerateNewProfileColor().color;
   Delegate::BubbleParameters bubble_parameters{
-      *interception_type, info, GetPrimaryAccountInfo(identity_manager_)};
+      *interception_type, info, GetPrimaryAccountInfo(identity_manager_),
+      GetAutogeneratedThemeColors(profile_color).frame_color};
   delegate_->ShowSigninInterceptionBubble(
       web_contents(), bubble_parameters,
       base::BindOnce(&DiceWebSigninInterceptor::OnProfileCreationChoice,
-                     base::Unretained(this)));
+                     base::Unretained(this), profile_color));
   was_interception_ui_displayed_ = true;
   RecordSigninInterceptionHeuristicOutcome(
       *interception_type == SigninInterceptionType::kEnterprise
@@ -317,7 +314,8 @@ void DiceWebSigninInterceptor::OnExtendedAccountInfoFetchTimeout() {
   Reset();
 }
 
-void DiceWebSigninInterceptor::OnProfileCreationChoice(bool create) {
+void DiceWebSigninInterceptor::OnProfileCreationChoice(SkColor profile_color,
+                                                       bool create) {
   if (!create) {
     Reset();
     return;
@@ -345,7 +343,7 @@ void DiceWebSigninInterceptor::OnProfileCreationChoice(bool create) {
           profile_, account_id_, profile_name,
           profiles::GetPlaceholderAvatarIndex(),
           base::BindOnce(&DiceWebSigninInterceptor::OnNewSignedInProfileCreated,
-                         base::Unretained(this)));
+                         base::Unretained(this), profile_color));
 }
 
 void DiceWebSigninInterceptor::OnProfileSwitchChoice(bool switch_profile) {
@@ -360,6 +358,7 @@ void DiceWebSigninInterceptor::OnProfileSwitchChoice(bool switch_profile) {
 }
 
 void DiceWebSigninInterceptor::OnNewSignedInProfileCreated(
+    SkColor profile_color,
     Profile* new_profile) {
   DCHECK(dice_signed_in_profile_creator_);
   dice_signed_in_profile_creator_.reset();
@@ -374,9 +373,9 @@ void DiceWebSigninInterceptor::OnNewSignedInProfileCreated(
     return;
   }
 
-  // Apply a new color to the profile.
+  // Apply the new color to the profile.
   ThemeServiceFactory::GetForProfile(new_profile)
-      ->BuildAutogeneratedThemeFromColor(GenerateNewProfileColor().color);
+      ->BuildAutogeneratedThemeFromColor(profile_color);
 
   // Work is done in this profile, the flow continues in the
   // DiceWebSigninInterceptor that is attached to the new profile.

chrome/browser/signin/dice_web_signin_interceptor.h

@@ -17,6 +17,7 @@
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "google_apis/gaia/core_account_id.h"
+#include "third_party/skia/include/core/SkColor.h"
 
 namespace content {
 class WebContents;
@@ -83,11 +84,10 @@ class DiceWebSigninInterceptor : public KeyedService,
    public:
     // Parameters for interception bubble UIs.
     struct BubbleParameters {
-      bool operator==(const BubbleParameters& rhs) const;
-
       SigninInterceptionType interception_type;
       AccountInfo intercepted_account;
       AccountInfo primary_account;
+      SkColor profile_highlight_color;
     };
 
     virtual ~Delegate() = default;
@@ -173,13 +173,13 @@ class DiceWebSigninInterceptor : public KeyedService,
   void OnExtendedAccountInfoFetchTimeout();
 
   // Called after the user chose whether a new profile would be created.
-  void OnProfileCreationChoice(bool create);
+  void OnProfileCreationChoice(SkColor profile_color, bool create);
   // Called after the user chose whether the session should continue in a new
   // profile.
   void OnProfileSwitchChoice(bool switch_profile);
 
   // Called when the new profile is created.
-  void OnNewSignedInProfileCreated(Profile* new_profile);
+  void OnNewSignedInProfileCreated(SkColor profile_color, Profile* new_profile);
 
   // Deletes session_startup_helper_
   void DeleteSessionStartupHelper();

chrome/browser/signin/dice_web_signin_interceptor_unittest.cc

@@ -43,6 +43,26 @@ class MockDiceWebSigninInterceptorDelegate
               (override));
 };
 
+// Matches BubbleParameters fields excepting the color. This is useful in the
+// test because the color is randomly generated.
+testing::Matcher<const DiceWebSigninInterceptor::Delegate::BubbleParameters&>
+MatchBubbleParameters(
+    const DiceWebSigninInterceptor::Delegate::BubbleParameters& parameters) {
+  return testing::AllOf(
+      testing::Field("interception_type",
+                     &DiceWebSigninInterceptor::Delegate::BubbleParameters::
+                         interception_type,
+                     parameters.interception_type),
+      testing::Field("intercepted_account",
+                     &DiceWebSigninInterceptor::Delegate::BubbleParameters::
+                         intercepted_account,
+                     parameters.intercepted_account),
+      testing::Field("primary_account",
+                     &DiceWebSigninInterceptor::Delegate::BubbleParameters::
+                         primary_account,
+                     parameters.primary_account));
+}
+
 // If the account info is valid, does nothing. Otherwise fills the extended
 // fields with default values.
 void MakeValidAccountInfo(AccountInfo* info) {
@@ -335,10 +355,11 @@ TEST_F(DiceWebSigninInterceptorTest, NoInterception) {
   histogram_tester = std::make_unique<base::HistogramTester>();
   DiceWebSigninInterceptor::Delegate::BubbleParameters expected_parameters = {
       DiceWebSigninInterceptor::SigninInterceptionType::kProfileSwitch,
-      account_info, AccountInfo()};
+      account_info, AccountInfo(), SkColor()};
   EXPECT_CALL(*mock_delegate(),
-              ShowSigninInterceptionBubble(web_contents(), expected_parameters,
-                                           testing::_));
+              ShowSigninInterceptionBubble(
+                  web_contents(), MatchBubbleParameters(expected_parameters),
+                  testing::_));
   interceptor()->MaybeInterceptWebSignin(web_contents(),
                                          account_info.account_id,
                                          /*is_new_account=*/true,
@@ -362,11 +383,12 @@ TEST_F(DiceWebSigninInterceptorTest, InterceptionInProgress) {
   // Start an interception.
   DiceWebSigninInterceptor::Delegate::BubbleParameters expected_parameters = {
       DiceWebSigninInterceptor::SigninInterceptionType::kProfileSwitch,
-      account_info, AccountInfo()};
+      account_info, AccountInfo(), SkColor()};
   base::OnceCallback<void(bool)> delegate_callback;
   EXPECT_CALL(*mock_delegate(),
-              ShowSigninInterceptionBubble(web_contents(), expected_parameters,
-                                           testing::_))
+              ShowSigninInterceptionBubble(
+                  web_contents(), MatchBubbleParameters(expected_parameters),
+                  testing::_))
       .WillOnce(testing::WithArg<2>(testing::Invoke(
           [&delegate_callback](base::OnceCallback<void(bool)> callback) {
             delegate_callback = std::move(callback);
@@ -389,8 +411,9 @@ TEST_F(DiceWebSigninInterceptorTest, InterceptionInProgress) {
 
   // A new interception can now start.
   EXPECT_CALL(*mock_delegate(),
-              ShowSigninInterceptionBubble(web_contents(), expected_parameters,
-                                           testing::_));
+              ShowSigninInterceptionBubble(
+                  web_contents(), MatchBubbleParameters(expected_parameters),
+                  testing::_));
   MaybeIntercept(account_info.account_id);
 }
 
@@ -441,10 +464,11 @@ TEST_F(DiceWebSigninInterceptorTest, ProfileCreationDisallowed) {
   // Profile switch interception still works.
   DiceWebSigninInterceptor::Delegate::BubbleParameters expected_parameters = {
       DiceWebSigninInterceptor::SigninInterceptionType::kProfileSwitch,
-      account_info, AccountInfo()};
+      account_info, AccountInfo(), SkColor()};
   EXPECT_CALL(*mock_delegate(),
-              ShowSigninInterceptionBubble(web_contents(), expected_parameters,
-                                           testing::_));
+              ShowSigninInterceptionBubble(
+                  web_contents(), MatchBubbleParameters(expected_parameters),
+                  testing::_));
   MaybeIntercept(account_info.account_id);
 }
 
@@ -462,10 +486,11 @@ TEST_F(DiceWebSigninInterceptorTest, WaitForAccountInfoAvailable) {
   // Account info becomes available, interception happens.
   DiceWebSigninInterceptor::Delegate::BubbleParameters expected_parameters = {
       DiceWebSigninInterceptor::SigninInterceptionType::kEnterprise,
-      account_info, primary_account_info};
+      account_info, primary_account_info, SkColor()};
   EXPECT_CALL(*mock_delegate(),
-              ShowSigninInterceptionBubble(web_contents(), expected_parameters,
-                                           testing::_));
+              ShowSigninInterceptionBubble(
+                  web_contents(), MatchBubbleParameters(expected_parameters),
+                  testing::_));
   MakeValidAccountInfo(&account_info);
   account_info.hosted_domain = "example.com";
   identity_test_env()->UpdateAccountInfoForAccount(account_info);
@@ -487,10 +512,11 @@ TEST_F(DiceWebSigninInterceptorTest, AccountInfoAlreadyAvailable) {
   // Account info is already available, interception happens immediately.
   DiceWebSigninInterceptor::Delegate::BubbleParameters expected_parameters = {
       DiceWebSigninInterceptor::SigninInterceptionType::kEnterprise,
-      account_info, primary_account_info};
+      account_info, primary_account_info, SkColor()};
   EXPECT_CALL(*mock_delegate(),
-              ShowSigninInterceptionBubble(web_contents(), expected_parameters,
-                                           testing::_));
+              ShowSigninInterceptionBubble(
+                  web_contents(), MatchBubbleParameters(expected_parameters),
+                  testing::_));
   MaybeIntercept(account_info.account_id);
   histogram_tester.ExpectTotalCount("Signin.Intercept.AccountInfoFetchDuration",
                                     1);
@@ -512,10 +538,11 @@ TEST_F(DiceWebSigninInterceptorTest, MultiUserInterception) {
   // Account info is already available, interception happens immediately.
   DiceWebSigninInterceptor::Delegate::BubbleParameters expected_parameters = {
       DiceWebSigninInterceptor::SigninInterceptionType::kMultiUser,
-      account_info, primary_account_info};
+      account_info, primary_account_info, SkColor()};
   EXPECT_CALL(*mock_delegate(),
-              ShowSigninInterceptionBubble(web_contents(), expected_parameters,
-                                           testing::_));
+              ShowSigninInterceptionBubble(
+                  web_contents(), MatchBubbleParameters(expected_parameters),
+                  testing::_));
   MaybeIntercept(account_info.account_id);
   histogram_tester.ExpectUniqueSample(
       "Signin.Intercept.HeuristicOutcome",

chrome/browser/ui/webui/signin/dice_web_signin_intercept_handler.cc

@@ -13,11 +13,13 @@
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
+#include "chrome/browser/ui/signin/profile_colors_util.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/web_ui.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/webui/web_ui_util.h"
+#include "ui/gfx/color_utils.h"
 #include "ui/gfx/image/image.h"
 #include "url/gurl.h"
 
@@ -159,6 +161,13 @@ base::Value DiceWebSigninInterceptHandler::GetInterceptionParametersValue() {
   parameters.SetStringKey("bodyText", GetBodyText());
   parameters.SetKey("interceptedAccount",
                     GetAccountInfoValue(intercepted_account()));
+  parameters.SetStringKey("headerBackgroundColor",
+                          color_utils::SkColorToRgbaString(
+                              bubble_parameters_.profile_highlight_color));
+  parameters.SetStringKey(
+      "headerTextColor",
+      color_utils::SkColorToRgbaString(GetProfileForegroundTextColor(
+          bubble_parameters_.profile_highlight_color)));
   return parameters;
 }
 

chrome/browser/ui/webui/signin/dice_web_signin_intercept_ui.cc

@@ -7,7 +7,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
-#include "chrome/browser/ui/signin/profile_colors_util.h"
 #include "chrome/browser/ui/webui/signin/dice_web_signin_intercept_handler.h"
 #include "chrome/browser/ui/webui/webui_util.h"
 #include "chrome/common/webui_url_constants.h"
@@ -18,7 +17,6 @@
 #include "services/network/public/mojom/content_security_policy.mojom.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/webui/web_ui_util.h"
-#include "ui/gfx/color_utils.h"
 #include "ui/resources/grit/webui_resources.h"
 
 DiceWebSigninInterceptUI::DiceWebSigninInterceptUI(content::WebUI* web_ui)
@@ -34,15 +32,6 @@ DiceWebSigninInterceptUI::DiceWebSigninInterceptUI(content::WebUI* web_ui)
   source->AddResourcePath("signin_shared_css.js", IDR_SIGNIN_SHARED_CSS_JS);
   source->AddResourcePath("signin_vars_css.js", IDR_SIGNIN_VARS_CSS_JS);
 
-  Profile* profile = Profile::FromWebUI(web_ui);
-  SkColor background_color =
-      GetThemeColorsForProfile(profile).profile_highlight_color;
-  source->AddString("headerBackgroundColor",
-                    color_utils::SkColorToRgbaString(background_color));
-  source->AddString("headerTextColor",
-                    color_utils::SkColorToRgbaString(
-                        GetProfileForegroundTextColor(background_color)));
-
   // Localized strings.
   source->UseStringsJs();
   source->EnableReplaceI18nInJS();
@@ -62,7 +51,7 @@ DiceWebSigninInterceptUI::DiceWebSigninInterceptUI(content::WebUI* web_ui)
   source->AddResourcePath("test_loader.js", IDR_WEBUI_JS_TEST_LOADER);
   source->AddResourcePath("test_loader.html", IDR_WEBUI_HTML_TEST_LOADER);
 
-  content::WebUIDataSource::Add(profile, source);
+  content::WebUIDataSource::Add(Profile::FromWebUI(web_ui), source);
 }
 
 DiceWebSigninInterceptUI::~DiceWebSigninInterceptUI() = default;

chrome/test/data/webui/signin/dice_web_signin_intercept_test.js

@@ -35,6 +35,8 @@ suite('DiceWebSigninInterceptTest', function() {
       headerText: 'header_text',
       bodyTitle: 'body_title',
       bodyText: 'body_text',
+      headerTextColor: 'rgba(255, 255, 255, 1)',
+      headerBackgroundColor: 'rgba(255, 0, 0, 1)',
       interceptedAccount: {isManaged: false, pictureUrl: AVATAR_URL_1},
       primaryAccount: {isManaged: false, pictureUrl: AVATAR_URL_2}
     });
@@ -89,6 +91,8 @@ suite('DiceWebSigninInterceptTest', function() {
       headerText: 'new_header_text',
       bodyTitle: 'new_body_title',
       bodyText: 'new_body_text',
+      headerTextColor: 'rgba(255, 255, 255, 1)',
+      headerBackgroundColor: 'rgba(255, 0, 0, 1)',
       interceptedAccount: {isManaged: false, pictureUrl: AVATAR_URL_1},
     });
     checkTextValues('new_header_text', 'new_body_title', 'new_body_text');
@@ -103,6 +107,8 @@ suite('DiceWebSigninInterceptTest', function() {
       headerText: 'header_text',
       bodyTitle: 'body_title',
       bodyText: 'body_text',
+      headerTextColor: 'rgba(255, 255, 255, 1)',
+      headerBackgroundColor: 'rgba(255, 0, 0, 1)',
       interceptedAccount: {isManaged: false, pictureUrl: AVATAR_URL_2},
     };
 

chrome/test/data/webui/signin/test_dice_web_signin_intercept_browser_proxy.js

@@ -15,6 +15,8 @@ export class TestDiceWebSigninInterceptBrowserProxy extends TestBrowserProxy {
       headerText: '',
       bodyTitle: '',
       bodyText: '',
+      headerTextColor: '',
+      headerBackgroundColor: '',
       interceptedAccount: {isManaged: false, pictureUrl: ''},
     };
   }