Commit a4e55e2e authored by Devlin Cronin's avatar Devlin Cronin Committed by Commit Bot

[Extensions UI] Add an indication when prompts close without user action

A dialog may close through:
- The user clicking the "accept" button (-> accepted)
- The user clicking the "cancel" button (-> canceled)
- The user clicking the 'x' button or escape key (-> closed)
- The dialog closing for some other reason, such as the parent window
  closing (-> ??)

This CL adds a separate entry to track those times, as well as adding
unit and browser tests to cover the flow.

Bug: 1079364
Change-Id: Iad60fd42a0ebbcbbb6fed16f9cedff77ad14eea8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225143Reviewed-by: default avatarElly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773780}
parent 0fa034b2
......@@ -152,6 +152,7 @@ void ExtensionSettingsOverriddenDialog::HandleDialogResult(
AcknowledgeControllingExtension();
break;
case DialogResult::kDialogDismissed:
case DialogResult::kDialogClosedWithoutUserAction:
// Do nothing; the dialog will display on the next run of Chrome.
break;
}
......
......@@ -159,6 +159,25 @@ TEST_F(ExtensionSettingsOverriddenDialogUnitTest,
EXPECT_FALSE(IsExtensionAcknowledged(extension->id()));
}
TEST_F(
ExtensionSettingsOverriddenDialogUnitTest,
ExtensionIsNeitherDisabledNorAcknowledgedOnDialogCloseWithoutUserAction) {
base::HistogramTester histogram_tester;
const extensions::Extension* extension = AddExtension();
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), profile());
controller.OnDialogShown();
controller.HandleDialogResult(DialogResult::kDialogClosedWithoutUserAction);
histogram_tester.ExpectUniqueSample(
kTestDialogResultHistogramName,
DialogResult::kDialogClosedWithoutUserAction, 1);
EXPECT_TRUE(registry()->enabled_extensions().Contains(extension->id()));
EXPECT_FALSE(IsExtensionAcknowledged(extension->id()));
}
TEST_F(ExtensionSettingsOverriddenDialogUnitTest,
WontShowTwiceForTheSameExtensionInTheSameSession) {
const extensions::Extension* extension = AddExtension();
......
......@@ -34,10 +34,14 @@ class SettingsOverriddenDialogController {
kChangeSettingsBack = 0,
// The user wants to keep the new settings, as configured by the extension.
kKeepNewSettings = 1,
// The dialog was dismissed without the user making a decision.
// The dialog was dismissed without the user making a decision through the
// close ('x') button, escape key, or similar.
kDialogDismissed = 2,
// The dialog was dismissed because it was destroyed, e.g. from the parent
// window closing.
kDialogClosedWithoutUserAction = 3,
kMaxValue = kDialogDismissed,
kMaxValue = kDialogClosedWithoutUserAction,
};
virtual ~SettingsOverriddenDialogController() = default;
......
......@@ -7,7 +7,6 @@
#include "base/bind.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/extensions/settings_overridden_dialog_controller.h"
#include "chrome/browser/ui/views/chrome_layout_provider.h"
#include "chrome/browser/ui/views/chrome_typography.h"
#include "chrome/grit/generated_resources.h"
......@@ -37,8 +36,8 @@ SettingsOverriddenDialogView::SettingsOverriddenDialogView(
// NOTE: The following Bind's are safe because the callback is
// owned by this object (indirectly, as a DialogDelegate).
return base::BindOnce(
&SettingsOverriddenDialogController::HandleDialogResult,
base::Unretained(controller_.get()), result);
&SettingsOverriddenDialogView::NotifyControllerOfResult,
base::Unretained(this), result);
};
SetAcceptCallback(make_result_callback(DialogResult::kChangeSettingsBack));
SetCancelCallback(make_result_callback(DialogResult::kKeepNewSettings));
......@@ -69,7 +68,16 @@ SettingsOverriddenDialogView::SettingsOverriddenDialogView(
AddChildView(std::move(message_label));
}
SettingsOverriddenDialogView::~SettingsOverriddenDialogView() = default;
SettingsOverriddenDialogView::~SettingsOverriddenDialogView() {
if (!result_) {
// The dialog may close without firing any of the [accept | cancel | close]
// callbacks if e.g. the parent window closes. In this case, notify the
// controller that the dialog closed without user action.
controller_->HandleDialogResult(
SettingsOverriddenDialogController::DialogResult::
kDialogClosedWithoutUserAction);
}
}
void SettingsOverriddenDialogView::Show(gfx::NativeWindow parent) {
constrained_window::CreateBrowserModalDialogViews(this, parent)->Show();
......@@ -87,6 +95,16 @@ gfx::Size SettingsOverriddenDialogView::CalculatePreferredSize() const {
return gfx::Size(width, GetHeightForWidth(width));
}
void SettingsOverriddenDialogView::NotifyControllerOfResult(
SettingsOverriddenDialogController::DialogResult result) {
DCHECK(!result_)
<< "Trying to re-notify controller of result. Previous result: "
<< static_cast<int>(*result_)
<< ", new result: " << static_cast<int>(result);
result_ = result;
controller_->HandleDialogResult(result);
}
namespace chrome {
void ShowExtensionSettingsOverriddenDialog(
......
......@@ -7,11 +7,11 @@
#include <memory>
#include "base/optional.h"
#include "chrome/browser/ui/extensions/settings_overridden_dialog_controller.h"
#include "ui/gfx/native_widget_types.h"
#include "ui/views/window/dialog_delegate.h"
class SettingsOverriddenDialogController;
// A dialog that displays a warning to the user that their settings have been
// overridden by an extension.
class SettingsOverriddenDialogView : public views::DialogDelegateView {
......@@ -31,6 +31,13 @@ class SettingsOverriddenDialogView : public views::DialogDelegateView {
ui::ModalType GetModalType() const override;
gfx::Size CalculatePreferredSize() const override;
// Notifies the |controller_| of the |result|.
void NotifyControllerOfResult(
SettingsOverriddenDialogController::DialogResult result);
// The result of the dialog; set when notifying the controller.
base::Optional<SettingsOverriddenDialogController::DialogResult> result_;
std::unique_ptr<SettingsOverriddenDialogController> controller_;
};
......
......@@ -24,14 +24,19 @@
#include "chrome/test/base/ui_test_utils.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "ui/views/test/widget_test.h"
namespace {
// A stub dialog controller that displays the dialog with the supplied params.
class TestDialogController : public SettingsOverriddenDialogController {
public:
explicit TestDialogController(ShowParams show_params)
: show_params_(std::move(show_params)) {}
TestDialogController(ShowParams show_params,
base::Optional<DialogResult>* dialog_result_out)
: show_params_(std::move(show_params)),
dialog_result_out_(dialog_result_out) {
DCHECK(dialog_result_out_);
}
TestDialogController(const TestDialogController&) = delete;
TestDialogController& operator=(const TestDialogController&) = delete;
~TestDialogController() override = default;
......@@ -40,9 +45,15 @@ class TestDialogController : public SettingsOverriddenDialogController {
bool ShouldShow() override { return true; }
ShowParams GetShowParams() override { return show_params_; }
void OnDialogShown() override {}
void HandleDialogResult(DialogResult result) override {}
void HandleDialogResult(DialogResult result) override {
ASSERT_FALSE(dialog_result_out_->has_value());
*dialog_result_out_ = result;
}
const ShowParams show_params_;
// The result to populate. Must outlive this object.
base::Optional<DialogResult>* const dialog_result_out_;
};
} // namespace
......@@ -58,9 +69,9 @@ class SettingsOverriddenDialogViewBrowserTest : public DialogBrowserTest {
void ShowUi(const std::string& name) override {
test_name_ = name;
if (name == "SimpleDialog")
ShowSimpleDialog(false);
ShowSimpleDialog(false, browser());
else if (name == "SimpleDialogWithIcon")
ShowSimpleDialog(true);
ShowSimpleDialog(true, browser());
else if (name == "NtpOverriddenDialog")
ShowNtpOverriddenDialog();
else if (name == "SearchOverriddenDialog")
......@@ -69,7 +80,10 @@ class SettingsOverriddenDialogViewBrowserTest : public DialogBrowserTest {
NOTREACHED() << name;
}
void ShowSimpleDialog(bool show_icon) {
// Creates, shows, and returns a dialog anchored to the given |browser|. The
// dialog is owned by the views framework.
SettingsOverriddenDialogView* ShowSimpleDialog(bool show_icon,
Browser* browser) {
SettingsOverriddenDialogController::ShowParams params{
base::ASCIIToUTF16("Settings overridden dialog title"),
base::ASCIIToUTF16(
......@@ -77,9 +91,12 @@ class SettingsOverriddenDialogViewBrowserTest : public DialogBrowserTest {
"longer than the title alone")};
if (show_icon)
params.icon = &kProductIcon;
auto* dialog = new SettingsOverriddenDialogView(
std::make_unique<TestDialogController>(std::move(params)));
dialog->Show(browser()->window()->GetNativeWindow());
auto* dialog =
new SettingsOverriddenDialogView(std::make_unique<TestDialogController>(
std::move(params), &dialog_result_));
dialog->Show(browser->window()->GetNativeWindow());
return dialog;
}
void ShowNtpOverriddenDialog() {
......@@ -136,12 +153,23 @@ class SettingsOverriddenDialogViewBrowserTest : public DialogBrowserTest {
return true;
}
base::Optional<SettingsOverriddenDialogController::DialogResult>
dialog_result() const {
return dialog_result_;
}
private:
std::string test_name_;
base::Optional<SettingsOverriddenDialogController::DialogResult>
dialog_result_;
base::test::ScopedFeatureList scoped_feature_list_;
};
////////////////////////////////////////////////////////////////////////////////
// UI Browser Tests
IN_PROC_BROWSER_TEST_F(SettingsOverriddenDialogViewBrowserTest,
InvokeUi_SimpleDialog) {
ShowAndVerifyUi();
......@@ -169,3 +197,26 @@ IN_PROC_BROWSER_TEST_F(SettingsOverriddenDialogViewBrowserTest,
ShowAndVerifyUi();
}
#endif // defined(OS_WIN) || defined(OS_MACOSX)
////////////////////////////////////////////////////////////////////////////////
// Functional Browser Tests
// Verify that if the parent window is closed, the dialog notifies the
// controller that it was closed without any user action.
IN_PROC_BROWSER_TEST_F(SettingsOverriddenDialogViewBrowserTest,
DialogWindowClosed) {
Browser* second_browser = CreateBrowser(browser()->profile());
ASSERT_TRUE(second_browser);
SettingsOverriddenDialogView* dialog =
ShowSimpleDialog(false, second_browser);
views::test::WidgetDestroyedWaiter widget_destroyed_waiter(
dialog->GetWidget());
CloseBrowserSynchronously(second_browser);
widget_destroyed_waiter.Wait();
ASSERT_TRUE(dialog_result());
EXPECT_EQ(SettingsOverriddenDialogController::DialogResult::
kDialogClosedWithoutUserAction,
*dialog_result());
}
......@@ -77,6 +77,14 @@ class SettingsOverriddenDialogViewUnitTest : public ChromeViewsTestBase {
return anchor_widget_->GetNativeWindow();
}
void CloseAnchorWindow() {
// Move out the anchor widget since we'll be closing it.
auto anchor_widget = std::move(anchor_widget_);
views::test::WidgetDestroyedWaiter destroyed_waiter(anchor_widget.get());
anchor_widget->Close();
destroyed_waiter.Wait();
}
private:
std::unique_ptr<views::Widget> anchor_widget_;
};
......@@ -129,3 +137,15 @@ TEST_F(SettingsOverriddenDialogViewUnitTest, DialogResult_DismissDialog) {
EXPECT_EQ(SettingsOverriddenDialogController::DialogResult::kDialogDismissed,
*state.result);
}
TEST_F(SettingsOverriddenDialogViewUnitTest, DialogResult_CloseParentWidget) {
DialogState state;
auto controller = std::make_unique<TestDialogController>(&state);
CreateAndShowDialog(std::move(controller));
CloseAnchorWindow();
ASSERT_TRUE(state.result);
EXPECT_EQ(SettingsOverriddenDialogController::DialogResult::
kDialogClosedWithoutUserAction,
*state.result);
}
......@@ -61398,6 +61398,7 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
<int value="0" label="Change settings back"/>
<int value="1" label="Keep new settings"/>
<int value="2" label="Dialog dismissed"/>
<int value="3" label="Dialog closed without user action"/>
</enum>
<enum name="SettingsPageInteractions">
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment