Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795053002:
[subresource_filter] Implement the "Smart" UI on Android (Closed)
|
Created:
3 years, 8 months ago by Charlie Harrison Modified:
3 years, 7 months ago CC:
chromium-reviews, msramek+watch_chromium.org, subresource-filter-reviews_chromium.org, jam, raymes+watch_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, markusheintz_ Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Implement the "Smart" UI on Android
This patch implements a V1 for the "smart" UI, which just logs a
timestamp every time the UI is shown, and will not show the UI on
subsequent navigations if it is within 30 minutes of a previous impression.
This is implemented using a new WebsiteSetting, which holds a timestamp.
BUG=689992
Review-Url: https://codereview.chromium.org/2795053002
Cr-Commit-Position: refs/heads/master@{#470217}
Committed: https://chromium.googlesource.com/chromium/src/+/3f8cdfd3ab1a3e87f90c37f3d7763483ac9e03c4
Patch Set 1 #Patch Set 2 : minor fixes #Patch Set 3 : fix tests #
Total comments: 18
Patch Set 4 : lots of changes #Patch Set 5 : rebase on #463637 #
Total comments: 46
Patch Set 6 : engedy review #
Total comments: 4
Patch Set 7 : raymes review #
Total comments: 21
Patch Set 8 : jkarlin review #
Total comments: 1
Patch Set 9 : msramek review #Patch Set 10 : remove problematic code #Patch Set 11 : rebase #
Total comments: 6
Patch Set 12 : update histograms.xmkl #Patch Set 13 : rebase on #468985 #
Total comments: 5
Patch Set 14 : msramek review #Patch Set 15 : minor tweaks #Dependent Patchsets: Messages
Total messages: 94 (65 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [subresource_filter] Implement the "Smart" UI on Android BUG=689992 ========== to ========== [subresource_filter] Implement the "Smart" UI on Android This patch implements a V1 for the "smart" UI, which just logs a timestamp every time the UI is shown, and will not show the UI on subsequent navigations if it is within 24 hours of a previous impression. BUG=689992 ==========
csharrison@chromium.org changed reviewers: + engedy@chromium.org
engedy: This is a strawman implementation for the smart ui. Have we ironed out the requirements? I'm mainly wondering if we want to keep another bit of metadata "num UI shown for site". It seemed like consensus was that we should stop showing the UI after a single impression though.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/03 21:17:21, Charlie Harrison wrote: > engedy: This is a strawman implementation for the smart ui. Have we ironed out > the requirements? I'm mainly wondering if we want to keep another bit of > metadata "num UI shown for site". It seemed like consensus was that we should > stop showing the UI after a single impression though. That was my impression too, but let me double-check that.
Looking good, some initial comments. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:29: const int kUIShowThresholdHours = 24; nit: constexpr base::TimeDelta k...Threshold = base::TimeDelta::FromHours(24); https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:47: const GURL& url, nit: Have you considered simply using web_contents_->GetLastCommittedURL() here? Would that be unreliable? https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:98: bool ChromeSubresourceFilterClient::UsingSmartUI() const { nit: How about calling this ShouldUseSmartUI? https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:106: HostContentSettingsMap* ChromeSubresourceFilterClient::GetSettingsMap() const { nit: Can we make this a global function in an anonymous namespace? https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:126: url, url, For REQUESTING_ORIGIN_ONLY_SCOPE website settings, |secondary_url| is ignored, so let's just pass GURL(). https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:82: void set_clock_for_testing(std::unique_ptr<base::Clock> tick_clock) { nit: Have you considered supplying this in the ctor instead? https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:90: void OnUIShown(const GURL& url); nit: How about OnDidShowUI? https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:95: // For testing. nit: I find this comment a bit misleading, as this clock is always used, it is only that it can be mocked out for testing.
On 2017/04/04 13:35:17, engedy wrote: > On 2017/04/03 21:17:21, Charlie Harrison wrote: > > engedy: This is a strawman implementation for the smart ui. Have we ironed out > > the requirements? I'm mainly wondering if we want to keep another bit of > > metadata "num UI shown for site". It seemed like consensus was that we should > > stop showing the UI after a single impression though. > > That was my impression too, but let me double-check that. Yeah, reading through all the discussions, looks like that was indeed the consensus. In any case, if we need it, we can certainly add that counter in a later CL, as the whole thing is behind the flag.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
engedy: Would you take another look? This is now heavily refactored (+ dependent on another refactor). Practically speaking, I could split this up into two changes, with one patch dealing with just content settings and ignoring website settings for now. LMK what you think. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:29: const int kUIShowThresholdHours = 24; On 2017/04/04 13:35:59, engedy wrote: > nit: constexpr base::TimeDelta k...Threshold = base::TimeDelta::FromHours(24); Done. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:47: const GURL& url, On 2017/04/04 13:35:59, engedy wrote: > nit: Have you considered simply using web_contents_->GetLastCommittedURL() here? > Would that be unreliable? Ahh, much better :) Done. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:98: bool ChromeSubresourceFilterClient::UsingSmartUI() const { On 2017/04/04 13:35:59, engedy wrote: > nit: How about calling this ShouldUseSmartUI? Done. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:106: HostContentSettingsMap* ChromeSubresourceFilterClient::GetSettingsMap() const { On 2017/04/04 13:35:59, engedy wrote: > nit: Can we make this a global function in an anonymous namespace? Would prefer not to since we'd need to inject the web_contents_ into every call site. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:126: url, url, On 2017/04/04 13:35:59, engedy wrote: > For REQUESTING_ORIGIN_ONLY_SCOPE website settings, |secondary_url| is ignored, > so let's just pass GURL(). Done. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:82: void set_clock_for_testing(std::unique_ptr<base::Clock> tick_clock) { On 2017/04/04 13:36:00, engedy wrote: > nit: Have you considered supplying this in the ctor instead? Yeah, but it isn't ideal, because we want to reset it in browser tests, which don't get to construct their own client. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:90: void OnUIShown(const GURL& url); On 2017/04/04 13:36:00, engedy wrote: > nit: How about OnDidShowUI? Done. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:95: // For testing. On 2017/04/04 13:35:59, engedy wrote: > nit: I find this comment a bit misleading, as this clock is always used, it is > only that it can be mocked out for testing. Updated the comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM % comments. Once those are addressed, I think this covers all the corner cases we have identified. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:106: HostContentSettingsMap* ChromeSubresourceFilterClient::GetSettingsMap() const { On 2017/04/10 14:58:53, Charlie Harrison wrote: > On 2017/04/04 13:35:59, engedy wrote: > > nit: Can we make this a global function in an anonymous namespace? > > Would prefer not to since we'd need to inject the web_contents_ into every call > site. Acknowledged. https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2795053002/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:82: void set_clock_for_testing(std::unique_ptr<base::Clock> tick_clock) { On 2017/04/10 14:58:53, Charlie Harrison wrote: > On 2017/04/04 13:36:00, engedy wrote: > > nit: Have you considered supplying this in the ctor instead? > > Yeah, but it isn't ideal, because we want to reset it in browser tests, which > don't get to construct their own client. Acknowledged. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:23: settings_manager_ = nit: Can we put this on the initializer list? We would lose the DCHECK, though. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:64: const GURL& url) { nit: Do you think it would add some clarity to s/url/top_level_url/gc where relevant in method signatures throughout this CL? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:75: if (state.activation_level == subresource_filter::ActivationLevel::DISABLED && Should we also remove the content setting if this is DRYRUN? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:87: void ChromeSubresourceFilterClient::OnDidShowUI(const GURL& url) { This is now three lines and called from a single call site. WDYT about inlining this? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:58: // on navigations within a certain time period. nit: How about: // on navigations on the same origin within a certain time. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:64: // kActionContentSettingsBlcoked. nit: typo nit: Clarify what `being a subset` means, i.e. this is reported alongside that other action. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:877: settings_manager()->should_use_smart_ui() ? 1 : 2); I'm not sure I understand. If it's cross site, shouldn't this be 2 in both cases? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1024: EXPECT_EQ(CONTENT_SETTING_ALLOW, settings_manager()->GetContentSetting(url)); nit: Can you please add an expectation that this is at the default value before the navigation? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1034: ConfigureAsPhishingURL(GURL("https://example.other/")); Does this really work? I thought we would just have two blacklisted URLs after this line. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1106: kActionUISuppressed, 1); Don't we need to make this conditional on |use_smart_ui| too? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:25: const char kUILastShowTime[] = "LastShowTime"; nit: InfobarLastShownTime ? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:38: SubresourceFilterContentSettingsManager::kUIShowThresholdTime = nit: Something along the lines of kDelayBeforeShowingInfobarAgain or kInfobarCooldown[Duration]? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:71: base::AutoReset<bool>(&ignore_settings_changes_, !log_metrics); |ignore_settings_changes_| is never read. Let's also add a unit test. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:81: auto predicate = base::Bind( #include "base/bind.h" https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:87: ContentSettingsPattern::FromURLNoWildcard(url)); If I understand correctly, this is per-origin granularity. Safe Browsing allows site-chunk based activation (using at most 4 path segments). I believe this is very much an edge case, but might be worth thinking through that nothing really bad happens. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:100: auto dict = base::MakeUnique<base::DictionaryValue>(); nit: Do we need to include "base/values.h"? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:196: kActionContentSettingsBlockedWhileUISuppressed); Wouldn't we reach this even if the user toggled the feature on the infobar itself we had just shown for the very first time? Let's add a unit test for this. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:8: #include <memory> #include <utility> for std::move https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:28: // collection. It is also contains helpers to get/set content settings. nit: Swap the order. I'd argue that it is now the second sentence that defines the primary responsibility of this class. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:53: static const base::TimeDelta kUIShowThresholdTime; #include "base/time.h" https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory_unittest.cc (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory_unittest.cc:31: auto test_clock = base::MakeUnique<base::SimpleTestClock>(); #include "base/memory/ptr_util.h" https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory_unittest.cc:33: settings_manager_->set_clock_for_testing(std::move(test_clock)); #include <utility> https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory_unittest.cc:135: // Showing the UI should trigger a forced content setting update, but no nit: Let's enforce the first part of this sentence here too.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [subresource_filter] Implement the "Smart" UI on Android This patch implements a V1 for the "smart" UI, which just logs a timestamp every time the UI is shown, and will not show the UI on subsequent navigations if it is within 24 hours of a previous impression. BUG=689992 ========== to ========== [subresource_filter] Implement the "Smart" UI on Android This patch implements a V1 for the "smart" UI, which just logs a timestamp every time the UI is shown, and will not show the UI on subsequent navigations if it is within 24 hours of a previous impression. This is implemented using a new WebsiteSetting, which holds a timestamp. Additionally, this CL adds logic to ensure that specific domains will always show up in the settings UI if they are activated by safe browsing, and will disappear from the settings UI if safe browsing considers them inactive. This is done by explicitly marking those sites as Allowed when they are activated, and clearing the content setting if a subsequent navigation considers them inactive. BUG=689992 ==========
csharrison@chromium.org changed reviewers: + msramek@chromium.org
Thanks engedy. msramek, would you PTAL at everything here? https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:23: settings_manager_ = On 2017/04/12 14:02:50, engedy wrote: > nit: Can we put this on the initializer list? We would lose the DCHECK, though. Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:64: const GURL& url) { On 2017/04/12 14:02:50, engedy wrote: > nit: Do you think it would add some clarity to s/url/top_level_url/gc where > relevant in method signatures throughout this CL? Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:75: if (state.activation_level == subresource_filter::ActivationLevel::DISABLED && On 2017/04/12 14:02:50, engedy wrote: > Should we also remove the content setting if this is DRYRUN? Sure. Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:87: void ChromeSubresourceFilterClient::OnDidShowUI(const GURL& url) { On 2017/04/12 14:02:50, engedy wrote: > This is now three lines and called from a single call site. WDYT about inlining > this? Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:58: // on navigations within a certain time period. On 2017/04/12 14:02:50, engedy wrote: > nit: How about: > > // on navigations on the same origin within a certain time. Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:64: // kActionContentSettingsBlcoked. On 2017/04/12 14:02:50, engedy wrote: > nit: typo > nit: Clarify what `being a subset` means, i.e. this is reported alongside that > other action. Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:877: settings_manager()->should_use_smart_ui() ? 1 : 2); On 2017/04/12 14:02:50, engedy wrote: > I'm not sure I understand. If it's cross site, shouldn't this be 2 in both > cases? Yeah, this is an error. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1024: EXPECT_EQ(CONTENT_SETTING_ALLOW, settings_manager()->GetContentSetting(url)); On 2017/04/12 14:02:50, engedy wrote: > nit: Can you please add an expectation that this is at the default value before > the navigation? Done, it's still just CONTENT_SETTING_ALLOW. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1034: ConfigureAsPhishingURL(GURL("https://example.other/")); On 2017/04/12 14:02:50, engedy wrote: > Does this really work? I thought we would just have two blacklisted URLs after > this line. Yes this is how the test utils are currently designed. I might be able to improve the test utils in a follow up though. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1106: kActionUISuppressed, 1); On 2017/04/12 14:02:50, engedy wrote: > Don't we need to make this conditional on |use_smart_ui| too? Yes, done. Sorry there were a bunch of mistakes, it's a bit hard to test both paths locally. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:25: const char kUILastShowTime[] = "LastShowTime"; On 2017/04/12 14:02:50, engedy wrote: > nit: InfobarLastShownTime ? Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:38: SubresourceFilterContentSettingsManager::kUIShowThresholdTime = On 2017/04/12 14:02:51, engedy wrote: > nit: Something along the lines of kDelayBeforeShowingInfobarAgain or > kInfobarCooldown[Duration]? Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:71: base::AutoReset<bool>(&ignore_settings_changes_, !log_metrics); On 2017/04/12 14:02:50, engedy wrote: > |ignore_settings_changes_| is never read. Let's also add a unit test. Done. Great catch. Fallout from the previous refactor. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:81: auto predicate = base::Bind( On 2017/04/12 14:02:50, engedy wrote: > #include "base/bind.h" Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:87: ContentSettingsPattern::FromURLNoWildcard(url)); On 2017/04/12 14:02:50, engedy wrote: > If I understand correctly, this is per-origin granularity. Safe Browsing allows > site-chunk based activation (using at most 4 path segments). I believe this is > very much an edge case, but might be worth thinking through that nothing really > bad happens. Ok. So, if safebrowsing does chunking it means that an origin could have matching URLs and non-matching URLs. If a user navigates to a non-matching URL, the entire origin is cleared and it no longer shows up in the content setting UI. Subsequent navigations to that origin will trigger the UI to show and ignore all previous whitelisting. I think this is fine, unless the safebrowsing lists use a lot of chunking. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:100: auto dict = base::MakeUnique<base::DictionaryValue>(); On 2017/04/12 14:02:51, engedy wrote: > nit: Do we need to include "base/values.h"? Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:196: kActionContentSettingsBlockedWhileUISuppressed); On 2017/04/12 14:02:50, engedy wrote: > Wouldn't we reach this even if the user toggled the feature on the infobar > itself we had just shown for the very first time? > > Let's add a unit test for this. I've fixed this by changing "log_metrics" to "from_ui" in the code, and SetContentSetting never logs metrics here. Now this method will only trigger for content settings changed via the content setting UI. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:8: #include <memory> On 2017/04/12 14:02:51, engedy wrote: > #include <utility> for std::move Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:28: // collection. It is also contains helpers to get/set content settings. On 2017/04/12 14:02:51, engedy wrote: > nit: Swap the order. I'd argue that it is now the second sentence that defines > the primary responsibility of this class. Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:53: static const base::TimeDelta kUIShowThresholdTime; On 2017/04/12 14:02:51, engedy wrote: > #include "base/time.h" Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory_unittest.cc (right): https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory_unittest.cc:31: auto test_clock = base::MakeUnique<base::SimpleTestClock>(); On 2017/04/12 14:02:51, engedy wrote: > #include "base/memory/ptr_util.h" Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory_unittest.cc:33: settings_manager_->set_clock_for_testing(std::move(test_clock)); On 2017/04/12 14:02:51, engedy wrote: > #include <utility> Done. https://codereview.chromium.org/2795053002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory_unittest.cc:135: // Showing the UI should trigger a forced content setting update, but no On 2017/04/12 14:02:51, engedy wrote: > nit: Let's enforce the first part of this sentence here too. Done.
csharrison@chromium.org changed reviewers: + jkarlin@chromium.org
jkarlin: PTAL as shadow
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
raymes@chromium.org changed reviewers: + raymes@chromium.org
Some drive-by nits. https://codereview.chromium.org/2795053002/diff/100001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2795053002/diff/100001/components/content_set... components/content_settings/core/common/content_settings.cc:65: CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER_DATA, Could you please omit this for now until I've landed https://codereview.chromium.org/2728303002/ ? I can add it afterwards. https://codereview.chromium.org/2795053002/diff/100001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2795053002/diff/100001/components/content_set... components/content_settings/core/common/content_settings_types.h:52: CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER_DATA, nit: Please add a comment about what will be stored in this.
Thanks raymes! https://codereview.chromium.org/2795053002/diff/100001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2795053002/diff/100001/components/content_set... components/content_settings/core/common/content_settings.cc:65: CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER_DATA, On 2017/04/12 22:35:15, raymes wrote: > Could you please omit this for now until I've landed > https://codereview.chromium.org/2728303002/ ? I can add it afterwards. Done. https://codereview.chromium.org/2795053002/diff/100001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2795053002/diff/100001/components/content_set... components/content_settings/core/common/content_settings_types.h:52: CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER_DATA, On 2017/04/12 22:35:15, raymes wrote: > nit: Please add a comment about what will be stored in this. Done.
lgtm (not a super close review) with a couple of comments https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:81: ChromeSubresourceFilterClient::LogAction(kActionContentSettingsBlocked); Is this meant to be part of the same condition? https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:54: static const base::TimeDelta kDelayBeforeShowingInfobarAgain; static duration variables must be POD
https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:54: static const base::TimeDelta kDelayBeforeShowingInfobarAgain; On 2017/04/13 12:57:18, jkarlin wrote: > static duration variables must be POD The fashionable thing to do nowadays is to make this a constexpr and initialize it at declaration time here (while retaining a definition without an initializer clause in the .cc file). TimeDelta is a literal type, making this qualify for constant initialization, and while the compiler is not required to precompute the binary representation of the object at compile-time, apparently this happens with all our toolchains.
https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:54: static const base::TimeDelta kDelayBeforeShowingInfobarAgain; On 2017/04/13 13:23:32, engedy wrote: > On 2017/04/13 12:57:18, jkarlin wrote: > > static duration variables must be POD > > The fashionable thing to do nowadays is to make this a constexpr and initialize > it at declaration time here (while retaining a definition without an initializer > clause in the .cc file). > > TimeDelta is a literal type, making this qualify for constant initialization, > and while the compiler is not required to precompute the binary representation > of the object at compile-time, apparently this happens with all our toolchains. Good point, and https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/static... agrees with you. This is fine with me as a constexpr.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
thanks! https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:81: ChromeSubresourceFilterClient::LogAction(kActionContentSettingsBlocked); On 2017/04/13 12:57:18, jkarlin wrote: > Is this meant to be part of the same condition? Hm, now that you mention it it does kind of make sense for this not to be logged here, so the buckets are exclusive.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
To be clear, I've updated the current PS to not log the standard "Block" enum when the blocking happens from our standard UI.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
I left a first round of comments, but need to take a closer look yet :) https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:80: kActionContentSettingsBlockedFromUI); We're logging "BlockedFromUI" regardless of |setting|. Can we either make a part of the condition, or DCHECK() and document why we think this can never be ALLOW? https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:95: settings_map_->ClearSettingsForOneTypeWithPredicate( Do you expect to match more than one setting? You only use origin-scoped primary patterns, and no secondary patterns, and for a given pattern pair you can't have more than one value. SetContentSetting(url, CONTENT_SETTING_DEFAULT, false /* from_ui */) should do the trick. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:139: void SubresourceFilterContentSettingsManager::SetWebsiteSetting( optional nit: I appreciate that I can now point people to your codebase to explain the difference between content settings and website settings. However, wouldn't it be more readable for you to call them more semantically? Like "permission" and "metadata" or something such? https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:207: // Reset the smart UI to ensure tne user can easily change their decision. typo: the https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:34: public content_settings::Observer { Also HistoryServiceObserver and please delete the [url.origin(), timestamp] pairs on history url deletions. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.cc (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.cc:16: SubresourceFilterContentSettingsManagerFactory::EnsureForProfile( IIRC you named EnsureForProfile() because it returned void. Should we name it GetForProfile() now? https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.h (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.h:11: class BrowserContext; nit: This has no effect. There is no class named BrowserContext in the empty namespace, and content::BrowserContext is already imported by one of the includes. https://codereview.chromium.org/2795053002/diff/120001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/2795053002/diff/120001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:172: WebsiteSettingsRegistry::PLATFORM_ANDROID, nit: Unnecessary classification.
s/take a closer look/talk to engedy@/ I was primarily confused about this: https://codereview.chromium.org/2795053002/diff/140001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2795053002/diff/140001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1027: // Should have two rules now, one explicitly setting the origin of |url| to This looks really wrong to me. I have one setting, I navigate somewhere, now I have two settings. There are two issues: 1. Privacy. There is no precedent to content setting exceptions being added automatically. It's always manually through the settings UI or through accepting a permission prompt. This is an important guarantee to keep, because content settings reveal your browsing behavior yet are not deleted together with browsing history. 2. As was explained to me, you're explicitly setting the exception to the same value as default in order to force it to appear in the page info dialog. That's a fragile dependency on an implementation detail of that dialog. Basically, the permissions team decided back in the day that showing all settings in page info is too much, and that a reasonable design would be to show only non-default settings. If their design doesn't fit your usecase, please tell them! Instead of working around it. And in short-term, please try to special-case your content setting in page info. I don't own that code, but I vaguely remember that it does a bunch of special-casing already, so I'm sure that's allowed.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The current PS doesn't change the troublesome content setting behavior, only responding to your first comments. Thanks for the detailed review, and the concerns you bring up about mucking with content settings this way is heard loud and clear. I'll follow up with the permission folks to see what they think (+ look at page info as you mentioned). For this patch, let's remove that behavior. engedy: does it sound good to you? If we're concerned that 24 hours is too long a threshold without this behavior we can reduce it to something like 30min. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:80: kActionContentSettingsBlockedFromUI); On 2017/04/13 16:08:27, msramek wrote: > We're logging "BlockedFromUI" regardless of |setting|. Can we either make a part > of the condition, or DCHECK() and document why we think this can never be ALLOW? DCHECK sounds good. As of now, the "standard" UI for subresource filtering doesn't allow for changing the setting to ALLOW. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:95: settings_map_->ClearSettingsForOneTypeWithPredicate( On 2017/04/13 16:08:27, msramek wrote: > Do you expect to match more than one setting? You only use origin-scoped primary > patterns, and no secondary patterns, and for a given pattern pair you can't have > more than one value. > > SetContentSetting(url, CONTENT_SETTING_DEFAULT, false /* from_ui */) should do > the trick. Done. That makes sense. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:139: void SubresourceFilterContentSettingsManager::SetWebsiteSetting( On 2017/04/13 16:08:27, msramek wrote: > optional nit: I appreciate that I can now point people to your codebase to > explain the difference between content settings and website settings. However, > wouldn't it be more readable for you to call them more semantically? Like > "permission" and "metadata" or something such? Sure. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:207: // Reset the smart UI to ensure tne user can easily change their decision. On 2017/04/13 16:08:27, msramek wrote: > typo: the Done. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h:34: public content_settings::Observer { On 2017/04/13 16:08:27, msramek wrote: > Also HistoryServiceObserver and please delete the [url.origin(), timestamp] > pairs on history url deletions. Done + some tests. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.cc (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.cc:16: SubresourceFilterContentSettingsManagerFactory::EnsureForProfile( On 2017/04/13 16:08:27, msramek wrote: > IIRC you named EnsureForProfile() because it returned void. Should we name it > GetForProfile() now? Done. https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.h (right): https://codereview.chromium.org/2795053002/diff/120001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.h:11: class BrowserContext; On 2017/04/13 16:08:27, msramek wrote: > nit: This has no effect. There is no class named BrowserContext in the empty > namespace, and content::BrowserContext is already imported by one of the > includes. Oops, removed. I thought we had a presubmit for this issue.. https://codereview.chromium.org/2795053002/diff/120001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/2795053002/diff/120001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:172: WebsiteSettingsRegistry::PLATFORM_ANDROID, On 2017/04/13 16:08:27, msramek wrote: > nit: Unnecessary classification. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> For this patch, let's remove that behavior. engedy: does it sound good to you? > If we're concerned that 24 hours is too long a threshold without this behavior > we can reduce it to something like 30min. Sounds good to me.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK, PS 10 removes the problematic code that implicitly sets / clears content settings. This entails deleting the now unused OnActivationComputed callback. I kept the refactoring in the driver_factory in case we add the callback in the future. I've also reduced the cooldown to 30 minutes (from 24 hours).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [subresource_filter] Implement the "Smart" UI on Android This patch implements a V1 for the "smart" UI, which just logs a timestamp every time the UI is shown, and will not show the UI on subsequent navigations if it is within 24 hours of a previous impression. This is implemented using a new WebsiteSetting, which holds a timestamp. Additionally, this CL adds logic to ensure that specific domains will always show up in the settings UI if they are activated by safe browsing, and will disappear from the settings UI if safe browsing considers them inactive. This is done by explicitly marking those sites as Allowed when they are activated, and clearing the content setting if a subsequent navigation considers them inactive. BUG=689992 ========== to ========== [subresource_filter] Implement the "Smart" UI on Android This patch implements a V1 for the "smart" UI, which just logs a timestamp every time the UI is shown, and will not show the UI on subsequent navigations if it is within 30 minutes of a previous impression. This is implemented using a new WebsiteSetting, which holds a timestamp. BUG=689992 ==========
raymes, msramek: Would you PTAL at this change now? For context: I spoke offline with raymes and we preferred implementing the smart UI with a custom website setting rather than hooking into the permission embargo set up (for now, at least). https://codereview.chromium.org/2795053002/diff/200001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/200001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:215: for (const auto& deleted_row : deleted_rows) { msramek: Is this the right approach? This will delete the metadata for an origin when the first URL from that origin is deleted when we really want to delete the metadata when the *last* URL is deleted. Is there an easy way to express that with this API? https://codereview.chromium.org/2795053002/diff/200001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2795053002/diff/200001/components/content_set... components/content_settings/core/common/content_settings_types.h:55: // Android. raymes: We could re-use this website setting to implement the blacklist cache we've talked about.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
csharrison@chromium.org changed reviewers: + isherman@chromium.org
+isherman could you PTAL at histograms.xml?
histograms.xml lgtm
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a few comments. Sorry again for the long wait :/ https://codereview.chromium.org/2795053002/diff/200001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/200001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:215: for (const auto& deleted_row : deleted_rows) { On 2017/04/26 16:33:23, Charlie Harrison wrote: > msramek: Is this the right approach? This will delete the metadata for an origin > when the first URL from that origin is deleted when we really want to delete the > metadata when the *last* URL is deleted. Is there an easy way to express that > with this API? Deleting metadata from origin with the last URL is really difficult, since for users who sync history, that could be years back. I guess we could do it just with local history (but you'd have to query HistoryService again to find out if any URL on that origin still exists). From privacy perspective, I think the aggressive deletion makes sense. If a user visited: https://embarrassingsite.com/page1 https://embarrassingsite.com/page2 and then deleted https://embarrassingsite.com/page1 it's a reasonable bet that they're trying to cover their tracks from embarassingsite.com and forgot about one visit, rather than they had a problem with that one particular visit. Btw, this is a common problem and I'm actually planning to write a doc about it soon; will send it around :) https://codereview.chromium.org/2795053002/diff/200001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2795053002/diff/200001/components/content_set... components/content_settings/core/common/content_settings_types.h:55: // Android. On 2017/04/26 16:33:23, Charlie Harrison wrote: > raymes: We could re-use this website setting to implement the blacklist cache > we've talked about. This wasn't addressed to me, I just want to +1 reusing the same setting. It's a dictionary, so you can just use different keys to access different metadata; this shouldn't add any complexity. What might add a bit of complexity is if the deletion criteria differ. https://codereview.chromium.org/2795053002/diff/240001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc (right): https://codereview.chromium.org/2795053002/diff/240001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:61: ContentSetting GetContentSettingMatchingUrlExactly(const GURL& url) { nit: The name is a bit misleading, because no pattern matches a web URL exactly[*]. [*] except file:// scheme settings which allow paths https://codereview.chromium.org/2795053002/diff/240001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:174: // Subsequent navigations to same-domains should not show UI. nit: Same domain or same origin? The website setting is scoped to the origin. https://codereview.chromium.org/2795053002/diff/240001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:272: history_service->AddPage(url, base::Time::Now(), history::SOURCE_BROWSED); Please add one more URL, otherwise this test cannot tell whether partial deletion worked or if we just always delete everything.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Many thanks for the review msramek. raymes: Since this is blocking a bunch of work I'm going to go ahead and land to the CQ now on msrameks LG (and your overall approval of the approach over email). Please TBR if you'd like. https://codereview.chromium.org/2795053002/diff/200001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2795053002/diff/200001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:215: for (const auto& deleted_row : deleted_rows) { On 2017/05/08 16:40:26, msramek (recovering) wrote: > On 2017/04/26 16:33:23, Charlie Harrison wrote: > > msramek: Is this the right approach? This will delete the metadata for an > origin > > when the first URL from that origin is deleted when we really want to delete > the > > metadata when the *last* URL is deleted. Is there an easy way to express that > > with this API? > > Deleting metadata from origin with the last URL is really difficult, since for > users who sync history, that could be years back. I guess we could do it just > with local history (but you'd have to query HistoryService again to find out if > any URL on that origin still exists). > > From privacy perspective, I think the aggressive deletion makes sense. If a user > visited: > > https://embarrassingsite.com/page1 > https://embarrassingsite.com/page2 > > and then deleted > > https://embarrassingsite.com/page1 > > it's a reasonable bet that they're trying to cover their tracks from > http://embarassingsite.com and forgot about one visit, rather than they had a problem > with that one particular visit. > > Btw, this is a common problem and I'm actually planning to write a doc about it > soon; will send it around :) Thanks for the explanation. For now I'll keep it as-is. Doing something with the local history might make sense later but I don't think it's necessary atm. Looking forward to the doc as well :) https://codereview.chromium.org/2795053002/diff/200001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2795053002/diff/200001/components/content_set... components/content_settings/core/common/content_settings_types.h:55: // Android. On 2017/05/08 16:40:26, msramek (recovering) wrote: > On 2017/04/26 16:33:23, Charlie Harrison wrote: > > raymes: We could re-use this website setting to implement the blacklist cache > > we've talked about. > > This wasn't addressed to me, I just want to +1 reusing the same setting. > > It's a dictionary, so you can just use different keys to access different > metadata; this shouldn't add any complexity. What might add a bit of complexity > is if the deletion criteria differ. I don't even think we need to add another key, we can just query for existence of this data. I have a WIP CL here implementing that: https://codereview.chromium.org/2859783002/ https://codereview.chromium.org/2795053002/diff/240001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc (right): https://codereview.chromium.org/2795053002/diff/240001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:61: ContentSetting GetContentSettingMatchingUrlExactly(const GURL& url) { On 2017/05/08 16:40:26, msramek (recovering) wrote: > nit: The name is a bit misleading, because no pattern matches a web URL > exactly[*]. > > [*] except file:// scheme settings which allow paths Renamed to GetContentSettingMatchingUrlWithEmptyPath https://codereview.chromium.org/2795053002/diff/240001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:174: // Subsequent navigations to same-domains should not show UI. On 2017/05/08 16:40:26, msramek (recovering) wrote: > nit: Same domain or same origin? The website setting is scoped to the origin. Same origin, sorry for lack of precision.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, jkarlin@chromium.org, isherman@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2795053002/#ps280001 (title: "minor tweaks")
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> raymes: Since this is blocking a bunch of work I'm going to go ahead and land to > the CQ now on msrameks LG (and your overall approval of the approach over > email). Please TBR if you'd like. Sorry for the delay - I wasn't following along closely. I had a high level look and nothing seems too problematic so I defer to msramek for the details. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1494291391361790,
"parent_rev": "39c4cda430ba484ae01e01a6158bb9ef65d1eef4", "commit_rev":
"3f8cdfd3ab1a3e87f90c37f3d7763483ac9e03c4"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Implement the "Smart" UI on Android This patch implements a V1 for the "smart" UI, which just logs a timestamp every time the UI is shown, and will not show the UI on subsequent navigations if it is within 30 minutes of a previous impression. This is implemented using a new WebsiteSetting, which holds a timestamp. BUG=689992 ========== to ========== [subresource_filter] Implement the "Smart" UI on Android This patch implements a V1 for the "smart" UI, which just logs a timestamp every time the UI is shown, and will not show the UI on subsequent navigations if it is within 30 minutes of a previous impression. This is implemented using a new WebsiteSetting, which holds a timestamp. BUG=689992 Review-Url: https://codereview.chromium.org/2795053002 Cr-Commit-Position: refs/heads/master@{#470217} Committed: https://chromium.googlesource.com/chromium/src/+/3f8cdfd3ab1a3e87f90c37f3d776... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/3f8cdfd3ab1a3e87f90c37f3d776... |
