DevTools protocol interception, blocking & modification of requests

DevTools protocol interception, blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for fetch requests and Network.interceptedRedirect for any redirects.  The fetch or redirect
is paused until one of the following commands is sent: Network.allowRequest, Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc: https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2Pzg/edit#

BUG=702384, 546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2739323003
Cr-Commit-Position: refs/heads/master@{#476635}
Committed: https://chromium.googlesource.com/chromium/src/+/6f5b3053ff11488b985c18e8d03472356e049fd5

[alex clarke (OOO till 29th), 2017-03-17 10:53:44]

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

[commit-bot: I haz the power, 2017-03-17 10:54:02]

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

[alex clarke (OOO till 29th), 2017-03-17 10:59:27]

Description was changed from

==========
PROTOTYPE: Devtools protocol interception blocking & modification of requests

NOT CURRENTLY INTENDED FOR LANDING

TODO: Explain what this is

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

to

==========
PROTOTYPE: Devtools protocol interception blocking & modification of requests

NOT CURRENTLY INTENDED FOR LANDING

TODO: Explain what this is

BUG=702384, 546953
==========

[alex clarke (OOO till 29th), 2017-03-17 11:42:47]

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

[commit-bot: I haz the power, 2017-03-17 11:43:03]

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-03-17 13:15:48]

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

[commit-bot: I haz the power, 2017-03-17 13:15:50]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-03-20 13:51:33]

Description was changed from

==========
PROTOTYPE: Devtools protocol interception blocking & modification of requests

NOT CURRENTLY INTENDED FOR LANDING

TODO: Explain what this is

BUG=702384, 546953
==========

to

==========
Devtools protocol interception blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
==========

[alex clarke (OOO till 29th), 2017-03-20 13:51:33]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[alex clarke (OOO till 29th), 2017-03-20 13:51:40]

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

[commit-bot: I haz the power, 2017-03-20 13:51:51]

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

[alex clarke (OOO till 29th), 2017-03-20 14:13:29]

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

[commit-bot: I haz the power, 2017-03-20 14:13:41]

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-03-20 14:53:50]

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

[commit-bot: I haz the power, 2017-03-20 14:53:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[alex clarke (OOO till 29th), 2017-03-20 15:20:45]

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

[commit-bot: I haz the power, 2017-03-20 15:21:04]

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-03-20 16:40:14]

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

[commit-bot: I haz the power, 2017-03-20 16:40:15]

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

[alex clarke (OOO till 29th), 2017-03-21 09:23:53]

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

[commit-bot: I haz the power, 2017-03-21 09:24:05]

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-03-21 11:05:01]

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

[commit-bot: I haz the power, 2017-03-21 11:05:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-03-21 16:22:32]

Description was changed from

==========
Devtools protocol interception blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
==========

to

==========
Devtools protocol interception blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alex clarke (OOO till 29th), 2017-03-21 16:22:41]

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

[commit-bot: I haz the power, 2017-03-21 16:23:12]

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-03-21 20:18:37]

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

[commit-bot: I haz the power, 2017-03-21 20:18:38]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-03-22 10:34:58]

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

[commit-bot: I haz the power, 2017-03-22 10:35:07]

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

[alex clarke (OOO till 29th), 2017-03-22 12:31:42]

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

[commit-bot: I haz the power, 2017-03-22 12:32:02]

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

[alex clarke (OOO till 29th), 2017-03-22 12:40:11]

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

[commit-bot: I haz the power, 2017-03-22 12:40: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, 2017-03-22 14:44:15]

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

[commit-bot: I haz the power, 2017-03-22 14:44:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sami, 2017-03-22 17:00:46]

My general feeling is that the functionality seems pretty powerful and a good
match for what we want, although  DevToolsURLInterceptorRequestJob feels a
little complicated. I wonder if there's any simpler way of delegating/mocking
request jobs that I'm missing. Maybe someone from net/ could take a look.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:31: :
net::URLRequestJob(origional_request, origional_network_delegate),
typo: original (2x)

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:69:
base::Unretained(this)));
The job is owned by net/ on the network thread, so this should probably be a
weak pointer.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:114: } else if
(io_thread_only().mock_response) {
Can we CHECK that the mock response exists here like in the other cases?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:163:
LOG(WARNING) << "Auth required to fetch URL, aborting.";
TODO to implement this?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:222:
base::Unretained(this), base::Owned(headers_dict.release()),
Ditto about WeakPtr.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:229:
std::string redirecturl) {
nit: const ref, redirect_url

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:238: void
DevToolsURLInterceptorRequestJob::OnSSLCertificateError(
I'm trying to work out if this integrates with Iris's SSL error handling patch
(https://codereview.chromium.org/2639203003)? Does the client also see these
errors there?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:275: "HTTP/1.1
%d", io_thread_only().redirect->status_code);
Missing the reason-phrase -- I'm sure there's a helper for that somewhere.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:287:
io_thread_only().sub_request.reset(new SubRequest(
Dumb question: Why can't we reuse the original request instead of creating a new
subrequest?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:313:
base::Unretained(this),
Ditto for WeakPtr

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:332: if
(modify_request->post_data.isJust())
nit: {}

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:362:
&DevToolsURLInterceptorRequestJob::ProcessMockResponeOnIoThread,
typo: response

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:401: return
std::string(reader->bytes(), reader->length());
This could be several GB. I wonder if there's a way to keep this as a stream?
Maybe add a TODO?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:456: request_ =
request_details.url_request_context->CreateRequest(
FWIW this method has a comment saying it shouldn't be used in Chromium, but
no-one is using the new alternative yet :P

https://cs.chromium.org/chromium/src/net/url_request/url_request_context.h?rc...

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:535:
std::memcpy(buf->data(), &response_bytes_.c_str()[read_offset_],
c_str() -> data() since this doesn't need null termination

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.h (right):

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:25: class
DevToolsURLInterceptorRequestJob : public net::URLRequestJob,
I'm wondering if there's any risk of wires getting crossed since
net::URLRequestJob is using the original request but this class uses a
subrequest? For instance URLRequestJob::NotifyHeadersComplete() looks at
request_ instead of the subrequest. I was wondering if it would help to delegate
to another URLRequestJob?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:61: bool
OnAllow();
minor naming nit: These 4 seem like normal methods rather than event handlers so
I'd drop the "On" prefix.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:128:
scoped_refptr<net::IOBuffer> buf_;
Is this used?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:198:
IoThreadOnly& io_thread_only() {
Thanks for making threading explicit!

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:54: std::string
parent_id = state_->GetRedirectChainParentId(request);
Just to make sure I understand, does the same |request| get reused across
redirects, but a new job may get created?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:106: std::string
id) {
nit: const ref

https://codereview.chromium.org/2739323003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2739323003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1361: { "name":
"enabled", "type": "boolean", "description": "Whether or not HTTP requests
should be intercepted and Network.interceptedRequest events sent." }
Did we decide if this can be disabled after being enabled? Or is it that the
outstanding requests still need to be allowed/blocked/etc?

https://codereview.chromium.org/2739323003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1399: { "name":
"rawResponse", "type": "string" }
Might be worth mentioning that this should contain the full HTTP response
including the status line and headers.

[alex clarke (OOO till 29th), 2017-03-23 12:25:02]

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

[alex clarke (OOO till 29th), 2017-03-23 12:25:11]

PTAL

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:31: :
net::URLRequestJob(origional_request, origional_network_delegate),
On 2017/03/22 17:00:44, Sami wrote:
> typo: original (2x)

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:69:
base::Unretained(this)));
On 2017/03/22 17:00:45, Sami wrote:
> The job is owned by net/ on the network thread, so this should probably be a
> weak pointer.

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:114: } else if
(io_thread_only().mock_response) {
On 2017/03/22 17:00:45, Sami wrote:
> Can we CHECK that the mock response exists here like in the other cases?

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:222:
base::Unretained(this), base::Owned(headers_dict.release()),
On 2017/03/22 17:00:45, Sami wrote:
> Ditto about WeakPtr.

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:229:
std::string redirecturl) {
On 2017/03/22 17:00:45, Sami wrote:
> nit: const ref, redirect_url

It can't be a const ref because this is posted by
DevToolsURLInterceptorRequestJob::OnReceivedRedirect and we'd be taking the
address of a temporary.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:275: "HTTP/1.1
%d", io_thread_only().redirect->status_code);
On 2017/03/22 17:00:44, Sami wrote:
> Missing the reason-phrase -- I'm sure there's a helper for that somewhere.

Seems pointless adding it because net::HttpResponseHeaders mostly ignores it.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:287:
io_thread_only().sub_request.reset(new SubRequest(
On 2017/03/22 17:00:45, Sami wrote:
> Dumb question: Why can't we reuse the original request instead of creating a
new
> subrequest?

I added a comment where I try to explain.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:313:
base::Unretained(this),
On 2017/03/22 17:00:44, Sami wrote:
> Ditto for WeakPtr

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:332: if
(modify_request->post_data.isJust())
On 2017/03/22 17:00:45, Sami wrote:
> nit: {}

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:362:
&DevToolsURLInterceptorRequestJob::ProcessMockResponeOnIoThread,
On 2017/03/22 17:00:45, Sami wrote:
> typo: response

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:401: return
std::string(reader->bytes(), reader->length());
On 2017/03/22 17:00:45, Sami wrote:
> This could be several GB. I wonder if there's a way to keep this as a stream?
> Maybe add a TODO?

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:456: request_ =
request_details.url_request_context->CreateRequest(
On 2017/03/22 17:00:45, Sami wrote:
> FWIW this method has a comment saying it shouldn't be used in Chromium, but
> no-one is using the new alternative yet :P
> 
>
https://cs.chromium.org/chromium/src/net/url_request/url_request_context.h?rc...

Yeah I noticed that.  I've no idea what to put in NetworkTrafficAnnotationTag.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:535:
std::memcpy(buf->data(), &response_bytes_.c_str()[read_offset_],
On 2017/03/22 17:00:45, Sami wrote:
> c_str() -> data() since this doesn't need null termination

Done.

[commit-bot: I haz the power, 2017-03-23 12:25:27]

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-03-23 14:30:30]

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

[commit-bot: I haz the power, 2017-03-23 14:30:31]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-03-23 16:12:26]

alexclarke@chromium.org changed reviewers:
+ dgozman@chromium.org, pfeldman@chromium.org

[alex clarke (OOO till 29th), 2017-03-23 16:12:27]

+Pavel and Dmitry can you take a look please?

[dgozman, 2017-03-24 00:33:14]

On 2017/03/23 16:12:27, alex clarke wrote:
> +Pavel and Dmitry can you take a look please?

Wow, that's a big patch! I'll take a careful look tomorrow.

[Sami, 2017-03-24 10:53:52]

Thanks. Btw looks like you might have missed a couple of comments in
devtools_url_interceptor_request_job.h?

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:275: "HTTP/1.1
%d", io_thread_only().redirect->status_code);
On 2017/03/23 12:25:10, alex clarke wrote:
> On 2017/03/22 17:00:44, Sami wrote:
> > Missing the reason-phrase -- I'm sure there's a helper for that somewhere.
> 
> Seems pointless adding it because net::HttpResponseHeaders mostly ignores it.

Okay, but maybe add a short comment about it since we're not following the spec
here.

[alex clarke (OOO till 29th), 2017-03-24 13:57:24]

PTAL

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.h (right):

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:25: class
DevToolsURLInterceptorRequestJob : public net::URLRequestJob,
On 2017/03/22 17:00:46, Sami wrote:
> I'm wondering if there's any risk of wires getting crossed since
> net::URLRequestJob is using the original request but this class uses a
> subrequest? For instance URLRequestJob::NotifyHeadersComplete() looks at
> request_ instead of the subrequest. I was wondering if it would help to
delegate
> to another URLRequestJob?

When we modify a request I think we do need to create a new net::URLRequest
since those changes are not supposed to be visible.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:61: bool
OnAllow();
On 2017/03/22 17:00:45, Sami wrote:
> minor naming nit: These 4 seem like normal methods rather than event handlers
so
> I'd drop the "On" prefix.

Done.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:128:
scoped_refptr<net::IOBuffer> buf_;
On 2017/03/22 17:00:45, Sami wrote:
> Is this used?

It was in earlier patch sets, can remove it now :)

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:198:
IoThreadOnly& io_thread_only() {
On 2017/03/22 17:00:45, Sami wrote:
> Thanks for making threading explicit!

Acknowledged.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:54: std::string
parent_id = state_->GetRedirectChainParentId(request);
On 2017/03/22 17:00:46, Sami wrote:
> Just to make sure I understand, does the same |request| get reused across
> redirects, but a new job may get created?

Yes. If a 302 is served or mocked then it's sent to the parent |request| which
creates a new URLRequestJob.

https://codereview.chromium.org/2739323003/diff/320001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:106: std::string
id) {
On 2017/03/22 17:00:46, Sami wrote:
> nit: const ref

Done.

[alex clarke (OOO till 29th), 2017-03-24 13:57:36]

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

[commit-bot: I haz the power, 2017-03-24 13:57:53]

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

[alex clarke (OOO till 29th), 2017-03-24 13:59:39]

We should probably land https://codereview.chromium.org/2776523005# first since
it will let us eliminate the InterceptionId and just use one consistent
RequestId.

[commit-bot: I haz the power, 2017-03-24 15:06:40]

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

[commit-bot: I haz the power, 2017-03-24 15:06:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-03-24 15:54:21]

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

[commit-bot: I haz the power, 2017-03-24 15:55:18]

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-03-24 17:53:28]

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

[commit-bot: I haz the power, 2017-03-24 17:53:29]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-03-29 15:21:44]

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

[alex clarke (OOO till 29th), 2017-03-29 15:22:08]

I've pulled the plumbing of id's out into a separate patch, PTAL

[commit-bot: I haz the power, 2017-03-29 15:22:35]

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

[dgozman, 2017-03-29 16:54:30]

I have looked into renderer-side intercepting, and it looks like the redirect
problem can be solved:

- ResourceDispatcher::OnReceivedRedirect and
ResourceDispatcher::FollowPendingRedirect do handle redirect in renderer before
proceeding in browser, so we get a chance to alter it;
- they only support "bool shouldFollow" currently, but we can probably plumb new
url instead;
- actually, the blink side was supporting new url a while ago, and that was
changed to boolean (see https://codereview.chromium.org/2230173002);
- there is a TODO around this in ResourceLoader::willFollowRedirect.

Should we talk to network team, e.g. tyoshino@, and discuss the possibility of
plumbing url instead of bool? In this case, FrameFetchContext should be able to
handle interceptions.

[commit-bot: I haz the power, 2017-03-29 16:59:34]

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

[commit-bot: I haz the power, 2017-03-29 16:59:36]

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

[alex clarke (OOO till 29th), 2017-03-30 10:42:35]

On 2017/03/29 16:54:30, dgozman wrote:
> I have looked into renderer-side intercepting, and it looks like the redirect
> problem can be solved:
> 
> - ResourceDispatcher::OnReceivedRedirect and
> ResourceDispatcher::FollowPendingRedirect do handle redirect in renderer
before
> proceeding in browser, so we get a chance to alter it;
> - they only support "bool shouldFollow" currently, but we can probably plumb
new
> url instead;

Can't blink just cancel the request and issue a new one with whatever requested
modifications?

Anyway that might be a mute point since blink can't block/modify/mock browser
side 
navigation fetches.  I think it's important to support those which suggests
interception
needs to be done browser side.


> - actually, the blink side was supporting new url a while ago, and that was
> changed to boolean (see https://codereview.chromium.org/2230173002);
> - there is a TODO around this in ResourceLoader::willFollowRedirect.
> 
> Should we talk to network team, e.g. tyoshino@, and discuss the possibility of
> plumbing url instead of bool? In this case, FrameFetchContext should be able
to
> handle interceptions.

[alex clarke (OOO till 29th), 2017-03-30 16:57:24]

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

[commit-bot: I haz the power, 2017-03-30 16:57:58]

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-03-30 17:58:36]

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

[commit-bot: I haz the power, 2017-03-30 17:58:38]

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

[alex clarke (OOO till 29th), 2017-04-12 08:37:09]

Description was changed from

==========
Devtools protocol interception blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Devtools protocol interception blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alex clarke (OOO till 29th), 2017-05-16 13:35:00]

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

[commit-bot: I haz the power, 2017-05-16 13:35:26]

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-05-16 14:11:12]

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

[commit-bot: I haz the power, 2017-05-16 14:11:13]

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

[alex clarke (OOO till 29th), 2017-05-17 08:37:06]

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

[commit-bot: I haz the power, 2017-05-17 08:37: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, 2017-05-17 09:16:38]

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

[commit-bot: I haz the power, 2017-05-17 09:16:39]

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

[alex clarke (OOO till 29th), 2017-05-17 09:21:24]

Patchset #24 (id:460001) has been deleted

[alex clarke (OOO till 29th), 2017-05-17 09:22:23]

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

[commit-bot: I haz the power, 2017-05-17 09:22: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, 2017-05-17 09:51:26]

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

[commit-bot: I haz the power, 2017-05-17 09:51:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-05-17 10:47:38]

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

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

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-05-17 11:24:54]

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

[commit-bot: I haz the power, 2017-05-17 11:24:56]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[alex clarke (OOO till 29th), 2017-05-17 12:24:05]

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

[commit-bot: I haz the power, 2017-05-17 12:24:23]

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-05-17 13:12:34]

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

[commit-bot: I haz the power, 2017-05-17 13:12:35]

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

[alex clarke (OOO till 29th), 2017-05-17 13:49:57]

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

[commit-bot: I haz the power, 2017-05-17 13:50:17]

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-05-17 14:10:33]

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

[commit-bot: I haz the power, 2017-05-17 14:10:34]

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

[alex clarke (OOO till 29th), 2017-05-17 14:23:49]

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

[commit-bot: I haz the power, 2017-05-17 14:24: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, 2017-05-17 15:36:15]

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

[commit-bot: I haz the power, 2017-05-17 15:36:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-05-18 07:27:48]

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

[commit-bot: I haz the power, 2017-05-18 07:28:26]

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-05-18 11:59:53]

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

[commit-bot: I haz the power, 2017-05-18 11:59:54]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-05-19 14:49:56]

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

[commit-bot: I haz the power, 2017-05-19 14:50:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2017-05-19 14:52:11]

Dmitry could you please take another look at this?  I've been able to get this
to work without plumbing lots of values over IPCs / headers etc...

[commit-bot: I haz the power, 2017-05-19 16:43:05]

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

[commit-bot: I haz the power, 2017-05-19 16:43:07]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-05-19 17:15:44]

Non-owner lgtm.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:24: class
ProxyUploadBytesElementReader : public net::UploadElementReader {
bikeshed: ProxyUploadElementReader? (not sure what "bytes" means here)

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.h (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:69: std::string
raw_response);
nit: const ref

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:97: // chooses
to mock the response.
Maybe also worth mentioning that the subrequest isn't visible to the interceptor
(or is it?)

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:133: const
scoped_refptr<net::HttpResponseHeaders>& response_headers() const {
nit: Maybe just return net::HttpResponseHeaders* since the caller isn't expected
to retain a reference

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:157: std::string
redirect_url);
nit: const ref

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:165: bool
waiting_for_user_response_;
Uninitialized?

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.h (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.h:75: class State {
We could also consider making the methods non-const -- seems a little odd that
they're const.

[dgozman, 2017-05-22 22:27:05]

Sorry, this patch took some time to look at :) I didn't check all the details
yet, but here are some top-level comments.

Also, there are multiple TODOs in the job implementation. Do you intent to
follow up? Is it even used in your scenarios?

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:158: int
render_frame_id = resource_request_info->GetRenderFrameID();
Note that render frame ids clash between different processes. How do you handle
it? Should we use GetFrameTreeNodeIdGetterForRequest instead?

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:219:
intercepted_frame_tree_nodes_[frame_tree_node_id] = network_handler;
It's unfortunate that you have to observe and keep track of these yourself. This
does not work with OOPIF for example, since we have complex logic which
determines NetworkHandler->RFHI mapping.

Can we retrieve NetworkHandler through RenderFrameDevToolsAgentHost instead?

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:480:
EnableFetchInterception(false);
Do this in Disable() instead.

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1115: "id":
"InterceptId",
InterceptionId ?

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1535: "name":
"modifyRequest",
What do you think about converging these methods into one?

continueRequest: {
  interceptId,
  errorReason,  // finish with error
  rawResponse,  // finish with response
  url, method, postData, headers  // optionally modify before sending to network
}

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1551: { "name":
"rawResponse", "type": "string" }
Is this base64-encoded?

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1716: "name":
"interceptedRequest",
We usually name events with noun first: requestIntercepted.

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1728: { "name":
"interceptId", "$ref": "InterceptId" },
Is this the same id as original request? Would it be easier to generate new id
and include original request to event? Something like:

requestIntercepted {
  interceptId,
  request,
  optional redirectUrl,
  optional redirectResponseHeaders,
  optional redirectStatusCode
}

[alex clarke (OOO till 29th), 2017-05-23 15:46:56]

PTAL

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:24: class
ProxyUploadBytesElementReader : public net::UploadElementReader {
On 2017/05/19 17:15:43, Sami wrote:
> bikeshed: ProxyUploadElementReader? (not sure what "bytes" means here)

Done.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.h (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:69: std::string
raw_response);
On 2017/05/19 17:15:43, Sami wrote:
> nit: const ref

Can't do that safely since this is called via posttask and the source would be
out of scope.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:97: // chooses
to mock the response.
On 2017/05/19 17:15:43, Sami wrote:
> Maybe also worth mentioning that the subrequest isn't visible to the
interceptor
> (or is it?)

Done.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:133: const
scoped_refptr<net::HttpResponseHeaders>& response_headers() const {
On 2017/05/19 17:15:43, Sami wrote:
> nit: Maybe just return net::HttpResponseHeaders* since the caller isn't
expected
> to retain a reference

Actually that's exactly what DevToolsURLInterceptorRequestJob::GetResponseInfo
does.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:157: std::string
redirect_url);
On 2017/05/19 17:15:43, Sami wrote:
> nit: const ref

Can't do that due to PostTask and the source going out of scope.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.h:165: bool
waiting_for_user_response_;
On 2017/05/19 17:15:43, Sami wrote:
> Uninitialized?

Done.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:158: int
render_frame_id = resource_request_info->GetRenderFrameID();
On 2017/05/22 22:27:04, dgozman wrote:
> Note that render frame ids clash between different processes. How do you
handle
> it? Should we use GetFrameTreeNodeIdGetterForRequest instead?

There was a bug here, it needed the process id too in the map key.

Ideally I'd use GetFrameTreeNodeIdGetterForRequest or
GetWebContentsGetterForRequest but both of those are UI thread only.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:219:
intercepted_frame_tree_nodes_[frame_tree_node_id] = network_handler;
On 2017/05/22 22:27:04, dgozman wrote:
> It's unfortunate that you have to observe and keep track of these yourself. 
Agreed, I'd rather not do that but we need to do the look up on the IO thread
which complicates matters.

> This does not work with OOPIF for example, since we have complex logic which
> determines NetworkHandler->RFHI mapping.
It seems to work, I visited several ad-encrusted sites with --site-per-process
enabled and I saw interception events for ad iframes.  Note multiple processes
can map to the same NetworkHandler.

> Can we retrieve NetworkHandler through RenderFrameDevToolsAgentHost instead?
Not sure, but it would need to work from the IO thread.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.h (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.h:75: class State {
On 2017/05/19 17:15:44, Sami wrote:
> We could also consider making the methods non-const -- seems a little odd that
> they're const.

That'd touch a lot of files, I'd prefer to do that later if we feel the need.

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2739323003/diff/560001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:480:
EnableFetchInterception(false);
On 2017/05/22 22:27:04, dgozman wrote:
> Do this in Disable() instead.

Done.

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1115: "id":
"InterceptId",
On 2017/05/22 22:27:04, dgozman wrote:
> InterceptionId ?

Done.

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1535: "name":
"modifyRequest",
On 2017/05/22 22:27:04, dgozman wrote:
> What do you think about converging these methods into one?
> 
> continueRequest: {
>   interceptId,
>   errorReason,  // finish with error
>   rawResponse,  // finish with response
>   url, method, postData, headers  // optionally modify before sending to
network
> }
> 

I guess it cuts down on the plumbing slightly.  Slightly less readable perhaps? 
Anyway it's done now.

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1551: { "name":
"rawResponse", "type": "string" }
On 2017/05/22 22:27:04, dgozman wrote:
> Is this base64-encoded?

Done.

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1716: "name":
"interceptedRequest",
On 2017/05/22 22:27:04, dgozman wrote:
> We usually name events with noun first: requestIntercepted.

Done.

https://codereview.chromium.org/2739323003/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1728: { "name":
"interceptId", "$ref": "InterceptId" },
On 2017/05/22 22:27:04, dgozman wrote:
> Is this the same id as original request? Would it be easier to generate new id
> and include original request to event? Something like:
> 
> requestIntercepted {
>   interceptId,
>   request,
>   optional redirectUrl,
>   optional redirectResponseHeaders,
>   optional redirectStatusCode
> }

I've merged them but not tried to generate a new id yet.  It would simplify
DevToolsURLRequestInterceptor::State a little bit to generate a new one but I'm
wondering if we're just shifting complexity over to the caller.  WDYT?

[alex clarke (OOO till 29th), 2017-05-23 15:47:06]

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

[commit-bot: I haz the power, 2017-05-23 15:47: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, 2017-05-23 17:54:17]

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

[commit-bot: I haz the power, 2017-05-23 17:54:18]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-05-23 18:54:30]

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

[alex clarke (OOO till 29th), 2017-05-23 18:54:53]

I've addressed the TODOs

[commit-bot: I haz the power, 2017-05-23 18:55:21]

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-05-23 21:58:54]

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

[commit-bot: I haz the power, 2017-05-23 21:58:55]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-05-24 07:38:15]

Any more feedback on this patch? :)

[dgozman, 2017-05-25 21:29:36]

Sorry for taking long to respond, I was OOO for two days. It also takes hours to
review this :)

I thought more about attributing to render frame or tree node on IO thread, and
didn't come up with anything significantly better. I feel like we should ask
loading team, and proceed with your approach for now. I can follow up with
email.

A bunch of comments below, mostly about thread safety. Overall looks pretty good
despite the complexity.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:91:
io_thread_task_runner_(
Is this used anywhere?

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:96:
devtools_url_request_interceptor_state_->RegisterJob(this, interception_id_);
if () ... since it's a weak pointer?
I also think that since interceptor state creates the job, it could register
automatically.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:125:
weak_ptr_factory_.GetWeakPtr()));
Cannot post to another thread with weak pointer created on this one.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:134:
protocol::NetworkHandler::CreateRequestFromURLRequest(request()));
You should call this method before posting to UI to avoid data races.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:216:
NOTREACHED();
Can this happen? Should we do something meaningful, like delegate to
NetworkDelegate we got when intercepting?

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:285:
weak_ptr_factory_.GetWeakPtr(),
Ditto.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:297:
protocol::NetworkHandler::CreateRequestFromURLRequest(request()),
Ditto.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:330: if
(redirect_) {
So, this is to proceed after redirect? Very nice!

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:423: if
(!devtools_url_request_interceptor_state_)
IIUC, this only happens on profile shutdown, so we can just return everywhere?

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:104:
std::move(callback).Run(CommandStatus::UnknownInterceptionId);
Wow, that's an interesting C++ construct.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:130: // because the
RenderFrameID isn't known (neither is the ChildIDe
nit: comment looks unfinished

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:138: return
sub_requests_.find(request) == sub_requests_.end();
Let's return |false| early from this method, as it's run for every single
request in chrome.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:204: for
(RenderFrameHost* render_frame_host : web_contents->GetAllFrames()) {
I don't think WebContents' methods are callable on IO thread. Same goes for
instantiation of WebContentsObserver. Let's instead do all of this on UI, and
post task per each frame (unfortunately) to register it's ids.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:218: void
DevToolsURLRequestInterceptor::State::StopInterceptingRequests(
We should also release all requests suspended at this point to continue as is.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:849:
devtools_url_request_interceptor->state().GetWeakPtr(),
It's prohibited to dereference weak pointer on the thread different from it's
creation. Instead, we should post to IO immediately, retrieve interceptor on IO
and call method directly. Mark the method as "async" in protocol_config to get a
callback instead of returning synchronously.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:903: DCHECK_EQ(1u,
stream->GetElementReaders()->size());
Is this guaranteed by net?

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:908: *post_data =
std::string(reader->bytes(), reader->length());
I think we'll need to base64-encode this? Let's have a TODO (or comment on the
bug).

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:948:
devtools_url_request_interceptor->state().GetWeakPtr(),
Similar thing here.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.h (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.h:12: #include
"base/synchronization/lock.h"
Unused?

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/interception-test.js
(right):

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/interception-test.js:34:
// sort based on the interception ID to try and make the test non-flaky.
Should we sort by url instead? That would be non-flaky at all.

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1511: "name":
"enableFetchInterception",
Sorry for bikeshedding, but let's normalize the names. I suggest having both
"request" and "intercept" in them:

InterceptionId
enableRequestInterception
continueInterceptedRequest
requestIntercepted

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1523: { "name":
"rawResponse", "type": "string", "optional": true, "description": "Id set this
makes the request complete with the provided base64 encoded mock response." },
- typo: Id set
- nit: remove "mock"
- Let's mention that this contains raw bytes sent over network including HTTP
headers and metadata.


WDYT about following up with a structured object for this? E.g. headersText,
statusText, body (basically, similar to Network.Response).

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1524: { "name":
"url", "type": "string", "optional": true, "description": "If set the request
url will be modified in a way that's not directly visible to blink." },
not observable by page

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1693:
"description": "Details of an intercepted HTTP request, which must be either
allowed, blocked, modified or mocked.",
Let's mention that redirected request gets additional notifications per
redirect, with the same interception id and redirectFOO fields from redirect
response.

[alex clarke (OOO till 29th), 2017-05-26 19:37:04]

Thanks Dmitry, I appreciate the feedback.  I'll be OOO on Monday (national
holiday).

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:91:
io_thread_task_runner_(
On 2017/05/25 21:29:35, dgozman wrote:
> Is this used anywhere?

Once several patch sets ago,  Its gone now.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:96:
devtools_url_request_interceptor_state_->RegisterJob(this, interception_id_);
On 2017/05/25 21:29:35, dgozman wrote:
> if () ... since it's a weak pointer?
> I also think that since interceptor state creates the job, it could register
> automatically.

Lets do the latter, we can reduce the number of methods.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:125:
weak_ptr_factory_.GetWeakPtr()));
On 2017/05/25 21:29:35, dgozman wrote:
> Cannot post to another thread with weak pointer created on this one.

Done.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:134:
protocol::NetworkHandler::CreateRequestFromURLRequest(request()));
On 2017/05/25 21:29:35, dgozman wrote:
> You should call this method before posting to UI to avoid data races.

Done.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:216:
NOTREACHED();
On 2017/05/25 21:29:35, dgozman wrote:
> Can this happen? Should we do something meaningful, like delegate to
> NetworkDelegate we got when intercepting?

I don't think it can happen because the SubRequest does the real fetch.  I've
added a comment to explain.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:285:
weak_ptr_factory_.GetWeakPtr(),
On 2017/05/25 21:29:35, dgozman wrote:
> Ditto.

Done.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:297:
protocol::NetworkHandler::CreateRequestFromURLRequest(request()),
On 2017/05/25 21:29:35, dgozman wrote:
> Ditto.

Done.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:330: if
(redirect_) {
On 2017/05/25 21:29:35, dgozman wrote:
> So, this is to proceed after redirect? Very nice!

Acknowledged.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:423: if
(!devtools_url_request_interceptor_state_)
On 2017/05/25 21:29:35, dgozman wrote:
> IIUC, this only happens on profile shutdown, so we can just return everywhere?

We can also make the state refcounted which guarantees correctness here.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:104:
std::move(callback).Run(CommandStatus::UnknownInterceptionId);
On 2017/05/25 21:29:35, dgozman wrote:
> Wow, that's an interesting C++ construct.

Agreed, I wish it wasn't necessary.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:130: // because the
RenderFrameID isn't known (neither is the ChildIDe
On 2017/05/25 21:29:36, dgozman wrote:
> nit: comment looks unfinished

Done.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:138: return
sub_requests_.find(request) == sub_requests_.end();
On 2017/05/25 21:29:35, dgozman wrote:
> Let's return |false| early from this method, as it's run for every single
> request in chrome.

I agree with the sentiment of failing fast, however the sub_request_ check
should only exit if the webcontents is one we've turned interception on for so I
think this is in the right place.

One thing I haven't done, and probably should is put a fast path for quitting in
the common case where we don't want to intercept anything.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:204: for
(RenderFrameHost* render_frame_host : web_contents->GetAllFrames()) {
On 2017/05/25 21:29:35, dgozman wrote:
> I don't think WebContents' methods are callable on IO thread. Same goes for
> instantiation of WebContentsObserver. Let's instead do all of this on UI, and
> post task per each frame (unfortunately) to register it's ids.

OK done. Too bad there are not more dchecks on the WebContents methods.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:218: void
DevToolsURLRequestInterceptor::State::StopInterceptingRequests(
On 2017/05/25 21:29:35, dgozman wrote:
> We should also release all requests suspended at this point to continue as is.

Good point, done.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:849:
devtools_url_request_interceptor->state().GetWeakPtr(),
On 2017/05/25 21:29:36, dgozman wrote:
> It's prohibited to dereference weak pointer on the thread different from it's
> creation. Instead, we should post to IO immediately, retrieve interceptor on
IO
> and call method directly. Mark the method as "async" in protocol_config to get
a
> callback instead of returning synchronously.

An alternative is we can make the state ref counted.  Anyway that function is
now on the UI thread.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:903: DCHECK_EQ(1u,
stream->GetElementReaders()->size());
On 2017/05/25 21:29:36, dgozman wrote:
> Is this guaranteed by net?

Looks like no although this is the common case.  I wonder what we should do if
there's more than one, maybe concatenate them together?

I wish there was a string builder class in chromium, concatenating large strings
results in unfortunate performance.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:908: *post_data =
std::string(reader->bytes(), reader->length());
On 2017/05/25 21:29:36, dgozman wrote:
> I think we'll need to base64-encode this? Let's have a TODO (or comment on the
> bug).

For reference it's simple to base 64 encode that here.  Unfortunately the other
places where Network::Request is created don't appear to base64 encode the
result.  Lets add a todo.

  base::Base64Encode(base::StringPiece(reader->bytes(), reader->length()),
                     post_data);

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:948:
devtools_url_request_interceptor->state().GetWeakPtr(),
On 2017/05/25 21:29:36, dgozman wrote:
> Similar thing here.

Done.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.h (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.h:12: #include
"base/synchronization/lock.h"
On 2017/05/25 21:29:36, dgozman wrote:
> Unused?

Done.

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/interception-test.js
(right):

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/interception-test.js:34:
// sort based on the interception ID to try and make the test non-flaky.
On 2017/05/25 21:29:36, dgozman wrote:
> Should we sort by url instead? That would be non-flaky at all.

AFAIK the order of requests is deterministic.

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1511: "name":
"enableFetchInterception",
On 2017/05/25 21:29:36, dgozman wrote:
> Sorry for bikeshedding, but let's normalize the names. I suggest having both
> "request" and "intercept" in them:
> 
> InterceptionId
> enableRequestInterception
> continueInterceptedRequest
> requestIntercepted

Done.

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1523: { "name":
"rawResponse", "type": "string", "optional": true, "description": "Id set this
makes the request complete with the provided base64 encoded mock response." },
On 2017/05/25 21:29:36, dgozman wrote:
> - typo: Id set
> - nit: remove "mock"
> - Let's mention that this contains raw bytes sent over network including HTTP
> headers and metadata.
> 
> 
> WDYT about following up with a structured object for this? E.g. headersText,
> statusText, body (basically, similar to Network.Response).

I can see that might be useful.  Let do that in a follow up!  Added a todo.

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1524: { "name":
"url", "type": "string", "optional": true, "description": "If set the request
url will be modified in a way that's not directly visible to blink." },
On 2017/05/25 21:29:36, dgozman wrote:
> not observable by page

Done.

https://codereview.chromium.org/2739323003/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1693:
"description": "Details of an intercepted HTTP request, which must be either
allowed, blocked, modified or mocked.",
On 2017/05/25 21:29:36, dgozman wrote:
> Let's mention that redirected request gets additional notifications per
> redirect, with the same interception id and redirectFOO fields from redirect
> response.

Done.

[alex clarke (OOO till 29th), 2017-05-26 19:37:16]

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

[commit-bot: I haz the power, 2017-05-26 19:38: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, 2017-05-26 19:47:19]

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

[commit-bot: I haz the power, 2017-05-26 19:47:20]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alex clarke (OOO till 29th), 2017-05-27 08:37:55]

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

[commit-bot: I haz the power, 2017-05-27 08:37:59]

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-05-27 11:11:29]

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

[commit-bot: I haz the power, 2017-05-27 11:11:31]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-05-30 21:44:27]

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:138: return
sub_requests_.find(request) == sub_requests_.end();
On 2017/05/26 19:37:03, alex clarke wrote:
> On 2017/05/25 21:29:35, dgozman wrote:
> > Let's return |false| early from this method, as it's run for every single
> > request in chrome.
> 
> I agree with the sentiment of failing fast, however the sub_request_ check
> should only exit if the webcontents is one we've turned interception on for so
I
> think this is in the right place.
Does this mean we have sub requests for webcontents we are not intercepting?
How's that possible?

> 
> One thing I haven't done, and probably should is put a fast path for quitting
in
> the common case where we don't want to intercept anything.
Good idea.

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:903: DCHECK_EQ(1u,
stream->GetElementReaders()->size());
On 2017/05/26 19:37:03, alex clarke wrote:
> On 2017/05/25 21:29:36, dgozman wrote:
> > Is this guaranteed by net?
> 
> Looks like no although this is the common case.  I wonder what we should do if
> there's more than one, maybe concatenate them together?
> 
> I wish there was a string builder class in chromium, concatenating large
strings
> results in unfortunate performance.

IIUC, std::string += operator is smart since it uses vector with 2x growth
factor inside.

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:72: return
state()->CreateDevToolsURLInterceptorRequestJob(
Perhaps, it makes sense to combine ShouldInterceptRequest with
CreateDevToolsURLInterceptorRequestJob, and make InterceptedPage a private
entity of the State?

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.h (right):

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.h:110: const
InterceptedPage** intercepted_page) const;
nit: can just return the struct by value, since you allocate one on the stack
anyway. I think this will also eliminate default and copy constructors.

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:943: BrowserThread::IO,
FROM_HERE,
For uniformity with enabling/disabling, I'd call
State::ContinueInterceptedRequest on UI, passing
ContinueInterceptedRequestCallback to it, and let it handle the thread hopping.
The helper ProcessContinueInterceptedRequestResultOnIoThread would just inline
there as well. We can also get rid of CommandStatus enum.

https://codereview.chromium.org/2739323003/diff/610001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2739323003/diff/610001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1705: { "name":
"InterceptionId", "$ref": "InterceptionId", "description": "Each request blink
makes will have a unique id, however if any redirects are encountered while
processing that fetch, they will be reported with the same id as the original
fetch." },
nit: blink -> page

[alex clarke (OOO till 29th), 2017-05-31 15:50:58]

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

[commit-bot: I haz the power, 2017-05-31 15:51:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2017-05-31 15:52:02]

PTAL

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:138: return
sub_requests_.find(request) == sub_requests_.end();
On 2017/05/30 21:44:27, dgozman wrote:
> On 2017/05/26 19:37:03, alex clarke wrote:
> > On 2017/05/25 21:29:35, dgozman wrote:
> > > Let's return |false| early from this method, as it's run for every single
> > > request in chrome.
> > 
> > I agree with the sentiment of failing fast, however the sub_request_ check
> > should only exit if the webcontents is one we've turned interception on for
so
> I
> > think this is in the right place.
> Does this mean we have sub requests for webcontents we are not intercepting?
> How's that possible?

It's not :)

There is a 1:1 relationship between a DevToolsURLInterceptorRequestJob and the
actual sub request used to perform the network fetch.  I implemented it this way
for a number of reasons:

1. To let one interceptor work across all protocols.
2. Because we want to be careful what the net::NetworkDelegate associated with
the intercepted request sees - e.g.if  we want to be able to convert a redirect
into a mocked resource, the net::NetworkDelegate must not see the redirect.


> 
> > 
> > One thing I haven't done, and probably should is put a fast path for
quitting
> in
> > the common case where we don't want to intercept anything.
> Good idea.
>

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2739323003/diff/590001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:903: DCHECK_EQ(1u,
stream->GetElementReaders()->size());
On 2017/05/30 21:44:27, dgozman wrote:
> On 2017/05/26 19:37:03, alex clarke wrote:
> > On 2017/05/25 21:29:36, dgozman wrote:
> > > Is this guaranteed by net?
> > 
> > Looks like no although this is the common case.  I wonder what we should do
if
> > there's more than one, maybe concatenate them together?
> > 
> > I wish there was a string builder class in chromium, concatenating large
> strings
> > results in unfortunate performance.
> 
> IIUC, std::string += operator is smart since it uses vector with 2x growth
> factor inside.

Mmm OK that should help.  I'm just worried about potential O(n^2) nastiness if
there's a lot of element readers. I hope that's unlikely!

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:72: return
state()->CreateDevToolsURLInterceptorRequestJob(
On 2017/05/30 21:44:27, dgozman wrote:
> Perhaps, it makes sense to combine ShouldInterceptRequest with
> CreateDevToolsURLInterceptorRequestJob, and make InterceptedPage a private
> entity of the State?

Done.

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.h (right):

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.h:110: const
InterceptedPage** intercepted_page) const;
On 2017/05/30 21:44:27, dgozman wrote:
> nit: can just return the struct by value, since you allocate one on the stack
> anyway. I think this will also eliminate default and copy constructors.

I was able to get rid of the default constructor by using emplace.  Chromium
style candidates the copy constructor however (as enforced by a clang plugin).

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:943: BrowserThread::IO,
FROM_HERE,
On 2017/05/30 21:44:27, dgozman wrote:
> For uniformity with enabling/disabling, I'd call
> State::ContinueInterceptedRequest on UI, passing
> ContinueInterceptedRequestCallback to it, and let it handle the thread
hopping.
> The helper ProcessContinueInterceptedRequestResultOnIoThread would just inline
> there as well. We can also get rid of CommandStatus enum.

Done.

https://codereview.chromium.org/2739323003/diff/610001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2739323003/diff/610001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1705: { "name":
"InterceptionId", "$ref": "InterceptionId", "description": "Each request blink
makes will have a unique id, however if any redirects are encountered while
processing that fetch, they will be reported with the same id as the original
fetch." },
On 2017/05/30 21:44:27, dgozman wrote:
> nit: blink -> page

Done.

[alex clarke (OOO till 29th), 2017-05-31 15:54:37]

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.h (right):

https://codereview.chromium.org/2739323003/diff/610001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.h:110: const
InterceptedPage** intercepted_page) const;
On 2017/05/31 15:52:02, alex clarke wrote:
> On 2017/05/30 21:44:27, dgozman wrote:
> > nit: can just return the struct by value, since you allocate one on the
stack
> > anyway. I think this will also eliminate default and copy constructors.
> 
> I was able to get rid of the default constructor by using emplace.  Chromium
> style candidates the copy constructor however (as enforced by a clang plugin).

Whoops I meant to say Chromium style mandates the copy constructor.

[commit-bot: I haz the power, 2017-05-31 16:25:44]

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

[commit-bot: I haz the power, 2017-05-31 16:25:46]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dgozman, 2017-05-31 17:01:10]

Do we cover both resources (e.g. script tag or XHR) and navigations in tests? Do
the tests pass with browser side navigation? I think we should follow up with
more coverage probably, for example checking that on detach we continue all the
requests.

Thank you for working on this big feature! lgtm

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:113:
base::Owned(callback.release()),
I think base::Passed(&callback) is the idiomatic way.

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:122:
base::Owned(callback.release())));
ditto

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:128:
base::Owned(callback.release()),
ditto

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:385:
DevToolsURLRequestInterceptor::State::InterceptedPage::InterceptedPage(
nit: could use =default for this.

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:395:
DevToolsURLRequestInterceptor::State::InterceptedPage::~InterceptedPage() {}
ditto

[alex clarke (OOO till 29th), 2017-06-01 10:28:14]

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

[commit-bot: I haz the power, 2017-06-01 10:28:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2017-06-01 10:36:33]

Patchset #34 (id:670001) has been deleted

[alex clarke (OOO till 29th), 2017-06-01 10:41:55]

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

[commit-bot: I haz the power, 2017-06-01 10:42:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2017-06-01 10:44:10]

On 2017/05/31 17:01:10, dgozman wrote:
> Do we cover both resources (e.g. script tag or XHR) and navigations in tests? 
I added a test for XHRs and a more explicit one for navigation fetches.

> the tests pass with browser side navigation? 
They do now.  I had to prevent (pointless) interception of blob: resources.

>I think we should follow up with
> more coverage probably, for example checking that on detach we continue all
the
> requests.
I added a test that detaches half way.  Please let me know if you'd like to see
any
other tests.

> Thank you for working on this big feature! lgtm

Thanks for the review!

> 
>
https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
> File content/browser/devtools/devtools_url_request_interceptor.cc (right):
> 
>
https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
> content/browser/devtools/devtools_url_request_interceptor.cc:113:
> base::Owned(callback.release()),
> I think base::Passed(&callback) is the idiomatic way.
> 
>
https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
> content/browser/devtools/devtools_url_request_interceptor.cc:122:
> base::Owned(callback.release())));
> ditto
> 
>
https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
> content/browser/devtools/devtools_url_request_interceptor.cc:128:
> base::Owned(callback.release()),
> ditto
> 
>
https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
> content/browser/devtools/devtools_url_request_interceptor.cc:385:
> DevToolsURLRequestInterceptor::State::InterceptedPage::InterceptedPage(
> nit: could use =default for this.
> 
>
https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
> content/browser/devtools/devtools_url_request_interceptor.cc:395:
> DevToolsURLRequestInterceptor::State::InterceptedPage::~InterceptedPage() {}
> ditto

[alex clarke (OOO till 29th), 2017-06-01 10:45:33]

The CQ bit was unchecked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-06-01 10:45:33]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-06-01 10:45:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2739323003/#ps690001
(title: "Don't try to intercept blob resources")

[commit-bot: I haz the power, 2017-06-01 10:45:46]

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-06-01 12:01:20]

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

[commit-bot: I haz the power, 2017-06-01 12:01:22]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[alex clarke (OOO till 29th), 2017-06-01 12:06:41]

Thanks for the review!

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
File content/browser/devtools/devtools_url_request_interceptor.cc (right):

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:113:
base::Owned(callback.release()),
On 2017/05/31 17:01:10, dgozman wrote:
> I think base::Passed(&callback) is the idiomatic way.

IIRC you have to do base::Passed(std::move(callback)) but yes I've changed this
to use  base::Passed instead.

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:122:
base::Owned(callback.release())));
On 2017/05/31 17:01:10, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:128:
base::Owned(callback.release()),
On 2017/05/31 17:01:10, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:385:
DevToolsURLRequestInterceptor::State::InterceptedPage::InterceptedPage(
On 2017/05/31 17:01:10, dgozman wrote:
> nit: could use =default for this.

Done.  Too bad it still has to be out of line.

https://codereview.chromium.org/2739323003/diff/630001/content/browser/devtoo...
content/browser/devtools/devtools_url_request_interceptor.cc:395:
DevToolsURLRequestInterceptor::State::InterceptedPage::~InterceptedPage() {}
On 2017/05/31 17:01:10, dgozman wrote:
> ditto

Done.

[alex clarke (OOO till 29th), 2017-06-01 12:17:26]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-06-01 12:17:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2739323003/#ps710001
(title: "Try remove explicit from InterceptedPage copy constructor.")

[commit-bot: I haz the power, 2017-06-01 12:17:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2017-06-01 12:18:55]

Description was changed from

==========
Devtools protocol interception blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
DevTools protocol interception, blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-06-01 13:28:48]

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

[commit-bot: I haz the power, 2017-06-01 13:28:50]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-06-01 13:46:48]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-06-01 13:46:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2739323003/#ps730001
(title: "Add missing file and deflake disable-interception-midway.html")

[commit-bot: I haz the power, 2017-06-01 13:47:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2017-06-01 15:59:31]

The CQ bit was unchecked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-06-01 21:11:32]

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

[commit-bot: I haz the power, 2017-06-01 21:12:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2017-06-01 22:41:54]

Looks like the Auth stuff actually was broken.  I think I've fixed it, could you
please take another look?  Thanks!

[alex clarke (OOO till 29th), 2017-06-01 22:42:33]

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

[commit-bot: I haz the power, 2017-06-01 22:43:49]

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-06-02 02:38:11]

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

[commit-bot: I haz the power, 2017-06-02 02:38:14]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-06-02 09:59:47]

Auth proxying changes lgtm from my point of view.

https://codereview.chromium.org/2739323003/diff/770001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/770001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:263:
NotifyHeadersComplete();
Let's a add a short comment explaining the flow here.

[alex clarke (OOO till 29th), 2017-06-02 10:16:59]

Thanks Sami.

https://codereview.chromium.org/2739323003/diff/770001/content/browser/devtoo...
File content/browser/devtools/devtools_url_interceptor_request_job.cc (right):

https://codereview.chromium.org/2739323003/diff/770001/content/browser/devtoo...
content/browser/devtools/devtools_url_interceptor_request_job.cc:263:
NotifyHeadersComplete();
On 2017/06/02 09:59:47, Sami wrote:
> Let's a add a short comment explaining the flow here.

Done.

[alex clarke (OOO till 29th), 2017-06-02 10:17:36]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-06-02 10:17:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2739323003/#ps790001
(title: "Add a comment")

[commit-bot: I haz the power, 2017-06-02 10:17:49]

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-06-02 12:16:02]

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

[commit-bot: I haz the power, 2017-06-02 12:16:05]

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

[alex clarke (OOO till 29th), 2017-06-02 12:20:17]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2017-06-02 12:20:34]

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-06-02 14:32:34]

CQ is committing da patch.

Bot data: {"patchset_id": 790001, "attempt_start_ts": 1496406017759140,
"parent_rev": "4cb7c3991639f0275c790e357cf80d3a0565a028", "commit_rev":
"6f5b3053ff11488b985c18e8d03472356e049fd5"}

[commit-bot: I haz the power, 2017-06-02 14:32:48]

Description was changed from

==========
DevTools protocol interception, blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
DevTools protocol interception, blocking & modification of requests

Adds Network.enableFetchInterception which sends Network.interceptedRequest for
fetch requests and Network.interceptedRedirect for any redirects.  The fetch or
redirect
is paused until one of the following commands is sent: Network.allowRequest,
Network.blockRequest, Network.mockResponse, Network.modifyRequest.

Design doc:
https://docs.google.com/document/d/1nEN0SqC1jEpHgKuQPrppwSabC8JRAuJb7MSTGCe2P...

BUG=702384, 546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2739323003
Cr-Commit-Position: refs/heads/master@{#476635}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f5b3053ff11488b985c18e8d034...
==========

[commit-bot: I haz the power, 2017-06-02 14:32:50]

Committed patchset #39 (id:790001) as
https://chromium.googlesource.com/chromium/src/+/6f5b3053ff11488b985c18e8d034...