Change HostContentSettingsMap::SetContentSetting to use GURLs instead of patterns

Change HostContentSettingsMap::SetContentSetting to use GURLs instead of patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

BUG=551747
Committed: https://crrev.com/48657c27ec55dc003cdad3e2472db8a3553ab3c4
Cr-Commit-Position: refs/heads/master@{#378960}

[lshang, 2016-02-15 02:20:18]

Description was changed from

==========
New version of HostContentSettingsMap::SetContentSetting which uses GURLs intead
of patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

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

to

==========
Change HostContentSettingsMap::SetContentSetting to use GURLs intead of patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

BUG=551747
==========

[lshang, 2016-02-15 03:48:03]

Patchset #8 (id:140001) has been deleted

[lshang, 2016-02-15 03:48:19]

Patchset #5 (id:80001) has been deleted

[lshang, 2016-02-15 03:48:31]

Patchset #6 (id:120001) has been deleted

[lshang, 2016-02-15 06:00:14]

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

[lshang, 2016-02-15 06:00:15]

PTAL thanks!

[lshang, 2016-02-17 04:37:04]

Description was changed from

==========
Change HostContentSettingsMap::SetContentSetting to use GURLs intead of patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

BUG=551747
==========

to

==========
Change HostContentSettingsMap::SetContentSetting to use GURLs instead of
patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

BUG=551747
==========

[lshang, 2016-02-18 04:59:40]

Patchset #2 (id:20001) has been deleted

[lshang, 2016-02-18 05:00:00]

Patchset #5 (id:160001) has been deleted

[lshang, 2016-02-18 05:00:14]

Patchset #3 (id:60001) has been deleted

[lshang, 2016-02-18 05:00:40]

Patchset #2 (id:40001) has been deleted

[lshang, 2016-03-01 02:51:53]

Hi Raymes, this is the up-to-date patch which includes
GetPatternsFromScopingType() and MigrateOldSetting(). PTAL thanks!

[raymes, 2016-03-01 06:17:32]

Hey Liu,

Could we split this up into several patches? 
-One adding the new SetContentSettingsDefaultScope
-One adding MigrateOldSettings
-One that updates the callsites in various places

Thanks!

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

https://codereview.chromium.org/1686343002/diff/260001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:110:
host_content_settings_map->SetContentSettingCustomScope(
Can we use the default scope in some of these cases?

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

https://codereview.chromium.org/1686343002/diff/260001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:497: void
HostContentSettingsMap::SetContentSettingCustomScope(
Maybe we shouldn't add this yet - instead we could just wait until we've changed
everything to SetContentSettingDefaultScope and then we can rename it to
"CustomScope"?

[lshang, 2016-03-02 04:02:28]

Raymes,

I moved MigrateOldSettings() to a separate CL(which is here:
https://codereview.chromium.org/1754073002/) and also moved changes about mic
and camera to a new CL.

I deleted CustomScope as well.

So this CL includes adding a new SetContentSettingDefaultScope() and changing
SetContentSetting() in some unit tests to demonstrate the usage of the new
method.
 
Could you please take another look? Thanks!:-)

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

https://codereview.chromium.org/1686343002/diff/260001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:110:
host_content_settings_map->SetContentSettingCustomScope(
On 2016/03/01 06:17:31, raymes wrote:
> Can we use the default scope in some of these cases?

Done.

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

https://codereview.chromium.org/1686343002/diff/260001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:497: void
HostContentSettingsMap::SetContentSettingCustomScope(
On 2016/03/01 06:17:32, raymes wrote:
> Maybe we shouldn't add this yet - instead we could just wait until we've
changed
> everything to SetContentSettingDefaultScope and then we can rename it to
> "CustomScope"?

Done.

[raymes, 2016-03-02 04:22:52]

Thanks Liu! Looks good, just a couple of comments

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

https://codereview.chromium.org/1686343002/diff/280001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:111:
struct PatternPair {
nit: it looks like there is already a content_settings::PatternPair defined
(https://code.google.com/p/chromium/codesearch#chromium/src/components/content...)
so maybe we should just use that instead

https://codereview.chromium.org/1686343002/diff/280001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1686343002/diff/280001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:144: //
SetWebsiteSettingDefaultScope() instead.
nit: Perhaps move this up to line 137 after the previous sentence

https://codereview.chromium.org/1686343002/diff/280001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:144: //
SetWebsiteSettingDefaultScope() instead.
nit: SetWebsiteSettingDefaultScope

[lshang, 2016-03-02 05:21:57]

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

https://codereview.chromium.org/1686343002/diff/280001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:111:
struct PatternPair {
On 2016/03/02 04:22:51, raymes wrote:
> nit: it looks like there is already a content_settings::PatternPair defined
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/content...)
> so maybe we should just use that instead

Done.
Good catch! Yeah I think we'd better use the existing one:-)

https://codereview.chromium.org/1686343002/diff/280001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1686343002/diff/280001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:144: //
SetWebsiteSettingDefaultScope() instead.
On 2016/03/02 04:22:51, raymes wrote:
> nit: Perhaps move this up to line 137 after the previous sentence

Done.

[raymes, 2016-03-03 01:38:19]

lgtm

https://codereview.chromium.org/1686343002/diff/300001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1686343002/diff/300001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:138: //
Unless adding a custom-scoped setting, most developers will want to use
nit: fill 80 chars on the previous line

[lshang, 2016-03-03 03:03:42]

Thanks Raymes!

https://codereview.chromium.org/1686343002/diff/300001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1686343002/diff/300001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:138: //
Unless adding a custom-scoped setting, most developers will want to use
On 2016/03/03 01:38:19, raymes wrote:
> nit: fill 80 chars on the previous line

Done.

[lshang, 2016-03-03 04:58:55]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-03-03 04:58:56]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1686343002/#ps320001
(title: "minor change in comments")

[commit-bot: I haz the power, 2016-03-03 04:59:07]

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

[commit-bot: I haz the power, 2016-03-03 05:08:15]

Description was changed from

==========
Change HostContentSettingsMap::SetContentSetting to use GURLs instead of
patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

BUG=551747
==========

to

==========
Change HostContentSettingsMap::SetContentSetting to use GURLs instead of
patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

BUG=551747
==========

[commit-bot: I haz the power, 2016-03-03 05:08:17]

Committed patchset #10 (id:320001)

[commit-bot: I haz the power, 2016-03-03 05:09:40]

Description was changed from

==========
Change HostContentSettingsMap::SetContentSetting to use GURLs instead of
patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

BUG=551747
==========

to

==========
Change HostContentSettingsMap::SetContentSetting to use GURLs instead of
patterns

A new version of SetContentSetting which takes GURLs as parameters. The patterns
are determined by WebsiteSettingsInfo::ScopingType from the ContentSettingsType.
Developers can directly pass in GURLs to decrease errors and inconsistency.

Will incrementally change all the callsites of SetContentSetting to use the new
version in the following CLs.

BUG=551747

Committed: https://crrev.com/48657c27ec55dc003cdad3e2472db8a3553ab3c4
Cr-Commit-Position: refs/heads/master@{#378960}
==========

[commit-bot: I haz the power, 2016-03-03 05:09:41]

Patchset 10 (id:??) landed as
https://crrev.com/48657c27ec55dc003cdad3e2472db8a3553ab3c4
Cr-Commit-Position: refs/heads/master@{#378960}