Converge <link rel=preload> loading logic with preloadScanner

Converge <link rel=preload> loading logic with preloadScanner

This CL converges the loader logic of preload links with the loading
logic of the HTMLPreloadScanner, in order to avoid double downloads,
and because it makes sense.

BUG=523036
Committed: https://crrev.com/8a4dc390ef0c85fb466e7cc36051d7bddf9037d7
Cr-Commit-Position: refs/heads/master@{#357820}

[Yoav Weiss, 2015-10-30 16:22:57]

yoav@yoav.ws changed reviewers:
+ gavinp@chromium.org, japhet@chromium.org

[Yoav Weiss, 2015-10-30 16:22:58]

Hey! :)

Starting alignment of <link rel=preload> to the spec. This is the first step.

Can you take a look?

[Nate Chapin, 2015-10-30 17:56:14]

Test failure looks legit.

https://codereview.chromium.org/1416673008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1416673008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:187: if
(!document.loader())
Can preloadIfNeeded be called for a detached Document? If not, this early exit
should be unecessary.

[Yoav Weiss, 2015-11-01 15:38:46]

On 2015/10/30 17:56:14, Nate Chapin wrote:
> Test failure looks legit.

Yup. Fixed up the test so that it's would match the new loading logic.

> 
>
https://codereview.chromium.org/1416673008/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):
> 
>
https://codereview.chromium.org/1416673008/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/LinkLoader.cpp:187: if
> (!document.loader())
> Can preloadIfNeeded be called for a detached Document? If not, this early exit
> should be unecessary.

Yeah, there's probably no need for that now. (It would probably be needed once I
add Link header support, but I'll add it at that point)

[Yoav Weiss, 2015-11-01 16:15:13]

On 2015/11/01 15:38:46, Yoav Weiss wrote:
> On 2015/10/30 17:56:14, Nate Chapin wrote:
> > Test failure looks legit.
> 
> Yup. Fixed up the test so that it's would match the new loading logic.
> 
> > 
> >
>
https://codereview.chromium.org/1416673008/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1416673008/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/loader/LinkLoader.cpp:187: if
> > (!document.loader())
> > Can preloadIfNeeded be called for a detached Document? If not, this early
exit
> > should be unecessary.
> 
> Yeah, there's probably no need for that now. (It would probably be needed once
I
> add Link header support, but I'll add it at that point)

Looks like the early exit is in fact needed when a link element is added to a
detached doc. Added it back.

[Yoav Weiss, 2015-11-03 13:25:45]

Japhet - can you take a second look, now that it's all green and shiny?

[Nate Chapin, 2015-11-03 20:18:11]

LGTM with a couple nits.

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

https://codereview.chromium.org/1416673008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:153:
WillBeHeapListHashSet<RawPtrWillBeMember<Resource>>* preloads() { return
m_preloads.get(); }
I don't love exposing this. Can we at least comment it as exposed for testing
only?

https://codereview.chromium.org/1416673008/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp (right):

https://codereview.chromium.org/1416673008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp:109: ASSERT_LT(nullptr,
preloads);
ASSERT_LT(nullptr, ...) looks fishy.

ASSERT_NE instead?

[Yoav Weiss, 2015-11-04 14:44:37]

On 2015/11/03 20:18:11, Nate Chapin wrote:
> LGTM with a couple nits.
> 
>
https://codereview.chromium.org/1416673008/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right):
> 
>
https://codereview.chromium.org/1416673008/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.h:153:
> WillBeHeapListHashSet<RawPtrWillBeMember<Resource>>* preloads() { return
> m_preloads.get(); }
> I don't love exposing this. Can we at least comment it as exposed for testing
> only?
> 

Added a comment

>
https://codereview.chromium.org/1416673008/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp (right):
> 
>
https://codereview.chromium.org/1416673008/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp:109:
ASSERT_LT(nullptr,
> preloads);
> ASSERT_LT(nullptr, ...) looks fishy.
> 
> ASSERT_NE instead?

Changed

[Yoav Weiss, 2015-11-04 15:09:46]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-11-04 15:09:46]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1416673008/#ps80001
(title: "Review comments")

[commit-bot: I haz the power, 2015-11-04 15:10:11]

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

[commit-bot: I haz the power, 2015-11-04 15:17:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-04 15:18:59]

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