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

Issue 2158743002: Register a pref to control migration status (Closed)

Created:
4 years, 5 months ago by lshang
Modified:
4 years, 4 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@change_scoping_type
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Register a pref to control migration status Domain scoped content settings are migrated to be origin scoped, but the migration process is expected to run only once. In this CL, a pref is registered to control this, which has three states: 1. not migrated, which means the migration process has not been started yet. 2. migrated in HostContentSettingsMap(HCSM), which will not do migration again in HCSM, but can do in PrefModelAssociator. 3. migrated in PrefModelAssociator, which means the migration is all finished and won't get called again. BUG=604612 Committed: https://crrev.com/e8d505658c6c05c87bc2f9adfb0a8f27b8ba94f8 Cr-Commit-Position: refs/heads/master@{#408064}

Patch Set 1 #

Patch Set 2 : add some comments #

Total comments: 12

Patch Set 3 : add test #

Patch Set 4 : add some comments to test #

Total comments: 8

Patch Set 5 : address review comments #

Total comments: 12

Patch Set 6 : minor change #

Total comments: 6

Patch Set 7 : address review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+155 lines, -5 lines) Patch
M chrome/browser/content_settings/host_content_settings_map_unittest.cc View 1 2 3 4 5 2 chunks +101 lines, -1 line 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.h View 1 2 3 4 5 6 2 chunks +10 lines, -3 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.cc View 1 2 3 4 5 6 4 chunks +37 lines, -1 line 0 comments Download
M components/content_settings/core/common/pref_names.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/common/pref_names.cc View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 45 (31 generated)
lshang
PTAL thanks!
4 years, 5 months ago (2016-07-18 06:43:33 UTC) #7
raymes
Thanks Liu! A couple of thoughts: 1) Could we land this before we land https://codereview.chromium.org/2075103002/ ...
4 years, 5 months ago (2016-07-19 01:48:15 UTC) #8
lshang
https://codereview.chromium.org/2158743002/diff/20001/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/2158743002/diff/20001/chrome/browser/content_settings/host_content_settings_map_factory.cc#newcode87 chrome/browser/content_settings/host_content_settings_map_factory.cc:87: content_settings::DONE_IN_HCSM_AND_SYNC); On 2016/07/19 01:48:15, raymes wrote: > Instead of ...
4 years, 5 months ago (2016-07-19 07:45:56 UTC) #9
lshang
Thanks Raymes! https://codereview.chromium.org/2158743002/diff/20001/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/2158743002/diff/20001/chrome/browser/content_settings/host_content_settings_map_factory.cc#newcode87 chrome/browser/content_settings/host_content_settings_map_factory.cc:87: content_settings::DONE_IN_HCSM_AND_SYNC); On 2016/07/19 01:48:15, raymes wrote: > ...
4 years, 5 months ago (2016-07-20 06:16:21 UTC) #16
lshang
On 2016/07/19 01:48:15, raymes wrote: > Thanks Liu! > > A couple of thoughts: > ...
4 years, 5 months ago (2016-07-20 06:17:31 UTC) #17
raymes
Thanks Liu! https://codereview.chromium.org/2158743002/diff/100001/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/2158743002/diff/100001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1480 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1480: EXPECT_EQ(1, prefs->GetInteger(prefs::kDomainToOriginMigrationStatus)); Rather than checking the value ...
4 years, 5 months ago (2016-07-21 00:58:54 UTC) #20
lshang
https://codereview.chromium.org/2158743002/diff/100001/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/2158743002/diff/100001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1480 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1480: EXPECT_EQ(1, prefs->GetInteger(prefs::kDomainToOriginMigrationStatus)); On 2016/07/21 00:58:54, raymes wrote: > Rather ...
4 years, 5 months ago (2016-07-22 01:33:07 UTC) #21
raymes
lgtm https://codereview.chromium.org/2158743002/diff/120001/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/2158743002/diff/120001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1467 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1467: TEST_F(HostContentSettingsMapTest, DomainToOriginMigrationStatus) { nit: add a comment explaining ...
4 years, 4 months ago (2016-07-25 03:56:39 UTC) #26
lshang
Also +msramek@ for review. PTAL thanks! https://codereview.chromium.org/2158743002/diff/120001/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/2158743002/diff/120001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode1467 chrome/browser/content_settings/host_content_settings_map_unittest.cc:1467: TEST_F(HostContentSettingsMapTest, DomainToOriginMigrationStatus) { ...
4 years, 4 months ago (2016-07-25 04:51:10 UTC) #30
msramek
LGTM with optional nits. https://codereview.chromium.org/2158743002/diff/140001/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/2158743002/diff/140001/components/content_settings/core/browser/host_content_settings_map.cc#newcode520 components/content_settings/core/browser/host_content_settings_map.cc:520: int status = prefs_->GetInteger(prefs::kDomainToOriginMigrationStatus); nit: ...
4 years, 4 months ago (2016-07-26 19:21:03 UTC) #33
lshang
Thanks Martin! https://codereview.chromium.org/2158743002/diff/140001/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/2158743002/diff/140001/components/content_settings/core/browser/host_content_settings_map.cc#newcode520 components/content_settings/core/browser/host_content_settings_map.cc:520: int status = prefs_->GetInteger(prefs::kDomainToOriginMigrationStatus); On 2016/07/26 19:21:02, ...
4 years, 4 months ago (2016-07-27 00:23:47 UTC) #36
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/2158743002/160001
4 years, 4 months ago (2016-07-27 06:58:49 UTC) #41
commit-bot: I haz the power
Committed patchset #7 (id:160001)
4 years, 4 months ago (2016-07-27 07:02:35 UTC) #43
commit-bot: I haz the power
4 years, 4 months ago (2016-07-27 07:04:02 UTC) #45
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/e8d505658c6c05c87bc2f9adfb0a8f27b8ba94f8
Cr-Commit-Position: refs/heads/master@{#408064}

Powered by Google App Engine
This is Rietveld 408576698