Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1741123002:
Add removal filter support for Cookies, Storage, and Content Settings. (Closed)
|
Created:
4 years, 10 months ago by dmurph Modified:
4 years, 8 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, romax Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd removal filter support for Cookies, Storage, and Content Settings.
We modify BrowsingDataRemover to add more origin filtering support for:
* Cookies
* Storage Partition (idb, cookies, etc)
* Content Settings map values, specifically
* CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT
* CONTENT_SETTINGS_TYPE_APP_BANNER
* Offline pages
We remove |remove_url|, which was never used except for tests. This
also removes support for (to be added in the future):
* origin-specific history removal
* origin-specific extension removal
Here is the followup patch which would do the JNI hookup:
https://codereview.chromium.org/1757163002
BUG=589586, 578162
Committed: https://crrev.com/d193beb95152b418357056525d1c6b6a641d7b16
Cr-Commit-Position: refs/heads/master@{#386804}
Patch Set 1 #
Total comments: 48
Patch Set 2 : comments #
Total comments: 12
Patch Set 3 : Added & removed tests, fixes, comments #Patch Set 4 : Added & removed tests, fixes, comments #Patch Set 5 : Added Cookies #Patch Set 6 : rebase #
Total comments: 8
Patch Set 7 : build fix #Patch Set 8 : Added SafeBrowsing cookie support #
Total comments: 6
Patch Set 9 : Fixed Cookies, Tests, etc #
Total comments: 7
Patch Set 10 : Fixes to tests, comments #Patch Set 11 : IOS support, android compile fix #Patch Set 12 : ios fix, and fixed test #
Total comments: 34
Patch Set 13 : comments, tests, fixes, and Questions! #Patch Set 14 : Split cookie changes to separate patch #Patch Set 15 : moved android changes #Patch Set 16 : rebase #Patch Set 17 : Renamed class, comments #
Total comments: 27
Patch Set 18 : comments #Patch Set 19 : Added offline pages test #
Total comments: 6
Patch Set 20 : Comments #Patch Set 21 : ContentSettingsPattern test clarification #
Total comments: 2
Patch Set 22 : Fixed Android #Depends on Patchset: Messages
Total messages: 57 (13 generated)
Description was changed from ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER BUG=589586,578162 ========== to ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove remove_url, which was never used except for tests. We are also removing support for (to be added in the future): * origin-specific history removal. BUG=589586,578162 ==========
Description was changed from ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove remove_url, which was never used except for tests. We are also removing support for (to be added in the future): * origin-specific history removal. BUG=589586,578162 ========== to ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove |remove_url|, which was never used except for tests. This also removes support for (to be added in the future): * origin-specific history removal * origin-specific extension removal * origin-specific password removal BUG=589586,578162 ==========
dmurph@chromium.org changed reviewers: + brettw@chromium.org, mkwst@chromium.org, msramek@chromium.org, ttr314@chromium.org
Hello! I'm not totally done w/ this change (still need to filter safe browsing cookies and create BrowsingDataRemover tests), but can you guys take a quick look at these changes? Specifically the content_settings ones. Does this architecture look like it's on the right track? Thanks, Daniel
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/1741123002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1741123002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.h:197: void ClearSettingsForOneTypeWithPredicate( Hey Daniel, we chatted about potentially using GetSettingsForOneType to get the settings and then iterate through them and delete them. Would that be possible? I'd rather not add new APIs here if we can avoid it - the interface is already a bit of a mess. Thanks!
The changes to BDR look like they're on a reasonable track (though I'm interested in msramek@ and ttr314@'s input, as they've been more directly involved in creating the origin filters). I'm concerned about just papering over the existing GURL-based deletion mechanisms, however, as it seems likely that those codepaths will end up both unmaintained and unremoved unless there's a clear plan and responsibility for cleaning them up. Is there a plan? Who's on the hook for it? :) Also, as you noted: tests. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (left): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:724: password_store->RemoveLoginsByOriginAndTime( Ditto. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:174: return !url.is_valid() || predicate.Run(url); When can an invalid URL appear here? Why would an invalid URL be treated as matching? https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:408: WebHistoryServiceFactory::GetForProfile(profile_), std::set<GURL>(), If you're going to remove this mechanism in favor of the origin filter, I'd prefer not to leave vestiges of it throughout the codebase. What's the plan for replacing/updating this API? https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:419: std::set<GURL>()); Ditto. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:474: GURL(), delete_begin_, delete_end_); Ditto. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:621: // TODONOW ? https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:858: storage_partition_remove_mask, quota_storage_remove_mask, GURL(), It looks like you're passing in the filter below, which means you don't need this |remove_url| argument anymore. Why not remove it from the API entirely?
msramek@chromium.org changed reviewers: + ttr314@googlemail.com - ttr314@chromium.org
-ttr314@chromium.org +ttr314@googlemail.com Also, added my first round of comments :) https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:159: bool DoesOriginMatchMask( nit: DoesOriginMatchMaskAndUrls? https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:161: const base::Callback<bool(const GURL&)>& predicate, nit: Can you please generally refer to this as |url_filter|, like in my previous CL, for consistency? Unless I'm missing some nuance, filter and predicate are synonymous in this context. I would also emphasize that it's a filter for URLs, since 'predicate' or 'filter' in itself is very vague. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:174: return !url.is_valid() || predicate.Run(url); On 2016/02/29 09:27:38, Mike West wrote: > When can an invalid URL appear here? Why would an invalid URL be treated as > matching? Heh, this is quite tricky - if I have a content setting pattern such as "https://*", and I remove the origin "https://google.com", should we remove the pattern, because it matches the origin, or not, since it has nothing to do with the origin specifically? Fortunately, we're dealing here with APP_BANNER and SITE_ENGAGEMENT, which are website settings, and not content settings, so users cannot manually input complex primary patterns, they don't use secondary patterns, and the default setting [*,*] doesn't exist. SITE_ENGAGEMENT is always stored per origin, APP_BANNER per domain (plus subdomains, i.e. *://[*.]domain:*). So for SITE_ENGAGEMENT we can do exact origin matching, for APP_BANNER we should probably delete the entry when any origin with that domain as host is deleted. In both cases, ContentSettingsPattern::Matches is probably what you want to use. Furthermore, please document that this method only makes sense with the mentioned constraints (the primary pattern is always origin or host, and the secondary pattern is wildcard). https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:346: const OriginFilterBuilder& origin_filter) { This is actually a filter builder, not a filter. I would perhaps also argue that we should pass the filter itself, but that depends on where do we want to make the decision between exact origin vs subdomains, and I'm afraid we still haven't fleshed that out. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:492: if (origin_filter.IsEmptyBlacklist()) { optional: Remove or RemoveWithFilter could remember whether or not a filter was passed, then we can remove this method. It's a pity that we can't directly test the base::Callback itself for emptiness. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1023: OriginFilterBuilder builder(OriginFilterBuilder::BLACKLIST); The semantic of |remove_origin| is WHITELIST. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:272: host_content_settings_map->ClearSettingsForOneType( Is this related to this CL? https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:281: bool MatchPrimaryPatter(const ContentSettingsPattern& expected_primary, nit: Pattern https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:324: // First, test that we clear only IMAGES (not app banner), and pattern1. s/pattern1/pattern2/ also nit: APP_BANNER (just for consistency, if you capitalize IMAGES) https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:329: // |host_settings| contains default & block. Can you also test that what remains in the array is actually [*,*] and [pattern,*]? https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:337: // Next, test that we do correct pattern stuff w/ an origin policy item. Please improve the comment (pattern stuff?) https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:366: // Verify we only have one. Please also verify that we actually have the correct one. https://codereview.chromium.org/1741123002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1741123002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.h:197: void ClearSettingsForOneTypeWithPredicate( On 2016/02/29 00:44:59, raymes wrote: > Hey Daniel, we chatted about potentially using GetSettingsForOneType to get the > settings and then iterate through them and delete them. Would that be possible? > > I'd rather not add new APIs here if we can avoid it - the interface is already a > bit of a mess. > > Thanks! I would disagree. If the deletion is done in an iterative manner, and the only distinction is whether or not a filter is applied, then we can use the same method. However, ClearContentSettingsForOneType is implemented by nuking the preference, so this would decrease performance. Arguably, it's not super-important for content settings, since the amount of data stored tends to be low, but ClearAll() and ClearWithSomeConstraints(...) is going to be a pattern that will appear all over Chrome as we keep plumbing origin-based deletion to other backends.
Just a few comments from my side. Apart from that, I'd rather prefer to keep support for origin-specific removal of history, password, and extension data in BrowsingDataRemover. IIUC, the original idea of crbug.com/113621 was to extend all backends first and eventually expose the functionality via the browsingData extension API. Basically, I'm following the same pattern to implement crbug.com/78093. To me, that's a reasonable approach as it makes the final API exposure a matter of changing the frontend part while all backends will be ready. If it helps, I'd be happy to tackle those three types first and work on converting them to filter-based removal. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:169: bool PrimaryPatternGURLPredicate( I think the function name could be improved. Prefixing it by some descriptive verb would help. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:172: const ContentSettingsPattern& secondary_pattern) { |secondary_pattern| doesn't seem to be used in this function?
Thanks for the reviews everyone! Some notes: Still TODO: * implement the safe browsing cookies. * add/fix tests. Changes: * I moved the ContentSettings clear to a static method in BrowsingDataRemover * I implemented a new callback in OriginFilterBuilder to work better with ContentSettingsPatterns. Replies: * mkwst: It looks like ttr314, crbug.com/589586, and crbug.com/113621 are keeping track of this. I can split everything out into separate bugs so we can better assign everything if that makes things clearer. Note that nothing currently uses the GURL filter, it's just used in tests. * ttr314: The crbug.com/589586 bug outlines specifically what I need for an MVP of the 'Important Sites' feature, launch bug here: crbug.com/571461. I plan on helping out with the other backends. I just removed the support of the backends that weren't a straightforward convert so the patch wasn't unnecessarily big w/ lots of approval needed. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (left): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:724: password_store->RemoveLoginsByOriginAndTime( On 2016/02/29 at 09:27:38, Mike West wrote: > Ditto. See above comment. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:159: bool DoesOriginMatchMask( On 2016/02/29 at 17:46:07, msramek wrote: > nit: DoesOriginMatchMaskAndUrls? Done. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:161: const base::Callback<bool(const GURL&)>& predicate, On 2016/02/29 at 17:46:07, msramek wrote: > nit: Can you please generally refer to this as |url_filter|, like in my previous CL, for consistency? Unless I'm missing some nuance, filter and predicate are synonymous in this context. I would also emphasize that it's a filter for URLs, since 'predicate' or 'filter' in itself is very vague. Predicate implies that if you return true, then the action happens. Filter does not, and confused me a lot. Thus I'd rather use predicate. Let me know if you don't like that, it just means I would want to put comments every that say "true means remove". https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:169: bool PrimaryPatternGURLPredicate( On 2016/02/29 at 22:38:15, Timo Reimann wrote: > I think the function name could be improved. Prefixing it by some descriptive verb would help. Done. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:172: const ContentSettingsPattern& secondary_pattern) { On 2016/02/29 at 22:38:15, Timo Reimann wrote: > |secondary_pattern| doesn't seem to be used in this function? Nope. I need to convert from the two patterns to the predicate. See my comment below. I'll implement a new callback for this. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:174: return !url.is_valid() || predicate.Run(url); On 2016/02/29 at 17:46:07, msramek wrote: > On 2016/02/29 09:27:38, Mike West wrote: > > When can an invalid URL appear here? Why would an invalid URL be treated as > > matching? > > Heh, this is quite tricky - if I have a content setting pattern such as "https://*", and I remove the origin "https://google.com", should we remove the pattern, because it matches the origin, or not, since it has nothing to do with the origin specifically? > > Fortunately, we're dealing here with APP_BANNER and SITE_ENGAGEMENT, which are website settings, and not content settings, so users cannot manually input complex primary patterns, they don't use secondary patterns, and the default setting [*,*] doesn't exist. > > SITE_ENGAGEMENT is always stored per origin, APP_BANNER per domain (plus subdomains, i.e. *://[*.]domain:*). So for SITE_ENGAGEMENT we can do exact origin matching, for APP_BANNER we should probably delete the entry when any origin with that domain as host is deleted. > > In both cases, ContentSettingsPattern::Matches is probably what you want to use. > > Furthermore, please document that this method only makes sense with the mentioned constraints (the primary pattern is always origin or host, and the secondary pattern is wildcard). Unfortunately, I can't use |Matches|, as I'm not given a GURL. Instead, I need to pass the GURL to the predicate. Please advise on how to correctly do this. Maybe the Filter class should support bool(ContentSettingsPattern) callback generation? I did this, tell me what you think. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:346: const OriginFilterBuilder& origin_filter) { On 2016/02/29 at 17:46:07, msramek wrote: > This is actually a filter builder, not a filter. I would perhaps also argue that we should pass the filter itself, but that depends on where do we want to make the decision between exact origin vs subdomains, and I'm afraid we still haven't fleshed that out. I understand. For now I was just passing the builder, as we might need to generate different callbacks. (See comment above, where we might want to have the builder give us another callback that could accept ContentSettingsPatterns). https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:408: WebHistoryServiceFactory::GetForProfile(profile_), std::set<GURL>(), On 2016/02/29 at 09:27:38, Mike West wrote: > If you're going to remove this mechanism in favor of the origin filter, I'd prefer not to leave vestiges of it throughout the codebase. What's the plan for replacing/updating this API? See the bug. This API is NOT in use anywhere except for tests for this class. This initial patch adds support for all of the backends that I need to unblock the "Important Sites" CBD feature, and then I want to follow up when I'm unblocked w/ the rest of the backends. Is this alright? https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:419: std::set<GURL>()); On 2016/02/29 at 09:27:38, Mike West wrote: > Ditto. See above. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:474: GURL(), delete_begin_, delete_end_); On 2016/02/29 at 09:27:38, Mike West wrote: > Ditto. See above. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:621: // TODONOW On 2016/02/29 at 09:27:38, Mike West wrote: > ? Haha I need to impl this still. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:858: storage_partition_remove_mask, quota_storage_remove_mask, GURL(), On 2016/02/29 at 09:27:38, Mike West wrote: > It looks like you're passing in the filter below, which means you don't need this |remove_url| argument anymore. Why not remove it from the API entirely? I can do that, it hits a couple other areas in the code, so I created a bug for the followup CL: https://bugs.chromium.org/p/chromium/issues/detail?id=590891 I believe we're the last client of the 3rd argument, so this shouldn't be too bad, just removing that argument for a bunch of callsites. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1023: OriginFilterBuilder builder(OriginFilterBuilder::BLACKLIST); On 2016/02/29 at 17:46:07, msramek wrote: > The semantic of |remove_origin| is WHITELIST. Done. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:272: host_content_settings_map->ClearSettingsForOneType( On 2016/02/29 at 17:46:07, msramek wrote: > Is this related to this CL? No, I can remove. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:281: bool MatchPrimaryPatter(const ContentSettingsPattern& expected_primary, On 2016/02/29 at 17:46:07, msramek wrote: > nit: Pattern Done. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:324: // First, test that we clear only IMAGES (not app banner), and pattern1. On 2016/02/29 at 17:46:07, msramek wrote: > s/pattern1/pattern2/ > > also nit: APP_BANNER (just for consistency, if you capitalize IMAGES) Done. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:329: // |host_settings| contains default & block. On 2016/02/29 at 17:46:07, msramek wrote: > Can you also test that what remains in the array is actually [*,*] and [pattern,*]? Done. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:337: // Next, test that we do correct pattern stuff w/ an origin policy item. On 2016/02/29 at 17:46:07, msramek wrote: > Please improve the comment (pattern stuff?) Done. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:366: // Verify we only have one. On 2016/02/29 at 17:46:07, msramek wrote: > Please also verify that we actually have the correct one. Done. https://codereview.chromium.org/1741123002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1741123002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.h:197: void ClearSettingsForOneTypeWithPredicate( On 2016/02/29 at 17:46:07, msramek wrote: > On 2016/02/29 00:44:59, raymes wrote: > > Hey Daniel, we chatted about potentially using GetSettingsForOneType to get the > > settings and then iterate through them and delete them. Would that be possible? > > > > I'd rather not add new APIs here if we can avoid it - the interface is already a > > bit of a mess. > > > > Thanks! > > I would disagree. If the deletion is done in an iterative manner, and the only distinction is whether or not a filter is applied, then we can use the same method. However, ClearContentSettingsForOneType is implemented by nuking the preference, so this would decrease performance. > > Arguably, it's not super-important for content settings, since the amount of data stored tends to be low, but ClearAll() and ClearWithSomeConstraints(...) is going to be a pattern that will appear all over Chrome as we keep plumbing origin-based deletion to other backends. I moved the method to a static method in the BrowsingDataRemover (and moved test), as it doesn't really need any private data. Let me know what you guys prefer. It can be moved back.
content_settings lgtm with nit https://codereview.chromium.org/1741123002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1741123002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.h:197: void ClearSettingsForOneTypeWithPredicate( Great, thanks! This sounds much better :) https://codereview.chromium.org/1741123002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1741123002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:15: #include "base/callback_forward.h" nit: is this still needed?
content_settings lgtm with nit https://codereview.chromium.org/1741123002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1741123002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.h:197: void ClearSettingsForOneTypeWithPredicate( Great, thanks! This sounds much better :) https://codereview.chromium.org/1741123002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1741123002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:15: #include "base/callback_forward.h" nit: is this still needed?
Another round of comments. General direction still looks good. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:161: const base::Callback<bool(const GURL&)>& predicate, On 2016/03/01 00:10:00, dmurph wrote: > On 2016/02/29 at 17:46:07, msramek wrote: > > nit: Can you please generally refer to this as |url_filter|, like in my > previous CL, for consistency? Unless I'm missing some nuance, filter and > predicate are synonymous in this context. I would also emphasize that it's a > filter for URLs, since 'predicate' or 'filter' in itself is very vague. > > Predicate implies that if you return true, then the action happens. Filter does > not, and confused me a lot. Thus I'd rather use predicate. Let me know if you > don't like that, it just means I would want to put comments every that say "true > means remove". I don't particularly mind the word 'predicate', but the inconsistency. The class is named OriginFilterBuilder, so what it produces should be called a filter. Though I admit that filter could be interpreted as "true means it survives the deletion". We can keep 'predicate' in standalone methods like this that are not strongly related to OriginFilterBuilder, but I would still call it at least |url_predicate|. This reads to me as "int number;" or "std::vector<T> vector;". https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:346: const OriginFilterBuilder& origin_filter) { On 2016/03/01 00:10:00, dmurph wrote: > On 2016/02/29 at 17:46:07, msramek wrote: > > This is actually a filter builder, not a filter. I would perhaps also argue > that we should pass the filter itself, but that depends on where do we want to > make the decision between exact origin vs subdomains, and I'm afraid we still > haven't fleshed that out. > > I understand. For now I was just passing the builder, as we might need to > generate different callbacks. (See comment above, where we might want to have > the builder give us another callback that could accept ContentSettingsPatterns). Acknowledged. Yes, after adding ContentSettingPattern, we'll have to pass the builder. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:258: // TODO(dmurph): Support all backends with filter (crbug.com/589586). Please add a comment, best in capital letters, not to use this method unless the caller knows what they're doing. Calling this for datatypes that do not support origin-based deletion will delete everything instead of only a few items. That's a very destructive operation that we don't want to happen by accident. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:275: static void ClearSettingsForOneTypeWithPredicate( This probably doesn't belong to the public interface of BDR. Make it private, or even better, put it into a no-name namespace in .cc. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/origin_filter_builder.cc:63: OriginFilterBuilder::BuildSameOriginContentSettingsFilter() const { nit: indent (or unindent in the other methods if you feel strongly, but keep it consistent :) ) https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/origin_filter_builder.cc:89: bool OriginFilterBuilder::MatchesContentSettingsPattern( Please add a test for this method to OriginFilterBuilderTest. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/origin_filter_builder.cc:94: if ((mode == WHITELIST) && pattern.Matches(GURL(origin.Serialize()))) This method always returns false if mode == BLACKLIST.
On 2016/03/01 00:10:00, dmurph wrote: > * ttr314: The crbug.com/589586 bug outlines specifically > what I need for an MVP of the 'Important Sites' feature, > launch bug here: crbug.com/571461. I plan on helping out > with the other backends. I just removed the support of > the backends that weren't a straightforward convert so > the patch wasn't unnecessarily big w/ lots of approval > needed. I have some concerns about removing support for the existing backends: - Owners of the affected backends might start altering or removing the related origin functionality as they won't see it being in use anymore. - Related BrowsingDataRemoverTests would need to be removed or disabled only to be re-added/-enabled later again. - Bringing back any removed code may become more difficult the further the code keeps changing. Plus, we'd have to re-review functionality that had already been reviewed before, at least to a greater extent than necessary. Unless I'm missing something, sticking with the |remove_url|-based approach should actually keep the diff smaller compared to what the current CL suggests. OriginFilterBuilders don't seem to be used outside of tests yet either. If that changes before we have switched all backends over to supporting filters, we can extend BrowsingDataRemover's interface to offer a fitting Remove() overload.
ttr314: This is confusing to me, and I disagree. Replying inline. On Wed, Mar 2, 2016 at 1:42 PM <ttr314@googlemail.com> wrote: > On 2016/03/01 00:10:00, dmurph wrote: > > * ttr314: The crbug.com/589586 bug outlines specifically > > what I need for an MVP of the 'Important Sites' feature, > > launch bug here: crbug.com/571461. I plan on helping out > > with the other backends. I just removed the support of > > the backends that weren't a straightforward convert so > > the patch wasn't unnecessarily big w/ lots of approval > > needed. > > I have some concerns about removing support for the existing backends: > > - Owners of the affected backends might start altering or removing the > related > origin functionality as they won't see it being in use anymore. > I doubt people are this proactive, and I don't see this being a problem as we'll be working on adding support again. No one is going to do a large refactoring of how they store data now that one client is in the middle of changing how they filter. If they're doing this amount of sleuthing, then they will easily see the TODO comment. Also, if people remove single gurl-based deletion, that's perfectly fine with us, as we no longer are going to use that anyways. > - Related BrowsingDataRemoverTests would need to be removed or disabled > only to > be re-added/-enabled later again. > Well, why do we need to test stuff that isn't supported/used anyways? That's just wasted cycles. This is the definition of Dead Code. > - Bringing back any removed code may become more difficult the further the > code > keeps changing. Plus, we'd have to re-review functionality that had > already been > reviewed before, at least to a greater extent than necessary. > Well, yes, there will be a code review for the new backends. > > Unless I'm missing something, sticking with the |remove_url|-based approach > should actually keep the diff smaller compared to what the current CL > suggests. > OriginFilterBuilders don't seem to be used outside of tests yet either. If > that > changes before we have switched all backends over to supporting filters, > we can > extend BrowsingDataRemover's interface to offer a fitting Remove() > overload. > Please see the bugs I attached. We are creating a feature in chrome to remove important origins (multiple) from being removed by Clear Browsing Dialog. It is impossible to do this with a single GURL argument. This feature is not done yet, this change being a blocking issue. We will be changing this call <https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...>, and the accompanying java code, to use this feature. > > https://codereview.chromium.org/1741123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Responses inline. On 2016/03/02 21:52:38, dmurph wrote: > ttr314: > This is confusing to me, and I disagree. Replying inline. > > On Wed, Mar 2, 2016 at 1:42 PM <mailto:ttr314@googlemail.com> wrote: > > > On 2016/03/01 00:10:00, dmurph wrote: > > > * ttr314: The crbug.com/589586 bug outlines specifically > > > what I need for an MVP of the 'Important Sites' feature, > > > launch bug here: crbug.com/571461. I plan on helping out > > > with the other backends. I just removed the support of > > > the backends that weren't a straightforward convert so > > > the patch wasn't unnecessarily big w/ lots of approval > > > needed. > > > > I have some concerns about removing support for the existing backends: > > > > - Owners of the affected backends might start altering or removing the > > related > > origin functionality as they won't see it being in use anymore. > > > > I doubt people are this proactive, and I don't see this being a problem as > we'll be working on adding support again. No one is going to do a large > refactoring of how they store data now that one client is in the middle of > changing how they filter. If they're doing this amount of sleuthing, then > they will easily see the TODO comment. You're probably in a better position than me to judge on what the chances are for people changing code around the area in question, so I'm happy to follow your assessment here. > Also, if people remove single gurl-based deletion, that's perfectly fine > with us, as we no longer are going to use that anyways. Existing GURL-based deletion may lay the groundwork for moving to a filter-based one more quickly. To give an example, I initially modified all password stores to take a single GURL and I'm currently landing a CL to switch to filters. That second CL proved to be very easy as the major effort has already been made. Making sure we keep what's already there could thus be beneficial. > > - Related BrowsingDataRemoverTests would need to be removed or disabled > > only to > > be re-added/-enabled later again. > > > Well, why do we need to test stuff that isn't supported/used anyways? > That's just wasted cycles. This is the definition of Dead Code. I generally agree but not in this very case. To quote myself from my very first comment: "IIUC, the original idea of crbug.com/113621 was to extend all backends first and eventually expose the functionality via the browsingData extension API. Basically, I'm following the same pattern to implement crbug.com/78093. To me, that's a reasonable approach as it makes the final API exposure a matter of changing the frontend part while all backends will be ready." From my perspective, the test-only approach can be considered as a form of branch-by-abstraction to prepare for a bigger switch. If there's an alternative way that doesn't involve either working on big CLs turning on many diverse things at once, or exposing an inconsistent looking extension API, I'd be happy to know about. > > Unless I'm missing something, sticking with the |remove_url|-based approach > > should actually keep the diff smaller compared to what the current CL > > suggests. > > OriginFilterBuilders don't seem to be used outside of tests yet either. If > > that > > changes before we have switched all backends over to supporting filters, > > we can > > extend BrowsingDataRemover's interface to offer a fitting Remove() > > overload. > > > > Please see the bugs I attached. We are creating a feature in chrome to > remove important origins (multiple) from being removed by Clear Browsing > Dialog. It is impossible to do this with a single GURL argument. This > feature is not done yet, this change being a blocking issue. We will be > changing this call > <https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...>, > and the accompanying java code, to use this feature. Thanks for the details. I understand the need to switch to a filter-based approach; in fact, it's exactly what I need for crbug.com/78093 as well, so I highly welcome the change. My remark was meant to address your effort to keeping the patch small and minimizing approval. I'm fine with the way things are in this regard.
Description was changed from ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove |remove_url|, which was never used except for tests. This also removes support for (to be added in the future): * origin-specific history removal * origin-specific extension removal * origin-specific password removal BUG=589586,578162 ========== to ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove |remove_url|, which was never used except for tests. This also removes support for (to be added in the future): * origin-specific history removal * origin-specific extension removal * origin-specific password removal Here is the followup patch which would do the JNI hookup: https://codereview.chromium.org/1757163002 BUG=589586,578162 ==========
dmurph@chromium.org changed reviewers: + michaeln@chromium.org
Still to do: * Safe browsing cookies * Testing the cookies filtering for browser_data_remover Thanks for all the feedback everyone! The cookie filtering idea w/ additions to the storage partition impl were a result of a discussion between Michael and I. I added him as a reviewer for those changes. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:258: // TODO(dmurph): Support all backends with filter (crbug.com/589586). On 2016/03/01 at 17:24:09, msramek wrote: > Please add a comment, best in capital letters, not to use this method unless the caller knows what they're doing. > > Calling this for datatypes that do not support origin-based deletion will delete everything instead of only a few items. That's a very destructive operation that we don't want to happen by accident. Done. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:275: static void ClearSettingsForOneTypeWithPredicate( On 2016/03/01 at 17:24:09, msramek wrote: > This probably doesn't belong to the public interface of BDR. Make it private, or even better, put it into a no-name namespace in .cc. I'll make it private, as I want to be able to test it thoroughly. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/origin_filter_builder.cc:63: OriginFilterBuilder::BuildSameOriginContentSettingsFilter() const { On 2016/03/01 at 17:24:09, msramek wrote: > nit: indent > > (or unindent in the other methods if you feel strongly, but keep it consistent :) ) Done. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/origin_filter_builder.cc:89: bool OriginFilterBuilder::MatchesContentSettingsPattern( On 2016/03/01 at 17:24:09, msramek wrote: > Please add a test for this method to OriginFilterBuilderTest. Done. https://codereview.chromium.org/1741123002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/origin_filter_builder.cc:94: if ((mode == WHITELIST) && pattern.Matches(GURL(origin.Serialize()))) On 2016/03/01 at 17:24:09, msramek wrote: > This method always returns false if mode == BLACKLIST. Ah, fixed. https://codereview.chromium.org/1741123002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1741123002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:15: #include "base/callback_forward.h" On 2016/03/01 at 00:14:50, raymes wrote: > nit: is this still needed? Fixed.
Description was changed from ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove |remove_url|, which was never used except for tests. This also removes support for (to be added in the future): * origin-specific history removal * origin-specific extension removal * origin-specific password removal Here is the followup patch which would do the JNI hookup: https://codereview.chromium.org/1757163002 BUG=589586,578162 ========== to ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove |remove_url|, which was never used except for tests. This also removes support for (to be added in the future): * origin-specific history removal * origin-specific extension removal Here is the followup patch which would do the JNI hookup: https://codereview.chromium.org/1757163002 BUG=589586,578162 ==========
dmurph@chromium.org changed reviewers: + bsittler@chromium.org
Alrighty! I finished safe browsing cookie support, and added tests. REGARDING COOKIES: In order to not break things, the cookie filter is a domain-based cookie filter. Mike & Ben: Please take a look at the logic in 'MatchesCookieWithAllSchemes' in OriginFilterBuilder. I believe this will not break the web, as for whitelists I keep any cookie that that origin can see. Let me know what you think! I think this is complete.
I've been informed that my cookie handling would break the web. I need to do eTLD + 1 domain matching. So plus.google.com wouldn't clear accounts.google.com. We would have the UI situation now where play.google.com, plus.google.com, and www.google.com all are the same thing.
On 2016/03/09 at 01:26:00, dmurph wrote: > I've been informed that my cookie handling would break the web. I need to do eTLD + 1 domain matching. So plus.google.com wouldn't clear accounts.google.com. We would have the UI situation now where play.google.com, plus.google.com, and www.google.com all are the same thing. I concur. If there were no cookies set at the .google.com level, clearing an individual xyz.google.com seems reasonable. On the other hand, a signed-in session with cookies at .google.com can't be assumed to be divisible without breakage (e.g. clearing the LSID cookie on accounts.google.com causes all sorts of badness on other xyz.google.com sites because they all share a google.com SID.)
storagepartition impl changes generally lgtm, a couple small comments to consider https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:161: const base::Callback<bool(const GURL&)>& predicate, On 2016/03/01 17:24:09, msramek wrote: > On 2016/03/01 00:10:00, dmurph wrote: > > On 2016/02/29 at 17:46:07, msramek wrote: > > > nit: Can you please generally refer to this as |url_filter|, like in my > > previous CL, for consistency? Unless I'm missing some nuance, filter and > > predicate are synonymous in this context. I would also emphasize that it's a > > filter for URLs, since 'predicate' or 'filter' in itself is very vague. > > > > Predicate implies that if you return true, then the action happens. Filter > does > > not, and confused me a lot. Thus I'd rather use predicate. Let me know if you > > don't like that, it just means I would want to put comments every that say > "true > > means remove". > > I don't particularly mind the word 'predicate', but the inconsistency. The class > is named OriginFilterBuilder, so what it produces should be called a filter. > Though I admit that filter could be interpreted as "true means it survives the > deletion". > > We can keep 'predicate' in standalone methods like this that are not strongly > related to OriginFilterBuilder, but I would still call it at least > |url_predicate|. This reads to me as "int number;" or "std::vector<T> vector;". naming nit: How about origin_matcher_fn? Also if this truly is about origin rather than url matching, should we consider using the url::Origin datatype here? https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... content/browser/storage_partition_impl.cc:54: void ClearCookiesOnIOThread( you could sort out which of the three very similar branches to go down in this method and nix the 2nd method https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... content/browser/storage_partition_impl.cc:85: if (cookie_matcher.is_null()) { i think this block is unreachable at the moment https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... content/browser/storage_partition_impl.cc:806: base::Bind(&ClearCookiesOnIOThread, make_scoped_refptr(rq_context), might be clearer to plumb all inputs to a single ClearCookiesOnIOThread method on iothread and put all the cookieistic branching logic in the same place... less code too https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... File content/browser/storage_partition_impl_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... content/browser/storage_partition_impl_unittest.cc:313: const content::StoragePartition::CookieMatcherFunction& cookie_matcher, content:: not needed? https://codereview.chromium.org/1741123002/diff/140001/content/public/browser... File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/1741123002/diff/140001/content/public/browser... content/public/browser/storage_partition.h:132: using CookieMatcherFunction = net::CookieStore::ClearCookiesPredicate; CookieMatcherFunction is a clearer and more broadly applicable name than ClearCookiesPredicate, maybe that should be the name used in the net lib too? https://codereview.chromium.org/1741123002/diff/140001/content/public/browser... content/public/browser/storage_partition.h:143: const OriginMatcherFunction& origin_matcher, Not new to this cl, this one is a little confusing, a storage_origin and origin_matcher as inputs? What if an storage_origin is not empty and it fails the matching function, what happens? Might be nice to clarify that in the doc comments. https://codereview.chromium.org/1741123002/diff/140001/content/public/browser... content/public/browser/storage_partition.h:161: const CookieMatcherFunction& cookie_matcher, Hooray for punting to the higher level logic to sort thru the tea leaves of user input indicating what to delete or keep!
I left a comment and a question. Apart from that, and when you fix the compilation error, browsing data and origin filter parts LGTM. However, I think the core of this CL is cookies and storage_partition, which I would leave for someone more familiar to review. https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:161: const base::Callback<bool(const GURL&)>& predicate, On 2016/03/09 20:25:51, michaeln wrote: > On 2016/03/01 17:24:09, msramek wrote: > > On 2016/03/01 00:10:00, dmurph wrote: > > > On 2016/02/29 at 17:46:07, msramek wrote: > > > > nit: Can you please generally refer to this as |url_filter|, like in my > > > previous CL, for consistency? Unless I'm missing some nuance, filter and > > > predicate are synonymous in this context. I would also emphasize that it's a > > > filter for URLs, since 'predicate' or 'filter' in itself is very vague. > > > > > > Predicate implies that if you return true, then the action happens. Filter > > does > > > not, and confused me a lot. Thus I'd rather use predicate. Let me know if > you > > > don't like that, it just means I would want to put comments every that say > > "true > > > means remove". > > > > I don't particularly mind the word 'predicate', but the inconsistency. The > class > > is named OriginFilterBuilder, so what it produces should be called a filter. > > Though I admit that filter could be interpreted as "true means it survives the > > deletion". > > > > We can keep 'predicate' in standalone methods like this that are not strongly > > related to OriginFilterBuilder, but I would still call it at least > > |url_predicate|. This reads to me as "int number;" or "std::vector<T> > vector;". > > naming nit: How about origin_matcher_fn? Also if this truly is about origin > rather than url matching, should we consider using the url::Origin datatype > here? Origin matching is also not exact, since sometimes it's origin+subdomains. The idea is to use a URL->boolean predicate, so that each backend can simply ask "should I delete this URL?" and doesn't have to know about url::Origin. The OriginFilterBuilder then supplies special kinds of URL->boolean predicates that just happen to have an equivalence relation over URLs of the same origin, or origin+subdomains. Origin->boolean is still just a special case of URL->boolean. I understand that in storage partition, it's going to be messy, since items are stored per origin there, so it doesn't make sense to delete google.com/a and not google.com/b. But for example in history or download manager, we could easily delete URLs matching an arbitrary predicate. https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:64: OriginFilterBuilder::BuildSameOriginContentSettingsFilter() const { Consider renaming to BuildWebsiteSettingsFilter(). Two reasons: 1. The matching semantics here are not really about same origin, but about ContentSettingsPattern::Matches(). The pattern can be much broader than just origin (such as [*.]google.com or *:80). 2. s/ContentSettings/WebsiteSettings/ The state of the code here is a bit unfortunate. The component is called content_settings. The class is called ContentSettingsPattern. However, the nomenclature has recently changed. We call the settings "website settings", and "content settings" are a special subclass that only support ContentSetting as a value. This is well illustrated in the classes WebsiteSettingsRegistry and ContentSettingsRegistry. Most importantly - we use this method on APP_BANNER and SITE_ENGAGEMENT, which are website settings, but NOT content settings. In fact, we really don't want to use this on actual content settings. If I, as a user, set up a setting for pattern "https://*" and then delete the origin "https://google.com", I would be really surprised if that broad setting was deleted. But no such surprises should happen for APP_BANNER and SITE_ENGAGEMENT, since they are not manually settable by the user, and thus (to my knowledge) only use origins as their patterns. But I really appreciate that you did test various possible patterns in this method's unittest. https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:122: net::registry_controlled_domains::GetDomainAndRegistry( I'm not really familiar with cookies, so I'll trust bsittler@'s judgement. However, as a sanity check. My understanding is that some cookies, those where Set-Cookie header was empty, are only scoped to the particular domain, and not shared with superdomains. Do we still want to do eTLD+1 comparison for those?
https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... content/browser/storage_partition_impl.cc:54: void ClearCookiesOnIOThread( On 2016/03/09 at 20:25:51, michaeln wrote: > you could sort out which of the three very similar branches to go down in this method and nix the 2nd method SGTM https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... content/browser/storage_partition_impl.cc:85: if (cookie_matcher.is_null()) { On 2016/03/09 at 20:25:51, michaeln wrote: > i think this block is unreachable at the moment Removed. https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... content/browser/storage_partition_impl.cc:806: base::Bind(&ClearCookiesOnIOThread, make_scoped_refptr(rq_context), On 2016/03/09 at 20:25:51, michaeln wrote: > might be clearer to plumb all inputs to a single ClearCookiesOnIOThread method on iothread and put all the cookieistic branching logic in the same place... less code too Done. https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... File content/browser/storage_partition_impl_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/100001/content/browser/storag... content/browser/storage_partition_impl_unittest.cc:313: const content::StoragePartition::CookieMatcherFunction& cookie_matcher, On 2016/03/09 at 20:25:51, michaeln wrote: > content:: not needed? Done. https://codereview.chromium.org/1741123002/diff/140001/content/public/browser... File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/1741123002/diff/140001/content/public/browser... content/public/browser/storage_partition.h:132: using CookieMatcherFunction = net::CookieStore::ClearCookiesPredicate; On 2016/03/09 at 20:25:51, michaeln wrote: > CookieMatcherFunction is a clearer and more broadly applicable name than ClearCookiesPredicate, maybe that should be the name used in the net lib too? mmmmmm I like the word Predicate because that's exactly what this function is (true/false from a value). I'll change the name of the function so it doesn't have "Clear", but I think I'd like to keep the function names the same, unless other people have more objections. I used CookieMatcherFunction here to match the OriginMatcherFunction words. https://codereview.chromium.org/1741123002/diff/140001/content/public/browser... content/public/browser/storage_partition.h:143: const OriginMatcherFunction& origin_matcher, On 2016/03/09 at 20:25:51, michaeln wrote: > Not new to this cl, this one is a little confusing, a storage_origin and origin_matcher as inputs? What if an storage_origin is not empty and it fails the matching function, what happens? Might be nice to clarify that in the doc comments. They are used by different subsystems. I clarified a bit. https://codereview.chromium.org/1741123002/diff/140001/content/public/browser... content/public/browser/storage_partition.h:161: const CookieMatcherFunction& cookie_matcher, On 2016/03/09 at 20:25:51, michaeln wrote: > Hooray for punting to the higher level logic to sort thru the tea leaves of user input indicating what to delete or keep! :) https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:64: OriginFilterBuilder::BuildSameOriginContentSettingsFilter() const { On 2016/03/10 at 20:10:08, msramek wrote: > Consider renaming to BuildWebsiteSettingsFilter(). > > Two reasons: > 1. The matching semantics here are not really about same origin, but about ContentSettingsPattern::Matches(). The pattern can be much broader than just origin (such as [*.]google.com or *:80). > > 2. s/ContentSettings/WebsiteSettings/ > The state of the code here is a bit unfortunate. The component is called content_settings. The class is called ContentSettingsPattern. However, the nomenclature has recently changed. We call the settings "website settings", and "content settings" are a special subclass that only support ContentSetting as a value. This is well illustrated in the classes WebsiteSettingsRegistry and ContentSettingsRegistry. > > Most importantly - we use this method on APP_BANNER and SITE_ENGAGEMENT, which are website settings, but NOT content settings. In fact, we really don't want to use this on actual content settings. If I, as a user, set up a setting for pattern "https://*" and then delete the origin "https://google.com", I would be really surprised if that broad setting was deleted. But no such surprises should happen for APP_BANNER and SITE_ENGAGEMENT, since they are not manually settable by the user, and thus (to my knowledge) only use origins as their patterns. But I really appreciate that you did test various possible patterns in this method's unittest. Hmm.... sounds good, should I add more documentation around this filter to make sure this isn't misused? What do you recommend? https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:122: net::registry_controlled_domains::GetDomainAndRegistry( On 2016/03/10 at 20:10:08, msramek wrote: > I'm not really familiar with cookies, so I'll trust bsittler@'s judgement. > > However, as a sanity check. My understanding is that some cookies, those where Set-Cookie header was empty, are only scoped to the particular domain, and not shared with superdomains. Do we still want to do eTLD+1 comparison for those? We basically break websites unless we treat this as eTLD+1 (we can't clear a subdomain and leave the domain still there, and vice versa). I would also like Mike West's feedback here.
The decision to remove any cookie for a registrable domain makes sense, though I'm a bit worried that it will still lead to breakage if we leave a site's data intact while removing its cookies. That is, if `localStorage` says I'm logged in, but I don't have any cookies, sites will likely get confused. That said, this seems like a model worth experimenting with to figure out the right balance. I'm a little concerned about this implementation, as it leaves a large parallel path for deleting cookies for a host. It's not clear to me that we need that anymore, and I'd greatly prefer to reduce duplication by removing it if possible. https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:120: } Nit: No {} for single-line clauses. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:109: bool OriginFilterBuilder::MatchesCookieForTLDPlusOne( Nit: Would you mind using something like "MatchesCookiesRegistrableDomain" as opposed to "eTLD+1"? The former seems more likely to be clear. If you prefer the latter, the "e" is important, as `appspot.com`'s TLD is "com", but it's _effective_ TLD is "appspot.com". :) https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:120: } Nit: No {} around single-line clauses. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:124: net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES); You'll need to check for IP addresses here as well, right? You can probably simplify this by checking for the cookie domain being an IP address, and skipping all the registry controlled calls entirely by just doing a straight comparison of the cookie's domain to the origin's host. Then you can also skip the IP address bits in the for loop, because you know they won't match. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.h (right): https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.h:26: // This means that the origins given will NOT be deleted. Nit: "This means that everything EXCEPT the origins given will be deleted." right? I think that's important information that's somewhat obscured with the current text. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.h:41: // Returns if we're an empty blacklist, where we delete everything. Nit: Returns true https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.h:64: // * 192.168.1.1 I'd rephrase this a bit for clarity, perhaps something like: """ Builds a filter that compares the registrable domain of the filter's origins and the registrable domain of the cookie's domain. A whitelist filter will return true if any of its origins match the cookie. A blacklist filter will return true only if none of its origins match the cookie. For example, a whitelist which contains the origin "https://site.example.com/" will return true for a cookie whose domain is "site.example.com", but also for cookies whose domains are "example.com", ".example.com", "subdomain.example.com" and so on. See net/base/registry_controlled_domains/registry_controlled_domain.h for more details on registrable domains and the current list of effective TLDs. """ https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:52: std::string cookie_line = "A=2"; It would be helpful to verify that cookie attributes like `HttpOnly`, `Secure`, etc. do not have an effect on the filter. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:65: << " should not match."; This pattern could be simplified to `EXPECT_EQ(test_case.should_match, filter.Run(*cookie))`, here and elsewhere. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:257: builder.AddOrigin(Origin(GURL("https://www.google.com"))); I'd like to see an origin with multiple subdomains in the filter and multiple subdomains in the tests. For instance: sub.sub.google.com -> google.com, sub.google.com, sub.sub.google.com, www.google.com, sub.www.google.com https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:260: // misawa.aomori.jp is a TLD and possibly not. and possibly not what? https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:287: {"https://www.sp.nom.br", false}, This would be clearer if it was something like "site1.sp.nom.br" in the filter, and "site2.sp.nom.br" in the test. Ditto for "misawa.aomori.jp". https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... File net/cookies/cookie_monster_unittest.cc (left): https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:2213: Nit: Why remove this newline? https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:1304: // Create 3 cookies with creation date of today, yesterday and the day before. You're creating 5 cookies. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:1338: // False means 'less than or equal', so we test both ways for full equal. Nit: Please move this comment up with the matcher. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... File net/cookies/cookie_store.cc (right): https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... net/cookies/cookie_store.cc:7: #include "base/callback.h" Do we need this? We're just passing the callback through here, aren't we? https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... File net/cookies/cookie_store.h (right): https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... net/cookies/cookie_store.h:165: virtual void DeleteAllCreatedBetweenForHostAsync( Do we still need this if we have the method you're adding? I'd prefer not to have two mechanisms for doing the same thing, as there's so much boilerplate involved. In fact, it might make sense to split the cookie bits off from the rest of this CL, first replacing this method with the predicated-based deletion mechanism, then expanding the usage. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... net/cookies/cookie_store.h:175: // Returns the number of cookies deleted. It calls |callback| with the number of cookies deleted, right? Would you mind changing this in the two previous comments as well?
https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:64: OriginFilterBuilder::BuildSameOriginContentSettingsFilter() const { On 2016/03/10 23:00:11, dmurph wrote: > On 2016/03/10 at 20:10:08, msramek wrote: > > Consider renaming to BuildWebsiteSettingsFilter(). > > > > Two reasons: > > 1. The matching semantics here are not really about same origin, but about > ContentSettingsPattern::Matches(). The pattern can be much broader than just > origin (such as [*.]google.com or *:80). > > > > 2. s/ContentSettings/WebsiteSettings/ > > The state of the code here is a bit unfortunate. The component is called > content_settings. The class is called ContentSettingsPattern. However, the > nomenclature has recently changed. We call the settings "website settings", and > "content settings" are a special subclass that only support ContentSetting as a > value. This is well illustrated in the classes WebsiteSettingsRegistry and > ContentSettingsRegistry. > > > > Most importantly - we use this method on APP_BANNER and SITE_ENGAGEMENT, which > are website settings, but NOT content settings. In fact, we really don't want > to use this on actual content settings. If I, as a user, set up a setting for > pattern "https://*" and then delete the origin "https://google.com", I would be > really surprised if that broad setting was deleted. But no such surprises should > happen for APP_BANNER and SITE_ENGAGEMENT, since they are not manually settable > by the user, and thus (to my knowledge) only use origins as their patterns. But > I really appreciate that you did test various possible patterns in this method's > unittest. > > Hmm.... sounds good, should I add more documentation around this filter to make > sure this isn't misused? What do you recommend? We're currently only dealing with website settings (and not content settings) in BrowsingDataRemover, so it isn't that likely to be misused. If the method is called BuildWebsiteSettingsFilter and its parameter is ContentSettingsPattern, it will hopefully be clear that it matches according to pattern. But you might want to add a comment to the tests, noting that we're testing broader-scoped patterns just for the sake of completeness, and we don't expect website settings to actually use them.
Ping?
Sorry, I'm ooo. I added domain support to the origin filter (which should probably be renamed). It was taking a decent number of changes so I wasn't able to finish On Thu, Mar 17, 2016, 11:22 AM <mkwst@chromium.org> wrote: > Ping? > > https://codereview.chromium.org/1741123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hello all! I'm back from vacation and I have a few points regarding my new changes: 1. Mike pointed out it might be better to move the cookie changes to a new CL so I'll be doing this soon. 2. I added the ability to filter a registerable domain, which is how the 'important sites' feature will be working. 3. The OriginFilterBuilder name is now a little wrong, as it now allows you to add 'registerable domains' as well as origins. I'm thinking of renaming it to 'BrowsingDataRemovalFilter' so it's a little more specific. Any other ideas? Thanks again everyone, and sorry about the 2 week delay. Daniel https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:109: bool OriginFilterBuilder::MatchesCookieForTLDPlusOne( On 2016/03/11 at 08:36:10, Mike West (OOO through 31st) wrote: > Nit: Would you mind using something like "MatchesCookiesRegistrableDomain" as opposed to "eTLD+1"? The former seems more likely to be clear. If you prefer the latter, the "e" is important, as `appspot.com`'s TLD is "com", but it's _effective_ TLD is "appspot.com". :) sgtm https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:120: } On 2016/03/11 at 08:36:10, Mike West (OOO through 31st) wrote: > Nit: No {} around single-line clauses. Done. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:124: net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES); On 2016/03/11 at 08:36:10, Mike West (OOO through 31st) wrote: > You'll need to check for IP addresses here as well, right? You can probably simplify this by checking for the cookie domain being an IP address, and skipping all the registry controlled calls entirely by just doing a straight comparison of the cookie's domain to the origin's host. > > Then you can also skip the IP address bits in the for loop, because you know they won't match. Did this in a slightly different way, but it now works. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.h (right): https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.h:26: // This means that the origins given will NOT be deleted. On 2016/03/11 at 08:36:10, Mike West (OOO through 31st) wrote: > Nit: "This means that everything EXCEPT the origins given will be deleted." right? I think that's important information that's somewhat obscured with the current text. Done. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.h:41: // Returns if we're an empty blacklist, where we delete everything. On 2016/03/11 at 08:36:10, Mike West wrote: > Nit: Returns true Done. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.h:64: // * 192.168.1.1 On 2016/03/11 at 08:36:10, Mike West (OOO through 31st) wrote: > I'd rephrase this a bit for clarity, perhaps something like: > > """ > Builds a filter that compares the registrable domain of the filter's origins > and the registrable domain of the cookie's domain. A whitelist filter will return > true if any of its origins match the cookie. A blacklist filter will return true > only if none of its origins match the cookie. > > For example, a whitelist which contains the origin "https://site.example.com/" > will return true for a cookie whose domain is "site.example.com", but also for > cookies whose domains are "example.com", ".example.com", "subdomain.example.com" > and so on. > > See net/base/registry_controlled_domains/registry_controlled_domain.h for more > details on registrable domains and the current list of effective TLDs. > """ Done, thanks. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... File net/cookies/cookie_monster_unittest.cc (left): https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:2213: On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > Nit: Why remove this newline? Fixed. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:1304: // Create 3 cookies with creation date of today, yesterday and the day before. On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > You're creating 5 cookies. Done. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:1338: // False means 'less than or equal', so we test both ways for full equal. On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > Nit: Please move this comment up with the matcher. Done. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... File net/cookies/cookie_store.cc (right): https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... net/cookies/cookie_store.cc:7: #include "base/callback.h" On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > Do we need this? We're just passing the callback through here, aren't we? Done. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... File net/cookies/cookie_store.h (right): https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... net/cookies/cookie_store.h:165: virtual void DeleteAllCreatedBetweenForHostAsync( On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > Do we still need this if we have the method you're adding? I'd prefer not to have two mechanisms for doing the same thing, as there's so much boilerplate involved. > > In fact, it might make sense to split the cookie bits off from the rest of this CL, first replacing this method with the predicated-based deletion mechanism, then expanding the usage. Hmmm... yeah I could do this. I'll put my new patch up w/ these changes for now, but I can copy them into a new CL. https://codereview.chromium.org/1741123002/diff/210001/net/cookies/cookie_sto... net/cookies/cookie_store.h:175: // Returns the number of cookies deleted. On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > It calls |callback| with the number of cookies deleted, right? Would you mind changing this in the two previous comments as well? Done.
Patch split is done. The cookie changes are here now: https://codereview.chromium.org/1844243002
Here is a rebased and working patch w/ the file name change, with comments addressed below. Mike: Can you verify the craziness with the misawa.aomori.jp tests? Ben and I investigated how some (maybe not anymore) TLDs also are their own domains, and I wanted to capture this in the tests below. Does that look sane? Thanks, Daniel https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder.cc (right): https://codereview.chromium.org/1741123002/diff/150001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder.cc:120: } On 2016/03/11 at 08:36:10, Mike West (OOO through 31st) wrote: > Nit: No {} for single-line clauses. Done. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... File chrome/browser/browsing_data/origin_filter_builder_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:52: std::string cookie_line = "A=2"; On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > It would be helpful to verify that cookie attributes like `HttpOnly`, `Secure`, etc. do not have an effect on the filter. Done. QUESTION::: Does this mean that 'Secure' cookies should NOT be cleared for an http origin? https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:65: << " should not match."; On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > This pattern could be simplified to `EXPECT_EQ(test_case.should_match, filter.Run(*cookie))`, here and elsewhere. Done. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:257: builder.AddOrigin(Origin(GURL("https://www.google.com"))); On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > I'd like to see an origin with multiple subdomains in the filter and multiple subdomains in the tests. For instance: > > sub.sub.google.com -> google.com, sub.google.com, sub.sub.google.com, www.google.com, sub.www.google.com Done. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:260: // misawa.aomori.jp is a TLD and possibly not. On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > and possibly not what? Clarified. https://codereview.chromium.org/1741123002/diff/210001/chrome/browser/browsin... chrome/browser/browsing_data/origin_filter_builder_unittest.cc:287: {"https://www.sp.nom.br", false}, On 2016/03/11 at 08:36:11, Mike West (OOO through 31st) wrote: > This would be clearer if it was something like "site1.sp.nom.br" in the filter, and "site2.sp.nom.br" in the test. Ditto for "misawa.aomori.jp". Done.
dmurph@chromium.org changed reviewers: + fgorski@chromium.org, kinuko@chromium.org
+kinuko for storage_partition_impl +fgorski for offline_pages
storage_partition related code lgtm https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.h:263: // BACKENDS ARE SUPPORTED YET, AND MORE DATA THAN EXPECTED COULD BE DELETED. Phew... https://codereview.chromium.org/1741123002/diff/310001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1741123002/diff/310001/content/browser/storag... content/browser/storage_partition_impl.cc:60: // Cookie matcher and storage_origin are never both populated. Maybe have a DCHECK for this? https://codereview.chromium.org/1741123002/diff/310001/content/browser/storag... content/browser/storage_partition_impl.cc:73: begin, end, cookie_matcher, base::Bind(&OnClearedCookies, callback)); nit: if - if else - else could be revised with early return + if's https://codereview.chromium.org/1741123002/diff/310001/content/browser/storag... content/browser/storage_partition_impl.cc:268: ? remove this line
Regarding the renaming - I would still argue that the class is a *FilterBuilder, and *Filter is what it produces. It is perhaps not a big deal, especially because BrowsingDataRemover::RemoveWithFilter actually takes this filter builder as a parameter, but the class still uses the builder pattern, so the name should match that. So - BrowsingDataFuilterBuilder? (we can omit the "Remover" part, it can potentially filter data for other reasons). I left a few nits, and I have mostly one major comment: What I don't understand is the duality of AddOrigin and AddRegisterableDomain. My understanding of the concept is that: 1. The caller's intention is to delete https://www.google.com 2. The filter builder offers to build a filter for Origin["https", "www.google.com", 443] or for the registerable domain "google.com" 3. BrowsingDataRemover knows to use the origin filter where applicable, and the domain filter for cookies The current solution where we store a set of origins and set of domains and there can be no relation between them is strange, and I don't think we're going to use such a filter anywhere, it's always going to be either-or. Since RemoveWithFilter gets only one filter builder as a parameter, and needs to pass it to cookies, that filter must contain domains. But the same filter builder then also does BuildOriginOrDomainFilter() and will delete entire domains even for other datatypes. https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:366: const BrowsingDataRemoverFilter& origin_filter) { nit: rename the parameter to match the new class name (here and below, in the .h file, in comments...). https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:495: keywords_model->RemoveAutoGeneratedForOriginBetween(GURL(), delete_begin_, This deletion very explicitly ignores the filter by passing an empty GURL into a method requesting origin. Please add a "// TODO: Support" here as well. https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_filter.cc (right): https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_filter.cc:50: DCHECK_EQ(GetDomainAndRegistry("www." + domain, INCLUDE_PRIVATE_REGISTRIES), Please explain this DCHECK. https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_filter.cc:78: ContentSettingsPattern::CreateBuilder(false); nit: /* validate */ https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_filter_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_filter_unittest.cc:342: // sp.nom.br is a TLD. nit: eTLD? Here and elsewhere.
Adding Yafei, who is working on storage integration on our side. Also, how/when is the origin based clean up going to be triggered for offline pages? https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.cc:294: DCHECK(is_loaded_); You can push back to delayed tasks here if you want this to take effect once the model is loaded. See implementation of DeletePagesByOfflineId. https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.cc:298: if (predicate.Run(GURL(id_page_pair.second.url))) id_page_pair.second.url is already a GURL. Another thing is: Some of the pages might have already been marked for deletion. Which means they are only temporarily hanging around (in case someone tries to undo deleting a bookmark. Given it is extremely unlikely someone will get a chance to undelete a bookmark after clearing site data, I think the risk is small, but I'd filter out pages marked for deletion here: if (predicate.Run(id_page_pair.second.url) && !id_page_pair.second.IsMarkedForDeletion()) https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.cc:303: base::Bind(&OfflinePageModel::OnRemoveAllFilesDoneForClearAll, No this will not do, as this wipes the whole store, which I am sure you don't want to do if you selectively remove some pages. I don't think you have extra clean up tasks after you delete here. This should be enough: DeletePagesByOfflineId(offline_ids, callback); https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.h:180: // clearing the store. Deletes offline pages matching the URL predicate. https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.h:181: void ClearWithURLPredicate(const base::Callback<bool(const GURL&)> predicate, Rename: DeletePagesByURLPredicate Add tests as well, please.
Hello all, I didn't have enough time today to finish the offline pages tests, but here are comment replies. I removed the origin support from the filter in favor of the registerable domain support, which is what we need. Let me know if anyone needs that, and I can figure out the best way to add that back in. Thanks! Daniel https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:366: const BrowsingDataRemoverFilter& origin_filter) { On 2016/04/07 at 16:19:12, msramek wrote: > nit: rename the parameter to match the new class name (here and below, in the .h file, in comments...). Done. https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:495: keywords_model->RemoveAutoGeneratedForOriginBetween(GURL(), delete_begin_, On 2016/04/07 at 16:19:12, msramek wrote: > This deletion very explicitly ignores the filter by passing an empty GURL into a method requesting origin. Please add a "// TODO: Support" here as well. Done. https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_filter.cc (right): https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_filter.cc:50: DCHECK_EQ(GetDomainAndRegistry("www." + domain, INCLUDE_PRIVATE_REGISTRIES), On 2016/04/07 at 16:19:12, msramek wrote: > Please explain this DCHECK. Done. https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_filter.cc:78: ContentSettingsPattern::CreateBuilder(false); On 2016/04/07 at 16:19:12, msramek wrote: > nit: /* validate */ Done. https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_filter_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/310001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_filter_unittest.cc:342: // sp.nom.br is a TLD. On 2016/04/07 at 16:19:13, msramek wrote: > nit: eTLD? Here and elsewhere. Done. https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.cc:294: DCHECK(is_loaded_); On 2016/04/07 at 17:02:49, fgorski wrote: > You can push back to delayed tasks here if you want this to take effect once the model is loaded. See implementation of DeletePagesByOfflineId. Done. https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.cc:298: if (predicate.Run(GURL(id_page_pair.second.url))) On 2016/04/07 at 17:02:49, fgorski wrote: > id_page_pair.second.url is already a GURL. > > Another thing is: Some of the pages might have already been marked for deletion. Which means they are only temporarily hanging around (in case someone tries to undo deleting a bookmark. > > Given it is extremely unlikely someone will get a chance to undelete a bookmark after clearing site data, I think the risk is small, but I'd filter out pages marked for deletion here: > > if (predicate.Run(id_page_pair.second.url) && !id_page_pair.second.IsMarkedForDeletion()) Done. https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.cc:303: base::Bind(&OfflinePageModel::OnRemoveAllFilesDoneForClearAll, On 2016/04/07 at 17:02:49, fgorski wrote: > No this will not do, as this wipes the whole store, which I am sure you don't want to do if you selectively remove some pages. > > I don't think you have extra clean up tasks after you delete here. This should be enough: > > DeletePagesByOfflineId(offline_ids, callback); Whoops, thanks. https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.h:180: // clearing the store. On 2016/04/07 at 17:02:49, fgorski wrote: > Deletes offline pages matching the URL predicate. Done. https://codereview.chromium.org/1741123002/diff/310001/components/offline_pag... components/offline_pages/offline_page_model.h:181: void ClearWithURLPredicate(const base::Callback<bool(const GURL&)> predicate, On 2016/04/07 at 17:02:49, fgorski wrote: > Rename: DeletePagesByURLPredicate > > Add tests as well, please. Done. https://codereview.chromium.org/1741123002/diff/310001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1741123002/diff/310001/content/browser/storag... content/browser/storage_partition_impl.cc:60: // Cookie matcher and storage_origin are never both populated. On 2016/04/07 at 09:03:08, kinuko wrote: > Maybe have a DCHECK for this? Done. https://codereview.chromium.org/1741123002/diff/310001/content/browser/storag... content/browser/storage_partition_impl.cc:73: begin, end, cookie_matcher, base::Bind(&OnClearedCookies, callback)); On 2016/04/07 at 09:03:08, kinuko wrote: > nit: if - if else - else could be revised with early return + if's Done. https://codereview.chromium.org/1741123002/diff/310001/content/browser/storag... content/browser/storage_partition_impl.cc:268: On 2016/04/07 at 09:03:08, kinuko wrote: > ? remove this line Done.
Hi Dan! Thanks for the explanation in the email. Sorry, I wasn't quick enough to stop you with the removal :-/ It is true that for your feature you want host-based deletion, but you're forgetting that you're not the only client :) For my use case, which is deleting browsing data for a particular site, I really need origin. I confirmed today with mkwst@ that this is preferable for us to host-based deletion. So in the end we need both functionalities. Fortunately, the predicate GURL->boolean is general enough to accommodate both (phew!). My plan to move forward is the define an abstract BrowsingDataFilterBuilder that will define an interface: get general filter, get filter for cookies, get filter for content settings etc. BrowsingDataRemover will accept a BrowsingDataFilterBuilder. We will have two specializations: HostFilterBuilder and OriginFilterBuilder. One used by your code and the other by mine. When you land this, I will follow up by splitting the origin and host functionality to two separate classes. You can make my life slightly easier by running "git checkout origin/master -- origin_filter_builder*" so I don't have to rewrite them and get them reviewed from scratch. It will also make your CL smaller and won't break anything, since the files won't be used anywhere after your CL. Cheers, Martin
Thanks for the clarification Martin, that sounds perfect. Yes, I wasn't sure if anyone else needed this feature, but I defaulted to cleanup up as much as possible ;) whoops! I added back that class. I finished the offline pages tests. fgorski: can you PTAL? brettw: can you reivew the browsing_data changes? I believe you were on the origin_filter_builder review, but let me know if someone else should review (michaeln can I believe). Thanks everyone!
lgtm, but please address comments. https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... components/offline_pages/offline_page_model.cc:312: predicate.Run(id_page_pair.second.url)) wrap with {} since the if statement spans more than one line. https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... components/offline_pages/offline_page_model.h:181: const base::Callback<bool(const GURL&)> predicate, Pass by const ref as well. https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... components/offline_pages/offline_page_model.h:318: const base::Callback<bool(const GURL&)> predicate, pass by const ref as well.
https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... components/offline_pages/offline_page_model.cc:312: predicate.Run(id_page_pair.second.url)) On 2016/04/11 at 20:07:27, fgorski wrote: > wrap with {} since the if statement spans more than one line. Done. https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... components/offline_pages/offline_page_model.h:181: const base::Callback<bool(const GURL&)> predicate, On 2016/04/11 at 20:07:27, fgorski wrote: > Pass by const ref as well. Done. https://codereview.chromium.org/1741123002/diff/350001/components/offline_pag... components/offline_pages/offline_page_model.h:318: const base::Callback<bool(const GURL&)> predicate, On 2016/04/11 at 20:07:27, fgorski wrote: > pass by const ref as well. Done.
I just took one more pass, browsing_data parts still (or again) LGTM. And Thanks, Dan, for taking on this huge chunk of work. Happy to see it finally land :)
Martin/Brett: One last question about the ContentSettingsPattern behavior. https://codereview.chromium.org/1741123002/diff/390001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_filter_builder_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/390001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_filter_builder_unittest.cc:254: {"*", true}, Martin (and maybe Brett): I'm unsure if this is the correct behavior. I'm using IDENTITY and PREDECESSOR in my compare, which means that patterns that are more general than my domain pattern aren't matched. This makes sense for the whitelist, but maybe for the blacklist we shouldn't delete it? As our sites might need something from these content settings? We don't actually ever have our filter called with a general pattern like this, but I was testing for it just in case it happened later.
still lgtm
chrome/browser/browsing_data LGTM. Thanks for taking another pass and splitting out the other patch.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, kinuko@chromium.org, fgorski@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/1741123002/#ps410001 (title: "Fixed Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1741123002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1741123002/410001
Message was sent while issue was closed.
Committed patchset #22 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove |remove_url|, which was never used except for tests. This also removes support for (to be added in the future): * origin-specific history removal * origin-specific extension removal Here is the followup patch which would do the JNI hookup: https://codereview.chromium.org/1757163002 BUG=589586,578162 ========== to ========== Add removal filter support for Cookies, Storage, and Content Settings. We modify BrowsingDataRemover to add more origin filtering support for: * Cookies * Storage Partition (idb, cookies, etc) * Content Settings map values, specifically * CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT * CONTENT_SETTINGS_TYPE_APP_BANNER * Offline pages We remove |remove_url|, which was never used except for tests. This also removes support for (to be added in the future): * origin-specific history removal * origin-specific extension removal Here is the followup patch which would do the JNI hookup: https://codereview.chromium.org/1757163002 BUG=589586,578162 Committed: https://crrev.com/d193beb95152b418357056525d1c6b6a641d7b16 Cr-Commit-Position: refs/heads/master@{#386804} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/d193beb95152b418357056525d1c6b6a641d7b16 Cr-Commit-Position: refs/heads/master@{#386804}
Message was sent while issue was closed.
https://codereview.chromium.org/1741123002/diff/390001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_filter_builder_unittest.cc (right): https://codereview.chromium.org/1741123002/diff/390001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_filter_builder_unittest.cc:254: {"*", true}, On 2016/04/12 18:07:10, dmurph wrote: > Martin (and maybe Brett): I'm unsure if this is the correct behavior. I'm using > IDENTITY and PREDECESSOR in my compare, which means that patterns that are more > general than my domain pattern aren't matched. This makes sense for the > whitelist, but maybe for the blacklist we shouldn't delete it? As our sites > might need something from these content settings? > > We don't actually ever have our filter called with a general pattern like this, > but I was testing for it just in case it happened later. Your IDENTITY+PREDECESSOR matching works if the expectation is that the whitelist/blacklist contains origins or hosts, and website settings only contain origins. Things get really complicated if you consider broader patterns, and I'm not sure if there is a right answer - yes, "*" could contain some data related to "google.com", but do you really want to delete it if it also contains data for other domains? I am fine if the behavior here is undefined, because website settings really aren't supposed to use anything else than origin-scoped patterns. I'll see if we can add DCHECKs into the website settings code for this. |
