First-Party Cookies: Wire it up as an experimental web platform feature

First-Party Cookies: Wire it up as an experimental web platform feature

This patch adds a flag to the NetworkDelegate to control the first-party
cookies experiment, and implements the flag in the ChromeNetworkDelegate
and ShellNetworkDelegate by checking the value of the experimental web
platform features command-line flag.

Once we decide whether or not to ship this feature, we can revert
everything in this patch other than the tests and the change to
URLRequestHttpJob::DoLoadCookies.

BUG=459154
Committed: https://crrev.com/3f3daac24caec2913641fe2fc2685feb738676ad
Cr-Commit-Position: refs/heads/master@{#318295}

[Mike West, 2015-02-20 10:39:36]

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

[Mike West, 2015-02-20 10:39:37]

WDYT about this approach, Matt? I didn't add a flag to URLRequest, because the
toggle isn't at all request-specific. First-party cookies are either enabled by
the embedder or they aren't. I've expressed that by adding a flag to
NetworkDelegate that's mapped to the "Experimental Web Platform Features"
command-line flag by ChromeNetworkDelegate and ShellNetworkDelegate.

Cookies feel pretty web-platformy, so this seemed like a reasonable approach. I
know that //net has different launch processes than Blink, so if tying cookie
changes to that flag doesn't feel right, I'd be fine with adding another.

You'll probably be buried in last-minute reviews today; I don't plan on landing
any of this until after the branch point, so no rush if other things are
higher-priority. :)

Thanks!

-mike

[mmenke, 2015-02-20 16:30:40]

My review load is actually pretty light today, so far.

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:299:
experimental_web_platform_features_enabled_(false),
Can we just read the command line flag on construction?  Mainly suggesting that
because that means we can test all the new code, including checking the command
line flag, in unit tests.  As-is, could lost the profile line in a refactor, or
it could accidentally be moved to the non-incognito profile only or something.

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:335: void
ChromeNetworkDelegate::SetExperimentalWebPlatformFeaturesEnabled() {
If you choose to keep this (See earlier comment), this should probably be
inlined and use the corresponding naming style.

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate_unittest.cc:91: #if
defined(ENABLE_EXTENSIONS)
All these ifdefs seem ugly, and this setup all seems very specific to the
NeverThrottleTest.  Maybe rename this test fixture to
ChromeNetworkDelegateThrottleTest, and don't have your new test inherit from it?
 You don't even need to use a test fixture in your new one.

https://codereview.chromium.org/940373002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/940373002/diff/1/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:636:
options.set_first_party_url(request_->first_party_for_cookies());
Ahh...This makes sense.  I hadn't realized it was just a toggle for the trial,
was imagining it would be something we set on non-main-frame navigations.

https://codereview.chromium.org/940373002/diff/1/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:636:
options.set_first_party_url(request_->first_party_for_cookies());
As-is, these lines could be removed, or always set the first party URL, and
everything would still pass.  We should have integration tests for this. 
Suggest two where it's enabled (One where first party matches, one where it
doesn't), one where it's not (The second just out of paranoia).  Could make one
test for multiple, I'm fine either way.

Most integration tests of this nature are in url_request_unittest.cc.  The way
you'd write the tests is have one request set the cookie (May have to modify the
test server, may not.  If you do, taking the cookie to set as a query parameter
seems the most general), and then another request read the cookie - the test
server has a URL that makes the response body a request header of your choice,
so don't need to add anything for that.

[Mike West, 2015-02-23 15:51:17]

WDYT of these tests?

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:299:
experimental_web_platform_features_enabled_(false),
On 2015/02/20 at 16:30:39, mmenke wrote:
> Can we just read the command line flag on construction?  Mainly suggesting
that because that means we can test all the new code, including checking the
command line flag, in unit tests.  As-is, could lost the profile line in a
refactor, or it could accidentally be moved to the non-incognito profile only or
something.

Makes a lot of sense to me. Moved.

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:335: void
ChromeNetworkDelegate::SetExperimentalWebPlatformFeaturesEnabled() {
On 2015/02/20 at 16:30:39, mmenke wrote:
> If you choose to keep this (See earlier comment), this should probably be
inlined and use the corresponding naming style.

Done, thanks!

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/940373002/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate_unittest.cc:91: #if
defined(ENABLE_EXTENSIONS)
On 2015/02/20 16:30:39, mmenke wrote:
> All these ifdefs seem ugly, and this setup all seems very specific to the
> NeverThrottleTest.  Maybe rename this test fixture to
> ChromeNetworkDelegateThrottleTest, and don't have your new test inherit from
it?
>  You don't even need to use a test fixture in your new one.

I either have ifdefs here, or in the new test, as I can't construct a
ChromeNetworkDelegate without knowing whether or not ENABLE_EXTENSIONS is set
(and either adding an EventRouterForwarder or not). It seemed simpler to extract
those bits here, but I take your point.

https://codereview.chromium.org/940373002/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/940373002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate_unittest.cc:31: #if
defined(ENABLE_EXTENSIONS)
All of the `#if` madness is here just so I don't have to create a whole new
class to add a single test. :/

[Mike West, 2015-02-24 16:26:31]

Friendly ping. :)

[mmenke, 2015-02-24 16:30:35]

On 2015/02/24 16:26:31, Mike West wrote:
> Friendly ping. :)

Took a sick day yesterday, and while I am working a half day today, really not
up to doing a decent job reviewing.

[Mike West, 2015-02-24 16:54:12]

On 2015/02/24 at 16:30:35, mmenke wrote:
> On 2015/02/24 16:26:31, Mike West wrote:
> > Friendly ping. :)
> 
> Took a sick day yesterday, and while I am working a half day today, really not
up to doing a decent job reviewing.

Oh no! Don't worry about this, then, and feel better soon!

[mmenke, 2015-02-26 16:24:59]

LGTM, sorry for slowness!  Still recovering.

https://codereview.chromium.org/940373002/diff/40001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/940373002/diff/40001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:306:
switches::kEnableExperimentalWebPlatformFeatures);
nit:  Can we put this in the initializer list?

https://codereview.chromium.org/940373002/diff/40001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/940373002/diff/40001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate_unittest.cc:54: #endif
Hadn't realized you'd need to do this, but I still think this is a bit better.

[Mike West, 2015-02-26 18:22:29]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-02-26 18:22:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/940373002/#ps80001 (title:
"Nit.")

[Mike West, 2015-02-26 18:23:04]

On 2015/02/26 16:24:59, mmenke wrote:
> LGTM, sorry for slowness!  Still recovering.

No worries. Thanks for the review, and I hope you feel better soon.

>
https://codereview.chromium.org/940373002/diff/40001/chrome/browser/net/chrom...
> File chrome/browser/net/chrome_network_delegate.cc (right):
> 
>
https://codereview.chromium.org/940373002/diff/40001/chrome/browser/net/chrom...
> chrome/browser/net/chrome_network_delegate.cc:306:
> switches::kEnableExperimentalWebPlatformFeatures);
> nit:  Can we put this in the initializer list?

Done.

[commit-bot: I haz the power, 2015-02-26 18:23:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/940373002/80001

[commit-bot: I haz the power, 2015-02-26 20:15:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-26 20:16:51]

Patchset 5 (id:??) landed as
https://crrev.com/3f3daac24caec2913641fe2fc2685feb738676ad
Cr-Commit-Position: refs/heads/master@{#318295}