Change ContentSettingsType's scoping type and hookup migration code

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}

[lshang, 2016-07-18 01:30:10]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 01:30:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-07-18 01:31:40]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-07-18 02:24:51]

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

[commit-bot: I haz the power, 2016-07-18 02:24:52]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-07-18 02:54:37]

Description was changed from

==========
Change ContentSettingsType's scoping type and hookup migration code


change scoping type

BUG=
==========

to

==========
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
==========

[lshang, 2016-07-18 02:58:29]

lshang@chromium.org changed reviewers:
+ raymes@chromium.org

[lshang, 2016-07-18 02:58:30]

PTAL thanks!

[raymes, 2016-07-19 01:48:49]

Looks good, but I think we should land
https://codereview.chromium.org/2158743002/ first if we can. Take a look at the
thoughts on that CL :)

https://codereview.chromium.org/2075103002/diff/60001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/2075103002/diff/60001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.h:289:
base::WeakPtr<HostContentSettingsMap> GetWeakPtr();
the order of these functions in the .cc file should match the order here

[lshang, 2016-07-20 08:05:24]

Patchset #4 (id:80001) has been deleted

[lshang, 2016-07-20 08:26:45]

Patchset #4 (id:100001) has been deleted

[lshang, 2016-07-20 08:33:32]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 08:33:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-07-20 08:36:01]

Thanks Raymes, I've made this CL to depend on
https://codereview.chromium.org/2158743002/.

https://codereview.chromium.org/2075103002/diff/60001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/2075103002/diff/60001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.h:289:
base::WeakPtr<HostContentSettingsMap> GetWeakPtr();
On 2016/07/19 01:48:49, raymes wrote:
> the order of these functions in the .cc file should match the order here

Done.

I put GetWeakPtr() behind MigrateDomainScopedSettings(), but I found that many
new added methods in .cc file are not in the same order as here, I might open a
separate CL to reorder them all later.

[commit-bot: I haz the power, 2016-07-20 09:06:59]

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

[commit-bot: I haz the power, 2016-07-20 09:07:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lshang, 2016-07-21 00:10:24]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 00:10:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 00:52:41]

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

[commit-bot: I haz the power, 2016-07-21 00:52:42]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-07-21 01:13:57]

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_factory.cc
(right):

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_factory.cc:82:
settings_map->GetWeakPtr(), true));
nit: we tend to document bools that we pass around to make it easier for people
reading the code. In this case:

true /* after_sync */));

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

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1360:
prefs->SetInteger(prefs::kDomainToOriginMigrationStatus, 0);
Maybe we can set it to the default value:
prefs->Set(..., *prefs->GetDefaultPrefValue(...));

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1469:
prefs->SetInteger(prefs::kDomainToOriginMigrationStatus, 0);
nit: this isn't needed. The test state should get reset across tests.

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1498:
prefs->SetInteger(prefs::kDomainToOriginMigrationStatus, 0);
nit: this isn't needed

https://codereview.chromium.org/2075103002/diff/140001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/2075103002/diff/140001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:190:
MigrateDomainScopedSettings(false);
false /* after_sync */

[lshang, 2016-07-22 02:58:31]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[lshang, 2016-07-22 02:58:38]

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_factory.cc
(right):

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
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: we tend to document bools that we pass around to make it easier for
people
> reading the code. In this case:
> 
> true /* after_sync */));

Done.

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

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1360:
prefs->SetInteger(prefs::kDomainToOriginMigrationStatus, 0);
On 2016/07/21 01:13:57, raymes wrote:
> Maybe we can set it to the default value:
> prefs->Set(..., *prefs->GetDefaultPrefValue(...));

Done.

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1469:
prefs->SetInteger(prefs::kDomainToOriginMigrationStatus, 0);
On 2016/07/21 01:13:57, raymes wrote:
> nit: this isn't needed. The test state should get reset across tests.

Done.

https://codereview.chromium.org/2075103002/diff/140001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1498:
prefs->SetInteger(prefs::kDomainToOriginMigrationStatus, 0);
On 2016/07/21 01:13:57, raymes wrote:
> nit: this isn't needed

Done.

https://codereview.chromium.org/2075103002/diff/140001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/2075103002/diff/140001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:190:
MigrateDomainScopedSettings(false);
On 2016/07/21 01:13:57, raymes wrote:
> false /* after_sync */

Done.

[commit-bot: I haz the power, 2016-07-22 02:59:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 03:52:21]

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

[commit-bot: I haz the power, 2016-07-22 03:52:22]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-07-25 02:18:27]

lgtm

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

https://codereview.chromium.org/2075103002/diff/160001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1359: //
Set the pref to its initial state.
nit: say why we are doing this.
// Set the pref to its initial state so that migration can be done later in the
test (normally it is done on construction of HostContentSettingsMap).

[lshang, 2016-07-25 04:55:41]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 04:55:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-07-25 04:56:42]

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

[lshang, 2016-07-25 04:56:43]

+msramek@, PTAL thanks!

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

https://codereview.chromium.org/2075103002/diff/160001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1359: //
Set the pref to its initial state.
On 2016/07/25 02:18:27, raymes wrote:
> nit: say why we are doing this.
> // Set the pref to its initial state so that migration can be done later in
the
> test (normally it is done on construction of HostContentSettingsMap).

Done.

[commit-bot: I haz the power, 2016-07-25 05:34:35]

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

[commit-bot: I haz the power, 2016-07-25 05:34:36]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-07-26 20:33:05]

I'd like you to re-add the part of the test that you removed (see my comment),
but otherwise LGTM so that you don't lose a day iteration.

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

https://codereview.chromium.org/2075103002/diff/180001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1478: //
Migration is done on construction of HostContentSettingsMap.
Can we test this claim?

We can write directly to the preference first, then create HCSM. See how it's
done in e.g. CanonicalizeExceptionsUnicodeOnly.

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

https://codereview.chromium.org/2075103002/diff/180001/components/content_set...
components/content_settings/core/browser/content_settings_registry.cc:126:
WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE,
I'm wondering if this is going to cause some strange behavior.

Cookie visibility is scoped to eTLD+1. Content settings didn't respect that
until now either, since we were setting cookie exceptions for whatever domain
you're on even if it's a third- or lower-level domain. Blocking cookies by
default and enabling them for mail.google.com would still block you from gmail
because of cookies from accounts.google.com.

But we're widening the discrepancy, so maybe we'll see this kind of problems
more often.

[lshang, 2016-07-27 06:00:04]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 06:00:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-07-27 06:05:44]

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

https://codereview.chromium.org/2075103002/diff/180001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1478: //
Migration is done on construction of HostContentSettingsMap.
On 2016/07/26 20:33:05, msramek wrote:
> Can we test this claim?
> 
> We can write directly to the preference first, then create HCSM. See how it's
> done in e.g. CanonicalizeExceptionsUnicodeOnly.

Done.

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

https://codereview.chromium.org/2075103002/diff/180001/components/content_set...
components/content_settings/core/browser/content_settings_registry.cc:126:
WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE,
On 2016/07/26 20:33:05, msramek wrote:
> I'm wondering if this is going to cause some strange behavior.
> 
> Cookie visibility is scoped to eTLD+1. Content settings didn't respect that
> until now either, since we were setting cookie exceptions for whatever domain
> you're on even if it's a third- or lower-level domain. Blocking cookies by
> default and enabling them for http://mail.google.com would still block you
from gmail
> because of cookies from http://accounts.google.com.
> 
> But we're widening the discrepancy, so maybe we'll see this kind of problems
> more often.

That's a good point. I think we can give it a try, if user complains a lot about
cookies then we can change its scoping type back.

[commit-bot: I haz the power, 2016-07-27 06:57:51]

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

[commit-bot: I haz the power, 2016-07-27 06:57:53]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-07-27 07:03:38]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-07-27 07:03:38]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2075103002/#ps200001
(title: "revise test")

[commit-bot: I haz the power, 2016-07-27 07:03:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 07:10:51]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-07-27 07:10:53]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-07-27 07:12:50]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-07-27 07:12:52]

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