Split header modification out of willSendRequest

Split header modification out of willSendRequest

Add a new FetchContext callback, prepareRequest, which provides a final opportunity to modify ResourceRequest headers before it is sent. Call it before Resource creation, so that Resource::m_resourceRequest is exactly the request sent out of blink.

Make willSendRequest() take a const ResourceRequest&; it is now purely informational.

BUG=632580

[Nate Chapin, 2016-08-02 21:08:33]

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

[commit-bot: I haz the power, 2016-08-02 21:09: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, 2016-08-02 22:53:30]

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

[commit-bot: I haz the power, 2016-08-02 22:53:31]

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

[Nate Chapin, 2016-08-02 23:03:10]

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

[commit-bot: I haz the power, 2016-08-02 23:03:42]

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

[commit-bot: I haz the power, 2016-08-02 23:29:47]

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

[commit-bot: I haz the power, 2016-08-02 23:29:48]

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

[Nate Chapin, 2016-08-03 18:58:32]

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

[commit-bot: I haz the power, 2016-08-03 18:59:01]

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

[commit-bot: I haz the power, 2016-08-03 20:35:49]

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

[commit-bot: I haz the power, 2016-08-03 20:35:49]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-08-03 22:27:45]

Description was changed from

==========
Split header modification out of willSendRequest

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

to

==========
Split header modification out of willSendRequest

Add a new FetchContext callback, prepareRequest, which provides a final
opportunity to modify ResourceRequest headers before it is sent. Call it before
Resource creation, so that Resource::m_resourceRequest is exactly the request
sent out of blink.

Make willSendRequest() take a const ResourceRequest&; it is now purely
informational.

BUG=632580
==========

[Nate Chapin, 2016-08-03 22:28:09]

japhet@chromium.org changed reviewers:
+ hiroshige@chromium.org

[Nate Chapin, 2016-08-03 22:28:10]

hiroshige, how does this look?

[hiroshige, 2016-08-04 09:51:14]

Code basically looks good, thanks!

Could you explain why the chages to network-memory-cached-resource.html are
needed?

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (left):

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:545: //
Ignore the request initiated internally.
Don't we need these |if| statements in InspectorNetworkAgent::prepareRequest()?

[Nate Chapin, 2016-08-04 21:21:54]

japhet@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Nate Chapin, 2016-08-04 21:21:55]

pfeldman, do you have an opinion on this? InspectorNetworkAgent currently
refrains from putting its custom headers on internally initiated
ResourceRequests as a byproduct of ensuring that those requests don't appear in
the devtools frontend. I'm splitting the callbacks into two: one to set headers
and one to notify the devtools frontend. Should internal requests early-exit
from the callback for setting headers? Either way is fine, but there's some
additional plumbing required to skip setting headers on internal requests.

internal requests are, at the moment: blob loads from FileReaderLoader and
FetchBlobDataConsumerHandle, and InspectorResourceContentLoader.

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (left):

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:545: //
Ignore the request initiated internally.
On 2016/08/04 09:51:14, hiroshige wrote:
> Don't we need these |if| statements in
InspectorNetworkAgent::prepareRequest()?

Dropping the subsituteData() check is benign, since this code just sets HTTP
headers that will be ignored for a substituteData() load.

I'm not sure about the internal check.

[pfeldman, 2016-08-04 21:39:59]

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:572:
request.addHTTPHeaderField(HTTPNames::X_DevTools_Emulate_Network_Conditions_Client_Id,
AtomicString(m_hostId));
Do you mean that these are now getting reported over the protocol? We should
move it into the willSendRequest intead after reporting to the front-end (to the
line 548).

[Nate Chapin, 2016-08-04 21:46:50]

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:572:
request.addHTTPHeaderField(HTTPNames::X_DevTools_Emulate_Network_Conditions_Client_Id,
AtomicString(m_hostId));
On 2016/08/04 21:39:59, pfeldman wrote:
> Do you mean that these are now getting reported over the protocol? We should
> move it into the willSendRequest intead after reporting to the front-end (to
the
> line 548).

I think that's what this would mean, yeah. Will move.

[Nate Chapin, 2016-08-04 22:50:45]

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

[commit-bot: I haz the power, 2016-08-04 22:51:11]

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

[Nate Chapin, 2016-08-04 22:51:31]

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2203613003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:572:
request.addHTTPHeaderField(HTTPNames::X_DevTools_Emulate_Network_Conditions_Client_Id,
AtomicString(m_hostId));
On 2016/08/04 21:46:49, Nate Chapin wrote:
> On 2016/08/04 21:39:59, pfeldman wrote:
> > Do you mean that these are now getting reported over the protocol? We should
> > move it into the willSendRequest intead after reporting to the front-end (to
> the
> > line 548).
> 
> I think that's what this would mean, yeah. Will move.

So, my theory had been that willSendRequest would take a const ResourceRequest,
because this is exactly the request that blink is sending to the browser
process. Would it suffice to filter this header out when constructing the
protocol::Network::Headers in buildObjectForHeaders(), instead of delaying
setting the header until after requestWillBeSent()?

I've updated with the filter, let me know what you think.

[Nate Chapin, 2016-08-04 23:38:33]

On 2016/08/04 09:51:14, hiroshige wrote:
> Code basically looks good, thanks!
> 
> Could you explain why the chages to network-memory-cached-resource.html are
> needed?
> 

InspectorNetworkAgent now sets its custom headers in prepareRequest(), which is
called before the Resource is created and before
ResourceFetcher::determineRevalidationPolicy is called. When
NetworkAgentState::cacheDisabled is true, as it is in this test, it sets
WebCachePolicy::BypassingCache, which is a factor in
determineRevalidationPolicy(). Reenabling the cache policy earlier ensures that
the second request of abe.png is returned from the memory cache, as the test
expects.

[commit-bot: I haz the power, 2016-08-05 00:33:24]

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

[commit-bot: I haz the power, 2016-08-05 00:33:25]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-08-05 11:02:48]

On 2016/08/04 23:38:33, Nate Chapin wrote:
> On 2016/08/04 09:51:14, hiroshige wrote:
> > Code basically looks good, thanks!
> > 
> > Could you explain why the chages to network-memory-cached-resource.html are
> > needed?
> > 
> 
> InspectorNetworkAgent now sets its custom headers in prepareRequest(), which
is
> called before the Resource is created and before
> ResourceFetcher::determineRevalidationPolicy is called. When
> NetworkAgentState::cacheDisabled is true, as it is in this test, it sets
> WebCachePolicy::BypassingCache, which is a factor in
> determineRevalidationPolicy(). Reenabling the cache policy earlier ensures
that
> the second request of abe.png is returned from the memory cache, as the test
> expects.

Thanks!

Does this mean that "Disable Cache" functionality of DevTools didn't disable
MemoryCache?

[Nate Chapin, 2016-08-05 16:45:29]

On 2016/08/05 11:02:48, hiroshige wrote:
> On 2016/08/04 23:38:33, Nate Chapin wrote:
> > On 2016/08/04 09:51:14, hiroshige wrote:
> > > Code basically looks good, thanks!
> > > 
> > > Could you explain why the chages to network-memory-cached-resource.html
are
> > > needed?
> > > 
> > 
> > InspectorNetworkAgent now sets its custom headers in prepareRequest(), which
> is
> > called before the Resource is created and before
> > ResourceFetcher::determineRevalidationPolicy is called. When
> > NetworkAgentState::cacheDisabled is true, as it is in this test, it sets
> > WebCachePolicy::BypassingCache, which is a factor in
> > determineRevalidationPolicy(). Reenabling the cache policy earlier ensures
> that
> > the second request of abe.png is returned from the memory cache, as the test
> > expects.
> 
> Thanks!
> 
> Does this mean that "Disable Cache" functionality of DevTools didn't disable
> MemoryCache?

Sort of. I believe it clears the cache when "Disable cache" is set, but I don't
think it prevented Resources from being added to the cache after that point.

[hiroshige, 2016-08-09 10:08:39]

On 2016/08/05 16:45:29, Nate Chapin wrote:
> On 2016/08/05 11:02:48, hiroshige wrote:
> > On 2016/08/04 23:38:33, Nate Chapin wrote:
> > > On 2016/08/04 09:51:14, hiroshige wrote:
> > > > Code basically looks good, thanks!
> > > > 
> > > > Could you explain why the chages to network-memory-cached-resource.html
> are
> > > > needed?
> > > > 
> > > 
> > > InspectorNetworkAgent now sets its custom headers in prepareRequest(),
which
> > is
> > > called before the Resource is created and before
> > > ResourceFetcher::determineRevalidationPolicy is called. When
> > > NetworkAgentState::cacheDisabled is true, as it is in this test, it sets
> > > WebCachePolicy::BypassingCache, which is a factor in
> > > determineRevalidationPolicy(). Reenabling the cache policy earlier ensures
> > that
> > > the second request of abe.png is returned from the memory cache, as the
test
> > > expects.
> > 
> > Thanks!
> > 
> > Does this mean that "Disable Cache" functionality of DevTools didn't disable
> > MemoryCache?
> 
> Sort of. I believe it clears the cache when "Disable cache" is set, but I
don't
> think it prevented Resources from being added to the cache after that point.

Hmm. I'm now not sure what is expected behavior.
For example:
1. Open http://www.example.com/
2. Open DevTools
3. On the Network tab, switch on "Disable Cache"
4. On the Console tab, execute the following four times:
   img = new Image(); img.src =
'https://www.google.co.jp/logos/doodles/2016/2016-doodle-fruit-games-day-5-5688836437835776-res.png';
document.body.appendChild(img);
5. See the log on the Network tab.

Previously, only one request to the PNG file appeared in Step 5.
Other three requests reused the same Resource via MemoryCache (which didn't
appear on DevTools).

With this CL, four resuests to the PNG file appears in Step 5,
because reusing via MemoryCache is disabled by BypassingCache flag set before
determineRevalidationPolicy(), and thus each image create a separate Resource
and initiates a separate network request.
This is more like "disabling MemoryCache" than the previous behavior, but also
this is different from the behavior without DevTools; usually we assume one
Resource is shared across multiple <img> tags on a single page but "Disable
Cache" button breaks the assumption.

(Usual subresource loads are merged into a single Resource with or without this
CL, even with "Disable Cache", because the |if (existingResource->isLoading())
return Use;| check is placed above the check for WebCachePolicy::BypassingCache)

WDYT?

[hiroshige, 2016-09-27 09:38:52]

hiroshige@chromium.org changed reviewers:
+ dgozman@chromium.org, shaochuan@chromium.org

[hiroshige, 2016-09-27 09:38:53]

(Restarting the review as shaochuan@ is touching related code)

> My previous comment #29
https://codereview.chromium.org/2203613003/#msg29
DevTools people (pfeldman@, +dgozman@), how do you think about this behavior
change?
I expect the new behavior is more consistent in terms of loading code, but if
the user-observable change is not good, we might have to change e.g.
setCachePolicy() in InspectorNetworkAgent.cpp.

Except for that point, I think the code looks good.

[Shao-Chuan Lee, 2016-10-06 03:34:41]

https://codereview.chromium.org/2203613003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2203613003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1028:
loader->start(resource->resourceRequest(), context().loadingTaskRunner(),
context().defersLoading());
Maybe we can now remove the first param in ResourceLoader::start() and use
m_resource->resourceRequest()?

[Shao-Chuan Lee, 2016-10-25 05:47:46]

On 2016/10/06 03:34:41, Shao-Chuan Lee wrote:
>
https://codereview.chromium.org/2203613003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/2203613003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1028:
> loader->start(resource->resourceRequest(), context().loadingTaskRunner(),
> context().defersLoading());
> Maybe we can now remove the first param in ResourceLoader::start() and use
> m_resource->resourceRequest()?

May I follow up current progress? I'm having plenty of workarounds for this, it
would be nice to see this issue resolved. Thanks.

[dgozman, 2016-11-16 05:56:01]

dgozman@chromium.org changed reviewers:
- dgozman@chromium.org

[hiroshige, 2016-12-07 04:37:58]

hiroshige@chromium.org changed reviewers:
+ tyoshino@chromium.org

[pfeldman, 2017-02-23 01:20:12]

pfeldman@chromium.org changed reviewers:
- pfeldman@chromium.org