Unify the "get" and "set" cookie access settings.

Refactoring: Unify the "get" and "set" cookie access settings.

This is just code cleanup, no behavior change is expected.
The underlying "get" and "set" cookie access checks are
done in StaticCookiePolicy, but CanGetCookies() and
CanSetCookie() were identical code, since the commit
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.

Since the checks were identical, this patch unifies them
into a single function to simplify the API and callsites.

I'm using bug 350870 just to help link this commit with
the above-mentioned c90ddfcfd61a.

BUG=350870
TBR=jochen,rogerta for mechanical-only changes

Review-Url: https://codereview.chromium.org/2655443003
Cr-Commit-Position: refs/heads/master@{#446576}
Committed: https://chromium.googlesource.com/chromium/src/+/7169140ae183cec72446399946a4cd0b79ea6dec

[falken, 2017-01-24 07:10:25]

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

[commit-bot: I haz the power, 2017-01-24 07:10:39]

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

[falken, 2017-01-24 07:18:55]

Description was changed from

==========
Unify the "get" and "set" cookie access settings.

The underlying code was already the same. So this was just a confusing
distinction in the codebase.

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

to

==========
Unify the "get" and "set" cookie access settings.

The underlying code was already the same, since
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.
So it was just a confusing distinction.

BUG=350870
==========

[commit-bot: I haz the power, 2017-01-24 07:40:04]

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

[commit-bot: I haz the power, 2017-01-24 07:40:05]

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...)

[falken, 2017-01-24 08:08:42]

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

[commit-bot: I haz the power, 2017-01-24 08:08:54]

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, 2017-01-24 09:46:31]

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

[commit-bot: I haz the power, 2017-01-24 09:46:32]

Dry run: This issue passed the CQ dry run.

[falken, 2017-01-24 10:32:41]

falken@chromium.org changed reviewers:
+ mkwst@chromium.org

[falken, 2017-01-24 10:32:42]

ptal.

[Mike West, 2017-01-25 10:25:27]

It's entirely possible we'll have to revisit removing this in the somewhat near
future, because cookies are AWESOME. But that's what version control is for:
LGTM to remove this.

[falken, 2017-01-25 13:54:48]

Description was changed from

==========
Unify the "get" and "set" cookie access settings.

The underlying code was already the same, since
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.
So it was just a confusing distinction.

BUG=350870
==========

to

==========
Unify the "get" and "set" cookie access settings.

The underlying code in StaticCookiePolicy was already the
same, since
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.
So it was just a confusing distinction.

BUG=350870
==========

[falken, 2017-01-25 14:08:25]

falken@chromium.org changed reviewers:
+ bnc@chromium.org, msramek@chromium.org

[falken, 2017-01-25 14:08:26]

Thanks, adding some owners.
bnc: net/
msramek: chrome/browser/browsing_data/, chrome/browser/content_settings,
components/content_settings

[Bence, 2017-01-25 14:12:48]

net/ LGTM.

[msramek, 2017-01-25 14:41:30]

browsing_data/, content_settings/ LGTM

I appreciate that the code is now clearer on what the cookie setting actually
does :)

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

https://codereview.chromium.org/2655443003/diff/20001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:164: *cookie_setting
= block ? CONTENT_SETTING_BLOCK : setting;
Please add a DCHECK for this pointer as well.

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

https://codereview.chromium.org/2655443003/diff/20001/components/content_sett...
components/content_settings/core/browser/cookie_settings.h:46: // allowed to
access cookies.
nit: Maybe at this one place, I would actually say "access (i.e. read or write)
cookies" to be clear that this is the setting for both, especially if people
were used to the previous model.

[falken, 2017-01-25 14:55:51]

msramek: Thanks, but a question:

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

https://codereview.chromium.org/2655443003/diff/20001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:164: *cookie_setting
= block ? CONTENT_SETTING_BLOCK : setting;
On 2017/01/25 14:41:30, msramek wrote:
> Please add a DCHECK for this pointer as well.

|cookie_setting| is already DCHECK'd at the beginning of the function... do you
want another DCHECK here?

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

https://codereview.chromium.org/2655443003/diff/20001/components/content_sett...
components/content_settings/core/browser/cookie_settings.h:46: // allowed to
access cookies.
On 2017/01/25 14:41:30, msramek wrote:
> nit: Maybe at this one place, I would actually say "access (i.e. read or
write)
> cookies" to be clear that this is the setting for both, especially if people
> were used to the previous model.

Good idea. Done. I'll add it to the net/ interface too.

[msramek, 2017-01-25 15:03:23]

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

https://codereview.chromium.org/2655443003/diff/20001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:164: *cookie_setting
= block ? CONTENT_SETTING_BLOCK : setting;
On 2017/01/25 14:55:51, falken wrote:
> On 2017/01/25 14:41:30, msramek wrote:
> > Please add a DCHECK for this pointer as well.
> 
> |cookie_setting| is already DCHECK'd at the beginning of the function... do
you
> want another DCHECK here?

Nope! Sorry, looked at it wrong the first time.

[falken, 2017-01-25 15:05:40]

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

https://codereview.chromium.org/2655443003/diff/20001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:164: *cookie_setting
= block ? CONTENT_SETTING_BLOCK : setting;
On 2017/01/25 15:03:23, msramek wrote:
> On 2017/01/25 14:55:51, falken wrote:
> > On 2017/01/25 14:41:30, msramek wrote:
> > > Please add a DCHECK for this pointer as well.
> > 
> > |cookie_setting| is already DCHECK'd at the beginning of the function... do
> you
> > want another DCHECK here?
> 
> Nope! Sorry, looked at it wrong the first time.

Acknowledged.

[falken, 2017-01-25 15:21:49]

falken@chromium.org changed reviewers:
+ boliu@chromium.org, droger@chromium.org, mef@chromium.org,
rdevlin.cronin@chromium.org

[falken, 2017-01-25 15:21:50]

Adding more owners.
rdevlin.cronin: chrome/browser/extensions/, extensions/
droger: ios/chrome/browser/net/
boliu: android_webview/
mef: chrome/browser/net/chrome_network_delegate.cc

[falken, 2017-01-25 15:22:04]

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

[commit-bot: I haz the power, 2017-01-25 15:22:22]

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

[Devlin, 2017-01-25 16:04:23]

extensions lgtm

[commit-bot: I haz the power, 2017-01-25 16:27:27]

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

[commit-bot: I haz the power, 2017-01-25 16:27:28]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-01-25 16:38:51]

lgtm

android_webview rs lgtm

[mef, 2017-01-25 16:56:13]

Could you elaborate on this CL? 

I was under impression that CanGetCookie and CanSetCookie is intentionally
different, although I'm not entirely sure about the reason.
Has our policy changed? 

The bug 350870 seems to target more narrow scenario, and description of the CL
doesn't provide the motivation for this change.

Adding rsleevi to weigh in.

[Ryan Sleevi, 2017-01-25 17:08:19]

On 2017/01/25 16:56:13, mef wrote:
> Could you elaborate on this CL? 
> 
> I was under impression that CanGetCookie and CanSetCookie is intentionally
> different, although I'm not entirely sure about the reason.
> Has our policy changed? 
> 
> The bug 350870 seems to target more narrow scenario, and description of the CL
> doesn't provide the motivation for this change.
> 
> Adding rsleevi to weigh in.

Yeah, like mef@ said, it would be good to understand the broader context of what
this cleanup is trying to accomplish. 

I ask because we have LOAD_FLAG_DO_NOT_SEND_COOKIES and
LOAD_FLAG_DO_NOT_SAVE_COOKIES, which mirrors this interface and policy, and it's
unclear in your simplification how these concepts interact - that is, what
policy should be checked (in the absence of those load flags), and what load
flags should be set based on these policies.

To be clear: I'm not opposed to introducing this, and agree that with the code
being what it is, it's functionally a no-op. However, I'm trying to make sure
we're conceptually aligned, because I think this CL introduces some conceptual
complications (at least from the POV of a //net OWNER), and wanted to make sure
intent and actuality were aligned :)

[falken, 2017-01-26 01:17:34]

On 2017/01/25 17:08:19, Ryan Sleevi wrote:
> On 2017/01/25 16:56:13, mef wrote:
> > Could you elaborate on this CL? 
> > 
> > I was under impression that CanGetCookie and CanSetCookie is intentionally
> > different, although I'm not entirely sure about the reason.
> > Has our policy changed? 
> > 
> > The bug 350870 seems to target more narrow scenario, and description of the
CL
> > doesn't provide the motivation for this change.
> > 
> > Adding rsleevi to weigh in.
> 
> Yeah, like mef@ said, it would be good to understand the broader context of
what
> this cleanup is trying to accomplish. 
> 
> I ask because we have LOAD_FLAG_DO_NOT_SEND_COOKIES and
> LOAD_FLAG_DO_NOT_SAVE_COOKIES, which mirrors this interface and policy, and
it's
> unclear in your simplification how these concepts interact - that is, what
> policy should be checked (in the absence of those load flags), and what load
> flags should be set based on these policies.
> 
> To be clear: I'm not opposed to introducing this, and agree that with the code
> being what it is, it's functionally a no-op. However, I'm trying to make sure
> we're conceptually aligned, because I think this CL introduces some conceptual
> complications (at least from the POV of a //net OWNER), and wanted to make
sure
> intent and actuality were aligned :)

Yea, I should make a more descriptive CL description.

The story behind this is I was auditing how service workers are gated on cookie
settings (ChromeContentBrowserClient::AllowServiceWorker), and wondering why we
use IsSettingCookieAllowed instead of IsReadingCookieAllowed, and if we should
be using both like ChromeContentBrowserClient::AllowWebRTCIdentityCache does.
Then I discovered it all boils down to StaticCookiePolicy::CanGetCookies and
CanSetCookies, which confusingly turned out to be the same repeated code
(prompting a diff to verify they really are the same).

Since they are identical, it seemed best to just make them one function to spare
API callers the decision of which to use.

I guess we could still force callers to distinguish between which access they
want, but it seemed cleaner to just remove it, and eliminate awkward calling
code like:
   cookie_settings->GetCookieSetting(primary_url, secondary_url, nullptr,
nullptr /* reading_setting */, &setting);

And CookieSettings::GetReadingAndSettingCookieAllowed whose purpose is to
retrieve both settings in a performant way.

[droger, 2017-01-26 13:18:06]

ios lgtm

[mef, 2017-01-26 22:49:49]

On 2017/01/26 01:17:34, falken wrote:
> On 2017/01/25 17:08:19, Ryan Sleevi wrote:
> > On 2017/01/25 16:56:13, mef wrote:
> > > Could you elaborate on this CL? 
> > > 
> > > I was under impression that CanGetCookie and CanSetCookie is intentionally
> > > different, although I'm not entirely sure about the reason.
> > > Has our policy changed? 
> > > 
> > > The bug 350870 seems to target more narrow scenario, and description of
the
> CL
> > > doesn't provide the motivation for this change.
> > > 
> > > Adding rsleevi to weigh in.
> > 
> > Yeah, like mef@ said, it would be good to understand the broader context of
> what
> > this cleanup is trying to accomplish. 
> > 
> > I ask because we have LOAD_FLAG_DO_NOT_SEND_COOKIES and
> > LOAD_FLAG_DO_NOT_SAVE_COOKIES, which mirrors this interface and policy, and
> it's
> > unclear in your simplification how these concepts interact - that is, what
> > policy should be checked (in the absence of those load flags), and what load
> > flags should be set based on these policies.
> > 
> > To be clear: I'm not opposed to introducing this, and agree that with the
code
> > being what it is, it's functionally a no-op. However, I'm trying to make
sure
> > we're conceptually aligned, because I think this CL introduces some
conceptual
> > complications (at least from the POV of a //net OWNER), and wanted to make
> sure
> > intent and actuality were aligned :)
> 
> Yea, I should make a more descriptive CL description.
> 
> The story behind this is I was auditing how service workers are gated on
cookie
> settings (ChromeContentBrowserClient::AllowServiceWorker), and wondering why
we
> use IsSettingCookieAllowed instead of IsReadingCookieAllowed, and if we should
> be using both like ChromeContentBrowserClient::AllowWebRTCIdentityCache does.
> Then I discovered it all boils down to StaticCookiePolicy::CanGetCookies and
> CanSetCookies, which confusingly turned out to be the same repeated code
> (prompting a diff to verify they really are the same).
> 
> Since they are identical, it seemed best to just make them one function to
spare
> API callers the decision of which to use.
> 
> I guess we could still force callers to distinguish between which access they
> want, but it seemed cleaner to just remove it, and eliminate awkward calling
> code like:
>    cookie_settings->GetCookieSetting(primary_url, secondary_url, nullptr,
> nullptr /* reading_setting */, &setting);
> 
> And CookieSettings::GetReadingAndSettingCookieAllowed whose purpose is to
> retrieve both settings in a performant way.

Thanks for explanation, lgtm.

[falken, 2017-01-27 01:04:36]

Description was changed from

==========
Unify the "get" and "set" cookie access settings.

The underlying code in StaticCookiePolicy was already the
same, since
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.
So it was just a confusing distinction.

BUG=350870
==========

to

==========
Refactoring: Unify the "get" and "set" cookie access settings.

This is just code-cleanup, no behavior change is expected.
The underlying "get" and "set" cookie access checks are
done in StaticCookiePolicy, but CanGetCookies() and
CanSetCookie() were identical code, since the commit
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.

Since the checks are identical, this patch unifies them
into a single function to simplify the API and callsites.

I'm using bug 350870 just to help link this commit with
the above-mentioned c90ddfcfd61a.

BUG=350870
==========

[falken, 2017-01-27 01:06:04]

Description was changed from

==========
Refactoring: Unify the "get" and "set" cookie access settings.

This is just code-cleanup, no behavior change is expected.
The underlying "get" and "set" cookie access checks are
done in StaticCookiePolicy, but CanGetCookies() and
CanSetCookie() were identical code, since the commit
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.

Since the checks are identical, this patch unifies them
into a single function to simplify the API and callsites.

I'm using bug 350870 just to help link this commit with
the above-mentioned c90ddfcfd61a.

BUG=350870
==========

to

==========
Refactoring: Unify the "get" and "set" cookie access settings.

This is just code cleanup, no behavior change is expected.
The underlying "get" and "set" cookie access checks are
done in StaticCookiePolicy, but CanGetCookies() and
CanSetCookie() were identical code, since the commit
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.

Since the checks were identical, this patch unifies them
into a single function to simplify the API and callsites.

I'm using bug 350870 just to help link this commit with
the above-mentioned c90ddfcfd61a.

BUG=350870
==========

[falken, 2017-01-27 01:22:56]

Thanks, I've also updated the CL description. I will TBR owners for the
remaining purely mechanical changes.

[falken, 2017-01-27 01:24:57]

falken@chromium.org changed reviewers:
+ jochen@chromium.org, rogerta@chromium.org

[falken, 2017-01-27 01:24:58]

TBR'ing owners for the remaining mechanical only changes.

jochen:
    chrome/browser/renderer_host/chrome_render_message_filter.cc
    chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc
    chrome/browser/storage/durable_storage_permission_context.cc
    chrome/browser/ui/content_settings/content_setting_bubble_model.cc
rogerta:
    components/signin/core/browser/signin_header_helper.cc

[falken, 2017-01-27 01:25:22]

Description was changed from

==========
Refactoring: Unify the "get" and "set" cookie access settings.

This is just code cleanup, no behavior change is expected.
The underlying "get" and "set" cookie access checks are
done in StaticCookiePolicy, but CanGetCookies() and
CanSetCookie() were identical code, since the commit
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.

Since the checks were identical, this patch unifies them
into a single function to simplify the API and callsites.

I'm using bug 350870 just to help link this commit with
the above-mentioned c90ddfcfd61a.

BUG=350870
==========

to

==========
Refactoring: Unify the "get" and "set" cookie access settings.

This is just code cleanup, no behavior change is expected.
The underlying "get" and "set" cookie access checks are
done in StaticCookiePolicy, but CanGetCookies() and
CanSetCookie() were identical code, since the commit
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.

Since the checks were identical, this patch unifies them
into a single function to simplify the API and callsites.

I'm using bug 350870 just to help link this commit with
the above-mentioned c90ddfcfd61a.

BUG=350870
TBR=jochen,rogerta for mechanical-only changes
==========

[falken, 2017-01-27 01:32:23]

The CQ bit was checked by falken@chromium.org

[falken, 2017-01-27 01:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, bnc@chromium.org, mkwst@chromium.org,
rdevlin.cronin@chromium.org, droger@chromium.org, boliu@chromium.org,
mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2655443003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-27 01:34:10]

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-27 03:38:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485480743466570,
"parent_rev": "622f70d2786a5dd446c45fa8222ce2d14d98cbf5", "commit_rev":
"7169140ae183cec72446399946a4cd0b79ea6dec"}

[commit-bot: I haz the power, 2017-01-27 03:38:40]

Description was changed from

==========
Refactoring: Unify the "get" and "set" cookie access settings.

This is just code cleanup, no behavior change is expected.
The underlying "get" and "set" cookie access checks are
done in StaticCookiePolicy, but CanGetCookies() and
CanSetCookie() were identical code, since the commit
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.

Since the checks were identical, this patch unifies them
into a single function to simplify the API and callsites.

I'm using bug 350870 just to help link this commit with
the above-mentioned c90ddfcfd61a.

BUG=350870
TBR=jochen,rogerta for mechanical-only changes
==========

to

==========
Refactoring: Unify the "get" and "set" cookie access settings.

This is just code cleanup, no behavior change is expected.
The underlying "get" and "set" cookie access checks are
done in StaticCookiePolicy, but CanGetCookies() and
CanSetCookie() were identical code, since the commit
https://crrev.com/c90ddfcfd61ae1fb403814e7794ca3a86aa548b2.

Since the checks were identical, this patch unifies them
into a single function to simplify the API and callsites.

I'm using bug 350870 just to help link this commit with
the above-mentioned c90ddfcfd61a.

BUG=350870
TBR=jochen,rogerta for mechanical-only changes

Review-Url: https://codereview.chromium.org/2655443003
Cr-Commit-Position: refs/heads/master@{#446576}
Committed:
https://chromium.googlesource.com/chromium/src/+/7169140ae183cec72446399946a4...
==========

[commit-bot: I haz the power, 2017-01-27 03:38:43]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7169140ae183cec72446399946a4...