Refactoring starting a resource load

Refactoring starting a resource load

Move most of Resource::load to a new ResourceFetcher::startLoad,
which is well-positioned to verify that FetchContext is still
valid. This also means that deferred loads call ResourceFetcher
to actually start the load, just as they called ResourceFetcher
to initialize the load.

BUG=603428
Committed: https://crrev.com/5b689eeb4cc9dc5636a8db73c79a2674a154e967
Cr-Commit-Position: refs/heads/master@{#399017}

[Nate Chapin, 2016-04-15 19:22:59]

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

[commit-bot: I haz the power, 2016-04-15 19:23:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1889973002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1889973002/20001

[Nate Chapin, 2016-04-18 19:56:02]

Description was changed from

==========
Refactoring starting a resource load

BUG=603428
==========

to

==========
Refactoring starting a resource load

Move most of Resource::load to a new ResourceFetcher::startLoad,
which is well-positioned to verify that FetchContext is still
valid. This also means that deferred loads call ResourceFetcher
to actually start the load, just as they called ResourceFetcher
to initialize the load.

BUG=603428
==========

[Nate Chapin, 2016-04-18 19:56:55]

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

[Nate Chapin, 2016-04-18 19:56:55]

This is a followup to
https://chromium.googlesource.com/chromium/src/+/fa123915549f2be2b85ed62f2a43...,
partly because I think this way is cleaner, and partly to make it harder to make
the same mistake again in the future :)

[Yoav Weiss, 2016-04-20 08:52:07]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2016-04-20 08:52:08]

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:506:
fetcher->startLoad(this);
Can you DCHECK that fetcher is non-null here?

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

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:820:
ASSERT(resource->stillNeedsLoad());
I believe ASSERT is now deprecated and DCHECK is the new way to go here. Also,
can you DCHECK that resource is non-null?

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:101: if
(m_resource->options().synchronousPolicy == RequestSynchronously &&
defersLoading) {
Does that constitute a behavior change? I may be reading this wrong, but seems
to me like sync requests were sent before and are not sent now.

[hiroshige, 2016-04-21 08:27:40]

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:258: if
(!fetcher->loadingTaskRunner())
Is this check removed?
(Or loadingTaskRunner() is guaranteed to be non-null when
shouldLoadNewResource() is true?)

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:253: ResourceRequest&
Resource::prepareRequestForLoadStart()
I'm not sure whether it is good to split load() into
prepareRequestForLoadStart(), setLoader() and a part of
ResourceFetcher::startLoad():
- All of them should be called only when start loading and interact with each
other.
- prepareRequestForLoadStart() exposes a mutable reference to
|m_resourceRequest|.

How about keeping the original Resource::load() and adding |loadingTaskRunner|
and |defersLoading| parameters to Resource::load()?

(In this case, it would be better if we can make Resource::load() only callable
from ResourceFetcher, but I feel we can just add comments and/or rename it to
startLoadInternal() or something that doesn't look callable from everywhere)

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.h (right):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.h:44: class
WebLoadingTaskRunner;
Where is this forward daclaration used?

[Nate Chapin, 2016-04-22 20:30:13]

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

[commit-bot: I haz the power, 2016-04-22 20:30:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1889973002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1889973002/80001

[commit-bot: I haz the power, 2016-04-22 20:49:09]

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

[commit-bot: I haz the power, 2016-04-22 20:49:10]

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

[Mike West, 2016-04-26 07:55:44]

I think the approach looks reasonable, but the bots are super-unhappy, and the
failures look relevant. :(

[Nate Chapin, 2016-05-18 23:56:05]

Ok, I think I've worked through the problems and ended with something I'm more
or less happy with. Does this look cleaner than the status quo?

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:506:
fetcher->startLoad(this);
On 2016/04/20 08:52:08, Yoav Weiss wrote:
> Can you DCHECK that fetcher is non-null here?

This is only called by ResourceFetcher at the moment, FWIW. I'm not opposed to
the DCHECK, but given that none of the FooResource::fetch() methods null-check
their ResourceFethcer*, I'm not sure how important it is.

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:258: if
(!fetcher->loadingTaskRunner())
On 2016/04/21 08:27:40, hiroshige wrote:
> Is this check removed?
> (Or loadingTaskRunner() is guaranteed to be non-null when
> shouldLoadNewResource() is true?)

Correct, loadingTaskRunner() being null and shouldLoadNewResource() returning
false should be the same case.

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:253: ResourceRequest&
Resource::prepareRequestForLoadStart()
On 2016/04/21 08:27:40, hiroshige wrote:
> I'm not sure whether it is good to split load() into
> prepareRequestForLoadStart(), setLoader() and a part of
> ResourceFetcher::startLoad():
> - All of them should be called only when start loading and interact with each
> other.
> - prepareRequestForLoadStart() exposes a mutable reference to
> |m_resourceRequest|.
> 
> How about keeping the original Resource::load() and adding |loadingTaskRunner|
> and |defersLoading| parameters to Resource::load()?
> 
> (In this case, it would be better if we can make Resource::load() only
callable
> from ResourceFetcher, but I feel we can just add comments and/or rename it to
> startLoadInternal() or something that doesn't look callable from everywhere)

I refactored a bit more to enable this to be just requestForLoadStart() and be
const. It adds some complexity to ResourceFetcher::requestResource(), but I
think it's probably(?) cleaner on the whole.

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

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:820:
ASSERT(resource->stillNeedsLoad());
On 2016/04/20 08:52:08, Yoav Weiss wrote:
> I believe ASSERT is now deprecated and DCHECK is the new way to go here. Also,
> can you DCHECK that resource is non-null?

Done.

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:101: if
(m_resource->options().synchronousPolicy == RequestSynchronously &&
defersLoading) {
On 2016/04/20 08:52:08, Yoav Weiss wrote:
> Does that constitute a behavior change? I may be reading this wrong, but seems
> to me like sync requests were sent before and are not sent now.

No behavior change. I moved this from requestSynchronously() below, so that we
don't also have to pass the defersLoading bool to requestSynchronously.

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.h (right):

https://codereview.chromium.org/1889973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.h:44: class
WebLoadingTaskRunner;
On 2016/04/21 08:27:40, hiroshige wrote:
> Where is this forward daclaration used?

Oops, I don't know what happened there, removed.

https://codereview.chromium.org/1889973002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/loading/promote-img-in-viewport-priority-expected.txt
(right):

https://codereview.chromium.org/1889973002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/promote-img-in-viewport-priority-expected.txt:8:
http://127.0.0.1:8000/misc/resources/image-slow.pl has priority VeryLow
This gets spammed because FrameLoaderClient::willSendRequest() now gets called
for every MemoryCache hit.

https://codereview.chromium.org/1889973002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/elements/styles-1/css-outline-expected.txt
(right):

https://codereview.chromium.org/1889973002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/styles-1/css-outline-expected.txt:2:
Blocked access to external URL http://www.example.com/fonts/example
Due to moving willSendRequest() earlier.

https://codereview.chromium.org/1889973002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1889973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:724: if
(request.getCachePolicy() == WebCachePolicy::BypassingCache)
Turns out initializeResourceRequest() is called too late on trunk, and the cache
policy doesn't fallback to FetchContext::resourceRequestCachePolicy() until
*after* we call determineRevalidationPolicy(). Moving
initializeResourceRequest() to its proper earlier time means that this clause
needs to move to a lower priority: it was blocking even a preload from being
reused, which is not good. This makes it equivalent priority to
CachePolicyReload, which is a very similar concept.

[Mike West, 2016-05-30 11:05:39]

Yup. I still think it looks reasonable (sorry for the delay, was OOO): LGTM.

[Nate Chapin, 2016-05-31 21:35:40]

On 2016/05/30 11:05:39, Mike West (OOO until 30th) wrote:
> Yup. I still think it looks reasonable (sorry for the delay, was OOO): LGTM.

hiroshige, yoav, any further comments? I'll commit in a couple days if I don't
hear any objections.

[Yoav Weiss, 2016-05-31 21:40:27]

On 2016/05/31 21:35:40, Nate Chapin wrote:
> On 2016/05/30 11:05:39, Mike West (OOO until 30th) wrote:
> > Yup. I still think it looks reasonable (sorry for the delay, was OOO): LGTM.
> 
> hiroshige, yoav, any further comments? I'll commit in a couple days if I don't
> hear any objections.

no objections on my end :)

[Nate Chapin, 2016-06-07 21:14:28]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-06-07 21:14:28]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1889973002/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-07 21:22:52]

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

[commit-bot: I haz the power, 2016-06-07 22:03:54]

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

[commit-bot: I haz the power, 2016-06-07 22:03:55]

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

[Nate Chapin, 2016-06-08 21:16:41]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-06-08 21:16:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1889973002/#ps340001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-08 21:17:26]

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

[commit-bot: I haz the power, 2016-06-08 21:38:04]

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

[commit-bot: I haz the power, 2016-06-08 21:38:06]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Nate Chapin, 2016-06-08 21:39:57]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-06-08 21:40:28]

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

[commit-bot: I haz the power, 2016-06-08 22:22:18]

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

[commit-bot: I haz the power, 2016-06-08 22:22:19]

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

[Nate Chapin, 2016-06-09 19:38:08]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-06-09 19:38:09]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1889973002/#ps360001
(title: "Fix a browser_test")

[commit-bot: I haz the power, 2016-06-09 19:38:42]

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

[commit-bot: I haz the power, 2016-06-09 21:28:18]

Description was changed from

==========
Refactoring starting a resource load

Move most of Resource::load to a new ResourceFetcher::startLoad,
which is well-positioned to verify that FetchContext is still
valid. This also means that deferred loads call ResourceFetcher
to actually start the load, just as they called ResourceFetcher
to initialize the load.

BUG=603428
==========

to

==========
Refactoring starting a resource load

Move most of Resource::load to a new ResourceFetcher::startLoad,
which is well-positioned to verify that FetchContext is still
valid. This also means that deferred loads call ResourceFetcher
to actually start the load, just as they called ResourceFetcher
to initialize the load.

BUG=603428
==========

[commit-bot: I haz the power, 2016-06-09 21:28:20]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-06-09 21:28:38]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-09 21:31:25]

Description was changed from

==========
Refactoring starting a resource load

Move most of Resource::load to a new ResourceFetcher::startLoad,
which is well-positioned to verify that FetchContext is still
valid. This also means that deferred loads call ResourceFetcher
to actually start the load, just as they called ResourceFetcher
to initialize the load.

BUG=603428
==========

to

==========
Refactoring starting a resource load

Move most of Resource::load to a new ResourceFetcher::startLoad,
which is well-positioned to verify that FetchContext is still
valid. This also means that deferred loads call ResourceFetcher
to actually start the load, just as they called ResourceFetcher
to initialize the load.

BUG=603428

Committed: https://crrev.com/5b689eeb4cc9dc5636a8db73c79a2674a154e967
Cr-Commit-Position: refs/heads/master@{#399017}
==========

[commit-bot: I haz the power, 2016-06-09 21:31:26]

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