Remove the migration code from content_settings::PrefProvider.

Remove the migration code from content_settings::PrefProvider.

This is a follow-up to https://codereview.chromium.org/1252973002/. The situation is equivalent:
1. We expect most users to have been migrated from the old preferences, as the migration code has been live for 3 milestones.
2. We can remove the media and cookies migration code, as they are older and the deprecated values could not have made it to the new preferences.

TESTED=Effects on websites in the regular mode and incognito, syncing between two machines

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

[msramek, 2015-07-27 17:27:50]

Patchset #1 (id:1) has been deleted

[msramek, 2015-07-27 17:53:00]

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

[msramek, 2015-07-27 17:53:01]

Hi Markus, Dominic, and Raymes,

this CL is in the same spirit as the one I sent out on Friday, only this time
it's with content settings exceptions.

PTAL.

Thanks,
Martin

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_model_associator.cc:327: if
(content_settings::PrefProvider::IsContentSettingsExceptionsPref(name)) {
This change should have happened back at the time when we introduced these
preferences, not just now when we do the clean-up of the old news. 

However, I didn't know that merging of the local and synced dictionary needs to
be handled explicitly. Fortunately, this would only be a problem if the user
added two exceptions at different machines at the same time; one of them would
be ignored. Unsurprisingly, this is a rare enough case that no one has noticed.

[raymes, 2015-07-28 01:09:15]

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:685:
prefs, prefs::kContentSettingsPluginsPatternPairs);
Who did this change to plugins from images? Is it because only plugins stores a
dictionary of settings?

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:690:
dummy_payload->SetInteger("setting", CONTENT_SETTING_ALLOW);
in this case is "setting" the plugin name?

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:726:
prefs::kContentSettingsCookiesPatternPairs);
Who did this change to cookies from images?

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_model_associator_unittest.cc (right):

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_model_associator_unittest.cc:31: content_type, new
base::FundamentalValue(setting));
I'm not sure this function accurately reflects how we store the content settings
pref anymore.

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

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:34:
// TODO(msramek): Remove the cleanup code after two releases (i.e. in M48).
It's really unfortunate that we need to do this! I wish there were a general way
to mark prefs as deprecated and then forget about them :(

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:241:
void PrefProvider::DiscardObsoletePreferences() {
Hmm, where is this called from?

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.h:96:
DISALLOW_COPY_AND_ASSIGN(PrefProvider);
nit: this should go at the end

[msramek, 2015-07-28 09:44:58]

Thanks, Raymes!

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:685:
prefs, prefs::kContentSettingsPluginsPatternPairs);
On 2015/07/28 01:09:14, raymes wrote:
> Who did this change to plugins from images? Is it because only plugins stores
a
> dictionary of settings?

There were a lot of tests about prefs::kContentSettingsPatternPairs, and since
we replaced it with one pref per content setting, I wanted to use a different
pref for each of the tests. This test should work with any of them, I just
already used images above.

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:690:
dummy_payload->SetInteger("setting", CONTENT_SETTING_ALLOW);
On 2015/07/28 01:09:14, raymes wrote:
> in this case is "setting" the plugin name?

No. The format for each of the per-type prefs is:

{ "<pattern1>": { 
    "setting": <value>,
    "per_resource": { "<resid-1>": <value>, ...},
    "last_used": <timestamp> },
  "<pattern2>": ...,
  ...
}

Only plugins use resource identifiers and only a few settings use the last usage
timestamp. The previous format, when all types were lumped together, was:

{ "<pattern1>": { 
    "cookies": <value>,
    "images": <value>,
    "plugins": <value>,
    ...
    "per_plugin": { "<resid-1>": <value>, ...},
    "last_used": {
      "notifications": <timestamp>,
      "geolocation": <timestamp>,
      ...
    },
  ...
}

So the value was previously keyed by the type name string, now it's always keyed
by the string "setting", as the type is implied by the pref used.

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:726:
prefs::kContentSettingsCookiesPatternPairs);
On 2015/07/28 01:09:14, raymes wrote:
> Who did this change to cookies from images?

As above. This must work with any content type. I'm just trying to diversify to
make sure that PrefProvider works not only for images :)

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_model_associator_unittest.cc (right):

https://codereview.chromium.org/1257093002/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_model_associator_unittest.cc:31: content_type, new
base::FundamentalValue(setting));
On 2015/07/28 01:09:14, raymes wrote:
> I'm not sure this function accurately reflects how we store the content
settings
> pref anymore.

Good catch! The dictionaries are actually simpler now, so I'm confident that if
the more complicated merge goes well, the simpler one will as well, but I
changed it to reflect the current state.

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

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:34:
// TODO(msramek): Remove the cleanup code after two releases (i.e. in M48).
On 2015/07/28 01:09:14, raymes wrote:
> It's really unfortunate that we need to do this! I wish there were a general
way
> to mark prefs as deprecated and then forget about them :(

Yep. In the meantime I found out that MigrateObsoleteProfilePrefs in
browser_prefs.cc is where people usually put their migration code, but most of
it is an actual migration, not just deletion. So we can add a generic
ClearObsoletePrefs there, where you would just add the name of the pref to an
array. But let me do that in a follow-up CL.

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:241:
void PrefProvider::DiscardObsoletePreferences() {
On 2015/07/28 01:09:14, raymes wrote:
> Hmm, where is this called from?

Oops! Added to the constructor.

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.h:96:
DISALLOW_COPY_AND_ASSIGN(PrefProvider);
On 2015/07/28 01:09:14, raymes wrote:
> nit: this should go at the end

Done.

[msramek, 2015-07-28 09:44:59]

Thanks, Raymes!

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.h:96:
DISALLOW_COPY_AND_ASSIGN(PrefProvider);
On 2015/07/28 01:09:14, raymes wrote:
> nit: this should go at the end

Done.

[raymes, 2015-07-29 02:39:14]

lgtm

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

https://codereview.chromium.org/1257093002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:34:
// TODO(msramek): Remove the cleanup code after two releases (i.e. in M48).
Sounds good!

[Bernhard Bauer, 2015-07-31 15:52:03]

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

[Bernhard Bauer, 2015-07-31 15:52:04]

Drive-by review! ☺

Also, description nit: "TESTED=" serves no formal purpose (I believe there was a
check at some point that a CL description should have a "TEST=" line, but even
that doesn't seem to exist anymore). If you want to describe how you have tested
this change (which is a good thing!), just use plain English.

https://codereview.chromium.org/1257093002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/1257093002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:48:
EXPECT_TRUE(pref_->lock_.Try());
Could you keep lock access in a method? If you really want to, make it private
and friend this class, but personally I would prefer making it public (and
potentially giving it a name that ends with "ForTesting").

[markusheintz_, 2015-08-03 08:35:53]

LGTM

https://codereview.chromium.org/1257093002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/1257093002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:48:
EXPECT_TRUE(pref_->lock_.Try());
On 2015/07/31 15:52:04, Bernhard Bauer wrote:
> Could you keep lock access in a method? If you really want to, make it private
> and friend this class, but personally I would prefer making it public (and
> potentially giving it a name that ends with "ForTesting").

+1

[msramek, 2015-08-03 10:52:10]

Patchset #4 (id:80001) has been deleted

[msramek, 2015-08-03 10:53:18]

https://codereview.chromium.org/1257093002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/1257093002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:48:
EXPECT_TRUE(pref_->lock_.Try());
On 2015/08/03 08:35:53, markusheintz_ wrote:
> On 2015/07/31 15:52:04, Bernhard Bauer wrote:
> > Could you keep lock access in a method? If you really want to, make it
private
> > and friend this class, but personally I would prefer making it public (and
> > potentially giving it a name that ends with "ForTesting").
> 
> +1

Done. Well, having it public at least helps to get rid of the friendships
(...that would sound harsh if it were real life :) ).

[Bernhard Bauer, 2015-08-03 10:55:27]

https://codereview.chromium.org/1257093002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/1257093002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:48:
EXPECT_TRUE(pref_->lock_.Try());
On 2015/08/03 10:53:18, msramek wrote:
> On 2015/08/03 08:35:53, markusheintz_ wrote:
> > On 2015/07/31 15:52:04, Bernhard Bauer wrote:
> > > Could you keep lock access in a method? If you really want to, make it
> private
> > > and friend this class, but personally I would prefer making it public (and
> > > potentially giving it a name that ends with "ForTesting").
> > 
> > +1
> 
> Done. Well, having it public at least helps to get rid of the friendships
> (...that would sound harsh if it were real life :) ).

😃

(Klingon code would probably have you list all the classes that are _not_
allowed to access private members as "enemy".)

[msramek, 2015-08-04 08:43:25]

Friendly ping. Dominic, can you have a look?

[battre, 2015-08-04 09:39:26]

lgtm

[msramek, 2015-08-04 10:56:26]

Thanks! Landing this now.

[msramek, 2015-08-04 10:56:28]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-08-04 10:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
markusheintz@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1257093002/#ps100001
(title: "Lock testing function.")

[commit-bot: I haz the power, 2015-08-04 10:56:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257093002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257093002/100001

[commit-bot: I haz the power, 2015-08-04 11:51:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-04 11:51:12]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msramek, 2015-08-04 13:35:46]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2015-08-04 13:36:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257093002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257093002/100001

[commit-bot: I haz the power, 2015-08-04 14:26:21]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-08-04 14:28:25]

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