Skip to content

Commit

Permalink
[subresource_filter] Fix smart UI on Android
Browse files Browse the repository at this point in the history
This regressed in crrev.com/c/1081370 due to a combination of
1. Over-eager refactoring
2. Phantom tests that didn't actually test the functionality.

The regression is reverted here and the test for the feature is
fixed so it actually triggers the behavior.

TBR=jkarlin@chromium.org

Bug: 880829
Change-Id: Ic27a0718de685acea00af0b5d88e249b5fb0a6c2
Reviewed-on: https://chromium-review.googlesource.com/1206674
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588930}(cherry picked from commit d27345d)
Reviewed-on: https://chromium-review.googlesource.com/1211544
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#94}
Cr-Branched-From: 79f7c91-refs/heads/master@{#587811}
  • Loading branch information
csharrison committed Sep 6, 2018
1 parent 9756e17 commit e25f5dd
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,13 @@ bool SubresourceFilterContentSettingsManager::ShouldShowUIForSite(

void SubresourceFilterContentSettingsManager::
ResetSiteMetadataBasedOnActivation(const GURL& url, bool is_activated) {
SetSiteMetadata(
url, is_activated ? std::make_unique<base::DictionaryValue>() : nullptr);
// Do not reset the metadata if it exists already, it could clobber an
// existing timestamp.
if (!is_activated) {
SetSiteMetadata(url, nullptr);
} else if (!GetSiteMetadata(url)) {
SetSiteMetadata(url, std::make_unique<base::DictionaryValue>());
}
}

std::unique_ptr<base::DictionaryValue>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
// android-only feature.
IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
DoNotShowUIUntilThresholdReached) {
settings_manager()->set_should_use_smart_ui_for_testing(true);
ASSERT_NO_FATAL_FAILURE(
SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
GURL a_url(embedded_test_server()->GetURL(
Expand Down Expand Up @@ -261,12 +262,10 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
ui_test_utils::NavigateToURL(browser(), a_url);
EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));

bool use_smart_ui = settings_manager()->should_use_smart_ui();
EXPECT_EQ(client->did_show_ui_for_navigation(), !use_smart_ui);
EXPECT_EQ(client->did_show_ui_for_navigation(), false);

histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
SubresourceFilterAction::kUISuppressed,
use_smart_ui ? 1 : 0);
SubresourceFilterAction::kUISuppressed, 1);

ConfigureAsPhishingURL(b_url);

Expand All @@ -285,8 +284,7 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
EXPECT_TRUE(client->did_show_ui_for_navigation());

histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
SubresourceFilterAction::kUISuppressed,
use_smart_ui ? 1 : 0);
SubresourceFilterAction::kUISuppressed, 1);
}

} // namespace subresource_filter

0 comments on commit e25f5dd

Please sign in to comment.