Skip to content

Commit

Permalink
Suppress the privacy sandbox dialog during signed-in profile creation
Browse files Browse the repository at this point in the history 
https://crrev.com/c/4548474 tried to address the same problem but it
didn't take into account that the profile customization dialog is shown
asynchronously during the signed-in profile creation flow.

To fix the problem in that flow, we'd also like to suppress the
privacy sandbox dialog while the theme info is being downloaded from
Sync servers.

This a band-aid solution to the problem with multiple dialogs showing up
after the profile creation. We plan to address it with a more robust
solution that doesn't require dialogs to depend on each other in the
nearest future.

Bug: 1444751
Change-Id: I86e105dd0d6a84bbf7405c632fef86cdf2fd7f24
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4955733
Reviewed-by: Nicola Tommasi <tommasin@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Auto-Submit: Alex Ilin <alexilin@chromium.org>
Reviewed-by: Rainhard Findling <rainhard@chromium.org>
Commit-Queue: Nicola Tommasi <tommasin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1212817}
  • Loading branch information
Alex Ilin authored and Chromium LUCI CQ committed Oct 20, 2023
1 parent 7018f1b commit b3ec86e
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 59 deletions.
14 changes: 11 additions & 3 deletions chrome/browser/ui/privacy_sandbox/privacy_sandbox_prompt_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/privacy_sandbox/privacy_sandbox_prompt.h"
#include "chrome/browser/ui/profiles/profile_customization_bubble_sync_controller.h"
#include "chrome/common/extensions/chrome_manifest_url_handlers.h"
#include "chrome/common/webui_url_constants.h"
#include "components/privacy_sandbox/privacy_sandbox_features.h"
Expand Down Expand Up @@ -172,9 +173,16 @@ void PrivacySandboxPromptHelper::DidFinishNavigation(
auto* browser =
chrome::FindBrowserWithTab(navigation_handle->GetWebContents());

// If a sign-in dialog is being currently displayed, the prompt should
// not be shown to avoid conflict.
if (browser->signin_view_controller()->ShowsModalDialog()) {
// If a sign-in dialog is being currently displayed or is about to be
// displayed, the prompt should not be shown to avoid conflict.
bool signin_dialog_showing =
browser->signin_view_controller()->ShowsModalDialog();
#if !BUILDFLAG(IS_CHROMEOS_ASH)
signin_dialog_showing =
signin_dialog_showing ||
IsProfileCustomizationBubbleSyncControllerRunning(browser);
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
if (signin_dialog_showing) {
base::UmaHistogramEnumeration(
kPrivacySandboxPromptHelperEventHistogram,
SettingsPrivacySandboxPromptHelperEvent::kSigninDialogShown);
Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/ui/profiles/profile_customization_bubble_sync_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,12 @@ void ApplyProfileColorAndShowCustomizationBubbleWhenNoValueSynced(
Browser* browser,
SkColor suggested_profile_color);

// Returns whether the controller triggered by
// `ApplyProfileColorAndShowCustomizationBubbleWhenNoValueSynced()` is currently
// running in background. This is useful for determining whether the profile
// customization UI might be shown soon in `browser`.
// Implemented in
// chrome/browser/ui/views/profiles/profile_customization_bubble_sync_controller.cc
bool IsProfileCustomizationBubbleSyncControllerRunning(Browser* browser);

#endif // CHROME_BROWSER_UI_PROFILES_PROFILE_CUSTOMIZATION_BUBBLE_SYNC_CONTROLLER_H_
108 changes: 71 additions & 37 deletions chrome/browser/ui/views/profiles/profile_customization_bubble_sync_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/ui/views/profiles/profile_customization_bubble_sync_controller.h"

#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/sync/sync_service_factory.h"
#include "chrome/browser/themes/theme_service.h"
#include "chrome/browser/themes/theme_service_factory.h"
Expand All @@ -18,6 +19,8 @@

namespace {

const void* const kBrowserUserDataKey = &kBrowserUserDataKey;

void ShowBubble(Browser* browser,
views::View* anchor_view,
ProfileCustomizationBubbleSyncController::Outcome outcome) {
Expand All @@ -41,6 +44,17 @@ void ShowBubble(Browser* browser,
}
}

ProfileCustomizationBubbleSyncController* GetCurrentControllerIfExists(
Browser* browser) {
base::SupportsUserData::Data* data =
browser->GetUserData(kBrowserUserDataKey);
if (!data) {
return nullptr;
}

return static_cast<ProfileCustomizationBubbleSyncController*>(data);
}

} // namespace

// static
Expand All @@ -56,29 +70,30 @@ void ProfileCustomizationBubbleSyncController::
// needed.
if (!anchor_view || !sync_service)
return;
// The controller is owned by itself.
auto* controller = new ProfileCustomizationBubbleSyncController(
profile, anchor_view, sync_service,
ThemeServiceFactory::GetForProfile(profile),
base::BindOnce(&ShowBubble, browser, anchor_view),
suggested_profile_color);
controller->Init();

auto controller =
base::WrapUnique(new ProfileCustomizationBubbleSyncController(
browser, anchor_view, sync_service,
ThemeServiceFactory::GetForProfile(profile),
base::BindOnce(&ShowBubble, browser, anchor_view),
suggested_profile_color));
SetCurrentControllerAndInit(std::move(controller));
}

// static
void ProfileCustomizationBubbleSyncController::
ApplyColorAndShowBubbleWhenNoValueSyncedForTesting(
Profile* profile,
Browser* browser,
views::View* anchor_view,
syncer::SyncService* sync_service,
ThemeService* theme_service,
ShowBubbleCallback show_bubble_callback,
SkColor suggested_profile_color) {
// The controller is owned by itself.
auto* controller = new ProfileCustomizationBubbleSyncController(
profile, anchor_view, sync_service, theme_service,
std::move(show_bubble_callback), suggested_profile_color);
controller->Init();
auto controller =
base::WrapUnique(new ProfileCustomizationBubbleSyncController(
browser, anchor_view, sync_service, theme_service,
std::move(show_bubble_callback), suggested_profile_color));
SetCurrentControllerAndInit(std::move(controller));
}

// static
Expand All @@ -89,18 +104,34 @@ bool ProfileCustomizationBubbleSyncController::CanThemeSyncStart(
return ProfileCustomizationSyncedThemeWaiter::CanThemeSyncStart(sync_service);
}

// static
void ProfileCustomizationBubbleSyncController::SetCurrentControllerAndInit(
std::unique_ptr<ProfileCustomizationBubbleSyncController> controller) {
Browser* browser = controller->browser_;
if (ProfileCustomizationBubbleSyncController* old_controller =
GetCurrentControllerIfExists(browser);
old_controller) {
old_controller->InvokeCallbackAndDeleteItself(Outcome::kAbort);
}

ProfileCustomizationBubbleSyncController* controller_raw = controller.get();
browser->SetUserData(kBrowserUserDataKey, std::move(controller));
controller_raw->Init();
}

ProfileCustomizationBubbleSyncController::
ProfileCustomizationBubbleSyncController(
Profile* profile,
Browser* browser,
views::View* anchor_view,
syncer::SyncService* sync_service,
ThemeService* theme_service,
ShowBubbleCallback show_bubble_callback,
SkColor suggested_profile_color)
: theme_service_(theme_service),
: browser_(browser),
theme_service_(theme_service),
show_bubble_callback_(std::move(show_bubble_callback)),
suggested_profile_color_(suggested_profile_color) {
CHECK(profile);
CHECK(browser);
CHECK(anchor_view);
CHECK(sync_service);
CHECK(theme_service_);
Expand All @@ -114,14 +145,21 @@ ProfileCustomizationBubbleSyncController::
// `theme_waiter_`.
base::Unretained(this)));

profile_observation_.Observe(profile);
// TODO(b/306593826): stop observing the anchor view as the bubble is no
// longer anchored.
view_observation_.Observe(anchor_view);
}

ProfileCustomizationBubbleSyncController::
~ProfileCustomizationBubbleSyncController() = default;
~ProfileCustomizationBubbleSyncController() {
if (show_bubble_callback_) {
std::move(show_bubble_callback_).Run(Outcome::kAbort);
}
}

void ProfileCustomizationBubbleSyncController::Init() {
// Verify that the user data has been set before calling `Init()`.
CHECK_EQ(browser_->GetUserData(kBrowserUserDataKey), this);
theme_waiter_->Run();
}

Expand All @@ -133,7 +171,7 @@ void ProfileCustomizationBubbleSyncController::OnSyncedThemeReady(
bool using_custom_theme = !theme_service_->UsingDefaultTheme() &&
!theme_service_->UsingSystemTheme();
if (using_custom_theme)
SkipBubble();
InvokeCallbackAndDeleteItself(Outcome::kSkipBubble);
else
ApplyDefaultColorAndShowBubble();
break;
Expand All @@ -144,20 +182,14 @@ void ProfileCustomizationBubbleSyncController::OnSyncedThemeReady(
case ProfileCustomizationSyncedThemeWaiter::Outcome::
kSyncPassphraseRequired:
case ProfileCustomizationSyncedThemeWaiter::Outcome::kTimeout:
SkipBubble();
InvokeCallbackAndDeleteItself(Outcome::kSkipBubble);
break;
}
}

void ProfileCustomizationBubbleSyncController::OnProfileWillBeDestroyed(
Profile* profile) {
// This gets called before any keyed services for the profile are destroyed.
Abort(); // deletes this
}

void ProfileCustomizationBubbleSyncController::OnViewIsDeleting(
views::View* observed_view) {
Abort(); // deletes this
InvokeCallbackAndDeleteItself(Outcome::kAbort);
}

void ProfileCustomizationBubbleSyncController::
Expand All @@ -168,18 +200,14 @@ void ProfileCustomizationBubbleSyncController::
} else {
theme_service_->BuildAutogeneratedThemeFromColor(suggested_profile_color_);
}
std::move(show_bubble_callback_).Run(Outcome::kShowBubble);
delete this;
InvokeCallbackAndDeleteItself(Outcome::kShowBubble);
}

void ProfileCustomizationBubbleSyncController::SkipBubble() {
std::move(show_bubble_callback_).Run(Outcome::kSkipBubble);
delete this;
}

void ProfileCustomizationBubbleSyncController::Abort() {
std::move(show_bubble_callback_).Run(Outcome::kAbort);
delete this;
void ProfileCustomizationBubbleSyncController::InvokeCallbackAndDeleteItself(
Outcome outcome) {
std::move(show_bubble_callback_).Run(outcome);
CHECK_EQ(browser_->GetUserData(kBrowserUserDataKey), this);
browser_->RemoveUserData(kBrowserUserDataKey); // deletes `this`
}

// Defined in
Expand All @@ -200,3 +228,9 @@ void ApplyProfileColorAndShowCustomizationBubbleWhenNoValueSynced(
ApplyColorAndShowBubbleWhenNoValueSynced(browser, anchor_view,
suggested_profile_color);
}

// Defined in
// chrome/browser/ui/profiles/profile_customization_bubble_sync_controller.h
bool IsProfileCustomizationBubbleSyncControllerRunning(Browser* browser) {
return GetCurrentControllerIfExists(browser) != nullptr;
}
25 changes: 12 additions & 13 deletions chrome/browser/ui/views/profiles/profile_customization_bubble_sync_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@
#include "base/functional/callback.h"
#include "base/memory/raw_ptr.h"
#include "base/scoped_observation.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_observer.h"
#include "base/supports_user_data.h"
#include "chrome/browser/themes/theme_syncable_service.h"
#include "chrome/browser/ui/profiles/profile_customization_synced_theme_waiter.h"
#include "components/sync/service/sync_service.h"
Expand All @@ -26,9 +25,10 @@ class Browser;
class Profile;

// Helper class for logic to show / delay showing the profile customization
// bubble. Owns itself.
class ProfileCustomizationBubbleSyncController : public ProfileObserver,
public views::ViewObserver {
// bubble. Owned by a Browser.
class ProfileCustomizationBubbleSyncController
: public views::ViewObserver,
public base::SupportsUserData::Data {
public:
enum class Outcome {
kShowBubble,
Expand Down Expand Up @@ -60,7 +60,7 @@ class ProfileCustomizationBubbleSyncController : public ProfileObserver,
// A version of ApplyColorAndShowBubbleWhenNoValueSynced() that allows simpler
// mocking.
static void ApplyColorAndShowBubbleWhenNoValueSyncedForTesting(
Profile* profile,
Browser* browser,
views::View* anchor_view,
syncer::SyncService* sync_service,
ThemeService* theme_service,
Expand All @@ -72,17 +72,17 @@ class ProfileCustomizationBubbleSyncController : public ProfileObserver,
static bool CanThemeSyncStart(Profile* profile);

private:
static void SetCurrentControllerAndInit(
std::unique_ptr<ProfileCustomizationBubbleSyncController> controller);

ProfileCustomizationBubbleSyncController(
Profile* profile,
Browser* browser,
views::View* anchor_view,
syncer::SyncService* sync_service,
ThemeService* theme_service,
ShowBubbleCallback show_bubble_callback,
SkColor suggested_profile_color);

// ProfileObserver:
void OnProfileWillBeDestroyed(Profile* profile) override;

// views::ViewObserver:
void OnViewIsDeleting(views::View* observed_view) override;

Expand All @@ -95,15 +95,14 @@ class ProfileCustomizationBubbleSyncController : public ProfileObserver,
// Functions that finalize the control logic by either showing or skipping the
// bubble (or aborting completely) and deleting itself.
void ApplyDefaultColorAndShowBubble();
void SkipBubble();
void Abort();
void InvokeCallbackAndDeleteItself(Outcome outcome);

const raw_ptr<Browser> browser_;
const raw_ptr<ThemeService> theme_service_;
std::unique_ptr<ProfileCustomizationSyncedThemeWaiter> theme_waiter_;
ShowBubbleCallback show_bubble_callback_;
SkColor const suggested_profile_color_;

base::ScopedObservation<Profile, ProfileObserver> profile_observation_{this};
base::ScopedObservation<views::View, views::ViewObserver> view_observation_{
this};
};
Expand Down
30 changes: 24 additions & 6 deletions chrome/browser/ui/views/profiles/profile_customization_bubble_sync_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/time/time.h"
#include "chrome/browser/themes/theme_service.h"
#include "chrome/browser/themes/theme_syncable_service.h"
#include "chrome/test/base/test_browser_window.h"
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_profile_manager.h"
#include "components/sync/service/sync_service.h"
Expand Down Expand Up @@ -85,6 +86,9 @@ class ProfileCustomizationBubbleSyncControllerTest : public testing::Test {
testing_profile_ =
testing_profile_manager_.CreateTestingProfile(kTestingProfileName);

Browser::CreateParams params(testing_profile_, /*user_gesture=*/true);
params.window = &test_browser_window_;
browser_ = std::unique_ptr<Browser>(Browser::Create(params));
testing_view_ = std::make_unique<views::View>();
}

Expand All @@ -93,7 +97,7 @@ class ProfileCustomizationBubbleSyncControllerTest : public testing::Test {
show_bubble_callback) {
ProfileCustomizationBubbleSyncController::
ApplyColorAndShowBubbleWhenNoValueSyncedForTesting(
testing_profile_, testing_view_.get(), &test_sync_service_,
browser_.get(), testing_view_.get(), &test_sync_service_,
&fake_theme_service_, std::move(show_bubble_callback),
kNewProfileColor);
}
Expand All @@ -106,9 +110,7 @@ class ProfileCustomizationBubbleSyncControllerTest : public testing::Test {
fake_theme_service_.DoSetTheme(nullptr, false);
}

void DeleteTestingProfile() {
testing_profile_manager_.DeleteTestingProfile(kTestingProfileName);
}
void CloseBrowser() { browser_.reset(); }

void DeleteTestingView() { testing_view_.reset(); }

Expand All @@ -126,8 +128,12 @@ class ProfileCustomizationBubbleSyncControllerTest : public testing::Test {
syncer::TestSyncService test_sync_service_;

private:
raw_ptr<Profile, DanglingUntriaged> testing_profile_ = nullptr;
TestingProfileManager testing_profile_manager_;
raw_ptr<Profile> testing_profile_ = nullptr;

TestBrowserWindow test_browser_window_;
std::unique_ptr<Browser> browser_;

std::unique_ptr<views::View> testing_view_;
FakeThemeService fake_theme_service_;
ThemeSyncableService theme_syncable_service_;
Expand Down Expand Up @@ -220,7 +226,7 @@ TEST_F(ProfileCustomizationBubbleSyncControllerTest,
EXPECT_CALL(show_bubble, Run(Outcome::kAbort));

ApplyColorAndShowBubbleWhenNoValueSynced(show_bubble.Get());
DeleteTestingProfile();
CloseBrowser();
}

TEST_F(ProfileCustomizationBubbleSyncControllerTest,
Expand All @@ -232,4 +238,16 @@ TEST_F(ProfileCustomizationBubbleSyncControllerTest,
DeleteTestingView();
}

TEST_F(ProfileCustomizationBubbleSyncControllerTest, ShouldAbortIfCalledAgain) {
base::MockCallback<base::OnceCallback<void(Outcome)>> old_show_bubble;
EXPECT_CALL(old_show_bubble, Run(Outcome::kAbort));
base::MockCallback<base::OnceCallback<void(Outcome)>> new_show_bubble;
EXPECT_CALL(new_show_bubble, Run(Outcome::kShowBubble));

ApplyColorAndShowBubbleWhenNoValueSynced(old_show_bubble.Get());
ApplyColorAndShowBubbleWhenNoValueSynced(new_show_bubble.Get());

NotifyOnSyncStarted();
}

} // namespace

0 comments on commit b3ec86e

Please sign in to comment.