Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG=623616, 632580
Committed: https://crrev.com/1b4209891f51609fe5aa84e13be6dd482ca734c7
Cr-Commit-Position: refs/heads/master@{#408588}

[hiroshige, 2016-07-28 07:53:28]

Description was changed from

==========
Call ResourceFetcher::willSendRequest() just before actual load is started

BUG=623616
==========

to

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.

BUG=623616
==========

[hiroshige, 2016-07-28 07:55:33]

Description was changed from

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.

BUG=623616
==========

to

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG=623616
==========

[hiroshige, 2016-07-28 07:55:33]

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

[hiroshige, 2016-07-28 09:58:15]

PTAL. This should be merged into M-53.

[dgozman, 2016-07-28 17:03:16]

test lgtm

[Nate Chapin, 2016-07-28 17:20:37]

lgtm

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

https://codereview.chromium.org/2184823006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:993: ResourceRequest
request(resource->resourceRequest());
It'd be nice if we could eventually return to a situation where
Resource::resourceRequest() is *exactly* the request that was given to the
WebURLLoader. It seems like that would require splitting willSendRequest up into
a prepareRequest step and a sendingTheRequestForRealNow step. That shouldn't
block this fix, though, not least because of my inability to name the 2nd step
:)

[hiroshige, 2016-07-29 05:59:12]

Description was changed from

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG=623616
==========

to

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG=623616, 632580
==========

[hiroshige, 2016-07-29 05:59:24]

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

https://codereview.chromium.org/2184823006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:993: ResourceRequest
request(resource->resourceRequest());
On 2016/07/28 17:20:37, Nate Chapin wrote:
> It'd be nice if we could eventually return to a situation where
> Resource::resourceRequest() is *exactly* the request that was given to the
> WebURLLoader.
Agree. Filed crbug.com/632580.

Splitting willSendRequest will require more changes (including devtools-side
changes) and I didn't include such changes to make this CL mergeable to
beta/M-53.

[hiroshige, 2016-07-29 05:59:32]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-07-29 05:59:53]

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-07-29 06:03:26]

Description was changed from

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG=623616, 632580
==========

to

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG=623616, 632580
==========

[commit-bot: I haz the power, 2016-07-29 06:03:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-29 06:06:18]

Description was changed from

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG=623616, 632580
==========

to

==========
Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG=623616, 632580

Committed: https://crrev.com/1b4209891f51609fe5aa84e13be6dd482ca734c7
Cr-Commit-Position: refs/heads/master@{#408588}
==========

[commit-bot: I haz the power, 2016-07-29 06:06:19]

Patchset 2 (id:??) landed as
https://crrev.com/1b4209891f51609fe5aa84e13be6dd482ca734c7
Cr-Commit-Position: refs/heads/master@{#408588}