Skip to content

Commit

Permalink
Revert "[reland][privacy sandbox settings] Handle notice dismiss in w…
Browse files Browse the repository at this point in the history
…eb UI"

This reverts commit 1ada708.

Still failing on Wayland:
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests%20(Wayland)/12327/overview

Original change's description:
> [reland][privacy sandbox settings] Handle notice dismiss in web UI
>
> Dismissing the notice was handled in the dialog view. The action
> didn't go through the handler pipeline and didn't trigger HaTS. It also
> recorded kNoticeClosedNoAction as well as kNoticeDismiss.
>
> Handle pressing ESC in the web UI instead to go through the same path as
> other user actions.
>
> CL was reverted because new interactive tests are flaky on ChromeOS and
> Wayland:
> https://chromium-review.googlesource.com/c/chromium/src/+/3584774
> Disable them on ChromeOS and Wayland for now.
>
> Bug: 1312042
> Change-Id: Ib9f59ca83ea2efcd3d0ec2891378690e396a1935
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3581996
> Reviewed-by: Martin Šrámek <msramek@chromium.org>
> Reviewed-by: Rainhard Findling <rainhard@chromium.org>
> Commit-Queue: Olesia Marukhno <olesiamarukhno@google.com>
> Cr-Commit-Position: refs/heads/main@{#992472}

(cherry picked from commit 7cde19e)

Bug: 1312042
Change-Id: Ia0eccea499e81a99823ab5104f0cd1ba60c6ab63
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3587187
Auto-Submit: David Roger <droger@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Original-Commit-Position: refs/heads/main@{#992890}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3589012
Owners-Override: Robert Liao <robliao@google.com>
Auto-Submit: Robert Liao <robliao@chromium.org>
Reviewed-by: Olesia Marukhno <olesiamarukhno@google.com>
Commit-Queue: Olesia Marukhno <olesiamarukhno@google.com>
Cr-Commit-Position: refs/branch-heads/5005@{#27}
Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
  • Loading branch information
David Roger authored and Chromium LUCI CQ committed Apr 19, 2022
1 parent d3e09ed commit 2396a9e
Show file tree
Hide file tree
Showing 11 changed files with 96 additions and 186 deletions.
2 changes: 0 additions & 2 deletions chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -7819,8 +7819,6 @@ static_library("test_support") {
"hid/mock_hid_device_observer.h",
"notifications/notification_test_util.cc",
"notifications/notification_test_util.h",
"privacy_sandbox/mock_privacy_sandbox_service.cc",
"privacy_sandbox/mock_privacy_sandbox_service.h",
"profile_resetter/profile_resetter_test_base.cc",
"profile_resetter/profile_resetter_test_base.h",
"sessions/app_session_service_test_helper.cc",
Expand Down
17 changes: 0 additions & 17 deletions chrome/browser/privacy_sandbox/mock_privacy_sandbox_service.cc

This file was deleted.

33 changes: 0 additions & 33 deletions chrome/browser/privacy_sandbox/mock_privacy_sandbox_service.h

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@
}
</style>

<div class$="wrapper [[fitIntoDialogClass_]]" tabindex="-1">
<div class$="wrapper [[fitIntoDialogClass_]]">
<div id="contentArea" class$="[[canScrollClass_]]">
<div class="header">
<img>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,6 @@ export class PrivacySandboxDialogAppElement extends
proxy.showDialog();
});
});

window.addEventListener('keydown', event => {
// Only notice dialog can be dismissed by pressing "Esc".
if (event.key === 'Escape' && !this.isConsent_) {
this.dialogActionOccurred(PrivacySandboxDialogAction.NOTICE_DISMISS);
}
});
}

private onNoticeOpenSettings_() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ constexpr int kDefaultNoticeDialogHeight = 494;

class PrivacySandboxDialogDelegate : public views::DialogDelegate {
public:
explicit PrivacySandboxDialogDelegate(Browser* browser) : browser_(browser) {
explicit PrivacySandboxDialogDelegate(
Browser* browser,
PrivacySandboxService::DialogType dialog_type)
: browser_(browser), dialog_type_(dialog_type) {
if (auto* privacy_sandbox_serivce =
PrivacySandboxServiceFactory::GetForProfile(browser->profile())) {
privacy_sandbox_serivce->DialogOpenedForBrowser(browser);
Expand All @@ -39,10 +42,23 @@ class PrivacySandboxDialogDelegate : public views::DialogDelegate {
}

bool OnCloseRequested(views::Widget::ClosedReason close_reason) override {
// Any close reason is sufficient to close the notice.
if (dialog_type_ == PrivacySandboxService::DialogType::kNotice) {
// If the notice was dismissed via esc inform the Privacy Sandbox service.
if (close_reason == views::Widget::ClosedReason::kEscKeyPressed) {
if (auto* privacy_sandbox_serivce =
PrivacySandboxServiceFactory::GetForProfile(
browser_->profile())) {
privacy_sandbox_serivce->DialogActionOccurred(
PrivacySandboxService::DialogAction::kNoticeDismiss);
}
}
return true;
}

// Only an unspecified close reason, which only occurs when the user has
// actually made a choice is sufficient to close the consent. Reason for
// closing the dialog (like dismissing notice dialog with escape) is handled
// in WebUI.
// actually made a choice and not for things like pressing escape, is
// sufficient to close the consent.
return close_reason == views::Widget::ClosedReason::kUnspecified;
}

Expand All @@ -55,14 +71,16 @@ class PrivacySandboxDialogDelegate : public views::DialogDelegate {

private:
Browser* browser_;
PrivacySandboxService::DialogType dialog_type_;
};

} // namespace

// static
void ShowPrivacySandboxDialog(Browser* browser,
PrivacySandboxService::DialogType dialog_type) {
auto delegate = std::make_unique<PrivacySandboxDialogDelegate>(browser);
auto delegate =
std::make_unique<PrivacySandboxDialogDelegate>(browser, dialog_type);
delegate->SetButtons(ui::DIALOG_BUTTON_NONE);
delegate->SetModalType(ui::MODAL_TYPE_WINDOW);
delegate->SetShowCloseButton(false);
Expand Down Expand Up @@ -127,7 +145,6 @@ void PrivacySandboxDialogView::ResizeNativeView(int height) {

void PrivacySandboxDialogView::ShowNativeView() {
GetWidget()->Show();
web_view_->RequestFocus();

DCHECK(!dialog_created_time_.is_null());
base::UmaHistogramTimes("Settings.PrivacySandbox.DialogLoadTime",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// found in the LICENSE file.

#include "build/build_config.h"
#include "chrome/browser/privacy_sandbox/mock_privacy_sandbox_service.h"
#include "chrome/browser/privacy_sandbox/privacy_sandbox_service.h"
#include "chrome/browser/privacy_sandbox/privacy_sandbox_service_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser.h"
Expand All @@ -12,6 +12,7 @@
#include "chrome/browser/ui/test/test_browser_dialog.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/privacy_sandbox/privacy_sandbox_dialog_view.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test.h"
#include "testing/gmock/include/gmock/gmock.h"
Expand All @@ -23,6 +24,19 @@ namespace {
constexpr int kAverageBrowserWidth = 800;
constexpr int kAverageBrowserHeight = 700;

class MockPrivacySandboxService : public PrivacySandboxService {
public:
MOCK_METHOD(void,
DialogActionOccurred,
(PrivacySandboxService::DialogAction),
(override));
};

std::unique_ptr<KeyedService> BuildMockPrivacySandboxService(
content::BrowserContext* context) {
return std::make_unique<::testing::NiceMock<MockPrivacySandboxService>>();
}

} // namespace

class PrivacySandboxDialogViewBrowserTest : public DialogBrowserTest {
Expand Down Expand Up @@ -65,6 +79,63 @@ class PrivacySandboxDialogViewBrowserTest : public DialogBrowserTest {
raw_ptr<MockPrivacySandboxService> mock_service_;
};

IN_PROC_BROWSER_TEST_F(PrivacySandboxDialogViewBrowserTest,
EscapeClosesNotice) {
// Check that when the escape key is pressed, the notice is closed.
EXPECT_CALL(
*mock_service(),
DialogActionOccurred(PrivacySandboxService::DialogAction::kNoticeShown));
EXPECT_CALL(*mock_service(),
DialogActionOccurred(
PrivacySandboxService::DialogAction::kNoticeDismiss));
EXPECT_CALL(
*mock_service(),
DialogActionOccurred(
PrivacySandboxService::DialogAction::kNoticeClosedNoInteraction));
views::NamedWidgetShownWaiter waiter(
views::test::AnyWidgetTestPasskey{},
PrivacySandboxDialogView::kViewClassName);
ShowPrivacySandboxDialog(browser(),
PrivacySandboxService::DialogType::kNotice);
auto* dialog = waiter.WaitIfNeededAndGet();
dialog->CloseWithReason(views::Widget::ClosedReason::kEscKeyPressed);
EXPECT_TRUE(dialog->IsClosed());

// While IsClosed updated immediately, the widget will only actually close,
// and thus inform the service asynchronously so must be waited for.
base::RunLoop().RunUntilIdle();

// Shutting down the browser test will naturally shut the dialog, verify
// expectations before that happens.
testing::Mock::VerifyAndClearExpectations(mock_service());
}

IN_PROC_BROWSER_TEST_F(PrivacySandboxDialogViewBrowserTest,
EscapeDoesntCloseConsent) {
// Check that when the escape key is pressed, the consent is not closed.
EXPECT_CALL(
*mock_service(),
DialogActionOccurred(PrivacySandboxService::DialogAction::kConsentShown));
EXPECT_CALL(
*mock_service(),
DialogActionOccurred(
PrivacySandboxService::DialogAction::kConsentClosedNoDecision))
.Times(0);
views::NamedWidgetShownWaiter waiter(
views::test::AnyWidgetTestPasskey{},
PrivacySandboxDialogView::kViewClassName);
ShowPrivacySandboxDialog(browser(),
PrivacySandboxService::DialogType::kConsent);
auto* dialog = waiter.WaitIfNeededAndGet();
dialog->CloseWithReason(views::Widget::ClosedReason::kEscKeyPressed);
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(dialog->IsClosed());

// Shutting down the browser test will naturally shut the dialog, verify
// expectations before that happens.
testing::Mock::VerifyAndClearExpectations(mock_service());
}

#if BUILDFLAG(IS_WIN)
#define MAYBE_InvokeUi_Consent DISABLED_InvokeUi_Consent
#else
Expand Down

This file was deleted.

2 changes: 0 additions & 2 deletions chrome/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,6 @@ static_library("test_support") {
# target.
":test_support_ui",
"//chrome/app:test_support",
"//chrome/browser:test_support",
"//chrome/browser/web_applications",
"//components/crx_file",
"//components/keep_alive_registry",
Expand Down Expand Up @@ -8926,7 +8925,6 @@ if (!is_android) {
"../browser/ui/views/extensions/extensions_menu_view_interactive_uitest.cc",
"../browser/ui/views/extensions/extensions_tabbed_menu_view_interactive_uitest.cc",
"../browser/ui/views/page_info/page_info_bubble_view_interactive_uitest.cc",
"../browser/ui/views/privacy_sandbox/privacy_sandbox_dialog_view_interactive_ui_test.cc",
"../browser/ui/views/web_apps/frame_toolbar/web_app_frame_toolbar_interactive_uitest.cc",
"../browser/ui/views/web_dialog_view_browsertest.cc",
"../browser/webauth_interactive_uitest.cc",
Expand Down

0 comments on commit 2396a9e

Please sign in to comment.