Handle main resource preload request better

Handle main resource preload request better

Return early from preload request for main resource
to avoid spinning up ResourceLoader unnecessarily.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175815

[maheshkk, 2014-06-06 22:31:24]

japhet, My first guess is that we can just return early for preload request of
main resource. Thoughts?

[Nate Chapin, 2014-06-06 22:38:11]

On 2014/06/06 22:31:24, maheshkk wrote:
> japhet, My first guess is that we can just return early for preload request of
> main resource. Thoughts?

Is there a bug that's being fixed here? My inclination would be to leave this
logic as-is, so that we don't spread the main resource special cases any more
than they already are.

[kbalazs, 2014-06-06 22:53:43]

On 2014/06/06 22:38:11, Nate Chapin wrote:
> On 2014/06/06 22:31:24, maheshkk wrote:
> > japhet, My first guess is that we can just return early for preload request
of
> > main resource. Thoughts?
> 
> Is there a bug that's being fixed here? My inclination would be to leave this
> logic as-is, so that we don't spread the main resource special cases any more
> than they already are.

I guess the fix is that right now we actually load main resources for preload
and than we drop them which sounds like extra work for the network stack for no
good reason.

[Nate Chapin, 2014-06-06 23:00:01]

https://codereview.chromium.org/314423004/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/314423004/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:678: if (type == Resource::MainResource &&
request.forPreload()) {
Ok. What if we move both this and the old code block to fetchMainResource()?
That address my organizational complaint, and avoids spinning up a
ResourceLoader unnecessarily.

[maheshkk, 2014-06-09 17:50:26]

On 2014/06/06 23:00:01, Nate Chapin wrote:
>
https://codereview.chromium.org/314423004/diff/1/Source/core/fetch/ResourceFe...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/314423004/diff/1/Source/core/fetch/ResourceFe...
> Source/core/fetch/ResourceFetcher.cpp:678: if (type == Resource::MainResource
&&
> request.forPreload()) {
> Ok. What if we move both this and the old code block to fetchMainResource()?
> That address my organizational complaint, and avoids spinning up a
> ResourceLoader unnecessarily.

PTAL at my proposal. 

 - IMO Preload request for main resources happens while scanning for preload
requests. 
   WE can avoid lots of checks by early returning from requestPreload method.  
 - Avoiding caching of main resource and removing the copy if exists is
unchanged.

[Nate Chapin, 2014-06-09 17:55:00]

That's a good location for that check, LGTM. Just a nit about the comment.

https://codereview.chromium.org/314423004/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/314423004/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1196: // If loading main resource ensure
main resources aren't preloaded.
I'd work this with a little more detail. For example, "Ensure main resources
aren't preloaded, since the cache can't actually reuse the preload."

[Nate Chapin, 2014-06-09 17:55:22]

On 2014/06/09 17:55:00, Nate Chapin wrote:
> That's a good location for that check, LGTM. Just a nit about the comment.
> 
>
https://codereview.chromium.org/314423004/diff/20001/Source/core/fetch/Resour...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/314423004/diff/20001/Source/core/fetch/Resour...
> Source/core/fetch/ResourceFetcher.cpp:1196: // If loading main resource ensure
> main resources aren't preloaded.
> I'd work this with a little more detail. For example, "Ensure main resources
> aren't preloaded, since the cache can't actually reuse the preload."

s/work/write/

[maheshkk, 2014-06-09 18:10:21]

On 2014/06/09 17:55:22, Nate Chapin wrote:
> On 2014/06/09 17:55:00, Nate Chapin wrote:
> > That's a good location for that check, LGTM. Just a nit about the comment.
> > 
> >
>
https://codereview.chromium.org/314423004/diff/20001/Source/core/fetch/Resour...
> > File Source/core/fetch/ResourceFetcher.cpp (right):
> > 
> >
>
https://codereview.chromium.org/314423004/diff/20001/Source/core/fetch/Resour...
> > Source/core/fetch/ResourceFetcher.cpp:1196: // If loading main resource
ensure
> > main resources aren't preloaded.
> > I'd work this with a little more detail. For example, "Ensure main resources
> > aren't preloaded, since the cache can't actually reuse the preload."
> 
> s/work/write/

Thanks for the quick review! Fixed the comment.

[maheshkk, 2014-06-09 18:10:24]

The CQ bit was checked by mahesh.kk@samsung.com

[commit-bot: I haz the power, 2014-06-09 18:11:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/314423004/40001

[Nate Chapin, 2014-06-09 18:14:51]

Thanks again.

Sorry for leading you a little astray in the suggestion of putting that line in
requestMainResource(). I flaked and forgot that preloads go through a different
codepath :-/

[commit-bot: I haz the power, 2014-06-09 19:21:32]

Change committed as 175815