Provide child/frame IDs for WebSocket handshake request

Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach
third-party cookies. This CL provide the additional information to the
WebSocket handshake net::URLRequest.

BUG=634311
Committed: https://crrev.com/4a593833d44a457f177f99b2c907bd0f6ae397f7
Cr-Commit-Position: refs/heads/master@{#427109}

[yhirano, 2016-10-07 10:36:22]

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

[commit-bot: I haz the power, 2016-10-07 10:36:30]

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-10-07 10:45:43]

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

[commit-bot: I haz the power, 2016-10-07 10:45:44]

Dry run: Exceeded global retry quota

[yhirano, 2016-10-12 02:00:06]

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

[commit-bot: I haz the power, 2016-10-12 02:00:22]

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-10-12 02:18:55]

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

[commit-bot: I haz the power, 2016-10-12 02:18:56]

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

[yhirano, 2016-10-12 02:28:51]

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

[commit-bot: I haz the power, 2016-10-12 02:29:01]

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-10-12 03:06:34]

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

[commit-bot: I haz the power, 2016-10-12 03:06:35]

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

[yhirano, 2016-10-12 03:47:49]

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

[commit-bot: I haz the power, 2016-10-12 03:48:09]

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-10-12 05:02:26]

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

[commit-bot: I haz the power, 2016-10-12 05:02:27]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-12 05:07:37]

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

[commit-bot: I haz the power, 2016-10-12 05:07:55]

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

[yhirano, 2016-10-12 05:29:56]

Patchset #1 (id:1) has been deleted

[yhirano, 2016-10-12 05:30:09]

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

[yhirano, 2016-10-12 05:30:20]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-10-12 05:30:30]

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

[yhirano, 2016-10-12 05:30:35]

Patchset #1 (id:40001) has been deleted

[yhirano, 2016-10-12 05:31:09]

Patchset #2 (id:80001) has been deleted

[yhirano, 2016-10-12 05:33:42]

Description was changed from

==========
[WIP] websocket android webview cookie


wip wip wip

BUG=634311
==========

to

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a
WebSocket
connection to determine if it allows the connection to attach a
third-party cookies. This CL provide the addition information to
the WebSocket handshake net::URLRequest.

BUG=634311
==========

[yhirano, 2016-10-12 05:33:43]

yhirano@chromium.org changed reviewers:
+ ricea@chromium.org, tyoshino@chromium.org

[yhirano, 2016-10-12 05:34:03]

Description was changed from

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a
WebSocket
connection to determine if it allows the connection to attach a
third-party cookies. This CL provide the addition information to
the WebSocket handshake net::URLRequest.

BUG=634311
==========

to

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach a
third-party cookies. This CL provide the addition information to
the WebSocket handshake net::URLRequest.

BUG=634311
==========

[commit-bot: I haz the power, 2016-10-12 06:30:33]

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

[commit-bot: I haz the power, 2016-10-12 06:30:34]

Dry run: This issue passed the CQ dry run.

[Adam Rice, 2016-10-12 06:38:25]

lgtm with nits.

It's terrible that we have to do this. But I can't see any alternative.

This looks like it is privacy-critical. If so, it really should have a test. You
don't have to include one in this CL. I think the least-worst way to test it
would be to add functionality to net::TestNetworkDelegate to set a callback to
be called from OnCanSetCookie().

https://codereview.chromium.org/2397393002/diff/100001/android_webview/browse...
File android_webview/browser/aw_cookie_access_policy.cc (right):

https://codereview.chromium.org/2397393002/diff/100001/android_webview/browse...
android_webview/browser/aw_cookie_access_policy.cc:71: const
WebSocketHandshakeRequestInfo* websocketInfo =
s/websocketInfo/websocket_info/?

https://codereview.chromium.org/2397393002/diff/100001/android_webview/browse...
android_webview/browser/aw_cookie_access_policy.cc:73: if (websocketInfo) {
Maybe instead write

if (!websocket_info)
  return false;

and then remove the has_id variable?

[yhirano, 2016-10-12 06:52:04]

Description was changed from

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach a
third-party cookies. This CL provide the addition information to
the WebSocket handshake net::URLRequest.

BUG=634311
==========

to

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach
third-party cookies. This CL provide the addition information to the
WebSocket handshake net::URLRequest.

BUG=634311
==========

[yhirano, 2016-10-12 07:44:42]

Description was changed from

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach
third-party cookies. This CL provide the addition information to the
WebSocket handshake net::URLRequest.

BUG=634311
==========

to

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach
third-party cookies. This CL provide the additional information to the
WebSocket handshake net::URLRequest.

BUG=634311
==========

[tyoshino (SeeGerritForStatus), 2016-10-12 08:13:37]

https://codereview.chromium.org/2397393002/diff/100001/content/browser/websoc...
File content/browser/websockets/websocket_impl.cc (right):

https://codereview.chromium.org/2397393002/diff/100001/content/browser/websoc...
content/browser/websockets/websocket_impl.cc:23: #include
"content/browser/loader/resource_request_info_impl.h"
for what?

https://codereview.chromium.org/2397393002/diff/100001/content/browser/websoc...
content/browser/websockets/websocket_impl.cc:28: #include
"content/public/common/process_type.h"
for what?

https://codereview.chromium.org/2397393002/diff/100001/content/browser/websoc...
content/browser/websockets/websocket_impl.cc:36: #include
"net/url_request/url_request.h"
where is this used?

https://codereview.chromium.org/2397393002/diff/100001/net/websockets/websock...
File net/websockets/websocket_stream.h (right):

https://codereview.chromium.org/2397393002/diff/100001/net/websockets/websock...
net/websockets/websocket_stream.h:76: virtual void OnCreateRequest(URLRequest*
request) = 0;
did you omit URL from the name of this method intentionally?

[yhirano, 2016-10-12 10:45:32]

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

[commit-bot: I haz the power, 2016-10-12 10:45:51]

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

[yhirano, 2016-10-12 10:47:15]

https://codereview.chromium.org/2397393002/diff/100001/android_webview/browse...
File android_webview/browser/aw_cookie_access_policy.cc (right):

https://codereview.chromium.org/2397393002/diff/100001/android_webview/browse...
android_webview/browser/aw_cookie_access_policy.cc:71: const
WebSocketHandshakeRequestInfo* websocketInfo =
On 2016/10/12 06:38:25, Adam Rice wrote:
> s/websocketInfo/websocket_info/?

Done.

https://codereview.chromium.org/2397393002/diff/100001/android_webview/browse...
android_webview/browser/aw_cookie_access_policy.cc:73: if (websocketInfo) {
On 2016/10/12 06:38:25, Adam Rice wrote:
> Maybe instead write
> 
> if (!websocket_info)
>   return false;
> 
> and then remove the has_id variable?

Done.

https://codereview.chromium.org/2397393002/diff/100001/content/browser/websoc...
File content/browser/websockets/websocket_impl.cc (right):

https://codereview.chromium.org/2397393002/diff/100001/content/browser/websoc...
content/browser/websockets/websocket_impl.cc:23: #include
"content/browser/loader/resource_request_info_impl.h"
On 2016/10/12 08:13:36, tyoshino wrote:
> for what?

Sorry, it was used when I used ResourceRequestInfo instead of
WebSocketHandshakeRequestInfo. Deleted.

https://codereview.chromium.org/2397393002/diff/100001/content/browser/websoc...
content/browser/websockets/websocket_impl.cc:28: #include
"content/public/common/process_type.h"
On 2016/10/12 08:13:36, tyoshino wrote:
> for what?

Ditto

https://codereview.chromium.org/2397393002/diff/100001/content/browser/websoc...
content/browser/websockets/websocket_impl.cc:36: #include
"net/url_request/url_request.h"
On 2016/10/12 08:13:36, tyoshino wrote:
> where is this used?

Done.

https://codereview.chromium.org/2397393002/diff/100001/net/websockets/websock...
File net/websockets/websocket_stream.h (right):

https://codereview.chromium.org/2397393002/diff/100001/net/websockets/websock...
net/websockets/websocket_stream.h:76: virtual void OnCreateRequest(URLRequest*
request) = 0;
On 2016/10/12 08:13:37, tyoshino wrote:
> did you omit URL from the name of this method intentionally?

No. Renamed.

[tyoshino (SeeGerritForStatus), 2016-10-12 11:19:32]

lgtm

[yhirano, 2016-10-12 11:22:00]

yhirano@chromium.org changed reviewers:
+ jam@chromium.org, torne@chromium.org

[yhirano, 2016-10-12 11:22:00]

+jam@ for content/public and content/browser/BUILD.gn.
+torne@ for android_webview.

[commit-bot: I haz the power, 2016-10-12 11:51:26]

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

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

Dry run: This issue passed the CQ dry run.

[Torne, 2016-10-12 12:16:30]

Is there a reasonable way to implement a test for this in WebView, to verify it
actually works end to end?

https://codereview.chromium.org/2397393002/diff/120001/android_webview/browse...
File android_webview/browser/aw_cookie_access_policy.cc (right):

https://codereview.chromium.org/2397393002/diff/120001/android_webview/browse...
android_webview/browser/aw_cookie_access_policy.cc:72: return false;
Do you happen to know if there's any other case where there's neither a
ResourceRequestInfo or a WebSocketHandshakeRequestInfo? Maybe we could stick a
NOTREACHED() or something in here so that it's more obvious if we run into
another case where we can't check our setting?

[yhirano, 2016-10-12 12:36:56]

> Is there a reasonable way to implement a test for this in WebView, to
> verify it actually works end to end?

I thought webview people knew how to write tests for this kind of behavior, but
apparently I was wrong...

https://codereview.chromium.org/2397393002/diff/120001/android_webview/browse...
File android_webview/browser/aw_cookie_access_policy.cc (right):

https://codereview.chromium.org/2397393002/diff/120001/android_webview/browse...
android_webview/browser/aw_cookie_access_policy.cc:72: return false;
On 2016/10/12 12:16:30, Torne wrote:
> Do you happen to know if there's any other case where there's neither a
> ResourceRequestInfo or a WebSocketHandshakeRequestInfo? Maybe we could stick a
> NOTREACHED() or something in here so that it's more obvious if we run into
> another case where we can't check our setting?

Sorry, I don't know. If it should be done, it should be done after this change
lands, I believe.

[Torne, 2016-10-12 12:40:07]

torne@chromium.org changed reviewers:
+ timvolodine@chromium.org

[Torne, 2016-10-12 12:40:08]

On 2016/10/12 12:36:56, yhirano wrote:
> > Is there a reasonable way to implement a test for this in WebView, to
> > verify it actually works end to end?
> 
> I thought webview people knew how to write tests for this kind of behavior,
but
> apparently I was wrong...

I don't think we have any websocket related tests at all, and I'm not sure if we
have a test runner environment where there's a websocket-capable server
available to run tests against. +timvolodine who might know if there's a good
way to run a test like this in our instrumentation environment?

[yhirano, 2016-10-12 12:44:00]

On 2016/10/12 12:40:08, Torne wrote:
> On 2016/10/12 12:36:56, yhirano wrote:
> > > Is there a reasonable way to implement a test for this in WebView, to
> > > verify it actually works end to end?
> > 
> > I thought webview people knew how to write tests for this kind of behavior,
> but
> > apparently I was wrong...
> 
> I don't think we have any websocket related tests at all, and I'm not sure if
we
> have a test runner environment where there's a websocket-capable server
> available to run tests against. +timvolodine who might know if there's a good
> way to run a test like this in our instrumentation environment?

Is there a test about the cookie policy for http requests?

[Torne, 2016-10-12 12:48:53]

There's a Java instrumentation test
org.chromium.android_webview.test.CookieManagerTest.testThirdPartyCookie() -
that's probably what we'd want to extend I guess..

[yhirano, 2016-10-12 12:58:30]

On 2016/10/12 12:48:53, Torne wrote:
> There's a Java instrumentation test
> org.chromium.android_webview.test.CookieManagerTest.testThirdPartyCookie() -
> that's probably what we'd want to extend I guess..

Thanks, I'll look into it tomorrow.

[timvolodine, 2016-10-12 15:04:18]

On 2016/10/12 12:40:08, Torne wrote:
> On 2016/10/12 12:36:56, yhirano wrote:
> > > Is there a reasonable way to implement a test for this in WebView, to
> > > verify it actually works end to end?
> > 
> > I thought webview people knew how to write tests for this kind of behavior,
> but
> > apparently I was wrong...
> 
> I don't think we have any websocket related tests at all, and I'm not sure if
we
> have a test runner environment where there's a websocket-capable server
> available to run tests against. +timvolodine who might know if there's a good
> way to run a test like this in our instrumentation environment?

I remember using
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java for
something similar. Looks like the CookieManagerTest does that as well..

[jam, 2016-10-12 15:59:29]

lgtm with comments

https://codereview.chromium.org/2397393002/diff/120001/content/public/browser...
File content/public/browser/websocket_handshake_request_info.h (right):

https://codereview.chromium.org/2397393002/diff/120001/content/public/browser...
content/public/browser/websocket_handshake_request_info.h:23: // Returns the ID
of the renderrer frame where the WebSocket connection lives.
nit: renderer

https://codereview.chromium.org/2397393002/diff/120001/content/public/browser...
content/public/browser/websocket_handshake_request_info.h:24: virtual int
GetFrameId() const = 0;
call this GetRenderFrameID() to make it clear that it's a routing id of a
RenderFrame. a frame id is ambiguous because we also have frame tree id :)

[yhirano, 2016-10-13 04:42:14]

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

[commit-bot: I haz the power, 2016-10-13 04:42:36]

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-10-13 05:52:55]

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

[commit-bot: I haz the power, 2016-10-13 05:52:56]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-13 05:55:18]

https://codereview.chromium.org/2397393002/diff/120001/content/public/browser...
File content/public/browser/websocket_handshake_request_info.h (right):

https://codereview.chromium.org/2397393002/diff/120001/content/public/browser...
content/public/browser/websocket_handshake_request_info.h:23: // Returns the ID
of the renderrer frame where the WebSocket connection lives.
On 2016/10/12 15:59:28, jam wrote:
> nit: renderer

Done.

https://codereview.chromium.org/2397393002/diff/120001/content/public/browser...
content/public/browser/websocket_handshake_request_info.h:24: virtual int
GetFrameId() const = 0;
On 2016/10/12 15:59:28, jam wrote:
> call this GetRenderFrameID() to make it clear that it's a routing id of a
> RenderFrame. a frame id is ambiguous because we also have frame tree id :)

Done.

[yhirano, 2016-10-14 07:18:42]

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

[commit-bot: I haz the power, 2016-10-14 07:19:00]

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

[yhirano, 2016-10-14 07:22:54]

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

[commit-bot: I haz the power, 2016-10-14 07:23:12]

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

[yhirano, 2016-10-14 07:27:12]

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

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

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-10-14 08:12:42]

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

[commit-bot: I haz the power, 2016-10-14 08:12:43]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-14 08:42:43]

yhirano@chromium.org changed reviewers:
+ mef@chromium.org

[yhirano, 2016-10-14 08:42:43]

I added two tests.

+mef@ for net/test.

[Torne, 2016-10-14 09:59:25]

Thanks for adding the test, it looks good, but it'd be nice not to have two test
methods that are almost the same:

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:406:
@MediumTest
Can you put the common code for these tests in a separate method maybe, so that
it doesn't have to duplicate everything just for true/false?

[mef, 2016-10-14 16:52:25]

mef@chromium.org changed reviewers:
+ jbudorick@chromium.org

[mef, 2016-10-14 16:52:26]

https://codereview.chromium.org/2397393002/diff/200001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java
(right):

https://codereview.chromium.org/2397393002/diff/200001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java:12:
import org.apache.http.Header;
According to https://bugs.chromium.org/p/chromium/issues/detail?id=488192 the
use of org.apache.http was deprecated. 

Adding +jbudorick to comment on plans to remove TestWebServer, but I don't think
we should add new functionality to it.

[yhirano, 2016-10-17 10:18:40]

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

[commit-bot: I haz the power, 2016-10-17 10:18:51]

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-10-17 11:08:03]

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

[commit-bot: I haz the power, 2016-10-17 11:08:04]

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

[jbudorick, 2016-10-18 17:27:31]

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:408:
public void testThirdPartyCookieForWebSocketDisabledCase() throws Throwable {
If possible, new tests should use EmbeddedTestServer
(https://codesearch.chromium.org/chromium/src/net/test/android/javatests/src/o...)
rather than TestWebServer, especially if they need new functionality from the
server.

https://codereview.chromium.org/2397393002/diff/200001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java
(right):

https://codereview.chromium.org/2397393002/diff/200001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java:12:
import org.apache.http.Header;
On 2016/10/14 16:52:26, mef wrote:
> According to https://bugs.chromium.org/p/chromium/issues/detail?id=488192 the
> use of org.apache.http was deprecated. 
> 
> Adding +jbudorick to comment on plans to remove TestWebServer, but I don't
think
> we should add new functionality to it.

Agreed. We're still working to replace existing uses with EmbeddedTestServer,
though that has fallen behind some other tasks in priority.

[yhirano, 2016-10-19 05:49:14]

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

[commit-bot: I haz the power, 2016-10-19 05:50:13]

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-10-19 11:18:11]

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

[commit-bot: I haz the power, 2016-10-19 11:18:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-10-19 11:32:13]

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:406:
@MediumTest
On 2016/10/14 09:59:24, Torne wrote:
> Can you put the common code for these tests in a separate method maybe, so
that
> it doesn't have to duplicate everything just for true/false?

Done.

[yhirano, 2016-10-19 14:45:00]

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:408:
public void testThirdPartyCookieForWebSocketDisabledCase() throws Throwable {
On 2016/10/18 17:27:30, jbudorick wrote:
> If possible, new tests should use EmbeddedTestServer
>
(https://codesearch.chromium.org/chromium/src/net/test/android/javatests/src/o...)
> rather than TestWebServer, especially if they need new functionality from the
> server.

We need to add a response header (Sec-WebSocket-Accept) that depends on a
request header (Sec-WebSocket-Key) whose value is randomly chosen. I guess I can
do that with net::test_server::EmbeddedTestServer::RegisterRequestHandler, but
it's not exposed to Java, right?

[jbudorick, 2016-10-19 15:40:40]

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/2397393002/diff/200001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:408:
public void testThirdPartyCookieForWebSocketDisabledCase() throws Throwable {
On 2016/10/19 14:45:00, yhirano wrote:
> On 2016/10/18 17:27:30, jbudorick wrote:
> > If possible, new tests should use EmbeddedTestServer
> >
>
(https://codesearch.chromium.org/chromium/src/net/test/android/javatests/src/o...)
> > rather than TestWebServer, especially if they need new functionality from
the
> > server.
> 
> We need to add a response header (Sec-WebSocket-Accept) that depends on a
> request header (Sec-WebSocket-Key) whose value is randomly chosen. I guess I
can
> do that with net::test_server::EmbeddedTestServer::RegisterRequestHandler, but
> it's not exposed to Java, right?

Ah. Yeah, we haven't implemented that yet. Given that, I'm fine with this as-is
for now.

[mef, 2016-10-19 17:41:24]

https://codereview.chromium.org/2397393002/diff/240001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java
(right):

https://codereview.chromium.org/2397393002/diff/240001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java:367:
responseHeaders.add(Pair.create("Connection", "Upgrade"));
If responseHeaders passed in are not null, then they'll be modified as a side
effect of this method. Would it be better to make a copy and modify just the
copy to avoid affecting caller?

https://codereview.chromium.org/2397393002/diff/240001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java:521:
servedResponseFor(path, request);
Should  servedResponseFor(path, request); be also called for 'else' and 'catch'
cases?

[yhirano, 2016-10-20 01:52:29]

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

[commit-bot: I haz the power, 2016-10-20 01:53:01]

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

[yhirano, 2016-10-20 02:14:00]

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

[commit-bot: I haz the power, 2016-10-20 02:14:34]

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-10-20 04:14:17]

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

[commit-bot: I haz the power, 2016-10-20 04:14:19]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-20 04:21:07]

https://codereview.chromium.org/2397393002/diff/240001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java
(right):

https://codereview.chromium.org/2397393002/diff/240001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java:367:
responseHeaders.add(Pair.create("Connection", "Upgrade"));
On 2016/10/19 17:41:24, mef wrote:
> If responseHeaders passed in are not null, then they'll be modified as a side
> effect of this method. Would it be better to make a copy and modify just the
> copy to avoid affecting caller?

Done.

https://codereview.chromium.org/2397393002/diff/240001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java:521:
servedResponseFor(path, request);
On 2016/10/19 17:41:24, mef wrote:
> Should  servedResponseFor(path, request); be also called for 'else' and
'catch'
> cases?

Done.

[Torne, 2016-10-24 12:04:39]

LGTM, thanks for being patient with this (I was OOO last week).

[yhirano, 2016-10-24 17:00:13]

mef@, are you OK with this change?

[mef, 2016-10-24 17:04:35]

lgtm, sorry for the delay.

[yhirano, 2016-10-24 17:05:06]

Thanks!

[yhirano, 2016-10-24 17:05:18]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-10-24 17:05:19]

The patchset sent to the CQ was uploaded after l-g-t-m from ricea@chromium.org,
tyoshino@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2397393002/#ps280001
(title: "fix")

[commit-bot: I haz the power, 2016-10-24 17:05:43]

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-10-24 18:58:42]

Description was changed from

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach
third-party cookies. This CL provide the additional information to the
WebSocket handshake net::URLRequest.

BUG=634311
==========

to

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach
third-party cookies. This CL provide the additional information to the
WebSocket handshake net::URLRequest.

BUG=634311
==========

[commit-bot: I haz the power, 2016-10-24 18:58:44]

Committed patchset #11 (id:280001)

[commit-bot: I haz the power, 2016-10-24 19:23:40]

Description was changed from

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach
third-party cookies. This CL provide the additional information to the
WebSocket handshake net::URLRequest.

BUG=634311
==========

to

==========
Provide child/frame IDs for WebSocket handshake request

AndroidCookiePolicy needs the child ID and the frame ID of a WebSocket
connection to determine if it allows the connection to attach
third-party cookies. This CL provide the additional information to the
WebSocket handshake net::URLRequest.

BUG=634311

Committed: https://crrev.com/4a593833d44a457f177f99b2c907bd0f6ae397f7
Cr-Commit-Position: refs/heads/master@{#427109}
==========

[commit-bot: I haz the power, 2016-10-24 19:23:42]

Patchset 11 (id:??) landed as
https://crrev.com/4a593833d44a457f177f99b2c907bd0f6ae397f7
Cr-Commit-Position: refs/heads/master@{#427109}