Add a supervised user setting to allow all cookies.

Add a supervised user setting to allow all cookies.

This prevents inexperienced users from creating website compatibility
problems by blocking cookies.

As this is the first supervised user content setting that is ALLOW
rather than BLOCK, rename content_settings::BinaryValueMap to
GlobalValueMap and add support for arbitrary content settings.

Also some cleanup, most importantly:
* Remove the AutoLock from the map, as it doesn't need to hold on to the
  lock beyond the scope of a method call.
* Return a null RuleIterator when there is no content setting, which
  improves performance because it avoids memory churn.

BUG=678278
Review-Url: https://codereview.chromium.org/2628883008
Cr-Commit-Position: refs/heads/master@{#444071}
Committed: https://chromium.googlesource.com/chromium/src/+/c3d40a39d323b758f7afb314a36fbb6ee6bd7249

[Bernhard Bauer, 2017-01-13 15:57:05]

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

[commit-bot: I haz the power, 2017-01-13 15:57:31]

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

[Bernhard Bauer, 2017-01-13 16:28:07]

Patchset #1 (id:1) has been deleted

[Bernhard Bauer, 2017-01-13 17:02:47]

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

[commit-bot: I haz the power, 2017-01-13 17:03:29]

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

[Bernhard Bauer, 2017-01-13 17:06:46]

Description was changed from

==========
Add a supervised user setting to allow all cookies.

BUG=678278
==========

to

==========
Add a supervised user setting to allow all cookies.

As this is the first supervised user content setting that is ALLOW
rather than BLOCK, rename content_settings::BinaryValueMap to
GlobalValueMap and add support for arbitrary content settings.

Also some cleanup, most importantly removing the AutoLock from the map,
as it doesn't need to hold on to the lock beyond the scope of a method
call.

BUG=678278
==========

[Bernhard Bauer, 2017-01-13 17:09:25]

Description was changed from

==========
Add a supervised user setting to allow all cookies.

As this is the first supervised user content setting that is ALLOW
rather than BLOCK, rename content_settings::BinaryValueMap to
GlobalValueMap and add support for arbitrary content settings.

Also some cleanup, most importantly removing the AutoLock from the map,
as it doesn't need to hold on to the lock beyond the scope of a method
call.

BUG=678278
==========

to

==========
Add a supervised user setting to allow all cookies.

As this is the first supervised user content setting that is ALLOW
rather than BLOCK, rename content_settings::BinaryValueMap to
GlobalValueMap and add support for arbitrary content settings.

Also some cleanup, most importantly:
* Remove the AutoLock from the map, as it doesn't need to hold on to the
  lock beyond the scope of a method call.
* Return a null RuleIterator when there is no content setting, which
  improves performance because it avoids memory churn.

BUG=678278
==========

[Bernhard Bauer, 2017-01-13 17:12:15]

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

[Bernhard Bauer, 2017-01-13 17:12:16]

Please review!

[commit-bot: I haz the power, 2017-01-13 19:43:41]

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

[commit-bot: I haz the power, 2017-01-13 19:43:41]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-01-16 08:48:24]

lgtm

Some context (on the bug) about *why* this change is needed would be nice :)

[Bernhard Bauer, 2017-01-16 11:21:43]

Description was changed from

==========
Add a supervised user setting to allow all cookies.

As this is the first supervised user content setting that is ALLOW
rather than BLOCK, rename content_settings::BinaryValueMap to
GlobalValueMap and add support for arbitrary content settings.

Also some cleanup, most importantly:
* Remove the AutoLock from the map, as it doesn't need to hold on to the
  lock beyond the scope of a method call.
* Return a null RuleIterator when there is no content setting, which
  improves performance because it avoids memory churn.

BUG=678278
==========

to

==========
Add a supervised user setting to allow all cookies.

This prevents inexperienced users from creating website compatibility
problems by blocking cookies.

As this is the first supervised user content setting that is ALLOW
rather than BLOCK, rename content_settings::BinaryValueMap to
GlobalValueMap and add support for arbitrary content settings.

Also some cleanup, most importantly:
* Remove the AutoLock from the map, as it doesn't need to hold on to the
  lock beyond the scope of a method call.
* Return a null RuleIterator when there is no content setting, which
  improves performance because it avoids memory churn.

BUG=678278
==========

[Bernhard Bauer, 2017-01-16 16:28:47]

On 2017/01/16 08:48:24, Marc Treib wrote:
> lgtm
> 
> Some context (on the bug) about *why* this change is needed would be nice :)

Added a reference to the bug, and a brief explanation to the description. I also
added code to enable this by default for child accounts. Could you take another
quick look at ChildAccountService?

[Marc Treib, 2017-01-16 17:24:40]

https://codereview.chromium.org/2628883008/diff/100001/chrome/browser/supervi...
File chrome/browser/supervised_user/child_accounts/child_account_service.cc
(right):

https://codereview.chromium.org/2628883008/diff/100001/chrome/browser/supervi...
chrome/browser/supervised_user/child_accounts/child_account_service.cc:195:
nullptr);
Are these changes required? Were things broken before?

[Bernhard Bauer, 2017-01-17 13:54:13]

https://codereview.chromium.org/2628883008/diff/100001/chrome/browser/supervi...
File chrome/browser/supervised_user/child_accounts/child_account_service.cc
(right):

https://codereview.chromium.org/2628883008/diff/100001/chrome/browser/supervi...
chrome/browser/supervised_user/child_accounts/child_account_service.cc:195:
nullptr);
On 2017/01/16 17:24:40, Marc Treib wrote:
> Are these changes required? Were things broken before?

Probably? :) The case where a profile changes from child account to regular is
not very common, so I don't think we've had good coverage for that.

What I could do is add a method that clears all local settings and call that
here -- ChildAccountService is the only instance outside of testing code where
we set local settings.

[Marc Treib, 2017-01-17 13:57:54]

lgtm

https://codereview.chromium.org/2628883008/diff/100001/chrome/browser/supervi...
File chrome/browser/supervised_user/child_accounts/child_account_service.cc
(right):

https://codereview.chromium.org/2628883008/diff/100001/chrome/browser/supervi...
chrome/browser/supervised_user/child_accounts/child_account_service.cc:195:
nullptr);
On 2017/01/17 13:54:13, Bernhard Bauer wrote:
> On 2017/01/16 17:24:40, Marc Treib wrote:
> > Are these changes required? Were things broken before?
> 
> Probably? :) The case where a profile changes from child account to regular is
> not very common, so I don't think we've had good coverage for that.

Interesting ;-)
It's also possible that SU settings simply don't do anything for non-supervised
users, and so actually all these are unnecessary?

> What I could do is add a method that clears all local settings and call that
> here -- ChildAccountService is the only instance outside of testing code where
> we set local settings.

That sounds reasonable (but I'm okay either way).

[Bernhard Bauer, 2017-01-17 16:08:18]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2017-01-17 16:08:30]

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, 2017-01-17 17:05:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484669298962490,
"parent_rev": "a247cc78340405414f9ed1085976abc611ab2d1a", "commit_rev":
"c3d40a39d323b758f7afb314a36fbb6ee6bd7249"}

[commit-bot: I haz the power, 2017-01-17 17:05:48]

Description was changed from

==========
Add a supervised user setting to allow all cookies.

This prevents inexperienced users from creating website compatibility
problems by blocking cookies.

As this is the first supervised user content setting that is ALLOW
rather than BLOCK, rename content_settings::BinaryValueMap to
GlobalValueMap and add support for arbitrary content settings.

Also some cleanup, most importantly:
* Remove the AutoLock from the map, as it doesn't need to hold on to the
  lock beyond the scope of a method call.
* Return a null RuleIterator when there is no content setting, which
  improves performance because it avoids memory churn.

BUG=678278
==========

to

==========
Add a supervised user setting to allow all cookies.

This prevents inexperienced users from creating website compatibility
problems by blocking cookies.

As this is the first supervised user content setting that is ALLOW
rather than BLOCK, rename content_settings::BinaryValueMap to
GlobalValueMap and add support for arbitrary content settings.

Also some cleanup, most importantly:
* Remove the AutoLock from the map, as it doesn't need to hold on to the
  lock beyond the scope of a method call.
* Return a null RuleIterator when there is no content setting, which
  improves performance because it avoids memory churn.

BUG=678278

Review-Url: https://codereview.chromium.org/2628883008
Cr-Commit-Position: refs/heads/master@{#444071}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3d40a39d323b758f7afb314a36f...
==========

[commit-bot: I haz the power, 2017-01-17 17:05:49]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c3d40a39d323b758f7afb314a36f...