Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1895993003:
Add migration code to change existing domain scoped content settings to be origin scoped (Closed)
|
Created:
4 years, 8 months ago by lshang Modified:
4 years, 5 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd migration code to change existing domain scoped content settings to be origin scoped
Migrate existing content settings of cookies, images, javascript, plugins,
popups, mouselock and automatic-downloads to be origin scoped so that their
settings only affect origins.
This is the first step of migrating domain scoped settings to be origin scoped.
In the following CLs we will:
- Add callbacks in sync layer so that the migrated old settings are cleared
after the sync process.
- Change the scoping type of these content types so that new generated settings
are all origin scoped and hook up all the migration code and sync callbacks.
BUG=604612
Committed: https://crrev.com/9c9a2b8dde4e141db7fb814f9ff338927741e7c1
Cr-Commit-Position: refs/heads/master@{#404340}
Patch Set 1 #
Total comments: 10
Patch Set 2 : migrate old domain settings #Patch Set 3 : (test patch) #Patch Set 4 : rebase #Patch Set 5 : add callback to syncable_prefs and deps #Patch Set 6 : rebase #Patch Set 7 : add resetcallback in destructor #Patch Set 8 : remove logs and format #
Total comments: 33
Patch Set 9 : address review comments and split the CL #
Total comments: 24
Patch Set 10 : rewrite ContentSettingsPatterns::MigrateFromDomaintoOrigin #
Total comments: 23
Patch Set 11 : format #Patch Set 12 : enhance tests #
Total comments: 17
Patch Set 13 : address review comments #
Total comments: 2
Patch Set 14 : Merge branch 'master' into migrate_domain_scoped_settings #Patch Set 15 : rebase #
Messages
Total messages: 53 (33 generated)
Description was changed from ========== Change domain scoped content setting types to be REQUESTING_ORIGIN_ONLY_SCOPE BUG=604612 ========== to ========== Change domain scoped content setting types to be REQUESTING_ORIGIN_ONLY_SCOPE Change the scoping type of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. BUG=604612 ==========
lshang@chromium.org changed reviewers: + raymes@chromium.org
Hi Raymes, PTAL thanks!
msramek@chromium.org changed reviewers: + msramek@chromium.org
Drive-by LGTM with a short rant :) https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:235: TEST_F(HostContentSettingsMapTest, Origins) { Hmm. This test indeed doesn't test patterns anymore, but it should. It seems that in https://codereview.chromium.org/1686343002 we removed patterns from this test (because the HostContentSettingsMap interface stopped supporting them), but in fact we should have just used another way of setting them (like writing directly into the preference file), so that this test still could test patterns. We lost that test coverage. The same is true of NestedSettings below. Origins and NestedSettings tests are almost equivalent right now. (this is unrelated to this CL, I just spotted it because of this change; I can look into that myself later)
https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:235: TEST_F(HostContentSettingsMapTest, Origins) { That's a good point. We can still test patterns here by setting a custom-scoped setting and that seems like a good idea to keep test coverage. https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:547: // Cookies and Images only take effect per-origin. Same here, I think since this test explicitly tests patterns, we should keep that test coverage by manually setting up the patterns. https://codereview.chromium.org/1895993003/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_registry.cc (left): https://codereview.chromium.org/1895993003/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_registry.cc:225: WebsiteSettingsInfo::TOP_LEVEL_DOMAIN_ONLY_SCOPE, I think this should become TOP_LEVEL_ORIGIN_ONLY_SCOPE https://codereview.chromium.org/1895993003/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1895993003/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_registry.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. I think we should also be able to change the instances in website_settings_registry.cc and remove/rename the enum values.
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Change domain scoped content setting types to be REQUESTING_ORIGIN_ONLY_SCOPE Change the scoping type of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. BUG=604612 ========== to ========== Change domain scoped content setting types to be origin scoped Change the scoping type of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. BUG=604612 ==========
Patchset #10 (id:240001) has been deleted
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:280001) has been deleted
Patchset #12 (id:340001) has been deleted
Patchset #10 (id:300001) has been deleted
Patchset #9 (id:220001) has been deleted
Patchset #10 (id:360001) has been deleted
Patchset #9 (id:320001) has been deleted
Patchset #9 (id:380001) has been deleted
Patchset #9 (id:400001) has been deleted
Patchset #8 (id:200001) has been deleted
Patchset #6 (id:160001) has been deleted
raymes@, I've updated this CL to fix the sync problem. Could you take a look again? Thanks! https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:235: TEST_F(HostContentSettingsMapTest, Origins) { On 2016/04/19 18:51:34, msramek wrote: > Hmm. This test indeed doesn't test patterns anymore, but it should. > > It seems that in https://codereview.chromium.org/1686343002 we removed patterns > from this test (because the HostContentSettingsMap interface stopped supporting > them), but in fact we should have just used another way of setting them (like > writing directly into the preference file), so that this test still could test > patterns. We lost that test coverage. > > The same is true of NestedSettings below. Origins and NestedSettings tests are > almost equivalent right now. > > (this is unrelated to this CL, I just spotted it because of this change; I can > look into that myself later) Yep, I've changed it back to explicitly use patterns to keep test coverage. https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:235: TEST_F(HostContentSettingsMapTest, Origins) { On 2016/04/20 01:53:21, raymes wrote: > That's a good point. We can still test patterns here by setting a custom-scoped > setting and that seems like a good idea to keep test coverage. As above. https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:547: // Cookies and Images only take effect per-origin. On 2016/04/20 01:53:21, raymes wrote: > Same here, I think since this test explicitly tests patterns, we should keep > that test coverage by manually setting up the patterns. Done. https://codereview.chromium.org/1895993003/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_registry.cc (left): https://codereview.chromium.org/1895993003/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_registry.cc:225: WebsiteSettingsInfo::TOP_LEVEL_DOMAIN_ONLY_SCOPE, On 2016/04/20 01:53:21, raymes wrote: > I think this should become TOP_LEVEL_ORIGIN_ONLY_SCOPE Done. https://codereview.chromium.org/1895993003/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1895993003/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_registry.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/20 01:53:21, raymes wrote: > I think we should also be able to change the instances in > website_settings_registry.cc and remove/rename the enum values. Done.
Thanks Liu. I think we should split this up as it's getting quite complicated. We could have 2 or 3 CLs: -Add the migration code with tests -Add hooks to the sync infrastructure -Hookup the migration code and change scoping https://codereview.chromium.org/1895993003/diff/190017/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:99: LOG(ERROR) << "--------lshang--------Get-----" << type; nit: remove this https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:166: WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE, Shouldn't these be TOP_LEVEL_ORIGIN_ONLY? https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:206: WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE, and this one? https://codereview.chromium.org/1895993003/diff/190017/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:504: #if !defined(OS_ANDROID) && !defined(OS_IOS) Rather than the ifdefs, can we just check to see if content_settings::ContentSettingsRegistry::GetInstance()->Get returns true? https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:528: setting_entry.secondary_pattern == ContentSettingsPattern::Wildcard()) nit: add {} https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:529: continue; Should we also DCHECK that the secondary_pattern is a wildcard pattern? https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:530: if (setting_entry.primary_pattern.IsGeneratedFromURLDomainScoped()) { What about using "continue" here (if this evaluates to false) to avoid the indenting below https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:535: if (origin.is_valid()) { Same here https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:536: ContentSetting origin_setting = Maybe say: // Ensure that the current resolved content setting for this origin is allowed. Otherwise we may be overriding some narrower setting which is set to block. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:539: // Remove the domiain scoped pattern. // Remove the domain scoped pattern. If |origin_setting| is not CONTENT_SETTING_ALLOW it implies there is some narrower pattern in effect, so it's still safe to remove the domain-scoped pattern. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:545: // new setting entry of this origin. // If the current resolved content setting is allowed it's safe to set the origin-scoped pattern. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:317: // domain settings to origins so that there will not cause security issues. security->privacy/security https://codereview.chromium.org/1895993003/diff/190017/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:452: if (domain_pattern.parts_.is_port_wildcard) { in this case I think we can just assume the http port https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:552: bool ContentSettingsPattern::IsGeneratedFromURLDomainScoped() const { Maybe call this: IsGeneratedUsingFromURLNoWildcard() https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:553: return (!parts_.host.empty()) && parts_.has_domain_wildcard; I think there are more checks we can do here. Namely: // Generated patterns must either have a scheme wildcard or be https. if (scheme != https && !is_scheme_wildcard) return false; // Generated patterns will either have a port wildcard or be https (but not both). if (is_port_wildcard && scheme == https) return false; // Generated patterns will always have a domain wildcard. if (!has_domain_wildcard) return false; // Generated patterns will always have a host. if (host.empty()) return false; return true; https://codereview.chromium.org/1895993003/diff/190017/components/password_ma... File components/password_manager/sync/browser/password_manager_setting_migrator_service_unittest.cc (left): https://codereview.chromium.org/1895993003/diff/190017/components/password_ma... components/password_manager/sync/browser/password_manager_setting_migrator_service_unittest.cc:316: nit: this isn't needed https://codereview.chromium.org/1895993003/diff/190017/components/syncable_pr... File components/syncable_prefs/pref_model_associator.h (right): https://codereview.chromium.org/1895993003/diff/190017/components/syncable_pr... components/syncable_prefs/pref_model_associator.h:123: static void SetMergeDataFinishedCallback(const base::Closure& callback); I don't think we will want these to be static. Instead I think we will want to get access to the PrefModelAssociator and add a listener. We can get access to the PrefServiceSyncable from syncable_prefs::PrefServiceSyncable* PrefServiceSyncableFromProfile() and passing in the profile and then we could pass the callback down. We should check with Pavel about all this because I'm just guessing :) Usually it's good to write functions like this in a general way such that others can reuse them. That usually means tracking a vector of callbacks in this class. One way would be to have a function like this: void AddMergeDataFinishedCallback(... callback) { if (merge_finished_) { callback.Run(); else merge_finished_callbacks_.push_back(callback); } and then in the function where the merge finishes: ... for (Callback callback : merge_finished_callbacks_) callback.Run(); merge_finished_callbacks_.clear(); Again we should probably run that by Pavel for his thoughts.
Patchset #11 (id:490001) has been deleted
Patchset #10 (id:470001) has been deleted
Patchset #9 (id:450001) has been deleted
Description was changed from ========== Change domain scoped content setting types to be origin scoped Change the scoping type of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. BUG=604612 ========== to ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Change the scoping type of these content types so that new generated settings are all origin scoped. - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. BUG=604612 ==========
Description was changed from ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Change the scoping type of these content types so that new generated settings are all origin scoped. - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. BUG=604612 ========== to ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Change the scoping type of these content types so that new generated settings are all origin scoped. - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. BUG=604612 ==========
Description was changed from ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Change the scoping type of these content types so that new generated settings are all origin scoped. - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. BUG=604612 ========== to ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Change the scoping type of these content types so that new generated settings are all origin scoped. - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. BUG=604612 ==========
Patchset #9 (id:510001) has been deleted
Hi Raymes, I split the CL into 3 CLs and this CL only add migration code and tests. I will hookup the migration code when we change the scoping type. Please confirm the change of this part, thanks! https://codereview.chromium.org/1895993003/diff/190017/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:99: LOG(ERROR) << "--------lshang--------Get-----" << type; On 2016/06/16 03:23:50, raymes wrote: > nit: remove this Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:166: WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE, On 2016/06/16 03:23:50, raymes wrote: > Shouldn't these be TOP_LEVEL_ORIGIN_ONLY? Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:206: WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE, On 2016/06/16 03:23:50, raymes wrote: > and this one? Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:504: #if !defined(OS_ANDROID) && !defined(OS_IOS) On 2016/06/16 03:23:51, raymes wrote: > Rather than the ifdefs, can we just check to see if > content_settings::ContentSettingsRegistry::GetInstance()->Get returns true? Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:528: setting_entry.secondary_pattern == ContentSettingsPattern::Wildcard()) On 2016/06/16 03:23:51, raymes wrote: > nit: add {} Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:529: continue; On 2016/06/16 03:23:50, raymes wrote: > Should we also DCHECK that the secondary_pattern is a wildcard pattern? Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:530: if (setting_entry.primary_pattern.IsGeneratedFromURLDomainScoped()) { On 2016/06/16 03:23:50, raymes wrote: > What about using "continue" here (if this evaluates to false) to avoid the > indenting below Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:535: if (origin.is_valid()) { On 2016/06/16 03:23:51, raymes wrote: > Same here Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:536: ContentSetting origin_setting = On 2016/06/16 03:23:51, raymes wrote: > Maybe say: > // Ensure that the current resolved content setting for this origin is allowed. > Otherwise we may be overriding some narrower setting which is set to block. Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:539: // Remove the domiain scoped pattern. On 2016/06/16 03:23:50, raymes wrote: > // Remove the domain scoped pattern. If |origin_setting| is not > CONTENT_SETTING_ALLOW it implies there is some narrower pattern in effect, so > it's still safe to remove the domain-scoped pattern. Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:545: // new setting entry of this origin. On 2016/06/16 03:23:51, raymes wrote: > // If the current resolved content setting is allowed it's safe to set the > origin-scoped pattern. Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:317: // domain settings to origins so that there will not cause security issues. On 2016/06/16 03:23:51, raymes wrote: > security->privacy/security Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:452: if (domain_pattern.parts_.is_port_wildcard) { On 2016/06/16 03:23:51, raymes wrote: > in this case I think we can just assume the http port Done. https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:552: bool ContentSettingsPattern::IsGeneratedFromURLDomainScoped() const { On 2016/06/16 03:23:51, raymes wrote: > Maybe call this: > IsGeneratedUsingFromURLNoWildcard() Done. IsGeneratedUsingFromURL():) https://codereview.chromium.org/1895993003/diff/190017/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:553: return (!parts_.host.empty()) && parts_.has_domain_wildcard; On 2016/06/16 03:23:51, raymes wrote: > I think there are more checks we can do here. Namely: > > // Generated patterns must either have a scheme wildcard or be https. > if (scheme != https && !is_scheme_wildcard) > return false; > > // Generated patterns will either have a port wildcard or be https (but not > both). > if (is_port_wildcard && scheme == https) > return false; > > // Generated patterns will always have a domain wildcard. > if (!has_domain_wildcard) > return false; > > // Generated patterns will always have a host. > if (host.empty()) > return false; > > return true; Done. Yeah this is more clear. https://codereview.chromium.org/1895993003/diff/190017/components/password_ma... File components/password_manager/sync/browser/password_manager_setting_migrator_service_unittest.cc (left): https://codereview.chromium.org/1895993003/diff/190017/components/password_ma... components/password_manager/sync/browser/password_manager_setting_migrator_service_unittest.cc:316: On 2016/06/16 03:23:51, raymes wrote: > nit: this isn't needed Done.
Thanks! https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1290: // Change default setting to BLCOK. nit: BLOCK https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1316: // Change default setting to BLCOK. nit: BLOCK https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:528: DCHECK(setting_entry.secondary_pattern == DCHECK_EQ https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:538: if (!origin.is_valid()) Are there cases where this is possible? Should we just DCHECK this instead? https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:317: // domain settings to origins so that there will not cause privacy/security nit: there->this https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:318: // issues. We should add a similar TODO to the above function, saying to remove this after 2 milestones. We should also probably file a bug to do that. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/common/content_settings_pattern.h (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern.h:132: static ContentSettingsPattern FromURL(const GURL& url); We should remove this at some point (when we've finished using it) https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern.h:149: const ContentSettingsPattern domain_pattern); Could we get away with just having one new function? We could call it: MigrateFromDomainToOrigin (it could be a member function or a static function, whatever makes the code nicer). It could return a bool which is true if the pattern was suitable to migrate and put the ContentSettingsPattern into an out-param. We should also add a TODO which says to remove it when we remove the other Migrate function (and probably file a bug about that) https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/common/content_settings_pattern_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:755: .IsGeneratedUsingFromURL()); We should make sure that the tests cover each of the cases that exist in the function, e.g. we should have an example that uses a http scheme to check the first condition. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:763: .c_str()); does expect_eq work, if you get rid of the call to c_str()? https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:773: .c_str()); We should also test a case with a port wildcard
Patchset #10 (id:550001) has been deleted
https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1290: // Change default setting to BLCOK. On 2016/06/20 04:03:26, raymes wrote: > nit: BLOCK Done. https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1316: // Change default setting to BLCOK. On 2016/06/20 04:03:25, raymes wrote: > nit: BLOCK Done. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:528: DCHECK(setting_entry.secondary_pattern == On 2016/06/20 04:03:26, raymes wrote: > DCHECK_EQ Compiler complains when I change to use DCHECK_EQ instead of DCHECK ==. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:538: if (!origin.is_valid()) On 2016/06/20 04:03:26, raymes wrote: > Are there cases where this is possible? Should we just DCHECK this instead? Done. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:317: // domain settings to origins so that there will not cause privacy/security On 2016/06/20 04:03:26, raymes wrote: > nit: there->this Done. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:318: // issues. On 2016/06/20 04:03:26, raymes wrote: > We should add a similar TODO to the above function, saying to remove this after > 2 milestones. We should also probably file a bug to do that. Done. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/common/content_settings_pattern.h (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern.h:132: static ContentSettingsPattern FromURL(const GURL& url); On 2016/06/20 04:03:26, raymes wrote: > We should remove this at some point (when we've finished using it) Done. I added a TODO here. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern.h:149: const ContentSettingsPattern domain_pattern); On 2016/06/20 04:03:26, raymes wrote: > Could we get away with just having one new function? We could call it: > MigrateFromDomainToOrigin (it could be a member function or a static function, > whatever makes the code nicer). It could return a bool which is true if the > pattern was suitable to migrate and put the ContentSettingsPattern into an > out-param. > > We should also add a TODO which says to remove it when we remove the other > Migrate function (and probably file a bug about that) Done. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/common/content_settings_pattern_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:755: .IsGeneratedUsingFromURL()); On 2016/06/20 04:03:26, raymes wrote: > We should make sure that the tests cover each of the cases that exist in the > function, e.g. we should have an example that uses a http scheme to check the > first condition. Done. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:763: .c_str()); On 2016/06/20 04:03:26, raymes wrote: > does expect_eq work, if you get rid of the call to c_str()? Done. https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:773: .c_str()); On 2016/06/20 04:03:26, raymes wrote: > We should also test a case with a port wildcard Https patterns generated using FromURL() should always have a port (url's port or https's default port 443 if url's port is empty).
https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:528: DCHECK(setting_entry.secondary_pattern == What's the error? https://codereview.chromium.org/1895993003/diff/530001/components/content_set... File components/content_settings/core/common/content_settings_pattern_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:773: .c_str()); Ahh that makes sense https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1315: // |host_settings| contains default setting and a custom setting. host_content_settings_map? (here and below) https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1320: EXPECT_EQ("[*.]example.com", setting_entry.primary_pattern.ToString()); This won't exactly check that things are what we expect because both patterns may be wildcards. Instead you could have: bool found_setting = false; for (const ContentSettingPatternSource& setting_entry : settings) { if (setting_entry.primary_pattern.ToString() == ...) found_setting = true; } EXPECT_TRUE(found_setting); We probably don't need to check that the wildcard exists. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:524: ContentSettingsPattern::Wildcard()) { nit: we can remove the 2nd half of this check because we are already asserting that the secondary pattern will be a wildcard https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:533: setting_entry.primary_pattern, &origin_pattern)) nit: add {} https://codereview.chromium.org/1895993003/diff/570001/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:442: bool ContentSettingsPattern::MigrateFromDomaintoOrigin( MigrateFromDomainToOrigin (capital T) https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:447: // First test if the domain pattern is generated using FromURL(). nit: remove this line, I think it should be clear enough https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:450: !domain_pattern.parts_.is_scheme_wildcard) nit: add {} https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:456: domain_pattern.parts_.scheme == url::kHttpsScheme) nit: add {} https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:474: if (domain_pattern.parts_.has_domain_wildcard) I think we know this will be true, so we can remove this check https://codereview.chromium.org/1895993003/diff/570001/components/content_set... File components/content_settings/core/common/content_settings_pattern_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:767: // Other schemes, won't be migrated. Maybe: // Other schemes and IP address patterns won't be migrated. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:802: EXPECT_EQ("https://google.com:443", origin_pattern.ToString()); We should also test one with a custom port (it should work)
Thanks Raymes! https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1315: // |host_settings| contains default setting and a custom setting. On 2016/06/27 07:23:29, raymes wrote: > host_content_settings_map? (here and below) Done. https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1320: EXPECT_EQ("[*.]example.com", setting_entry.primary_pattern.ToString()); On 2016/06/27 07:23:29, raymes wrote: > This won't exactly check that things are what we expect because both patterns > may be wildcards. Instead you could have: > > bool found_setting = false; > for (const ContentSettingPatternSource& setting_entry : settings) { > if (setting_entry.primary_pattern.ToString() == ...) > found_setting = true; > } > EXPECT_TRUE(found_setting); > > We probably don't need to check that the wildcard exists. There should be a default setting (*, *) and a custom setting ([*.]example.com, *) here, and I use a test "primary_pattern == wildcard" to filter the default setting, then check if the custom setting is domain scoped. I think it's the same with using a found_setting boolean? (Please correct me if I'm wrong) https://codereview.chromium.org/1895993003/diff/570001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:524: ContentSettingsPattern::Wildcard()) { On 2016/06/27 07:23:29, raymes wrote: > nit: we can remove the 2nd half of this check because we are already asserting > that the secondary pattern will be a wildcard Done. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:533: setting_entry.primary_pattern, &origin_pattern)) On 2016/06/27 07:23:29, raymes wrote: > nit: add {} Done. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:442: bool ContentSettingsPattern::MigrateFromDomaintoOrigin( On 2016/06/27 07:23:29, raymes wrote: > MigrateFromDomainToOrigin > > (capital T) Done. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:447: // First test if the domain pattern is generated using FromURL(). On 2016/06/27 07:23:29, raymes wrote: > nit: remove this line, I think it should be clear enough Done. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:450: !domain_pattern.parts_.is_scheme_wildcard) On 2016/06/27 07:23:29, raymes wrote: > nit: add {} Done. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:456: domain_pattern.parts_.scheme == url::kHttpsScheme) On 2016/06/27 07:23:29, raymes wrote: > nit: add {} Done. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:474: if (domain_pattern.parts_.has_domain_wildcard) On 2016/06/27 07:23:29, raymes wrote: > I think we know this will be true, so we can remove this check Done. https://codereview.chromium.org/1895993003/diff/570001/components/content_set... File components/content_settings/core/common/content_settings_pattern_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/570001/components/content_set... components/content_settings/core/common/content_settings_pattern_unittest.cc:767: // Other schemes, won't be migrated. On 2016/06/27 07:23:29, raymes wrote: > Maybe: > // Other schemes and IP address patterns won't be migrated. Done.
lgtm
https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1320: EXPECT_EQ("[*.]example.com", setting_entry.primary_pattern.ToString()); Note: as discussed we should still change this :)
Description was changed from ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Change the scoping type of these content types so that new generated settings are all origin scoped. - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. BUG=604612 ========== to ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. - Change the scoping type of these content types so that new generated settings are all origin scoped and hook up all the migration code and sync callbacks. BUG=604612 ==========
msramek@, this CL changed a lot since you l-g-t-m-ed at the first place. I've split the whole thing into 3 CLs and adjusted the tests. Could you take another look at this CL and see if you're OK with the current version? https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1320: EXPECT_EQ("[*.]example.com", setting_entry.primary_pattern.ToString()); On 2016/06/30 01:11:27, raymes wrote: > Note: as discussed we should still change this :) Done.
LGTM with comments. https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1317: bool found_setting = false; This can be simplified as: EXPECT_TRUE(settings[0].primary_pattern.ToString() == "*") EXPECT_TRUE(settings[1].primary_pattern.ToString() == "[*.]example.com") https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1379: // |host_content_settings_map| contains default setting and a custom setting. In my understanding, "custom setting" is a setting with a custom nonstandard pattern (i.e. SetContentSettingCustomPattern()), while these actually become the new standard. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:525: DCHECK(setting_entry.secondary_pattern == I would prefer if (...) { NOTREACHED(); continue; } If this DCHECK were false, SetContentSettingCustomScope(..., Wildcard()) could accidentally allow something that wasn't allowed. Besides, how do we know that this must be true? We're reading content settings from the user's Preferences file; a single call of SetContentSetting() with secondary_pattern != Wildcard() in a years-old version of Chrome could break that expectation. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:320: // migrated (~M55). Since this CL is probably going to miss M53 branchpoint, please increment :) https://codereview.chromium.org/1895993003/diff/610001/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:453: // Generated patterns will either have a port wildcard or be https (but not Not sure if this is just me, but this sentence reads to me like they should have one XOR the other. That is not the case, because the HTTP scheme may or may not have a port wildcard, which is also captured in the tests. Meanwhile, the code says one NAND the other. Can we change the comment to e.g. "Generated patterns with the HTTPs scheme can not have a port wildcard.", or "Only generated patterns with a scheme wildcard can have a port wildcard", or something like that? https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:460: // Generated patterns will always have a domain wildcard. nit: Patterns generated with ::FromUrl (which we want to migrate). Those generated with ::FromUrlNoWildcard don't. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:468: *origin_pattern = domain_pattern; Would it make sense to use Builder here instead of these in-place changes? If we ever needed to change something in how a pattern is built, there would only be one place to change. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:471: (*origin_pattern).parts_.scheme = url::kHttpScheme; The scheme wildcard exception was valid for both http and https. Shouldn't we add both exceptions? (although actually there aren't many sites that use both schemes, so this would probably just pollute HCSM with duplicate entries)
Patchset #13 (id:630001) has been deleted
https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1317: bool found_setting = false; On 2016/06/30 16:16:21, msramek wrote: > This can be simplified as: > > EXPECT_TRUE(settings[0].primary_pattern.ToString() == "*") > EXPECT_TRUE(settings[1].primary_pattern.ToString() == "[*.]example.com") Done. https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1379: // |host_content_settings_map| contains default setting and a custom setting. On 2016/06/30 16:16:21, msramek wrote: > In my understanding, "custom setting" is a setting with a custom nonstandard > pattern (i.e. SetContentSettingCustomPattern()), while these actually become the > new standard. Done. Yeah you're right, 'custom setting' should refer to custom defined nonstandard setting (like user manually input ones), we should not confuse it with patterns generated from permission prompts. I've changed the comments here and below. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:525: DCHECK(setting_entry.secondary_pattern == On 2016/06/30 16:16:21, msramek wrote: > I would prefer > > if (...) { > NOTREACHED(); > continue; > } > > If this DCHECK were false, SetContentSettingCustomScope(..., Wildcard()) could > accidentally allow something that wasn't allowed. > > Besides, how do we know that this must be true? We're reading content settings > from the user's Preferences file; a single call of SetContentSetting() with > secondary_pattern != Wildcard() in a years-old version of Chrome could break > that expectation. Done. Fair enough. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:320: // migrated (~M55). On 2016/06/30 16:16:21, msramek wrote: > Since this CL is probably going to miss M53 branchpoint, please increment :) Done. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:453: // Generated patterns will either have a port wildcard or be https (but not On 2016/06/30 16:16:22, msramek wrote: > Not sure if this is just me, but this sentence reads to me like they should have > one XOR the other. That is not the case, because the HTTP scheme may or may not > have a port wildcard, which is also captured in the tests. > > Meanwhile, the code says one NAND the other. Can we change the comment to e.g. > "Generated patterns with the HTTPs scheme can not have a port wildcard.", or > "Only generated patterns with a scheme wildcard can have a port wildcard", or > something like that? Done. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:460: // Generated patterns will always have a domain wildcard. On 2016/06/30 16:16:22, msramek wrote: > nit: Patterns generated with ::FromUrl (which we want to migrate). Those > generated with ::FromUrlNoWildcard don't. Done. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:468: *origin_pattern = domain_pattern; On 2016/06/30 16:16:22, msramek wrote: > Would it make sense to use Builder here instead of these in-place changes? If we > ever needed to change something in how a pattern is built, there would only be > one place to change. Done. https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:471: (*origin_pattern).parts_.scheme = url::kHttpScheme; On 2016/06/30 16:16:22, msramek wrote: > The scheme wildcard exception was valid for both http and https. Shouldn't we > add both exceptions? > > (although actually there aren't many sites that use both schemes, so this would > probably just pollute HCSM with duplicate entries) That's a good point! Patterns generated from permission request prompts for http sites will have scheme wildcards, while for https sites the generated patterns will always have a https scheme explicitly. I think for scheme wildcard patterns, even though the pattern system will also make it take effect on https sites, the decision was made originally for http sites, so it will be reasonable we now only add a http exception and make it only affect http sites. And yes for the other reason you pointed out, this will avoid polluting HCSM and confuse users (imagine how users will feel if there suddenly pop out a lot duplicate entries and some of them don't even exist). The drawback of this is that if a user added a scheme wildcard pattern (if a pattern is like [*.]example.com, we can't actually distinguish if it's from the prompt or user manually input) and wanted it to affect both schemes, after the migration it will only affect http scheme. Does this sounds reasonable to you?
Thanks for taking another pass, still LGTM! :) https://codereview.chromium.org/1895993003/diff/610001/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/610001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:471: (*origin_pattern).parts_.scheme = url::kHttpScheme; On 2016/07/04 02:46:01, Liu Shang wrote: > On 2016/06/30 16:16:22, msramek wrote: > > The scheme wildcard exception was valid for both http and https. Shouldn't we > > add both exceptions? > > > > (although actually there aren't many sites that use both schemes, so this > would > > probably just pollute HCSM with duplicate entries) > > That's a good point! > > Patterns generated from permission request prompts for http sites will have > scheme wildcards, while for https sites the generated patterns will always have > a https scheme explicitly. > > I think for scheme wildcard patterns, even though the pattern system will also > make it take effect on https sites, the decision was made originally for http > sites, so it will be reasonable we now only add a http exception and make it > only affect http sites. > > And yes for the other reason you pointed out, this will avoid polluting HCSM and > confuse users (imagine how users will feel if there suddenly pop out a lot > duplicate entries and some of them don't even exist). > > The drawback of this is that if a user added a scheme wildcard pattern (if a > pattern is like [*.]example.com, we can't actually distinguish if it's from the > prompt or user manually input) and wanted it to affect both schemes, after the > migration it will only affect http scheme. > > Does this sounds reasonable to you? Yup, makes sense :) https://codereview.chromium.org/1895993003/diff/650001/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/650001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:480: if (domain_pattern.parts_.is_port_wildcard) { This logic is exactly what the PatternParser does (https://cs.chromium.org/chromium/src/components/content_settings/core/common/...). I wonder if it can therefore be simplified, but probably not without outputting it to a string and then using the parser.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/1895993003/#ps690001 (title: "rebase")
https://codereview.chromium.org/1895993003/diff/650001/components/content_set... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/650001/components/content_set... components/content_settings/core/common/content_settings_pattern.cc:480: if (domain_pattern.parts_.is_port_wildcard) { On 2016/07/07 12:19:24, msramek wrote: > This logic is exactly what the PatternParser does > (https://cs.chromium.org/chromium/src/components/content_settings/core/common/...). > > I wonder if it can therefore be simplified, but probably not without outputting > it to a string and then using the parser. Since we need to generate a new pattern which is based on the old pattern but have different pattern component values, I think this is clearer than using parsers to show how each component is set.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. - Change the scoping type of these content types so that new generated settings are all origin scoped and hook up all the migration code and sync callbacks. BUG=604612 ========== to ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. - Change the scoping type of these content types so that new generated settings are all origin scoped and hook up all the migration code and sync callbacks. BUG=604612 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:690001)
Message was sent while issue was closed.
Description was changed from ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. - Change the scoping type of these content types so that new generated settings are all origin scoped and hook up all the migration code and sync callbacks. BUG=604612 ========== to ========== Add migration code to change existing domain scoped content settings to be origin scoped Migrate existing content settings of cookies, images, javascript, plugins, popups, mouselock and automatic-downloads to be origin scoped so that their settings only affect origins. This is the first step of migrating domain scoped settings to be origin scoped. In the following CLs we will: - Add callbacks in sync layer so that the migrated old settings are cleared after the sync process. - Change the scoping type of these content types so that new generated settings are all origin scoped and hook up all the migration code and sync callbacks. BUG=604612 Committed: https://crrev.com/9c9a2b8dde4e141db7fb814f9ff338927741e7c1 Cr-Commit-Position: refs/heads/master@{#404340} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9c9a2b8dde4e141db7fb814f9ff338927741e7c1 Cr-Commit-Position: refs/heads/master@{#404340} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
