Commit 31e23637 authored by Carlos IL's avatar Carlos IL Committed by Commit Bot

Cleanup kSupervisedUserCommittedInterstitials flag.

Feature launched in M70, and has been default enabled since, this CL
removes the flag from Chrome's code, and the switch from
chrome://flags

Bug: 936008
Change-Id: I57772ccc041b137255e39b220777e739d72c9789
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1644788
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: default avatarMin Qin <qinmin@chromium.org>
Reviewed-by: default avatarHenrique Grandinetti <hgrandinetti@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667233}
parent 8f0b748a
...@@ -2939,12 +2939,6 @@ const FeatureEntry kFeatureEntries[] = { ...@@ -2939,12 +2939,6 @@ const FeatureEntry kFeatureEntries[] = {
FEATURE_VALUE_TYPE(features::kViewsCastDialog)}, FEATURE_VALUE_TYPE(features::kViewsCastDialog)},
#endif // defined(TOOLKIT_VIEWS) #endif // defined(TOOLKIT_VIEWS)
{"SupervisedUserCommittedInterstitials",
flag_descriptions::kSupervisedUserCommittedInterstitialsName,
flag_descriptions::kSupervisedUserCommittedInterstitialsDescription,
kOsAll,
FEATURE_VALUE_TYPE(features::kSupervisedUserCommittedInterstitials)},
#if defined(OS_ANDROID) #if defined(OS_ANDROID)
{"enable-horizontal-tab-switcher", {"enable-horizontal-tab-switcher",
flag_descriptions::kHorizontalTabSwitcherAndroidName, flag_descriptions::kHorizontalTabSwitcherAndroidName,
......
...@@ -128,16 +128,10 @@ void AddCompleteFileTypeInfo( ...@@ -128,16 +128,10 @@ void AddCompleteFileTypeInfo(
// error pages (failed DNS lookups, SSL errors, etc), which shouldn't affect // error pages (failed DNS lookups, SSL errors, etc), which shouldn't affect
// functionality. // functionality.
bool IsErrorPage(content::WebContents* web_contents) { bool IsErrorPage(content::WebContents* web_contents) {
if (base::FeatureList::IsEnabled( if (web_contents->GetController().GetActiveEntry() == NULL)
features::kSupervisedUserCommittedInterstitials)) { return false;
if (web_contents->GetController().GetActiveEntry() == NULL) return web_contents->GetController().GetLastCommittedEntry()->GetPageType() ==
return false; content::PAGE_TYPE_ERROR;
return web_contents->GetController()
.GetLastCommittedEntry()
->GetPageType() == content::PAGE_TYPE_ERROR;
}
// Fallback if someone ever disables committed interstitials.
return web_contents->ShowingInterstitialPage();
} }
} // anonymous namespace } // anonymous namespace
......
...@@ -1657,12 +1657,6 @@ const char kHistoryManipulationInterventionDescription[] = ...@@ -1657,12 +1657,6 @@ const char kHistoryManipulationInterventionDescription[] =
"If a page does a client side redirect or adds to the history without a " "If a page does a client side redirect or adds to the history without a "
"user gesture, then skip it on back/forward UI."; "user gesture, then skip it on back/forward UI.";
const char kSupervisedUserCommittedInterstitialsName[] =
"Enable Supervised User Committed Interstitials";
const char kSupervisedUserCommittedInterstitialsDescription[] =
"Use committed error pages instead of transient navigation entries for "
"supervised user interstitials";
const char kSilentDebuggerExtensionApiName[] = "Silent Debugging"; const char kSilentDebuggerExtensionApiName[] = "Silent Debugging";
const char kSilentDebuggerExtensionApiDescription[] = const char kSilentDebuggerExtensionApiDescription[] =
"Do not show the infobar when an extension attaches to a page via " "Do not show the infobar when an extension attaches to a page via "
......
...@@ -981,9 +981,6 @@ extern const char kSkiaRendererDescription[]; ...@@ -981,9 +981,6 @@ extern const char kSkiaRendererDescription[];
extern const char kHistoryManipulationIntervention[]; extern const char kHistoryManipulationIntervention[];
extern const char kHistoryManipulationInterventionDescription[]; extern const char kHistoryManipulationInterventionDescription[];
extern const char kSupervisedUserCommittedInterstitialsName[];
extern const char kSupervisedUserCommittedInterstitialsDescription[];
extern const char kEnableDrawOcclusionName[]; extern const char kEnableDrawOcclusionName[];
extern const char kEnableDrawOcclusionDescription[]; extern const char kEnableDrawOcclusionDescription[];
......
...@@ -7,11 +7,9 @@ ...@@ -7,11 +7,9 @@
#include "base/bind.h" #include "base/bind.h"
#include "base/bind_helpers.h" #include "base/bind_helpers.h"
#include "base/command_line.h" #include "base/command_line.h"
#include "base/feature_list.h"
#include "base/run_loop.h" #include "base/run_loop.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/test/bind_test_util.h" #include "base/test/bind_test_util.h"
#include "base/test/scoped_feature_list.h"
#include "base/values.h" #include "base/values.h"
#include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/chrome_notification_types.h"
#include "chrome/browser/history/history_service_factory.h" #include "chrome/browser/history/history_service_factory.h"
...@@ -27,7 +25,6 @@ ...@@ -27,7 +25,6 @@
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h" #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/chrome_switches.h" #include "chrome/common/chrome_switches.h"
#include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h" #include "chrome/test/base/ui_test_utils.h"
...@@ -60,11 +57,6 @@ using content::WebContents; ...@@ -60,11 +57,6 @@ using content::WebContents;
namespace { namespace {
bool AreCommittedInterstitialsEnabled() {
return base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials);
}
class InterstitialPageObserver : public content::WebContentsObserver { class InterstitialPageObserver : public content::WebContentsObserver {
public: public:
InterstitialPageObserver(WebContents* web_contents, InterstitialPageObserver(WebContents* web_contents,
...@@ -72,17 +64,11 @@ class InterstitialPageObserver : public content::WebContentsObserver { ...@@ -72,17 +64,11 @@ class InterstitialPageObserver : public content::WebContentsObserver {
: content::WebContentsObserver(web_contents), callback_(callback) {} : content::WebContentsObserver(web_contents), callback_(callback) {}
~InterstitialPageObserver() override {} ~InterstitialPageObserver() override {}
void DidAttachInterstitialPage() override {
DCHECK(!AreCommittedInterstitialsEnabled());
callback_.Run();
}
void DidFinishNavigation( void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override { content::NavigationHandle* navigation_handle) override {
// With committed interstitials, DidAttachInterstitialPage is not called, so // With committed interstitials, DidAttachInterstitialPage is not called, so
// call the callback from here if there was an error. // call the callback from here if there was an error.
if (AreCommittedInterstitialsEnabled() && if (navigation_handle->IsErrorPage()) {
navigation_handle->IsErrorPage()) {
callback_.Run(); callback_.Run();
} }
} }
...@@ -91,13 +77,8 @@ class InterstitialPageObserver : public content::WebContentsObserver { ...@@ -91,13 +77,8 @@ class InterstitialPageObserver : public content::WebContentsObserver {
base::Closure callback_; base::Closure callback_;
}; };
// TODO(carlosil): These tests can be turned into regular (non-parameterized)
// tests once committed interstitials are the only code path, special cases for
// committed/non-committed interstitials should also be cleaned up.
// Tests filtering for supervised users. // Tests filtering for supervised users.
class SupervisedUserTest : public InProcessBrowserTest, class SupervisedUserTest : public InProcessBrowserTest {
public testing::WithParamInterface<bool> {
public: public:
// Indicates whether the interstitial should proceed or not. // Indicates whether the interstitial should proceed or not.
enum InterstitialAction { enum InterstitialAction {
...@@ -111,56 +92,29 @@ class SupervisedUserTest : public InProcessBrowserTest, ...@@ -111,56 +92,29 @@ class SupervisedUserTest : public InProcessBrowserTest,
bool ShownPageIsInterstitial(Browser* browser) { bool ShownPageIsInterstitial(Browser* browser) {
WebContents* tab = browser->tab_strip_model()->GetActiveWebContents(); WebContents* tab = browser->tab_strip_model()->GetActiveWebContents();
EXPECT_FALSE(tab->IsCrashed()); EXPECT_FALSE(tab->IsCrashed());
if (AreCommittedInterstitialsEnabled()) { base::string16 title;
base::string16 title; ui_test_utils::GetCurrentTabTitle(browser, &title);
ui_test_utils::GetCurrentTabTitle(browser, &title); return tab->GetController().GetLastCommittedEntry()->GetPageType() ==
return tab->GetController().GetLastCommittedEntry()->GetPageType() == content::PAGE_TYPE_ERROR &&
content::PAGE_TYPE_ERROR && title == base::ASCIIToUTF16("Site blocked");
title == base::ASCIIToUTF16("Site blocked");
}
return tab->ShowingInterstitialPage();
} }
void SendAccessRequest(WebContents* tab) { void SendAccessRequest(WebContents* tab) {
if (AreCommittedInterstitialsEnabled()) { tab->GetMainFrame()->ExecuteJavaScriptForTests(
tab->GetMainFrame()->ExecuteJavaScriptForTests( base::ASCIIToUTF16(
base::ASCIIToUTF16( "supervisedUserErrorPageController.requestPermission()"),
"supervisedUserErrorPageController.requestPermission()"), base::NullCallback());
base::NullCallback()); return;
return;
}
InterstitialPage* interstitial_page = tab->GetInterstitialPage();
ASSERT_TRUE(interstitial_page);
// Get the SupervisedUserInterstitial delegate.
content::InterstitialPageDelegate* delegate =
interstitial_page->GetDelegateForTesting();
// Simulate the click on the "request" button.
delegate->CommandReceived("\"request\"");
} }
void GoBack(WebContents* tab) { void GoBack(WebContents* tab) {
if (AreCommittedInterstitialsEnabled()) { tab->GetMainFrame()->ExecuteJavaScriptForTests(
tab->GetMainFrame()->ExecuteJavaScriptForTests( base::ASCIIToUTF16("supervisedUserErrorPageController.goBack()"),
base::ASCIIToUTF16("supervisedUserErrorPageController.goBack()"), base::NullCallback());
base::NullCallback()); return;
return;
}
InterstitialPage* interstitial_page = tab->GetInterstitialPage();
ASSERT_TRUE(interstitial_page);
// Get the SupervisedUserInterstitial delegate.
content::InterstitialPageDelegate* delegate =
interstitial_page->GetDelegateForTesting();
// Simulate the click on the "back" button
delegate->CommandReceived("\"back\"");
} }
void GoBackAndWaitForNavigation(WebContents* tab) { void GoBackAndWaitForNavigation(WebContents* tab) {
DCHECK(AreCommittedInterstitialsEnabled());
content::TestNavigationObserver observer(tab); content::TestNavigationObserver observer(tab);
GoBack(tab); GoBack(tab);
observer.Wait(); observer.Wait();
...@@ -168,14 +122,6 @@ class SupervisedUserTest : public InProcessBrowserTest, ...@@ -168,14 +122,6 @@ class SupervisedUserTest : public InProcessBrowserTest,
protected: protected:
void SetUpOnMainThread() override { void SetUpOnMainThread() override {
if (GetParam()) {
feature_list.InitAndEnableFeature(
features::kSupervisedUserCommittedInterstitials);
} else {
feature_list.InitAndDisableFeature(
features::kSupervisedUserCommittedInterstitials);
}
// Set up the SupervisedUserNavigationObserver manually since the profile // Set up the SupervisedUserNavigationObserver manually since the profile
// was not supervised when the browser was created. // was not supervised when the browser was created.
content::WebContents* web_contents = content::WebContents* web_contents =
...@@ -224,9 +170,6 @@ class SupervisedUserTest : public InProcessBrowserTest, ...@@ -224,9 +170,6 @@ class SupervisedUserTest : public InProcessBrowserTest,
} }
SupervisedUserService* supervised_user_service_; SupervisedUserService* supervised_user_service_;
private:
base::test::ScopedFeatureList feature_list;
}; };
// Tests the filter mode in which all sites are blocked by default. // Tests the filter mode in which all sites are blocked by default.
...@@ -293,14 +236,8 @@ class TabClosingObserver : public TabStripModelObserver { ...@@ -293,14 +236,8 @@ class TabClosingObserver : public TabStripModelObserver {
DISALLOW_COPY_AND_ASSIGN(TabClosingObserver); DISALLOW_COPY_AND_ASSIGN(TabClosingObserver);
}; };
INSTANTIATE_TEST_SUITE_P(, SupervisedUserTest, ::testing::Values(false, true));
INSTANTIATE_TEST_SUITE_P(,
SupervisedUserBlockModeTest,
::testing::Values(false, true));
// Navigates to a blocked URL. // Navigates to a blocked URL.
IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, IN_PROC_BROWSER_TEST_F(SupervisedUserBlockModeTest,
SendAccessRequestOnBlockedURL) { SendAccessRequestOnBlockedURL) {
GURL test_url("http://www.example.com/simple.html"); GURL test_url("http://www.example.com/simple.html");
ui_test_utils::NavigateToURL(browser(), test_url); ui_test_utils::NavigateToURL(browser(), test_url);
...@@ -313,10 +250,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, ...@@ -313,10 +250,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest,
// TODO(sergiu): Properly check that the access request was sent here. // TODO(sergiu): Properly check that the access request was sent here.
if (AreCommittedInterstitialsEnabled()) GoBackAndWaitForNavigation(tab);
GoBackAndWaitForNavigation(tab);
else
GoBack(tab);
// Make sure that the tab is still there. // Make sure that the tab is still there.
EXPECT_EQ(tab, browser()->tab_strip_model()->GetActiveWebContents()); EXPECT_EQ(tab, browser()->tab_strip_model()->GetActiveWebContents());
...@@ -326,7 +260,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, ...@@ -326,7 +260,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest,
// Navigates to a blocked URL in a new tab. We expect the tab to be closed // Navigates to a blocked URL in a new tab. We expect the tab to be closed
// automatically on pressing the "back" button on the interstitial. // automatically on pressing the "back" button on the interstitial.
IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, OpenBlockedURLInNewTab) { IN_PROC_BROWSER_TEST_F(SupervisedUserBlockModeTest, OpenBlockedURLInNewTab) {
TabStripModel* tab_strip = browser()->tab_strip_model(); TabStripModel* tab_strip = browser()->tab_strip_model();
WebContents* prev_tab = tab_strip->GetActiveWebContents(); WebContents* prev_tab = tab_strip->GetActiveWebContents();
...@@ -353,7 +287,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, OpenBlockedURLInNewTab) { ...@@ -353,7 +287,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, OpenBlockedURLInNewTab) {
// interstitial page behave differently from the preceding test, where the // interstitial page behave differently from the preceding test, where the
// navigation is blocked before it commits). The expected behavior is the same // navigation is blocked before it commits). The expected behavior is the same
// though: the tab should be closed when going back. // though: the tab should be closed when going back.
IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockNewTabAfterLoading) { IN_PROC_BROWSER_TEST_F(SupervisedUserTest, BlockNewTabAfterLoading) {
TabStripModel* tab_strip = browser()->tab_strip_model(); TabStripModel* tab_strip = browser()->tab_strip_model();
WebContents* prev_tab = tab_strip->GetActiveWebContents(); WebContents* prev_tab = tab_strip->GetActiveWebContents();
...@@ -369,11 +303,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockNewTabAfterLoading) { ...@@ -369,11 +303,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockNewTabAfterLoading) {
{ {
// Block the current URL. // Block the current URL.
// TODO(carlosil): Remove this run_loop once Committed interstitials are the
// only code path.
base::RunLoop run_loop;
InterstitialPageObserver interstitial_observer(tab, run_loop.QuitClosure());
SupervisedUserSettingsService* supervised_user_settings_service = SupervisedUserSettingsService* supervised_user_settings_service =
SupervisedUserSettingsServiceFactory::GetForKey( SupervisedUserSettingsServiceFactory::GetForKey(
browser()->profile()->GetProfileKey()); browser()->profile()->GetProfileKey());
...@@ -386,12 +315,8 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockNewTabAfterLoading) { ...@@ -386,12 +315,8 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockNewTabAfterLoading) {
ASSERT_EQ(SupervisedUserURLFilter::BLOCK, ASSERT_EQ(SupervisedUserURLFilter::BLOCK,
filter->GetFilteringBehaviorForURL(test_url)); filter->GetFilteringBehaviorForURL(test_url));
if (AreCommittedInterstitialsEnabled()) { content::TestNavigationObserver observer(tab);
content::TestNavigationObserver observer(tab); observer.Wait();
observer.Wait();
} else {
content::RunThisRunLoop(&run_loop);
}
// Check that we got the interstitial. // Check that we got the interstitial.
ASSERT_TRUE(ShownPageIsInterstitial(browser())); ASSERT_TRUE(ShownPageIsInterstitial(browser()));
...@@ -409,7 +334,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockNewTabAfterLoading) { ...@@ -409,7 +334,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockNewTabAfterLoading) {
// Tests that we don't end up canceling an interstitial (thereby closing the // Tests that we don't end up canceling an interstitial (thereby closing the
// whole tab) by attempting to show a second one above it. // whole tab) by attempting to show a second one above it.
IN_PROC_BROWSER_TEST_P(SupervisedUserTest, DontShowInterstitialTwice) { IN_PROC_BROWSER_TEST_F(SupervisedUserTest, DontShowInterstitialTwice) {
TabStripModel* tab_strip = browser()->tab_strip_model(); TabStripModel* tab_strip = browser()->tab_strip_model();
// Open URL in a new tab. // Open URL in a new tab.
...@@ -426,8 +351,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, DontShowInterstitialTwice) { ...@@ -426,8 +351,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, DontShowInterstitialTwice) {
SupervisedUserSettingsService* supervised_user_settings_service = SupervisedUserSettingsService* supervised_user_settings_service =
SupervisedUserSettingsServiceFactory::GetForKey( SupervisedUserSettingsServiceFactory::GetForKey(
browser()->profile()->GetProfileKey()); browser()->profile()->GetProfileKey());
base::RunLoop run_loop;
InterstitialPageObserver interstitial_observer(tab, run_loop.QuitClosure());
supervised_user_settings_service->SetLocalSetting( supervised_user_settings_service->SetLocalSetting(
supervised_users::kContentPackDefaultFilteringBehavior, supervised_users::kContentPackDefaultFilteringBehavior,
std::make_unique<base::Value>(SupervisedUserURLFilter::BLOCK)); std::make_unique<base::Value>(SupervisedUserURLFilter::BLOCK));
...@@ -437,12 +360,8 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, DontShowInterstitialTwice) { ...@@ -437,12 +360,8 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, DontShowInterstitialTwice) {
ASSERT_EQ(SupervisedUserURLFilter::BLOCK, ASSERT_EQ(SupervisedUserURLFilter::BLOCK,
filter->GetFilteringBehaviorForURL(test_url)); filter->GetFilteringBehaviorForURL(test_url));
if (AreCommittedInterstitialsEnabled()) { content::TestNavigationObserver observer(tab);
content::TestNavigationObserver observer(tab); observer.Wait();
observer.Wait();
} else {
content::RunThisRunLoop(&run_loop);
}
// Check that we got the interstitial. // Check that we got the interstitial.
ASSERT_TRUE(ShownPageIsInterstitial(browser())); ASSERT_TRUE(ShownPageIsInterstitial(browser()));
...@@ -460,7 +379,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, DontShowInterstitialTwice) { ...@@ -460,7 +379,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, DontShowInterstitialTwice) {
// Tests that it's possible to navigate from a blocked page to another blocked // Tests that it's possible to navigate from a blocked page to another blocked
// page. // page.
IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, IN_PROC_BROWSER_TEST_F(SupervisedUserBlockModeTest,
NavigateFromBlockedPageToBlockedPage) { NavigateFromBlockedPageToBlockedPage) {
GURL test_url("http://www.example.com/simple.html"); GURL test_url("http://www.example.com/simple.html");
ui_test_utils::NavigateToURL(browser(), test_url); ui_test_utils::NavigateToURL(browser(), test_url);
...@@ -477,7 +396,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, ...@@ -477,7 +396,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest,
} }
// Tests whether a visit attempt adds a special history entry. // Tests whether a visit attempt adds a special history entry.
IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, HistoryVisitRecorded) { IN_PROC_BROWSER_TEST_F(SupervisedUserBlockModeTest, HistoryVisitRecorded) {
GURL allowed_url("http://www.example.com/simple.html"); GURL allowed_url("http://www.example.com/simple.html");
const SupervisedUserURLFilter* filter = const SupervisedUserURLFilter* filter =
...@@ -508,10 +427,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, HistoryVisitRecorded) { ...@@ -508,10 +427,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, HistoryVisitRecorded) {
ASSERT_TRUE(ShownPageIsInterstitial(browser())); ASSERT_TRUE(ShownPageIsInterstitial(browser()));
WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents(); WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
if (AreCommittedInterstitialsEnabled()) GoBackAndWaitForNavigation(tab);
GoBackAndWaitForNavigation(tab);
else
GoBack(tab);
EXPECT_EQ(allowed_url.spec(), tab->GetURL().spec()); EXPECT_EQ(allowed_url.spec(), tab->GetURL().spec());
EXPECT_EQ(SupervisedUserURLFilter::ALLOW, EXPECT_EQ(SupervisedUserURLFilter::ALLOW,
...@@ -527,22 +443,15 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, HistoryVisitRecorded) { ...@@ -527,22 +443,15 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, HistoryVisitRecorded) {
history::QueryResults results; history::QueryResults results;
QueryHistory(history_service, "", options, &results); QueryHistory(history_service, "", options, &results);
// With committed interstitials enabled, going back to the site is an actual
// back navigation (instead of just closing the interstitial), so the most
// recent history entry will be the allowed site, with non-committed
// interstitials, the most recent one will be the blocked one.
int allowed = AreCommittedInterstitialsEnabled() ? 0 : 1;
int blocked = AreCommittedInterstitialsEnabled() ? 1 : 0;
// Check that the entries have the correct blocked_visit value. // Check that the entries have the correct blocked_visit value.
ASSERT_EQ(2u, results.size()); ASSERT_EQ(2u, results.size());
EXPECT_EQ(blocked_url.spec(), results[blocked].url().spec()); EXPECT_EQ(blocked_url.spec(), results[1].url().spec());
EXPECT_TRUE(results[blocked].blocked_visit()); EXPECT_TRUE(results[1].blocked_visit());
EXPECT_EQ(allowed_url.spec(), results[allowed].url().spec()); EXPECT_EQ(allowed_url.spec(), results[0].url().spec());
EXPECT_FALSE(results[allowed].blocked_visit()); EXPECT_FALSE(results[0].blocked_visit());
} }
IN_PROC_BROWSER_TEST_P(SupervisedUserTest, GoBackOnDontProceed) { IN_PROC_BROWSER_TEST_F(SupervisedUserTest, GoBackOnDontProceed) {
// We start out at the initial navigation. // We start out at the initial navigation.
WebContents* web_contents = WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents(); browser()->tab_strip_model()->GetActiveWebContents();
...@@ -559,9 +468,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, GoBackOnDontProceed) { ...@@ -559,9 +468,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, GoBackOnDontProceed) {
SupervisedUserSettingsService* supervised_user_settings_service = SupervisedUserSettingsService* supervised_user_settings_service =
SupervisedUserSettingsServiceFactory::GetForKey( SupervisedUserSettingsServiceFactory::GetForKey(
browser()->profile()->GetProfileKey()); browser()->profile()->GetProfileKey());
auto message_loop_runner = base::MakeRefCounted<content::MessageLoopRunner>();
InterstitialPageObserver interstitial_observer(
web_contents, message_loop_runner->QuitClosure());
supervised_user_settings_service->SetLocalSetting( supervised_user_settings_service->SetLocalSetting(
supervised_users::kContentPackManualBehaviorHosts, std::move(dict)); supervised_users::kContentPackManualBehaviorHosts, std::move(dict));
...@@ -570,12 +476,8 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, GoBackOnDontProceed) { ...@@ -570,12 +476,8 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, GoBackOnDontProceed) {
ASSERT_EQ(SupervisedUserURLFilter::BLOCK, ASSERT_EQ(SupervisedUserURLFilter::BLOCK,
filter->GetFilteringBehaviorForURL(test_url)); filter->GetFilteringBehaviorForURL(test_url));
if (AreCommittedInterstitialsEnabled()) { content::TestNavigationObserver block_observer(web_contents);
content::TestNavigationObserver observer(web_contents); block_observer.Wait();
observer.Wait();
} else {
message_loop_runner->Run();
}
content::WindowedNotificationObserver observer( content::WindowedNotificationObserver observer(
content::NOTIFICATION_LOAD_STOP, content::NOTIFICATION_LOAD_STOP,
...@@ -587,7 +489,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, GoBackOnDontProceed) { ...@@ -587,7 +489,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, GoBackOnDontProceed) {
EXPECT_EQ(0, web_contents->GetController().GetCurrentEntryIndex()); EXPECT_EQ(0, web_contents->GetController().GetCurrentEntryIndex());
} }
IN_PROC_BROWSER_TEST_P(SupervisedUserTest, ClosingBlockedTabDoesNotCrash) { IN_PROC_BROWSER_TEST_F(SupervisedUserTest, ClosingBlockedTabDoesNotCrash) {
WebContents* web_contents = WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents(); browser()->tab_strip_model()->GetActiveWebContents();
ASSERT_EQ(0, web_contents->GetController().GetCurrentEntryIndex()); ASSERT_EQ(0, web_contents->GetController().GetCurrentEntryIndex());
...@@ -603,9 +505,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, ClosingBlockedTabDoesNotCrash) { ...@@ -603,9 +505,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, ClosingBlockedTabDoesNotCrash) {
SupervisedUserSettingsService* supervised_user_settings_service = SupervisedUserSettingsService* supervised_user_settings_service =
SupervisedUserSettingsServiceFactory::GetForKey( SupervisedUserSettingsServiceFactory::GetForKey(
browser()->profile()->GetProfileKey()); browser()->profile()->GetProfileKey());
auto message_loop_runner = base::MakeRefCounted<content::MessageLoopRunner>();
InterstitialPageObserver interstitial_observer(
web_contents, message_loop_runner->QuitClosure());
supervised_user_settings_service->SetLocalSetting( supervised_user_settings_service->SetLocalSetting(
supervised_users::kContentPackManualBehaviorHosts, std::move(dict)); supervised_users::kContentPackManualBehaviorHosts, std::move(dict));
...@@ -614,13 +513,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, ClosingBlockedTabDoesNotCrash) { ...@@ -614,13 +513,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, ClosingBlockedTabDoesNotCrash) {
ASSERT_EQ(SupervisedUserURLFilter::BLOCK, ASSERT_EQ(SupervisedUserURLFilter::BLOCK,
filter->GetFilteringBehaviorForURL(test_url)); filter->GetFilteringBehaviorForURL(test_url));
message_loop_runner->Run();
if (!AreCommittedInterstitialsEnabled()) {
InterstitialPage* interstitial_page = web_contents->GetInterstitialPage();
ASSERT_TRUE(interstitial_page);
}
// Verify that there is no crash when closing the blocked tab // Verify that there is no crash when closing the blocked tab
// (https://crbug.com/719708). // (https://crbug.com/719708).
browser()->tab_strip_model()->CloseWebContentsAt( browser()->tab_strip_model()->CloseWebContentsAt(
...@@ -628,7 +520,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, ClosingBlockedTabDoesNotCrash) { ...@@ -628,7 +520,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, ClosingBlockedTabDoesNotCrash) {
content::RunAllPendingInMessageLoop(); content::RunAllPendingInMessageLoop();
} }
IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockThenUnblock) { IN_PROC_BROWSER_TEST_F(SupervisedUserTest, BlockThenUnblock) {
GURL test_url("http://www.example.com/simple.html"); GURL test_url("http://www.example.com/simple.html");
ui_test_utils::NavigateToURL(browser(), test_url); ui_test_utils::NavigateToURL(browser(), test_url);
...@@ -643,10 +535,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockThenUnblock) { ...@@ -643,10 +535,6 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockThenUnblock) {
SupervisedUserSettingsService* supervised_user_settings_service = SupervisedUserSettingsService* supervised_user_settings_service =
SupervisedUserSettingsServiceFactory::GetForKey( SupervisedUserSettingsServiceFactory::GetForKey(
browser()->profile()->GetProfileKey()); browser()->profile()->GetProfileKey());
// TODO(carlosil): Remove this run_loop once Committed interstitials are the
// only code path.
base::RunLoop run_loop;
InterstitialPageObserver observer(web_contents, run_loop.QuitClosure());
supervised_user_settings_service->SetLocalSetting( supervised_user_settings_service->SetLocalSetting(
supervised_users::kContentPackManualBehaviorHosts, std::move(dict)); supervised_users::kContentPackManualBehaviorHosts, std::move(dict));
...@@ -655,12 +543,9 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockThenUnblock) { ...@@ -655,12 +543,9 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockThenUnblock) {
ASSERT_EQ(SupervisedUserURLFilter::BLOCK, ASSERT_EQ(SupervisedUserURLFilter::BLOCK,
filter->GetFilteringBehaviorForURL(test_url)); filter->GetFilteringBehaviorForURL(test_url));
if (AreCommittedInterstitialsEnabled()) { content::TestNavigationObserver block_observer(web_contents);
content::TestNavigationObserver observer(web_contents); block_observer.Wait();
observer.Wait();
} else {
content::RunThisRunLoop(&run_loop);
}
ASSERT_TRUE(ShownPageIsInterstitial(browser())); ASSERT_TRUE(ShownPageIsInterstitial(browser()));
dict = std::make_unique<base::DictionaryValue>(); dict = std::make_unique<base::DictionaryValue>();
...@@ -670,17 +555,15 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockThenUnblock) { ...@@ -670,17 +555,15 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserTest, BlockThenUnblock) {
ASSERT_EQ(SupervisedUserURLFilter::ALLOW, ASSERT_EQ(SupervisedUserURLFilter::ALLOW,
filter->GetFilteringBehaviorForURL(test_url)); filter->GetFilteringBehaviorForURL(test_url));
if (AreCommittedInterstitialsEnabled()) { content::TestNavigationObserver unblock_observer(web_contents);
content::TestNavigationObserver observer(web_contents); unblock_observer.Wait();
observer.Wait();
}
ASSERT_EQ(test_url, web_contents->GetURL()); ASSERT_EQ(test_url, web_contents->GetURL());
EXPECT_FALSE(ShownPageIsInterstitial(browser())); EXPECT_FALSE(ShownPageIsInterstitial(browser()));
} }
IN_PROC_BROWSER_TEST_P(SupervisedUserBlockModeTest, Unblock) { IN_PROC_BROWSER_TEST_F(SupervisedUserBlockModeTest, Unblock) {
GURL test_url("http://www.example.com/simple.html"); GURL test_url("http://www.example.com/simple.html");
ui_test_utils::NavigateToURL(browser(), test_url); ui_test_utils::NavigateToURL(browser(), test_url);
......
...@@ -12,7 +12,6 @@ ...@@ -12,7 +12,6 @@
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros.h"
#include "base/scoped_observer.h" #include "base/scoped_observer.h"
#include "base/strings/string_number_conversions.h" #include "base/strings/string_number_conversions.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/task/post_task.h" #include "base/task/post_task.h"
#include "base/values.h" #include "base/values.h"
...@@ -22,7 +21,6 @@ ...@@ -22,7 +21,6 @@
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/supervised_user/supervised_user_service.h" #include "chrome/browser/supervised_user/supervised_user_service.h"
#include "chrome/browser/supervised_user/supervised_user_service_factory.h" #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/pref_names.h" #include "chrome/common/pref_names.h"
#include "chrome/grit/generated_resources.h" #include "chrome/grit/generated_resources.h"
#include "components/infobars/core/infobar.h" #include "components/infobars/core/infobar.h"
...@@ -120,36 +118,16 @@ const content::InterstitialPageDelegate::TypeID ...@@ -120,36 +118,16 @@ const content::InterstitialPageDelegate::TypeID
SupervisedUserInterstitial::kTypeForTesting = SupervisedUserInterstitial::kTypeForTesting =
&SupervisedUserInterstitial::kTypeForTesting; &SupervisedUserInterstitial::kTypeForTesting;
// TODO(carlosil): Remove Show function and the rest of non-committed
// interstitials code once committed interstitials are the only code path.
// static
void SupervisedUserInterstitial::Show(
WebContents* web_contents,
const GURL& url,
supervised_user_error_page::FilteringBehaviorReason reason,
bool initial_page_load,
const base::Callback<void(bool)>& callback) {
DCHECK(!base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials));
// |interstitial_page_| is responsible for deleting the interstitial.
SupervisedUserInterstitial* interstitial = new SupervisedUserInterstitial(
web_contents, url, reason, initial_page_load, callback);
interstitial->Init();
}
// static // static
std::unique_ptr<SupervisedUserInterstitial> SupervisedUserInterstitial::Create( std::unique_ptr<SupervisedUserInterstitial> SupervisedUserInterstitial::Create(
WebContents* web_contents, WebContents* web_contents,
const GURL& url, const GURL& url,
supervised_user_error_page::FilteringBehaviorReason reason, supervised_user_error_page::FilteringBehaviorReason reason,
bool initial_page_load, bool initial_page_load,
const base::Callback<void(bool)>& callback) { base::OnceClosure callback) {
DCHECK(base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials));
std::unique_ptr<SupervisedUserInterstitial> interstitial( std::unique_ptr<SupervisedUserInterstitial> interstitial(
new SupervisedUserInterstitial(web_contents, url, reason, new SupervisedUserInterstitial(web_contents, url, reason,
initial_page_load, callback)); initial_page_load, std::move(callback)));
// Caller is responsible for deleting the interstitial. // Caller is responsible for deleting the interstitial.
interstitial->Init(); interstitial->Init();
...@@ -162,15 +140,12 @@ SupervisedUserInterstitial::SupervisedUserInterstitial( ...@@ -162,15 +140,12 @@ SupervisedUserInterstitial::SupervisedUserInterstitial(
const GURL& url, const GURL& url,
supervised_user_error_page::FilteringBehaviorReason reason, supervised_user_error_page::FilteringBehaviorReason reason,
bool initial_page_load, bool initial_page_load,
const base::Callback<void(bool)>& callback) base::OnceClosure callback)
: web_contents_(web_contents), : web_contents_(web_contents),
profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())), profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())),
interstitial_page_(NULL),
url_(url), url_(url),
reason_(reason), reason_(reason),
initial_page_load_(initial_page_load), callback_(std::move(callback)),
proceeded_(false),
callback_(callback),
scoped_observer_(this), scoped_observer_(this),
weak_ptr_factory_(this) {} weak_ptr_factory_(this) {}
...@@ -207,15 +182,6 @@ void SupervisedUserInterstitial::Init() { ...@@ -207,15 +182,6 @@ void SupervisedUserInterstitial::Init() {
SupervisedUserService* supervised_user_service = SupervisedUserService* supervised_user_service =
SupervisedUserServiceFactory::GetForProfile(profile_); SupervisedUserServiceFactory::GetForProfile(profile_);
scoped_observer_.Add(supervised_user_service); scoped_observer_.Add(supervised_user_service);
if (!base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials)) {
// If committed interstitials are enabled we do not create an
// interstitial_page
interstitial_page_ = content::InterstitialPage::Create(
web_contents_, initial_page_load_, url_, this);
interstitial_page_->Show();
}
} }
// static // static
...@@ -259,12 +225,7 @@ void SupervisedUserInterstitial::CommandReceived(const std::string& command) { ...@@ -259,12 +225,7 @@ void SupervisedUserInterstitial::CommandReceived(const std::string& command) {
UMA_HISTOGRAM_ENUMERATION("ManagedMode.BlockingInterstitialCommand", UMA_HISTOGRAM_ENUMERATION("ManagedMode.BlockingInterstitialCommand",
BACK, BACK,
HISTOGRAM_BOUNDING_VALUE); HISTOGRAM_BOUNDING_VALUE);
if (base::FeatureList::IsEnabled( DontProceedInternal();
features::kSupervisedUserCommittedInterstitials)) {
DontProceedInternal();
} else {
interstitial_page_->DontProceed();
}
return; return;
} }
...@@ -275,9 +236,7 @@ void SupervisedUserInterstitial::CommandReceived(const std::string& command) { ...@@ -275,9 +236,7 @@ void SupervisedUserInterstitial::CommandReceived(const std::string& command) {
SupervisedUserService* supervised_user_service = SupervisedUserService* supervised_user_service =
SupervisedUserServiceFactory::GetForProfile(profile_); SupervisedUserServiceFactory::GetForProfile(profile_);
supervised_user_service->AddURLAccessRequest( supervised_user_service->AddURLAccessRequest(url_, base::DoNothing());
url_, base::Bind(&SupervisedUserInterstitial::OnAccessRequestAdded,
weak_ptr_factory_.GetWeakPtr()));
return; return;
} }
...@@ -335,29 +294,7 @@ SupervisedUserInterstitial::GetTypeForTesting() const { ...@@ -335,29 +294,7 @@ SupervisedUserInterstitial::GetTypeForTesting() const {
void SupervisedUserInterstitial::OnURLFilterChanged() { void SupervisedUserInterstitial::OnURLFilterChanged() {
if (ShouldProceed()) { if (ShouldProceed()) {
if (base::FeatureList::IsEnabled( ProceedInternal();
features::kSupervisedUserCommittedInterstitials)) {
ProceedInternal();
} else {
// Interstitial page deletes the interstitial when proceeding but not
// synchronously, so a check is required to avoid calling proceed twice.
if (!proceeded_)
interstitial_page_->Proceed();
proceeded_ = true;
}
}
}
void SupervisedUserInterstitial::OnAccessRequestAdded(bool success) {
DCHECK(!base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials));
VLOG(1) << "Sent access request for " << url_.spec()
<< (success ? " successfully" : " unsuccessfully");
std::string jsFunc =
base::StringPrintf("setRequestStatus(%s);", success ? "true" : "false");
if (interstitial_page_->GetMainFrame()) {
interstitial_page_->GetMainFrame()->ExecuteJavaScript(
base::ASCIIToUTF16(jsFunc), base::NullCallback());
} }
} }
...@@ -382,20 +319,6 @@ void SupervisedUserInterstitial::MoveAwayFromCurrentPage() { ...@@ -382,20 +319,6 @@ void SupervisedUserInterstitial::MoveAwayFromCurrentPage() {
if (web_contents_->IsBeingDestroyed()) if (web_contents_->IsBeingDestroyed())
return; return;
// If the interstitial was shown during a page load and there is no history
// entry to go back to, attempt to close the tab.
// This check is skipped when committed interstitials are on, because all
// interstitials are treated as initial page loads in this case, the case
// where there is nothing to go back to will be handled by the default case at
// the end.
if (!base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials) &&
initial_page_load_) {
if (web_contents_->GetController().IsInitialBlankNavigation())
TabCloser::MaybeClose(web_contents_);
return;
}
// If the interstitial was shown over an existing page, navigate back from // If the interstitial was shown over an existing page, navigate back from
// that page. If that is not possible, attempt to close the entire tab. // that page. If that is not possible, attempt to close the entire tab.
if (web_contents_->GetController().CanGoBack()) { if (web_contents_->GetController().CanGoBack()) {
...@@ -406,9 +329,8 @@ void SupervisedUserInterstitial::MoveAwayFromCurrentPage() { ...@@ -406,9 +329,8 @@ void SupervisedUserInterstitial::MoveAwayFromCurrentPage() {
TabCloser::MaybeClose(web_contents_); TabCloser::MaybeClose(web_contents_);
} }
void SupervisedUserInterstitial::DispatchContinueRequest( void SupervisedUserInterstitial::OnInterstitialDone() {
bool continue_request) { std::move(callback_).Run();
callback_.Run(continue_request);
// After this, the WebContents may be destroyed. Make sure we don't try to use // After this, the WebContents may be destroyed. Make sure we don't try to use
// it again. // it again.
...@@ -416,17 +338,15 @@ void SupervisedUserInterstitial::DispatchContinueRequest( ...@@ -416,17 +338,15 @@ void SupervisedUserInterstitial::DispatchContinueRequest(
} }
void SupervisedUserInterstitial::ProceedInternal() { void SupervisedUserInterstitial::ProceedInternal() {
if (base::FeatureList::IsEnabled( if (web_contents_) {
features::kSupervisedUserCommittedInterstitials) &&
web_contents_) {
// In the committed interstitials case, there will be nothing to resume, so // In the committed interstitials case, there will be nothing to resume, so
// refresh instead. // refresh instead.
web_contents_->GetController().Reload(content::ReloadType::NORMAL, true); web_contents_->GetController().Reload(content::ReloadType::NORMAL, true);
} }
DispatchContinueRequest(true); OnInterstitialDone();
} }
void SupervisedUserInterstitial::DontProceedInternal() { void SupervisedUserInterstitial::DontProceedInternal() {
MoveAwayFromCurrentPage(); MoveAwayFromCurrentPage();
DispatchContinueRequest(false); OnInterstitialDone();
} }
...@@ -18,7 +18,6 @@ ...@@ -18,7 +18,6 @@
#include "url/gurl.h" #include "url/gurl.h"
namespace content { namespace content {
class InterstitialPage;
class WebContents; class WebContents;
} }
...@@ -36,18 +35,12 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate, ...@@ -36,18 +35,12 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate,
// Interstitial type, used for testing. // Interstitial type, used for testing.
static const content::InterstitialPageDelegate::TypeID kTypeForTesting; static const content::InterstitialPageDelegate::TypeID kTypeForTesting;
static void Show(content::WebContents* web_contents,
const GURL& url,
supervised_user_error_page::FilteringBehaviorReason reason,
bool initial_page_load,
const base::Callback<void(bool)>& callback);
static std::unique_ptr<SupervisedUserInterstitial> Create( static std::unique_ptr<SupervisedUserInterstitial> Create(
content::WebContents* web_contents, content::WebContents* web_contents,
const GURL& url, const GURL& url,
supervised_user_error_page::FilteringBehaviorReason reason, supervised_user_error_page::FilteringBehaviorReason reason,
bool initial_page_load, bool initial_page_load,
const base::Callback<void(bool)>& callback); base::OnceClosure callback);
static std::string GetHTMLContents( static std::string GetHTMLContents(
Profile* profile, Profile* profile,
...@@ -70,7 +63,7 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate, ...@@ -70,7 +63,7 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate,
const GURL& url, const GURL& url,
supervised_user_error_page::FilteringBehaviorReason reason, supervised_user_error_page::FilteringBehaviorReason reason,
bool initial_page_load, bool initial_page_load,
const base::Callback<void(bool)>& callback); base::OnceClosure callback);
void Init(); void Init();
...@@ -84,8 +77,6 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate, ...@@ -84,8 +77,6 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate,
void OnURLFilterChanged() override; void OnURLFilterChanged() override;
// TODO(treib): Also listen to OnCustodianInfoChanged and update as required. // TODO(treib): Also listen to OnCustodianInfoChanged and update as required.
void OnAccessRequestAdded(bool success);
// Returns whether we should now proceed on a previously-blocked URL. // Returns whether we should now proceed on a previously-blocked URL.
// Called initially before the interstitial is shown (to catch race // Called initially before the interstitial is shown (to catch race
// conditions), or when the URL filtering prefs change. Note that this does // conditions), or when the URL filtering prefs change. Note that this does
...@@ -97,7 +88,7 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate, ...@@ -97,7 +88,7 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate,
// the request. // the request.
void MoveAwayFromCurrentPage(); void MoveAwayFromCurrentPage();
void DispatchContinueRequest(bool continue_request); void OnInterstitialDone();
void ProceedInternal(); void ProceedInternal();
...@@ -108,20 +99,10 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate, ...@@ -108,20 +99,10 @@ class SupervisedUserInterstitial : public content::InterstitialPageDelegate,
Profile* profile_; Profile* profile_;
content::InterstitialPage* interstitial_page_; // Owns us.
GURL url_; GURL url_;
supervised_user_error_page::FilteringBehaviorReason reason_; supervised_user_error_page::FilteringBehaviorReason reason_;
// True if the interstitial was shown while loading a page (with a pending base::OnceClosure callback_;
// navigation), false if it was shown over an already loaded page.
// Interstitials behave very differently in those cases.
bool initial_page_load_;
// True if we have already called Proceed() on the interstitial page.
bool proceeded_;
base::Callback<void(bool)> callback_;
ScopedObserver<SupervisedUserService, SupervisedUserInterstitial> ScopedObserver<SupervisedUserService, SupervisedUserInterstitial>
scoped_observer_; scoped_observer_;
......
...@@ -6,7 +6,6 @@ ...@@ -6,7 +6,6 @@
#include "base/bind.h" #include "base/bind.h"
#include "base/callback.h" #include "base/callback.h"
#include "base/feature_list.h"
#include "chrome/browser/history/history_service_factory.h" #include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/supervised_user/supervised_user_interstitial.h" #include "chrome/browser/supervised_user/supervised_user_interstitial.h"
...@@ -14,7 +13,6 @@ ...@@ -14,7 +13,6 @@
#include "chrome/browser/supervised_user/supervised_user_service.h" #include "chrome/browser/supervised_user/supervised_user_service.h"
#include "chrome/browser/supervised_user/supervised_user_service_factory.h" #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
#include "chrome/browser/tab_contents/tab_util.h" #include "chrome/browser/tab_contents/tab_util.h"
#include "chrome/common/chrome_features.h"
#include "components/history/content/browser/history_context_helper.h" #include "components/history/content/browser/history_context_helper.h"
#include "components/history/core/browser/history_service.h" #include "components/history/core/browser/history_service.h"
#include "components/history/core/browser/history_types.h" #include "components/history/core/browser/history_types.h"
...@@ -66,12 +64,10 @@ void SupervisedUserNavigationObserver::OnRequestBlocked( ...@@ -66,12 +64,10 @@ void SupervisedUserNavigationObserver::OnRequestBlocked(
void SupervisedUserNavigationObserver::DidFinishNavigation( void SupervisedUserNavigationObserver::DidFinishNavigation(
content::NavigationHandle* navigation_handle) { content::NavigationHandle* navigation_handle) {
// With committed interstitials on, if this is a different navigation than the // If this is a different navigation than the one that triggered the
// one that triggered the interstitial, clear is_showing_interstitial_ // interstitial, clear is_showing_interstitial_
if (is_showing_interstitial_ && if (is_showing_interstitial_ &&
navigation_handle->GetNavigationId() != interstitial_navigation_id_ && navigation_handle->GetNavigationId() != interstitial_navigation_id_) {
base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials)) {
is_showing_interstitial_ = false; is_showing_interstitial_ = false;
} }
...@@ -150,22 +146,8 @@ void SupervisedUserNavigationObserver::URLFilterCheckCallback( ...@@ -150,22 +146,8 @@ void SupervisedUserNavigationObserver::URLFilterCheckCallback(
if (!is_showing_interstitial_ && if (!is_showing_interstitial_ &&
behavior == SupervisedUserURLFilter::FilteringBehavior::BLOCK) { behavior == SupervisedUserURLFilter::FilteringBehavior::BLOCK) {
if (base::FeatureList::IsEnabled( web_contents()->GetController().Reload(content::ReloadType::NORMAL, false);
features::kSupervisedUserCommittedInterstitials)) { return;
web_contents()->GetController().Reload(content::ReloadType::NORMAL,
false);
return;
}
// TODO(carlosil): For now, we pass a 0 as the navigation id causing the
// interstitial for the non-committed interstitials case since we don't have
// the real id here, this doesn't cause issues since the navigation id is
// not used when committed interstitials are not enabled. This will be
// removed once committed interstitials are the only code path.
const bool initial_page_load = false;
MaybeShowInterstitial(
url, reason, initial_page_load, 0,
base::Callback<void(
SupervisedUserNavigationThrottle::CallbackActions)>());
} }
} }
...@@ -178,54 +160,30 @@ void SupervisedUserNavigationObserver::MaybeShowInterstitial( ...@@ -178,54 +160,30 @@ void SupervisedUserNavigationObserver::MaybeShowInterstitial(
void(SupervisedUserNavigationThrottle::CallbackActions)>& callback) { void(SupervisedUserNavigationThrottle::CallbackActions)>& callback) {
interstitial_navigation_id_ = navigation_id; interstitial_navigation_id_ = navigation_id;
is_showing_interstitial_ = true; is_showing_interstitial_ = true;
base::Callback<void(bool)> wrapped_callback = interstitial_ = SupervisedUserInterstitial::Create(
base::Bind(&SupervisedUserNavigationObserver::OnInterstitialResult, web_contents(), url, reason, initial_page_load,
weak_ptr_factory_.GetWeakPtr(), callback); base::BindOnce(&SupervisedUserNavigationObserver::OnInterstitialDone,
if (base::FeatureList::IsEnabled( weak_ptr_factory_.GetWeakPtr()));
features::kSupervisedUserCommittedInterstitials)) { callback.Run(SupervisedUserNavigationThrottle::CallbackActions::
interstitial_ = SupervisedUserInterstitial::Create( kCancelWithInterstitial);
web_contents(), url, reason, initial_page_load, wrapped_callback);
callback.Run(SupervisedUserNavigationThrottle::CallbackActions::
kCancelWithInterstitial);
return;
}
SupervisedUserInterstitial::Show(web_contents(), url, reason,
initial_page_load, wrapped_callback);
} }
void SupervisedUserNavigationObserver::OnInterstitialResult( void SupervisedUserNavigationObserver::OnInterstitialDone() {
const base::Callback<
void(SupervisedUserNavigationThrottle::CallbackActions)>& callback,
bool result) {
is_showing_interstitial_ = false; is_showing_interstitial_ = false;
// If committed interstitials are enabled, there is no navigation to cancel or
// defer at this point, so just clear the is_showing_interstitial variable.
if (callback && !base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials))
callback.Run(result ? SupervisedUserNavigationThrottle::CallbackActions::
kContinueNavigation
: SupervisedUserNavigationThrottle::CallbackActions::
kCancelNavigation);
} }
void SupervisedUserNavigationObserver::GoBack() { void SupervisedUserNavigationObserver::GoBack() {
DCHECK(base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials));
if (interstitial_ && is_showing_interstitial_) if (interstitial_ && is_showing_interstitial_)
interstitial_->CommandReceived("\"back\""); interstitial_->CommandReceived("\"back\"");
} }
void SupervisedUserNavigationObserver::RequestPermission( void SupervisedUserNavigationObserver::RequestPermission(
RequestPermissionCallback callback) { RequestPermissionCallback callback) {
DCHECK(base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials));
if (interstitial_ && is_showing_interstitial_) if (interstitial_ && is_showing_interstitial_)
interstitial_->RequestPermission(std::move(callback)); interstitial_->RequestPermission(std::move(callback));
} }
void SupervisedUserNavigationObserver::Feedback() { void SupervisedUserNavigationObserver::Feedback() {
DCHECK(base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials));
if (interstitial_ && is_showing_interstitial_) if (interstitial_ && is_showing_interstitial_)
interstitial_->CommandReceived("\"feedback\""); interstitial_->CommandReceived("\"feedback\"");
} }
......
...@@ -82,10 +82,7 @@ class SupervisedUserNavigationObserver ...@@ -82,10 +82,7 @@ class SupervisedUserNavigationObserver
const base::Callback< const base::Callback<
void(SupervisedUserNavigationThrottle::CallbackActions)>& callback); void(SupervisedUserNavigationThrottle::CallbackActions)>& callback);
void OnInterstitialResult( void OnInterstitialDone();
const base::Callback<
void(SupervisedUserNavigationThrottle::CallbackActions)>& callback,
bool result);
// supervised_user::mojom::SupervisedUserCommands implementation. Should not // supervised_user::mojom::SupervisedUserCommands implementation. Should not
// be called when an interstitial is no longer showing. This should be // be called when an interstitial is no longer showing. This should be
......
...@@ -5,7 +5,6 @@ ...@@ -5,7 +5,6 @@
#include "chrome/browser/supervised_user/supervised_user_navigation_throttle.h" #include "chrome/browser/supervised_user/supervised_user_navigation_throttle.h"
#include "base/bind.h" #include "base/bind.h"
#include "base/feature_list.h"
#include "base/location.h" #include "base/location.h"
#include "base/logging.h" #include "base/logging.h"
#include "base/memory/ptr_util.h" #include "base/memory/ptr_util.h"
...@@ -18,7 +17,6 @@ ...@@ -18,7 +17,6 @@
#include "chrome/browser/supervised_user/supervised_user_service.h" #include "chrome/browser/supervised_user/supervised_user_service.h"
#include "chrome/browser/supervised_user/supervised_user_service_factory.h" #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
#include "chrome/browser/supervised_user/supervised_user_url_filter.h" #include "chrome/browser/supervised_user/supervised_user_url_filter.h"
#include "chrome/common/chrome_features.h"
#include "content/public/browser/navigation_handle.h" #include "content/public/browser/navigation_handle.h"
#include "ui/base/page_transition_types.h" #include "ui/base/page_transition_types.h"
#include "url/gurl.h" #include "url/gurl.h"
...@@ -239,24 +237,16 @@ void SupervisedUserNavigationThrottle::OnCheckDone( ...@@ -239,24 +237,16 @@ void SupervisedUserNavigationThrottle::OnCheckDone(
void SupervisedUserNavigationThrottle::OnInterstitialResult( void SupervisedUserNavigationThrottle::OnInterstitialResult(
CallbackActions action) { CallbackActions action) {
switch (action) { switch (action) {
case kContinueNavigation: {
Resume();
break;
}
case kCancelNavigation: { case kCancelNavigation: {
CancelDeferredNavigation(CANCEL); CancelDeferredNavigation(CANCEL);
break; break;
} }
case kCancelWithInterstitial: { case kCancelWithInterstitial: {
DCHECK(base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials));
std::string interstitial_html = std::string interstitial_html =
SupervisedUserInterstitial::GetHTMLContents( SupervisedUserInterstitial::GetHTMLContents(
Profile::FromBrowserContext( Profile::FromBrowserContext(
navigation_handle()->GetWebContents()->GetBrowserContext()), navigation_handle()->GetWebContents()->GetBrowserContext()),
reason_); reason_);
// If committed interstitials are enabled, include the HTML content in the
// ThrottleCheckResult.
CancelDeferredNavigation(content::NavigationThrottle::ThrottleCheckResult( CancelDeferredNavigation(content::NavigationThrottle::ThrottleCheckResult(
CANCEL, net::ERR_BLOCKED_BY_CLIENT, interstitial_html)); CANCEL, net::ERR_BLOCKED_BY_CLIENT, interstitial_html));
} }
......
...@@ -17,11 +17,7 @@ ...@@ -17,11 +17,7 @@
class SupervisedUserNavigationThrottle : public content::NavigationThrottle { class SupervisedUserNavigationThrottle : public content::NavigationThrottle {
public: public:
enum CallbackActions { enum CallbackActions { kCancelNavigation = 0, kCancelWithInterstitial };
kContinueNavigation = 0,
kCancelNavigation,
kCancelWithInterstitial
};
// Returns a new throttle for the given navigation, or nullptr if no // Returns a new throttle for the given navigation, or nullptr if no
// throttling is required. // throttling is required.
......
...@@ -5,11 +5,9 @@ ...@@ -5,11 +5,9 @@
#include <memory> #include <memory>
#include "base/command_line.h" #include "base/command_line.h"
#include "base/feature_list.h"
#include "base/files/file_path.h" #include "base/files/file_path.h"
#include "base/path_service.h" #include "base/path_service.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "base/values.h" #include "base/values.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/supervised_user/supervised_user_constants.h" #include "chrome/browser/supervised_user/supervised_user_constants.h"
...@@ -19,7 +17,6 @@ ...@@ -19,7 +17,6 @@
#include "chrome/browser/supervised_user/supervised_user_settings_service_factory.h" #include "chrome/browser/supervised_user/supervised_user_settings_service_factory.h"
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/chrome_switches.h" #include "chrome/common/chrome_switches.h"
#include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h" #include "chrome/test/base/ui_test_utils.h"
...@@ -48,9 +45,7 @@ static const char* kIframeHost2 = "www.iframe2.com"; ...@@ -48,9 +45,7 @@ static const char* kIframeHost2 = "www.iframe2.com";
} // namespace } // namespace
class SupervisedUserNavigationThrottleTest class SupervisedUserNavigationThrottleTest : public InProcessBrowserTest {
: public InProcessBrowserTest,
public testing::WithParamInterface<bool> {
protected: protected:
SupervisedUserNavigationThrottleTest() {} SupervisedUserNavigationThrottleTest() {}
~SupervisedUserNavigationThrottleTest() override {} ~SupervisedUserNavigationThrottleTest() override {}
...@@ -66,44 +61,24 @@ class SupervisedUserNavigationThrottleTest ...@@ -66,44 +61,24 @@ class SupervisedUserNavigationThrottleTest
supervised_users::kContentPackManualBehaviorHosts, std::move(dict)); supervised_users::kContentPackManualBehaviorHosts, std::move(dict));
} }
bool AreCommittedInterstitialsEnabled();
bool IsInterstitialBeingShown(Browser* browser); bool IsInterstitialBeingShown(Browser* browser);
private: private:
void SetUpOnMainThread() override; void SetUpOnMainThread() override;
void SetUpCommandLine(base::CommandLine* command_line) override; void SetUpCommandLine(base::CommandLine* command_line) override;
base::test::ScopedFeatureList feature_list;
}; };
bool SupervisedUserNavigationThrottleTest::AreCommittedInterstitialsEnabled() {
return base::FeatureList::IsEnabled(
features::kSupervisedUserCommittedInterstitials);
}
bool SupervisedUserNavigationThrottleTest::IsInterstitialBeingShown( bool SupervisedUserNavigationThrottleTest::IsInterstitialBeingShown(
Browser* browser) { Browser* browser) {
WebContents* tab = browser->tab_strip_model()->GetActiveWebContents(); WebContents* tab = browser->tab_strip_model()->GetActiveWebContents();
if (AreCommittedInterstitialsEnabled()) { base::string16 title;
base::string16 title; ui_test_utils::GetCurrentTabTitle(browser, &title);
ui_test_utils::GetCurrentTabTitle(browser, &title); return tab->GetController().GetLastCommittedEntry()->GetPageType() ==
return tab->GetController().GetLastCommittedEntry()->GetPageType() == content::PAGE_TYPE_ERROR &&
content::PAGE_TYPE_ERROR && title == base::ASCIIToUTF16("Site blocked");
title == base::ASCIIToUTF16("Site blocked");
}
return tab->ShowingInterstitialPage();
} }
void SupervisedUserNavigationThrottleTest::SetUpOnMainThread() { void SupervisedUserNavigationThrottleTest::SetUpOnMainThread() {
if (GetParam()) {
feature_list.InitAndEnableFeature(
features::kSupervisedUserCommittedInterstitials);
} else {
feature_list.InitAndDisableFeature(
features::kSupervisedUserCommittedInterstitials);
}
// Resolve everything to localhost. // Resolve everything to localhost.
host_resolver()->AddIPLiteralRule("*", "127.0.0.1", "localhost"); host_resolver()->AddIPLiteralRule("*", "127.0.0.1", "localhost");
...@@ -121,13 +96,9 @@ void SupervisedUserNavigationThrottleTest::SetUpCommandLine( ...@@ -121,13 +96,9 @@ void SupervisedUserNavigationThrottleTest::SetUpCommandLine(
#endif #endif
} }
INSTANTIATE_TEST_SUITE_P(,
SupervisedUserNavigationThrottleTest,
::testing::Values(false, true));
// Tests that navigating to a blocked page simply fails if there is no // Tests that navigating to a blocked page simply fails if there is no
// SupervisedUserNavigationObserver. // SupervisedUserNavigationObserver.
IN_PROC_BROWSER_TEST_P(SupervisedUserNavigationThrottleTest, IN_PROC_BROWSER_TEST_F(SupervisedUserNavigationThrottleTest,
NoNavigationObserverBlock) { NoNavigationObserverBlock) {
Profile* profile = browser()->profile(); Profile* profile = browser()->profile();
SupervisedUserSettingsService* supervised_user_settings_service = SupervisedUserSettingsService* supervised_user_settings_service =
...@@ -150,7 +121,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserNavigationThrottleTest, ...@@ -150,7 +121,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserNavigationThrottleTest,
EXPECT_FALSE(observer.last_navigation_succeeded()); EXPECT_FALSE(observer.last_navigation_succeeded());
} }
IN_PROC_BROWSER_TEST_P(SupervisedUserNavigationThrottleTest, IN_PROC_BROWSER_TEST_F(SupervisedUserNavigationThrottleTest,
BlockMainFrameWithInterstitial) { BlockMainFrameWithInterstitial) {
BlockHost(kExampleHost2); BlockHost(kExampleHost2);
...@@ -165,7 +136,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserNavigationThrottleTest, ...@@ -165,7 +136,7 @@ IN_PROC_BROWSER_TEST_P(SupervisedUserNavigationThrottleTest,
EXPECT_TRUE(IsInterstitialBeingShown(browser())); EXPECT_TRUE(IsInterstitialBeingShown(browser()));
} }
IN_PROC_BROWSER_TEST_P(SupervisedUserNavigationThrottleTest, IN_PROC_BROWSER_TEST_F(SupervisedUserNavigationThrottleTest,
DontBlockSubFrame) { DontBlockSubFrame) {
BlockHost(kExampleHost2); BlockHost(kExampleHost2);
BlockHost(kIframeHost2); BlockHost(kIframeHost2);
...@@ -199,11 +170,7 @@ class SupervisedUserNavigationThrottleNotSupervisedTest ...@@ -199,11 +170,7 @@ class SupervisedUserNavigationThrottleNotSupervisedTest
void SetUpCommandLine(base::CommandLine* command_line) override {} void SetUpCommandLine(base::CommandLine* command_line) override {}
}; };
INSTANTIATE_TEST_SUITE_P(, IN_PROC_BROWSER_TEST_F(SupervisedUserNavigationThrottleNotSupervisedTest,
SupervisedUserNavigationThrottleNotSupervisedTest,
::testing::Values(false, true));
IN_PROC_BROWSER_TEST_P(SupervisedUserNavigationThrottleNotSupervisedTest,
DontBlock) { DontBlock) {
BlockHost(kExampleHost); BlockHost(kExampleHost);
......
...@@ -618,11 +618,6 @@ const base::Feature kNativeSmb{"NativeSmb", base::FEATURE_ENABLED_BY_DEFAULT}; ...@@ -618,11 +618,6 @@ const base::Feature kNativeSmb{"NativeSmb", base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kSoundContentSetting{"SoundContentSetting", const base::Feature kSoundContentSetting{"SoundContentSetting",
base::FEATURE_ENABLED_BY_DEFAULT}; base::FEATURE_ENABLED_BY_DEFAULT};
// Enables or disabled committed interstitials for Supervised User
// interstitials.
const base::Feature kSupervisedUserCommittedInterstitials{
"SupervisedUserCommittedInterstitials", base::FEATURE_ENABLED_BY_DEFAULT};
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
// Enables or disables chrome://sys-internals. // Enables or disables chrome://sys-internals.
const base::Feature kSysInternals{"SysInternals", const base::Feature kSysInternals{"SysInternals",
......
...@@ -390,9 +390,6 @@ COMPONENT_EXPORT(CHROME_FEATURES) extern const base::Feature kNativeSmb; ...@@ -390,9 +390,6 @@ COMPONENT_EXPORT(CHROME_FEATURES) extern const base::Feature kNativeSmb;
COMPONENT_EXPORT(CHROME_FEATURES) COMPONENT_EXPORT(CHROME_FEATURES)
extern const base::Feature kSoundContentSetting; extern const base::Feature kSoundContentSetting;
COMPONENT_EXPORT(CHROME_FEATURES)
extern const base::Feature kSupervisedUserCommittedInterstitials;
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
COMPONENT_EXPORT(CHROME_FEATURES) COMPONENT_EXPORT(CHROME_FEATURES)
extern const base::Feature kSysInternals; extern const base::Feature kSysInternals;
......
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