Remove the migration code from content_settings::DefaultProvider.

Remove the migration code from content_settings::DefaultProvider.

The migration of default content settings from one dictionary pref into individual prefs appeared in M43. The next branchpoint is M46. After three versions, it is safe to remove the migration code.

Question: What happens to users who weren't migrated during this time (i.e. jump from M<43 to M>=46)?
Answer: Their content settings will be reset, since their individual prefs will not be populated. However, their profile won't be damaged; the old dictionary will be simply ignored. Thus, the user will have their settings restored to default values, as if in a new profile. For the more sensitive settings, the user will be asked before a permission is granted to the site; however, for cookies, images and javascript the default value is to allow websites to use them.

TESTED=Change in the settings UI was correctly reflected in the same window, in incognito window, and for synced settings also on a synced computer.

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

[msramek, 2015-07-24 13:48:26]

Patchset #1 (id:1) has been deleted

[msramek, 2015-07-24 13:53:58]

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

[msramek, 2015-07-24 13:53:58]

Hi Markus and Raymes,

it's time to start removing the content settings migration code. PTAL :)

Thanks,
Martin

[msramek, 2015-07-24 13:56:15]

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

[msramek, 2015-07-24 13:56:15]

+Dominic PTAL for chrome/browser/prefs

Thanks,
Martin

[msramek, 2015-07-24 15:53:37]

Note that we also remove the COOKIES, PROTECTED_MEDIA_IDENTIFIER, and
MEDIASTREAM value migration code.

All three migrations are pre-M43 branchpoint, so no one can have a deprecated
value written in the new individual default settings prefs.

[raymes, 2015-07-27 02:04:32]

lgtm

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(left):

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:115:
// content settings should be read from the host content settings map.
I don't really understand this comment or TODO but is it ok to remove it?

[battre, 2015-07-27 08:32:36]

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
File components/content_settings/core/common/pref_names.cc (left):

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
components/content_settings/core/common/pref_names.cc:34:
"profile.migrated_default_content_settings";
I think there is traditionally one more step:

Move this out of the public .../pref_names.{cc,h} files into
content_settings_default_provider.cc as kObsolete...; then add a function
DiscardObsoletePreferences that is called from
DefaultProvider::RegisterProfilePrefs and deletes the entries from the
preferences file so that it does not collect unused junk over time.

[markusheintz_, 2015-07-27 08:51:32]

https://codereview.chromium.org/1252973002/diff/60001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_functional_browsertest.cc (right):

https://codereview.chromium.org/1252973002/diff/60001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_functional_browsertest.cc:90:
browser()->profile()->GetPrefs()->SetInteger(prefs::kDefaultImagesSetting, 2);
Can you use the enum value that corresponds to the int 2 here or add a comment
saying which value 2 represents.

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(left):

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:115:
// content settings should be read from the host content settings map.
On 2015/07/27 02:04:32, raymes wrote:
> I don't really understand this comment or TODO but is it ok to remove it?

I guess this is comment is obsolete since quite some time now. Sorry for not
cleaning it up.

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:329:
NOTREACHED() << "Unexpected preference observed";
I think if you tweak the message to:
"A change of the preference "<< name <<" was observed, but the preference could
not be mapped to a content settings type."

It is more clear what happened.

[markusheintz_, 2015-07-27 08:54:41]

Please mention in you CL description (in the "Answer to your question") that
resetting the content setting is safe because it will not grant any permissions
to websites without asking the user first."

LGTM after addressing my comments. Please wait for Dominic to LGTM too.

[msramek, 2015-07-27 09:13:27]

https://codereview.chromium.org/1252973002/diff/60001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_functional_browsertest.cc (right):

https://codereview.chromium.org/1252973002/diff/60001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_functional_browsertest.cc:90:
browser()->profile()->GetPrefs()->SetInteger(prefs::kDefaultImagesSetting, 2);
On 2015/07/27 08:51:32, markusheintz_ wrote:
> Can you use the enum value that corresponds to the int 2 here or add a comment
> saying which value 2 represents.

Done.

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:329:
NOTREACHED() << "Unexpected preference observed";
On 2015/07/27 08:51:32, markusheintz_ wrote:
> I think if you tweak the message to:
> "A change of the preference "<< name <<" was observed, but the preference
could
> not be mapped to a content settings type."
> 
> It is more clear what happened.

Done.

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
File components/content_settings/core/common/pref_names.cc (left):

https://codereview.chromium.org/1252973002/diff/60001/components/content_sett...
components/content_settings/core/common/pref_names.cc:34:
"profile.migrated_default_content_settings";
On 2015/07/27 08:32:36, battre wrote:
> I think there is traditionally one more step:
> 
> Move this out of the public .../pref_names.{cc,h} files into
> content_settings_default_provider.cc as kObsolete...; then add a function
> DiscardObsoletePreferences that is called from
> DefaultProvider::RegisterProfilePrefs and deletes the entries from the
> preferences file so that it does not collect unused junk over time.

Done. But it cannot be called from static RegisterProfilePrefs, as it refers to
non-static member PrefService.

[msramek, 2015-07-27 09:14:43]

On 2015/07/27 08:54:41, markusheintz_ wrote:
> Please mention in you CL description (in the "Answer to your question") that
> resetting the content setting is safe because it will not grant any
permissions
> to websites without asking the user first."
> 
> LGTM after addressing my comments. Please wait for Dominic to LGTM too.

Except cookies, images and JS are ALLOW by default, so if you blocked those
before, they will be allowed again.

[battre, 2015-07-27 09:54:04]

lgtm

https://codereview.chromium.org/1252973002/diff/80001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1252973002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:31:
// Obsolete prefs to be removed from the pref file.
Can you add a TODO with a milestone when to remove this?

[msramek, 2015-07-27 15:10:52]

https://codereview.chromium.org/1252973002/diff/80001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1252973002/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:31:
// Obsolete prefs to be removed from the pref file.
On 2015/07/27 09:54:04, battre wrote:
> Can you add a TODO with a milestone when to remove this?

Done.

[msramek, 2015-07-27 15:10:55]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-07-27 15:10:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
markusheintz@chromium.org, raymes@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1252973002/#ps90001
(title: "Added a TODO.")

[commit-bot: I haz the power, 2015-07-27 15:11:17]

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

[commit-bot: I haz the power, 2015-07-27 15:13:57]

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

[commit-bot: I haz the power, 2015-07-27 15:13:58]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[msramek, 2015-07-27 15:19:52]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-07-27 15:19:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
markusheintz@chromium.org, raymes@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1252973002/#ps110001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-07-27 15:20:36]

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

[commit-bot: I haz the power, 2015-07-27 16:00:14]

Committed patchset #6 (id:110001)

[commit-bot: I haz the power, 2015-07-27 16:00:40]

Patchset 6 (id:??) landed as
https://crrev.com/9681220b472add22fd10d1034d667c2160c4247d
Cr-Commit-Position: refs/heads/master@{#340475}