Split the aggregate dictionary of content settings exceptions into per-type dictionaries

Split the aggregate dictionary that contains content settings exceptions for all content types into separate dictionary holding one content type each.

Old dictionary entry example:

"www.google.com,*": {"cookies": <value>, "plugins": <value>, last_used: {"cookies": <timestamp>, "plugins": <timestamp>}, "per_plugin": {<resource_id>: <value>}}

Split into separate dictionaries:
cookies:
"www.google.com,*": {"setting": <value>, "last_used": <timestamp>}

plugins:
"www.google.com,*": {"setting": <value>, "last_used": <timestamp>, "per_resource": {<resource_id>: <value>}}

For migration between versions of Chrome, we use a similar logic as when we migrated the default preferences in https://codereview.chromium.org/1004733003/ .

1. On the first run, migrate all settings (PrefProvider::MigrateAllExceptions()).
2. Whenever the old dictionary preference changes, propagate the syncable entries into the new preferences (PrefProvider::OnOldContentSettingsPatternPairsChanged()).
3. Whenever one of the new preferences changes, and it is syncable, we write it back to the old dictionary preference as well (ContentSettingsPref::OnPrefChanged()).

Note that PrefProvider manages its old dictionary preference the same way as ContentSettingsPref does a new preference, and ContentSettingsPref needs access to the old preference as well, which leads to some duplication in the code (UpdateOldPref() is just a modified version of UpdatePref(); both use some of the same constructs). We use the convention of referring to attributes and methods managing the old dictionary preference as "old" in the name.

BUG=452388
Committed: https://crrev.com/c2bda48205f8246990caf262f87f52dd971c78a5
Cr-Commit-Position: refs/heads/master@{#323585}

[msramek, 2015-03-24 15:11:44]

msramek@chromium.org changed reviewers:
+ markusheintz@chromium.org, raymes@chromium.org

[msramek, 2015-03-24 15:11:45]

Hello Markus and Raymes,

I uploaded my CL on splitting exceptions. It currently works, but I want to
polish the design a bit more (I don't like the repetition between old-dictionary
and new-dictionary methods) and also make it faster (see my comments).

I didn't correctly anticipate some things when reviewing Raymes' CL, so I might
have moved some things around or even duplicated them. But it helped a lot to
work on top of the refactoring that Raymes did, so thanks for that!

Anyway, it's still WIP, but PTAL and comment on the design.

Thanks,
Martin

https://codereview.chromium.org/1005303003/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_pref.cc:152: void
ContentSettingsPref::ClearAllContentSettingsRules() {
I just noticed this: There doesn't seem to be a good reason why we remove the
rules one by one. We should just clear value_map_ and write empty dictionary to
the preference file.

https://codereview.chromium.org/1005303003/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_pref_provider.cc:438:
for (size_t i = 0; i < entries.size(); ++i) {
This for-cycle migrates exceptions one by one. Unless PrefService caches files,
this means a disk access for every exception, which might be very slow if you
have a lot of them.

It works as it is, but I think we should migrate the exceptions in bulk, by
reading the old dictionary, processing it in memory and then writing whole
dictionaries back to the pref file.

https://codereview.chromium.org/1005303003/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings.cc (right):

https://codereview.chromium.org/1005303003/diff/1/components/content_settings...
components/content_settings/core/common/content_settings.cc:76: bool
IsContentSettingsTypeSyncable(ContentSettingsType content_setting) {
Somehow this didn't come to my mind when doing the split in DefaultProvider.
I'll rewrite kDefaultSettings in DefaultProvider in a separate CL.

[msramek, 2015-03-25 11:58:50]

https://codereview.chromium.org/1005303003/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_pref_provider.cc:438:
for (size_t i = 0; i < entries.size(); ++i) {
On 2015/03/24 15:11:45, msramek wrote:
> This for-cycle migrates exceptions one by one. Unless PrefService caches
files,
> this means a disk access for every exception, which might be very slow if you
> have a lot of them.
> 
> It works as it is, but I think we should migrate the exceptions in bulk, by
> reading the old dictionary, processing it in memory and then writing whole
> dictionaries back to the pref file.

I tried this out with 1000 exceptions and the migration is pretty fast; I guess
PrefService does cache the preferences. Then again, the whole "Manage
exceptions" dialog is pretty slow, even before this change was applied.

[raymes, 2015-03-26 06:51:10]

Hey Martin, sorry I haven't gotten around to looking at this but I should have
time tomorrow or Monday. Thanks!

[msramek, 2015-03-26 18:27:38]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org

[msramek, 2015-03-26 18:27:39]

No problem, Raymes.

+Bernhard who is probably interested too.


Side note: Tested on Linux, the migration seems to work correctly. The previous
patchset didn't compile on Android though, this one doesn't compile on Mac.
Apparently copy constructors behave differently on every platform. Let me have a
look at it again :/

https://codereview.chromium.org/1005303003/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:324:
void PrefProvider::ReadContentSettingsFromOldPref(
So this whole method is basically duplicated from ContentSettingsPref (where it
got in Raymes' CL), with the difference that in ContentSettingsPref it was
slightly changed to match the new structure of the dictionary.

https://codereview.chromium.org/1005303003/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:440:
base::AutoLock auto_lock(old_lock_);
I readded the lock here to make sure that the migration of preferences is an
atomic operation.

[msramek, 2015-03-31 18:11:10]

Patchset #16 (id:300001) has been deleted

[raymes, 2015-04-01 06:51:21]

Hey Martin, I've started looking at this. I'm not finished but here are some
comments :)

https://codereview.chromium.org/1005303003/diff/180001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1005303003/diff/180001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:390:
LOG(ERROR) << "About to clear.";
nit: Are you intentionally adding this?

https://codereview.chromium.org/1005303003/diff/180001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/180001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:41: //
Otherwise, returns false.
nit: this comment is out of date now

https://codereview.chromium.org/1005303003/diff/180001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.h (right):

https://codereview.chromium.org/1005303003/diff/180001/components/content_set...
components/content_settings/core/browser/content_settings_pref.h:73: // so that
we reduce duplicity between the two.
nit: reduce duplication

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:130: if
(IsContentSettingsTypeSyncable(content_type_))
nit: add {}

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:169: if
(IsContentSettingsTypeSyncable(content_type_))
nit: {}

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:260:
ClearOldPreference();
Could you explain this a bit more? I'm not sure what it does.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:318:
setting_ptr.get());
Won't this take ownership of the value (it appears to in the old code)?

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:361: if
(content_type_ == CONTENT_SETTINGS_TYPE_COOKIES) {
At some point it could be worth subclassing this class to make a specific
version for cookies which contains this logic. Don't have to do it in this CL
though.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:474: }
The code here is so similar to UpdateOldPref. Is it worth combining them into a
function?

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
File components/content_settings/core/common/content_settings.cc (right):

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/common/content_settings.cc:109: default:
nit: do we need "default"? can we just put 
NOTREACHED();
return false;

outside the if-block

[msramek, 2015-04-01 09:34:26]

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:130: if
(IsContentSettingsTypeSyncable(content_type_))
On 2015/04/01 06:51:20, raymes wrote:
> nit: add {}

Done. Here and elsewhere.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:169: if
(IsContentSettingsTypeSyncable(content_type_))
On 2015/04/01 06:51:20, raymes wrote:
> nit: {}

Done.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:260:
ClearOldPreference();
On 2015/04/01 06:51:20, raymes wrote:
> Could you explain this a bit more? I'm not sure what it does.

I should probably rename this function. What it originally did was:

- read the preference
- write the exceptions into the value map

What it does now is

- read the new preference
- write the exceptions into the value map
- write the exceptions back into the old preference

As you can see below in this method, there are calls of UpdateOldPref() - this
adds an exception to the old preference. Thus, before we start writing the
exceptions from the new preference to the old preference, we must first clear
the old preference.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:318:
setting_ptr.get());
On 2015/04/01 06:51:20, raymes wrote:
> Won't this take ownership of the value (it appears to in the old code)?

Thanks! Fixed here and below.

I want to keep the order of putting the value to the map first and then calling
UpdateOldPref; since the value is already taken at that point, I'd keep it in
the scoped_ptr.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:361: if
(content_type_ == CONTENT_SETTINGS_TYPE_COOKIES) {
On 2015/04/01 06:51:20, raymes wrote:
> At some point it could be worth subclassing this class to make a specific
> version for cookies which contains this logic. Don't have to do it in this CL
> though.

Yes. The resource identifier logic is also currently only used by one setting.

Although in this case it would be actually nicer to have histograms for every
setting :)

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:474: }
On 2015/04/01 06:51:20, raymes wrote:
> The code here is so similar to UpdateOldPref. Is it worth combining them into
a
> function?

The differences are
- different DCHECKs
- one uses lock, one doesn't
- they use different flags
- they use different resource identifier strings
- they use different keyword for the setting itself
- and of course, the dictionary structure is slightly different

I'll see what it does if I put them in one function, but it will be full of
ternary operators.

My reason to keep them separate is that UpdateOldPref() is just a copy-pasted
code from the OldPrefProvider, which doesn't need to be maintained - since it
works with a deprecated pref. Any further changes only need to be done in
UpdatePref(). After two stable releases, we can just cut UpdateOldPref() out.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
File components/content_settings/core/common/content_settings.cc (right):

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/common/content_settings.cc:109: default:
On 2015/04/01 06:51:20, raymes wrote:
> nit: do we need "default"? can we just put 
> NOTREACHED();
> return false;
> 
> outside the if-block

The compiler requires every option to be represented in the switch. We have
DEFAULT and NUM_TYPES left, and I considered it nicer to just lump them
together.

But now that I'm thinking about it, adding a new content settings will cause a
runtime error here (NOTREACHED()) as opposed to compile time error (missing
option in the switch), so it's probably better to be explicit.

https://codereview.chromium.org/1005303003/diff/340001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/1005303003/diff/340001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.h:127:
ScopedVector<ContentSettingsPrefEntry>
ScopedVector is the best solution here, which I didn't know about until
recently. However, we cannot have a map of ScopedVectors, and we don't have a
ScopedMap. To prevent adding further boilerplate code of copy constructors,
let's just have a simple array here. It's not a good practice to define a
const-size array type, so let's just have it as an attribute. The only access to
this map was protected by a lock anyway, so we don't need more than one.

[markusheintz_, 2015-04-01 10:02:52]

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:169: if
(IsContentSettingsTypeSyncable(content_type_))
Please add { }

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:34:
// returns true and sets |pref_key| to the key in the content settings
nit: move this "Returns true and set ..." part to the beginning of the sentence
and the "if the given content type" to the end.

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:36:
// Otherwise, returns false.
nit: you can remove this line

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:46:
ContentSetting FixObsoleteCookiePromptMode(ContentSettingsType content_type,
I wonder if we still need this?

If you are unsure too please add a TODO to check and eventually remove it.

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:66:
const char* kContentSettingsExceptionsPrefs[] = {
Maybe add a comment that the order matters and has to match the order of the
enum ContentSettingsType

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:122:
for (int i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i)
Please add { }. I know for loop header is followed by only a single command. But
this requires six lines :-)

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:191:
resource_identifier,
bad indentation

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:216:
primary_pattern,
bad indentation.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:260:
ClearOldPreference();
On 2015/04/01 06:51:20, raymes wrote:
> Could you explain this a bit more? I'm not sure what it does.

Please add a comment. Does the migration work if we start the browser in
incognito mode?

[msramek, 2015-04-01 10:53:35]

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:169: if
(IsContentSettingsTypeSyncable(content_type_))
On 2015/04/01 10:02:51, markusheintz_ wrote:
> Please add { }

Done.

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:34:
// returns true and sets |pref_key| to the key in the content settings
On 2015/04/01 10:02:51, markusheintz_ wrote:
> nit: move this "Returns true and set ..." part to the beginning of the
sentence
> and the "if the given content type" to the end.

Done. (This text is actually just moved here from ContentSettingsPref, not
really a part of this CL).

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:36:
// Otherwise, returns false.
On 2015/04/01 10:02:51, markusheintz_ wrote:
> nit: you can remove this line

Done.

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:46:
ContentSetting FixObsoleteCookiePromptMode(ContentSettingsType content_type,
On 2015/04/01 10:02:51, markusheintz_ wrote:
> I wonder if we still need this?
> 
> If you are unsure too please add a TODO to check and eventually remove it.

It looks like a very old migration code. I believe we don't need it anymore, but
I am not sure either.

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:66:
const char* kContentSettingsExceptionsPrefs[] = {
On 2015/04/01 10:02:51, markusheintz_ wrote:
> Maybe add a comment that the order matters and has to match the order of the
> enum ContentSettingsType

Done.

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:122:
for (int i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i)
On 2015/04/01 10:02:51, markusheintz_ wrote:
> Please add { }. I know for loop header is followed by only a single command.
But
> this requires six lines :-)

Done.

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:191:
resource_identifier,
On 2015/04/01 10:02:51, markusheintz_ wrote:
> bad indentation

Done.

https://codereview.chromium.org/1005303003/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:216:
primary_pattern,
On 2015/04/01 10:02:51, markusheintz_ wrote:
> bad indentation.

Done.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:260:
ClearOldPreference();
On 2015/04/01 10:02:51, markusheintz_ wrote:
> On 2015/04/01 06:51:20, raymes wrote:
> > Could you explain this a bit more? I'm not sure what it does.
> 
> Please add a comment. Does the migration work if we start the browser in
> incognito mode?

Done.

Yes. I checked that in the incognito mode (even if you launch Chrome directly in
incognito using a commandline flag) we create 2 HostContentSettingsMaps - one
normal, one for incognito. It follows that two PrefProviders are created as
well. Thus, the one that is not incognito will do the migration.

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
File components/content_settings/core/common/content_settings.cc (right):

https://codereview.chromium.org/1005303003/diff/320001/components/content_set...
components/content_settings/core/common/content_settings.cc:109: default:
On 2015/04/01 09:34:26, msramek wrote:
> On 2015/04/01 06:51:20, raymes wrote:
> > nit: do we need "default"? can we just put 
> > NOTREACHED();
> > return false;
> > 
> > outside the if-block
> 
> The compiler requires every option to be represented in the switch. We have
> DEFAULT and NUM_TYPES left, and I considered it nicer to just lump them
> together.
> 
> But now that I'm thinking about it, adding a new content settings will cause a
> runtime error here (NOTREACHED()) as opposed to compile time error (missing
> option in the switch), so it's probably better to be explicit.

Oh yes, but now trybots can't see that all options are really there and that
it's not possible not to reach "return".

[msramek, 2015-04-01 16:25:30]

Hello again Markus and Raymes,

I did some manual testing today. Syncing between an old and new version of
Chrome works fine.

There was a bug in the migration where we copied new->old sooner than old->new
(funny thing is that this didn't happen in the earlier patchests...), which I
fixed.

Assuming the tests are green, I would like to land tomorrow to have it in before
branchpoint. PTAL :)

P.S.: I'm planning to spend the next week polishing details and improving speed,
but in this CL, I just want to make it work.

[raymes, 2015-04-02 00:22:09]

Thanks Martin! I just have to review the unittests. 

The migration logic is unfortunate and complex and I think it's good that you're
testing it thoroughly before landing. The code will be much cleaner when we can
remove it.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:339:
setting_ptr.get());
I think you might want setting_ptr.release() here otherwise the scoped_ptr will
try to free the pointer after ownership has been passed.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:589: for
(const std::string key : keys) {
nit: can you use const std::string& key : keys?

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:169:
ReadContentSettingsFromOldPref();
Why is it necessary to do this after we have migrated?

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:499:
pref_entry_map_[content_type][i]->value->DeepCopy());
nit: you could probably save a copy here by releasing the value and passing
ownership since you know the entry won't be reused

[markusheintz_, 2015-04-02 11:04:49]

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:153:
pref_change_registrar_.Add(prefs::kContentSettingsPatternPairs, base::Bind(
Do you need to register the pref change listener before migrating the prefs?

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:213:
if (primary_pattern == ContentSettingsPattern::Wildcard() &&
Is it possible that the content_settings_pref handles this?

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:517:
void PrefProvider::MigrateAllExceptions() {
Looks like this method can be in-lined.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.h:127:
ScopedVector<ContentSettingsPrefEntry>
Please add comments to the fields here and below.

[markusheintz_, 2015-04-02 13:19:11]

Please look at the locks as discussed offline

[markusheintz_, 2015-04-02 15:52:26]

LGTM after fixing the remaining comments.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:213:
if (primary_pattern == ContentSettingsPattern::Wildcard() &&
On 2015/04/02 11:04:48, markusheintz_ wrote:
> Is it possible that the content_settings_pref handles this?

nit: Let's fix this later as discussed offline

[msramek, 2015-04-02 16:22:55]

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:339:
setting_ptr.get());
On 2015/04/02 00:22:09, raymes wrote:
> I think you might want setting_ptr.release() here otherwise the scoped_ptr
will
> try to free the pointer after ownership has been passed.

UpdateOldPref() doesn't take ownership though. So we want to free value after it
was used.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:589: for
(const std::string key : keys) {
On 2015/04/02 00:22:09, raymes wrote:
> nit: can you use const std::string& key : keys?

Done.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:153:
pref_change_registrar_.Add(prefs::kContentSettingsPatternPairs, base::Bind(
On 2015/04/02 11:04:49, markusheintz_ wrote:
> Do you need to register the pref change listener before migrating the prefs?

No. I felt that it belongs here, since this is where we also register the other
listeners (in the constructors of ContentSettingsPrefs below).

Note that that this is not a problem, as callbacks triggered from the migration
/ initialization will return immediately when checking
|updating_old_preference|.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:169:
ReadContentSettingsFromOldPref();
On 2015/04/02 00:22:09, raymes wrote:
> Why is it necessary to do this after we have migrated?

Migration only happens once. The other one needs to happen always (although it
doesn't need to happen in that one particular run when we migrate). As Markus
suggested below, let's inline this.

It's also better to use OnOldContentSettingsPatternPairsChanged() here, to
trigger not only read, but also write to new preferences.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:213:
if (primary_pattern == ContentSettingsPattern::Wildcard() &&
On 2015/04/02 15:52:26, markusheintz_ wrote:
> On 2015/04/02 11:04:48, markusheintz_ wrote:
> > Is it possible that the content_settings_pref handles this?
> 
> nit: Let's fix this later as discussed offline

Acknowledged.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:499:
pref_entry_map_[content_type][i]->value->DeepCopy());
On 2015/04/02 00:22:09, raymes wrote:
> nit: you could probably save a copy here by releasing the value and passing
> ownership since you know the entry won't be reused

Done.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:517:
void PrefProvider::MigrateAllExceptions() {
On 2015/04/02 11:04:48, markusheintz_ wrote:
> Looks like this method can be in-lined.

Done.

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/1005303003/diff/420001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.h:127:
ScopedVector<ContentSettingsPrefEntry>
On 2015/04/02 11:04:49, markusheintz_ wrote:
> Please add comments to the fields here and below.

Done.

[msramek, 2015-04-02 16:23:48]

On 2015/04/02 13:19:11, markusheintz_ wrote:
> Please look at the locks as discussed offline

As discussed offline again - the pref change callbacks are all on UI thread, so
no additional locking should be needed.

[msramek, 2015-04-02 18:50:11]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-04-02 18:50:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/1005303003/#ps500001
(title: "Verbatim copying test.")

[commit-bot: I haz the power, 2015-04-02 18:51:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1005303003/500001

[msramek, 2015-04-02 19:09:40]

I tested the migration (with and without sync on), syncing, whether the
migration can happen in incognito, and whether the separation between normal and
incognito value maps is still correct. The CL should be ready to land now.

Markus, thanks again for all your help today!

Raymes, sorry that I didn't wait for your LGTM, but this should be in before
branch point. If you spot any issues, I'll make sure to merge them in. I will
also do another round of testing next week.

[msramek, 2015-04-02 19:48:00]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-04-02 19:48:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/1005303003/#ps520001
(title: "Memory leak in last test.")

[commit-bot: I haz the power, 2015-04-02 19:51:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1005303003/520001

[commit-bot: I haz the power, 2015-04-03 04:57:27]

Committed patchset #26 (id:520001)

[brucedawson, 2015-04-03 19:55:51]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-04-03 19:55:53]

https://codereview.chromium.org/1005303003/diff/520001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/520001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:409:
for (size_t i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) {
The VC++ /analyze builder pointed out that this loop is confusing. The loop
variable 'i' shadows the loop variable 'i' of the outer loop. There is no bug,
but there is potential for confusion among readers.

If you decide to fix this, crbug.com/427616 is the tracking bug for
/analyze-found issues, .

[commit-bot: I haz the power, 2015-04-03 20:30:11]

Patchset 26 (id:??) landed as
https://crrev.com/c2bda48205f8246990caf262f87f52dd971c78a5
Cr-Commit-Position: refs/heads/master@{#323585}

[msramek, 2015-04-03 22:13:49]

https://codereview.chromium.org/1005303003/diff/520001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/520001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:409:
for (size_t i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) {
On 2015/04/03 19:55:52, brucedawson wrote:
> The VC++ /analyze builder pointed out that this loop is confusing. The loop
> variable 'i' shadows the loop variable 'i' of the outer loop. There is no bug,
> but there is potential for confusion among readers.
> 
> If you decide to fix this, crbug.com/427616 is the tracking bug for
> /analyze-found issues, .

Thanks a lot for pointing this out! Apart from small changes, this is not my
code (just copied over), so I didn't notice. I can confirm that this is not a
bug, but it's ugly and I'll fix it.

(Also, I just learned about "-Wshadow")

[brucedawson, 2015-04-06 17:36:56]

I dream of a day when we can build with -Wshadow, but that day is a long way
off. Until then, I'll continue reporting the interesting new shadowing
instances.

And if anybody wants to turn -Wshadow on for sub-projects that would be great...

[raymes, 2015-04-07 01:55:56]

lgtm!

https://codereview.chromium.org/1005303003/diff/520001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/1005303003/diff/520001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:188: 
nit: unneeded extra line