Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(525)

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
Reviewers:
msramek, raymes
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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+340 lines, -0 lines) Patch
M chrome/browser/content_settings/host_content_settings_map_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +113 lines, -0 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +9 lines, -0 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +67 lines, -0 lines 0 comments Download
M components/content_settings/core/common/content_settings_pattern.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +8 lines, -0 lines 0 comments Download
M components/content_settings/core/common/content_settings_pattern.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +54 lines, -0 lines 0 comments Download
M components/content_settings/core/common/content_settings_pattern_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +89 lines, -0 lines 0 comments Download

Messages

Total messages: 53 (33 generated)
lshang
Hi Raymes, PTAL thanks!
4 years, 8 months ago (2016-04-19 07:25:06 UTC) #3
msramek
Drive-by LGTM with a short rant :) https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode235 chrome/browser/content_settings/host_content_settings_map_unittest.cc:235: TEST_F(HostContentSettingsMapTest, Origins) ...
4 years, 8 months ago (2016-04-19 18:51:34 UTC) #5
raymes
https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/1/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode235 chrome/browser/content_settings/host_content_settings_map_unittest.cc:235: TEST_F(HostContentSettingsMapTest, Origins) { That's a good point. We can ...
4 years, 8 months ago (2016-04-20 01:53:21 UTC) #6
lshang
raymes@, I've updated this CL to fix the sync problem. Could you take a look ...
4 years, 6 months ago (2016-06-15 06:26:42 UTC) #23
raymes
Thanks Liu. I think we should split this up as it's getting quite complicated. We ...
4 years, 6 months ago (2016-06-16 03:23:51 UTC) #24
lshang
Hi Raymes, I split the CL into 3 CLs and this CL only add migration ...
4 years, 6 months ago (2016-06-20 01:34:23 UTC) #32
raymes
Thanks! https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1290 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1290: // Change default setting to BLCOK. nit: BLOCK ...
4 years, 6 months ago (2016-06-20 04:03:26 UTC) #33
lshang
https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/530001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1290 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1290: // Change default setting to BLCOK. On 2016/06/20 04:03:26, ...
4 years, 6 months ago (2016-06-23 01:32:32 UTC) #35
raymes
https://codereview.chromium.org/1895993003/diff/530001/components/content_settings/core/browser/host_content_settings_map.cc File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1895993003/diff/530001/components/content_settings/core/browser/host_content_settings_map.cc#newcode528 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_settings/core/common/content_settings_pattern_unittest.cc File components/content_settings/core/common/content_settings_pattern_unittest.cc (right): ...
4 years, 5 months ago (2016-06-27 07:23:29 UTC) #36
lshang
Thanks Raymes! https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1315 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1315: // |host_settings| contains default setting and a ...
4 years, 5 months ago (2016-06-28 07:14:42 UTC) #37
raymes
lgtm
4 years, 5 months ago (2016-06-30 01:10:34 UTC) #38
raymes
https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/570001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1320 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 ...
4 years, 5 months ago (2016-06-30 01:11:27 UTC) #39
lshang
msramek@, this CL changed a lot since you l-g-t-m-ed at the first place. I've split ...
4 years, 5 months ago (2016-06-30 05:10:35 UTC) #41
msramek
LGTM with comments. https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1317 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1317: bool found_setting = false; This can ...
4 years, 5 months ago (2016-06-30 16:16:22 UTC) #42
lshang
https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1895993003/diff/610001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1317 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1317: bool found_setting = false; On 2016/06/30 16:16:21, msramek wrote: ...
4 years, 5 months ago (2016-07-04 02:46:01 UTC) #44
msramek
Thanks for taking another pass, still LGTM! :) https://codereview.chromium.org/1895993003/diff/610001/components/content_settings/core/common/content_settings_pattern.cc File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/610001/components/content_settings/core/common/content_settings_pattern.cc#newcode471 components/content_settings/core/common/content_settings_pattern.cc:471: (*origin_pattern).parts_.scheme ...
4 years, 5 months ago (2016-07-07 12:19:24 UTC) #45
lshang
https://codereview.chromium.org/1895993003/diff/650001/components/content_settings/core/common/content_settings_pattern.cc File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/1895993003/diff/650001/components/content_settings/core/common/content_settings_pattern.cc#newcode480 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: > ...
4 years, 5 months ago (2016-07-08 01:19:44 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1895993003/690001
4 years, 5 months ago (2016-07-08 01:19:48 UTC) #49
commit-bot: I haz the power
Committed patchset #15 (id:690001)
4 years, 5 months ago (2016-07-08 09:48:00 UTC) #51
commit-bot: I haz the power
4 years, 5 months ago (2016-07-08 09:49:28 UTC) #53
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/9c9a2b8dde4e141db7fb814f9ff338927741e7c1
Cr-Commit-Position: refs/heads/master@{#404340}

Powered by Google App Engine
This is Rietveld 408576698