Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(48)

Issue 2075103002: Change ContentSettingsType's scoping type and hookup migration code (Closed)

Created:
3 years, 1 month ago by lshang
Modified:
2 years, 11 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@do_migration_after_sync
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Change ContentSettingsType's scoping type and hookup migration code This is a follow up CL of https://codereview.chromium.org/1895993003/ and https://codereview.chromium.org/2078893002/. In this CL, the scoping type of content setting types are changed to origin scoped so that new settings generated for these types affect origins only. Also migration code is brought to take effect to deal with old domain scoped settings. BUG=604612 Committed: https://crrev.com/ca0ff0335dfc0552e5ba76c820a92f506c4baf23 Cr-Commit-Position: refs/heads/master@{#408067}

Patch Set 1 #

Patch Set 2 : use weakptr #

Patch Set 3 : rebase #

Total comments: 2

Patch Set 4 : format #

Patch Set 5 : revise test #

Total comments: 10

Patch Set 6 : address comments #

Total comments: 2

Patch Set 7 : minor change #

Total comments: 4

Patch Set 8 : revise test #

Messages

Total messages: 46 (34 generated)
lshang
PTAL thanks!
3 years ago (2016-07-18 02:58:30 UTC) #8
raymes
Looks good, but I think we should land https://codereview.chromium.org/2158743002/ first if we can. Take a ...
3 years ago (2016-07-19 01:48:49 UTC) #9
lshang
Thanks Raymes, I've made this CL to depend on https://codereview.chromium.org/2158743002/. https://codereview.chromium.org/2075103002/diff/60001/components/content_settings/core/browser/host_content_settings_map.h File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2075103002/diff/60001/components/content_settings/core/browser/host_content_settings_map.h#newcode289 ...
2 years, 12 months ago (2016-07-20 08:36:01 UTC) #14
raymes
https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content_settings/host_content_settings_map_factory.cc File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content_settings/host_content_settings_map_factory.cc#newcode82 chrome/browser/content_settings/host_content_settings_map_factory.cc:82: settings_map->GetWeakPtr(), true)); nit: we tend to document bools that ...
2 years, 12 months ago (2016-07-21 01:13:57 UTC) #21
lshang
https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content_settings/host_content_settings_map_factory.cc File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content_settings/host_content_settings_map_factory.cc#newcode82 chrome/browser/content_settings/host_content_settings_map_factory.cc:82: settings_map->GetWeakPtr(), true)); On 2016/07/21 01:13:57, raymes wrote: > nit: ...
2 years, 12 months ago (2016-07-22 02:58:38 UTC) #23
raymes
lgtm https://codereview.chromium.org/2075103002/diff/160001/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/2075103002/diff/160001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1359 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1359: // Set the pref to its initial state. ...
2 years, 12 months ago (2016-07-25 02:18:27 UTC) #27
lshang
+msramek@, PTAL thanks! https://codereview.chromium.org/2075103002/diff/160001/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/2075103002/diff/160001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1359 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1359: // Set the pref to its ...
2 years, 12 months ago (2016-07-25 04:56:43 UTC) #31
msramek
I'd like you to re-add the part of the test that you removed (see my ...
2 years, 11 months ago (2016-07-26 20:33:05 UTC) #34
lshang
https://codereview.chromium.org/2075103002/diff/180001/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/2075103002/diff/180001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1478 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1478: // Migration is done on construction of HostContentSettingsMap. On ...
2 years, 11 months ago (2016-07-27 06:05:44 UTC) #37
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/2075103002/200001
2 years, 11 months ago (2016-07-27 07:03:53 UTC) #42
commit-bot: I haz the power
Committed patchset #8 (id:200001)
2 years, 11 months ago (2016-07-27 07:10:53 UTC) #44
commit-bot: I haz the power
2 years, 11 months ago (2016-07-27 07:12:52 UTC) #46
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/ca0ff0335dfc0552e5ba76c820a92f506c4baf23
Cr-Commit-Position: refs/heads/master@{#408067}

Powered by Google App Engine
This is Rietveld 408576698