Skip to content

Commit

Permalink
[Parent Access] Add ParentAccessDialogUIHandlerDelegate
Browse files Browse the repository at this point in the history 
The delegate provides the interface that the ParentAccessUIHandler
uses to retrieve parameters and provide results to interested
classes.  In production, the ParentAccessDialog implements this
delegate interface.

The delegate enables us to fake the behaviors
for testing the ParentAccessUIHandlerImpl without creating a
ParentAccessDialog.

This then allows us to replace the flaky
ParentAccessUIHandlerImplBrowserTest with ParentAccessUIHandlerImplUnitTest.

Bug: b:257125080
Change-Id: Ia17dc6991b1803e12a4a358e5bca792d3c82dfb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4000071
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Commit-Queue: Dan S <danan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1068444}
  • Loading branch information
Danan S authored and Chromium LUCI CQ committed Nov 8, 2022
1 parent 62e4abd commit 0e9b16e
Show file tree
Hide file tree
Showing 13 changed files with 633 additions and 541 deletions.
1 change: 1 addition & 0 deletions chrome/browser/ui/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2886,6 +2886,7 @@ static_library("ui") {
"webui/ash/parent_access/parent_access_dialog.h",
"webui/ash/parent_access/parent_access_ui.cc",
"webui/ash/parent_access/parent_access_ui.h",
"webui/ash/parent_access/parent_access_ui_handler_delegate.h",
"webui/ash/parent_access/parent_access_ui_handler_impl.cc",
"webui/ash/parent_access/parent_access_ui_handler_impl.h",
"webui/ash/power_ui.cc",
Expand Down
45 changes: 36 additions & 9 deletions chrome/browser/ui/webui/ash/parent_access/parent_access_dialog.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/ui/webui/ash/parent_access/parent_access_dialog.h"

#include <memory>
#include <utility>

#include "chrome/browser/profiles/profile.h"
Expand Down Expand Up @@ -36,7 +37,7 @@ ParentAccessDialogProvider::ShowError ParentAccessDialogProvider::Show(
}

DCHECK(ParentAccessDialog::GetInstance() == nullptr);
// Note: |dialog_|'s memory is freed when
// Note: |dialog|'s memory is freed when
// SystemWebDialogDelegate::OnDialogClosed() is called.
ParentAccessDialog* dialog =
new ParentAccessDialog(std::move(params), std::move(callback));
Expand Down Expand Up @@ -68,17 +69,35 @@ ParentAccessDialog::CloneParentAccessParams() {
return parent_access_params_->Clone();
}

void ParentAccessDialog::SetResultAndClose(
std::unique_ptr<ParentAccessDialog::Result> result) {
DCHECK(!result_);
result_ = std::move(result);
// This will trigger dialog destruction, which will in turn result in the
// callback being called.
Close();
void ParentAccessDialog::SetApproved(const std::string& parent_access_token,
const base::Time& expire_timestamp) {
auto result = std::make_unique<ParentAccessDialog::Result>();
result->status = ParentAccessDialog::Result::Status::kApproved;
result->parent_access_token = parent_access_token;
result->parent_access_token_expire_timestamp = expire_timestamp;
CloseWithResult(std::move(result));
}

void ParentAccessDialog::SetDeclined() {
auto result = std::make_unique<ParentAccessDialog::Result>();
result->status = ParentAccessDialog::Result::Status::kDeclined;
CloseWithResult(std::move(result));
}

void ParentAccessDialog::SetCanceled() {
auto result = std::make_unique<ParentAccessDialog::Result>();
result->status = ParentAccessDialog::Result::Status::kCancelled;
CloseWithResult(std::move(result));
}

void ParentAccessDialog::SetError() {
auto result = std::make_unique<ParentAccessDialog::Result>();
result->status = ParentAccessDialog::Result::Status::kError;
CloseWithResult(std::move(result));
}

parent_access_ui::mojom::ParentAccessParams*
ParentAccessDialog::GetParentAccessParamsForTest() {
ParentAccessDialog::GetParentAccessParamsForTest() const {
return parent_access_params_.get();
}

Expand All @@ -97,4 +116,12 @@ ParentAccessDialog::~ParentAccessDialog() {
: std::make_unique<ParentAccessDialog::Result>());
}

void ParentAccessDialog::CloseWithResult(
std::unique_ptr<ParentAccessDialog::Result> result) {
result_ = std::move(result);
// This will trigger dialog destruction, which will in turn result in the
// callback being called.
Close();
}

} // namespace ash
38 changes: 22 additions & 16 deletions chrome/browser/ui/webui/ash/parent_access/parent_access_dialog.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,16 @@
#include "base/callback_forward.h"
#include "base/time/time.h"
#include "chrome/browser/ui/webui/ash/parent_access/parent_access_ui.mojom.h"
#include "chrome/browser/ui/webui/ash/parent_access/parent_access_ui_handler_delegate.h"
#include "chrome/browser/ui/webui/ash/system_web_dialog_delegate.h"

namespace ash {

// Dialog which embeds the Parent Access UI, which verifies a parent during
// a child session.
class ParentAccessDialog : public SystemWebDialogDelegate {
// Dialog which embeds the Parent Access UI, which verifies a
// parent during a child session.
class ParentAccessDialog : public ParentAccessUIHandlerDelegate,
public SystemWebDialogDelegate {
public:
// The result of the parent access request, passed back to the caller.
struct Result {
// The status of the result.
enum class Status {
Expand Down Expand Up @@ -50,16 +51,17 @@ class ParentAccessDialog : public SystemWebDialogDelegate {
void GetDialogSize(gfx::Size* size) const override;
bool ShouldCloseDialogOnEscape() const override;

// Makes a copy of the ParentAccessParams. The ParentAccessDialog should
// maintain one copy of the parent_access_params_ object, which is why a clone
// is made, instead of transferring ownership to the caller.
parent_access_ui::mojom::ParentAccessParamsPtr CloneParentAccessParams();
// ParentAccessUIHandlerDelegate:
parent_access_ui::mojom::ParentAccessParamsPtr CloneParentAccessParams()
override;
void SetApproved(const std::string& parent_access_token,
const base::Time& expire_timestamp) override;
void SetDeclined() override;
void SetCanceled() override;
void SetError() override;

// Used by the ParentAccessUI to set the result of the Parent Access
// request and close the dialog.
void SetResultAndClose(std::unique_ptr<ParentAccessDialog::Result> result);

parent_access_ui::mojom::ParentAccessParams* GetParentAccessParamsForTest();
parent_access_ui::mojom::ParentAccessParams* GetParentAccessParamsForTest()
const;

explicit ParentAccessDialog(
parent_access_ui::mojom::ParentAccessParamsPtr params,
Expand All @@ -69,15 +71,18 @@ class ParentAccessDialog : public SystemWebDialogDelegate {
~ParentAccessDialog() override;

private:
void CloseWithResult(std::unique_ptr<Result> result);

parent_access_ui::mojom::ParentAccessParamsPtr parent_access_params_;
Callback callback_;

// The Parent Access result. Set by the ParentAccessUI
// The Parent Access Dialog result passed back to the caller when the dialog
// completes.
std::unique_ptr<Result> result_;
};

// Interface that provides the ParentAccessDialog. The provider
// should be used to show the dialog. The default implementation
// Interface that provides the ParentAccessDialog to external clients.
// The provider should be used to show the dialog. The default implementation
// is can be overridden by tests to provide a fake implementation like this:
//
// class FakeParentAccessDialogProvider
Expand All @@ -86,6 +91,7 @@ class ParentAccessDialog : public SystemWebDialogDelegate {
// ParentAccessDialogProvider::ShowError Show(
// parent_access_ui::mojom::ParentAccessParamsPtr params,
// ash::ParentAccessDialog::Callback callback) override {}
// }
class ParentAccessDialogProvider {
public:
// Error state returned by the Show() function.
Expand Down
160 changes: 143 additions & 17 deletions chrome/browser/ui/webui/ash/parent_access/parent_access_dialog_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,18 @@ IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, ShowDialog) {
// Verify it is showing.
ASSERT_EQ(error, ParentAccessDialogProvider::ShowError::kNone);
ASSERT_NE(ParentAccessDialog::GetInstance(), nullptr);
const ParentAccessDialog* dialog = ParentAccessDialog::GetInstance();
parent_access_ui::mojom::ParentAccessParams* params =
ParentAccessDialog::GetInstance()->GetParentAccessParamsForTest();
dialog->GetParentAccessParamsForTest();
EXPECT_EQ(
params->flow_type,
parent_access_ui::mojom::ParentAccessParams::FlowType::kWebsiteAccess);

// Verify that it is correctly configured.
EXPECT_EQ(ParentAccessDialog::GetInstance()->GetDialogContentURL().spec(),
EXPECT_EQ(dialog->GetDialogContentURL().spec(),
chrome::kChromeUIParentAccessURL);
EXPECT_TRUE(ParentAccessDialog::GetInstance()->ShouldShowCloseButton());
EXPECT_EQ(ParentAccessDialog::GetInstance()->GetDialogModalType(),
ui::ModalType::MODAL_TYPE_SYSTEM);
EXPECT_TRUE(dialog->ShouldShowCloseButton());
EXPECT_EQ(dialog->GetDialogModalType(), ui::ModalType::MODAL_TYPE_SYSTEM);

// Send ESCAPE keypress. EventGenerator requires the root window, which has
// to be fetched from the Ash shell.
Expand All @@ -83,24 +83,151 @@ IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, ShowDialog) {
run_loop.Run();
}

// Verify that the dialog is shown and correctly configured.
IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, SetResultAndClose) {
// Verify that the dialog is closed on Approve.
IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, SetApproved) {
base::RunLoop run_loop;

auto expected_result = std::make_unique<ParentAccessDialog::Result>();
expected_result->status = ParentAccessDialog::Result::Status::kApproved;
expected_result->parent_access_token = "TEST_TOKEN";
expected_result->parent_access_token_expire_timestamp =
ParentAccessDialog::Result expected_result;
expected_result.status = ParentAccessDialog::Result::Status::kApproved;
expected_result.parent_access_token = "TEST_TOKEN";
expected_result.parent_access_token_expire_timestamp =
base::Time::FromDoubleT(123456L);

// Make a copy for use during verification since we std::move() it below.
auto expected_result_copy =
std::make_unique<ParentAccessDialog::Result>(*expected_result);
// Create the callback.
ParentAccessDialog::Callback callback = base::BindLambdaForTesting(
[&](std::unique_ptr<ParentAccessDialog::Result> result) -> void {
EXPECT_TRUE(DialogResultsEqual(*result, expected_result));
run_loop.Quit();
});

// Show the dialog.
ParentAccessDialogProvider provider;
provider.Show(
parent_access_ui::mojom::ParentAccessParams::New(
parent_access_ui::mojom::ParentAccessParams::FlowType::kWebsiteAccess,
parent_access_ui::mojom::FlowTypeParams::NewWebApprovalsParams(
parent_access_ui::mojom::WebApprovalsParams::New())),
std::move(callback));

// Set the result.
ParentAccessDialog::GetInstance()->SetApproved(
expected_result.parent_access_token,
expected_result.parent_access_token_expire_timestamp);

run_loop.Run();

// The dialog instance should be gone after SetResult() is called.
EXPECT_EQ(ParentAccessDialog::GetInstance(), nullptr);
}

// Verify that the dialog is closed on Decline.
IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, SetDeclined) {
base::RunLoop run_loop;

ParentAccessDialog::Result expected_result;
expected_result.status = ParentAccessDialog::Result::Status::kDeclined;

// Create the callback.
ParentAccessDialog::Callback callback = base::BindLambdaForTesting(
[&](std::unique_ptr<ParentAccessDialog::Result> result) -> void {
EXPECT_TRUE(DialogResultsEqual(*result, expected_result));
run_loop.Quit();
});

// Show the dialog.
ParentAccessDialogProvider provider;
provider.Show(
parent_access_ui::mojom::ParentAccessParams::New(
parent_access_ui::mojom::ParentAccessParams::FlowType::kWebsiteAccess,
parent_access_ui::mojom::FlowTypeParams::NewWebApprovalsParams(
parent_access_ui::mojom::WebApprovalsParams::New())),
std::move(callback));

// Set the result.
ParentAccessDialog::GetInstance()->SetDeclined();

run_loop.Run();

// The dialog instance should be gone after SetResult() is called.
EXPECT_EQ(ParentAccessDialog::GetInstance(), nullptr);
}

// Verify that the dialog is closed on Cancel.
IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, SetCanceled) {
base::RunLoop run_loop;

ParentAccessDialog::Result expected_result;
expected_result.status = ParentAccessDialog::Result::Status::kCancelled;

// Create the callback.
ParentAccessDialog::Callback callback = base::BindLambdaForTesting(
[&](std::unique_ptr<ParentAccessDialog::Result> result) -> void {
EXPECT_TRUE(DialogResultsEqual(*result, expected_result));
run_loop.Quit();
});

// Show the dialog.
ParentAccessDialogProvider provider;
provider.Show(
parent_access_ui::mojom::ParentAccessParams::New(
parent_access_ui::mojom::ParentAccessParams::FlowType::kWebsiteAccess,
parent_access_ui::mojom::FlowTypeParams::NewWebApprovalsParams(
parent_access_ui::mojom::WebApprovalsParams::New())),
std::move(callback));

// Set the result.
ParentAccessDialog::GetInstance()->SetCanceled();

run_loop.Run();

// The dialog instance should be gone after SetResult() is called.
EXPECT_EQ(ParentAccessDialog::GetInstance(), nullptr);
}

// Verify that the dialog is closed on Cancel.
IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, SetError) {
base::RunLoop run_loop;

ParentAccessDialog::Result expected_result;
expected_result.status = ParentAccessDialog::Result::Status::kError;

// Create the callback.
ParentAccessDialog::Callback callback = base::BindLambdaForTesting(
[&](std::unique_ptr<ParentAccessDialog::Result> result) -> void {
EXPECT_TRUE(DialogResultsEqual(*result, expected_result));
run_loop.Quit();
});

// Show the dialog.
ParentAccessDialogProvider provider;
provider.Show(
parent_access_ui::mojom::ParentAccessParams::New(
parent_access_ui::mojom::ParentAccessParams::FlowType::kWebsiteAccess,
parent_access_ui::mojom::FlowTypeParams::NewWebApprovalsParams(
parent_access_ui::mojom::WebApprovalsParams::New())),
std::move(callback));

// Set the result.
ParentAccessDialog::GetInstance()->SetError();

run_loop.Run();

// The dialog instance should be gone after SetResult() is called.
EXPECT_EQ(ParentAccessDialog::GetInstance(), nullptr);
}

// Verify that if dialog is destroyed without a Result, it reports being
// canceled.
IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, DestroyedWithoutResult) {
base::RunLoop run_loop;

ParentAccessDialog::Result expected_result;
expected_result.status = ParentAccessDialog::Result::Status::kCancelled;

// Create the callback.
ParentAccessDialog::Callback callback = base::BindLambdaForTesting(
[&](std::unique_ptr<ParentAccessDialog::Result> result) -> void {
EXPECT_TRUE(DialogResultsEqual(*result, *expected_result_copy));
EXPECT_TRUE(DialogResultsEqual(*result, expected_result));
run_loop.Quit();
});

Expand All @@ -114,8 +241,7 @@ IN_PROC_BROWSER_TEST_F(ParentAccessDialogBrowserTest, SetResultAndClose) {
std::move(callback));

// Set the result.
ParentAccessDialog::GetInstance()->SetResultAndClose(
std::move(expected_result));
ParentAccessDialog::GetInstance()->Close();

run_loop.Run();

Expand Down
7 changes: 6 additions & 1 deletion chrome/browser/ui/webui/ash/parent_access/parent_access_ui.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/ui/webui/ash/parent_access/parent_access_ui.h"

#include <memory>
#include <string>
#include <utility>

Expand All @@ -13,7 +14,9 @@
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/identity_manager_factory.h"
#include "chrome/browser/ui/webui/ash/parent_access/parent_access_dialog.h"
#include "chrome/browser/ui/webui/ash/parent_access/parent_access_ui.mojom.h"
#include "chrome/browser/ui/webui/ash/parent_access/parent_access_ui_handler_impl.h"
#include "chrome/browser/ui/webui/webui_util.h"
#include "chrome/common/webui_url_constants.h"
#include "chrome/grit/browser_resources.h"
Expand Down Expand Up @@ -85,8 +88,10 @@ void ParentAccessUI::BindInterface(
? test_identity_manager_
: IdentityManagerFactory::GetForProfile(Profile::FromWebUI(web_ui()));

// The dialog instance could be null if the webui's url is entered in the
// browser address bar. The handler should handle that scenario.
mojo_api_handler_ = std::make_unique<ParentAccessUIHandlerImpl>(
std::move(receiver), web_ui(), identity_manager);
std::move(receiver), identity_manager, ParentAccessDialog::GetInstance());
}

const GURL ParentAccessUI::GetWebContentURLForTesting() {
Expand Down

0 comments on commit 0e9b16e

Please sign in to comment.