HTML Imports: Teach preloader about HTML Imports.

HTML Imports: Teach preloader about HTML Imports.

This change tells HTMLPreloadScanner about <link rel=import> so that
it can preload the resource for the import.
This patch let import load not blocked by previous scripts.

This is a revised patch. The original was written by morrita@ but reverted:
https://codereview.chromium.org/243793006

LayoutTests/http/tests/htmlimports/import-async.html,
LayoutTests/http/tests/resources/redirect.php:
- This test confirms <link rel=import async> doesn't block next script but this 
patch
 let import load not blocked by previous script. To clarify such two cases, delayed 
loading.

LayoutTests/http/tests/htmlimports/import-cors-credentials.html,
LayoutTests/http/tests/htmlimports/resources/import-cors-credentials-body.html:
- Similar to previous case. Load the cookie surely before loading.

LayoutTests/http/tests/htmlimports/import-preload.html,
LayoutTests/http/tests/htmlimports/resources/hello.css,
LayoutTests/http/tests/htmlimports/resources/hello.js,
LayoutTests/http/tests/htmlimports/resources/preload.html:
- This is a main test. While there is slow script before import, we prefetch 
resources.

Source/core/fetch/ResourceFetcher.cpp:
- Let ResourceFetcher::preload load import resource.

Source/core/html/parser/HTMLPreloadScanner.cpp:
- Let HTMLPreloadScanner::shouldPreload return true if import.

Source/core/html/parser/PreloadRequest.cpp:
- Set CORS enabled.

Source/core/testing/Internals*:
- Add the helper isPreloadedby(url, document) function generalized of
 isPreloaded(url).

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

[yoichio, 2015-06-09 03:57:12]

Patchset #1 (id:1) has been deleted

[yoichio, 2015-06-09 03:57:20]

Patchset #1 (id:20001) has been deleted

[yoichio, 2015-06-09 03:57:28]

Patchset #1 (id:40001) has been deleted

[yoichio, 2015-06-09 03:57:37]

Patchset #1 (id:60001) has been deleted

[yoichio, 2015-06-09 03:57:44]

Patchset #1 (id:80001) has been deleted

[yoichio, 2015-06-09 03:57:51]

Patchset #1 (id:100001) has been deleted

[yoichio, 2015-06-09 04:22:22]

yoichio@chromium.org changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-06-09 07:30:23]

On 2015/06/09 04:22:22, yoichio wrote:

Thanks! :)
For the ResourceFetcher parts, you may want to make sure there are no collisions
with https://codereview.chromium.org/1170503003/ (and wait for it to land if
there are).
The preloadscanner part looks good. I'll take a deeper look at the various tests
later today.

Also - why was moritta's patch reverted?

[Yoav Weiss, 2015-06-09 07:53:40]

A few comments and questions.

Also, from the spec: "The import is fetched and applied regardless of the media
attribute of the link matches the environment or not" - I'd argue that this
shouldn't be the behavior (and was probably copied from rel=stylesheet). In any
case, this patch would not preload link imports with a non-matching media (which
should be the correct behavior IMO)

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/import-cors-credentials.html (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/import-cors-credentials.html:9:
window.location = "resources/import-cors-credentials-body.html";
Why is this change related to the code change?

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/import-preload-expected.txt (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/import-preload-expected.txt:3: CONSOLE
MESSAGE: line 9: PASS: resources/preload.html is preloaded.
That's the reverse order of preloading I'd expect. I suspect fixing the tests
would fix that as well.

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/import-preload.html (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/import-preload.html:18:
testPreload('resources/preload.html', document);
Can you move the test script to be above the <link> tag? Otherwise, I suspect
you're getting false positives.

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/htmlimports/resources/import-cors-credentials-body.html
(left):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/resources/import-cors-credentials-body.html:13:

Can you elaborate on why these changes are related to the code changes?

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/resources/preload.html (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/resources/preload.html:7:
testPreload('hello.js', document.currentScript.ownerDocument);
Again, testPreload should run before the resources themselves, otherwise, you
may be seeing false positives.

https://codereview.chromium.org/976263002/diff/120001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/976263002/diff/120001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:234: if (match(attributeName,
hrefAttr)) {
Should we look into the async attribute here as well? Should loading be somewhat
different for async imports vs. blocking ones?

[yoichio, 2015-06-11 04:27:44]

> Also - why was moritta's patch reverted?
The patch was reverted because of other tests flakiness.
It is a kind of double requesting but I’m not sure because the log was old and I

didn’t look
 flakiness in my patch. In addition, Blink loading has been changing so let me
confirm 
if the flakiness
 happens again in bots after submit.

> Also, from the spec: "The import is fetched and applied regardless of the
media
> attribute of the link matches the environment or not" - I'd argue that this
> shouldn't be the behavior (and was probably copied from rel=stylesheet). In
any
> case, this patch would not preload link imports with a non-matching media
(which
> should be the correct behavior IMO)
Current normal import imports regardless of media matching. Preloading only if
media 
matches seems
 little strange and confusing.

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/import-cors-credentials.html (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/import-cors-credentials.html:9:
window.location = "resources/import-cors-credentials-body.html";
On 2015/06/09 07:53:40, Yoav Weiss wrote:
> Why is this change related to the code change?

Because we preload not waiting this script inserting cookie so tests fail.

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/import-preload-expected.txt (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/import-preload-expected.txt:3: CONSOLE
MESSAGE: line 9: PASS: resources/preload.html is preloaded.
On 2015/06/09 07:53:40, Yoav Weiss wrote:
> That's the reverse order of preloading I'd expect. I suspect fixing the tests
> would fix that as well.

Done.

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/import-preload.html (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/import-preload.html:18:
testPreload('resources/preload.html', document);
On 2015/06/09 07:53:40, Yoav Weiss wrote:
> Can you move the test script to be above the <link> tag? Otherwise, I suspect
> you're getting false positives.

Done.

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/htmlimports/resources/import-cors-credentials-body.html
(left):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/resources/import-cors-credentials-body.html:13:

On 2015/06/09 07:53:40, Yoav Weiss wrote:
> Can you elaborate on why these changes are related to the code changes?
resources/import-cors-credentials-body.html is split from
import-cors-credentials.html.

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/resources/preload.html (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/resources/preload.html:7:
testPreload('hello.js', document.currentScript.ownerDocument);
On 2015/06/09 07:53:40, Yoav Weiss wrote:
> Again, testPreload should run before the resources themselves, otherwise, you
> may be seeing false positives.

Done.

https://codereview.chromium.org/976263002/diff/120001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/976263002/diff/120001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:234: if (match(attributeName,
hrefAttr)) {
On 2015/06/09 07:53:40, Yoav Weiss wrote:
> Should we look into the async attribute here as well? Should loading be
somewhat
> different for async imports vs. blocking ones?

The async attribute for import means that we execute later script not waiting
loading completion
 in contradiction to preloading in import means that we start loading import not
waiting former script.

[Yoav Weiss, 2015-06-11 10:38:30]

On 2015/06/11 04:27:44, yoichio wrote:
> > Also - why was moritta's patch reverted?
> The patch was reverted because of other tests flakiness.
> It is a kind of double requesting but I’m not sure because the log was old and
I
> 
> didn’t look
>  flakiness in my patch. In addition, Blink loading has been changing so let me
> confirm 
> if the flakiness
>  happens again in bots after submit.

Yeah, please run the trybots so that we would see any flakiness before
committing.

> 
> > Also, from the spec: "The import is fetched and applied regardless of the
> media
> > attribute of the link matches the environment or not" - I'd argue that this
> > shouldn't be the behavior (and was probably copied from rel=stylesheet). In
> any
> > case, this patch would not preload link imports with a non-matching media
> (which
> > should be the correct behavior IMO)
> Current normal import imports regardless of media matching. Preloading only if
> media 
> matches seems
>  little strange and confusing.

Why? This resource is not really needed for first load, and therefore adding it
early to the queue (to compete with other required resources) doesn't make
sense.
Same applies to external style sheets.

> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> File LayoutTests/http/tests/htmlimports/import-cors-credentials.html (right):
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> LayoutTests/http/tests/htmlimports/import-cors-credentials.html:9:
> window.location = "resources/import-cors-credentials-body.html";
> On 2015/06/09 07:53:40, Yoav Weiss wrote:
> > Why is this change related to the code change?
> 
> Because we preload not waiting this script inserting cookie so tests fail.
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> File LayoutTests/http/tests/htmlimports/import-preload-expected.txt (right):
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> LayoutTests/http/tests/htmlimports/import-preload-expected.txt:3: CONSOLE
> MESSAGE: line 9: PASS: resources/preload.html is preloaded.
> On 2015/06/09 07:53:40, Yoav Weiss wrote:
> > That's the reverse order of preloading I'd expect. I suspect fixing the
tests
> > would fix that as well.
> 
> Done.
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> File LayoutTests/http/tests/htmlimports/import-preload.html (right):
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> LayoutTests/http/tests/htmlimports/import-preload.html:18:
> testPreload('resources/preload.html', document);
> On 2015/06/09 07:53:40, Yoav Weiss wrote:
> > Can you move the test script to be above the <link> tag? Otherwise, I
suspect
> > you're getting false positives.
> 
> Done.
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> File
> LayoutTests/http/tests/htmlimports/resources/import-cors-credentials-body.html
> (left):
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
>
LayoutTests/http/tests/htmlimports/resources/import-cors-credentials-body.html:13:
> 
> On 2015/06/09 07:53:40, Yoav Weiss wrote:
> > Can you elaborate on why these changes are related to the code changes?
> resources/import-cors-credentials-body.html is split from
> import-cors-credentials.html.
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> File LayoutTests/http/tests/htmlimports/resources/preload.html (right):
> 
>
https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
> LayoutTests/http/tests/htmlimports/resources/preload.html:7:
> testPreload('hello.js', document.currentScript.ownerDocument);
> On 2015/06/09 07:53:40, Yoav Weiss wrote:
> > Again, testPreload should run before the resources themselves, otherwise,
you
> > may be seeing false positives.
> 
> Done.
> 
>
https://codereview.chromium.org/976263002/diff/120001/Source/core/html/parser...
> File Source/core/html/parser/HTMLPreloadScanner.cpp (right):
> 
>
https://codereview.chromium.org/976263002/diff/120001/Source/core/html/parser...
> Source/core/html/parser/HTMLPreloadScanner.cpp:234: if (match(attributeName,
> hrefAttr)) {
> On 2015/06/09 07:53:40, Yoav Weiss wrote:
> > Should we look into the async attribute here as well? Should loading be
> somewhat
> > different for async imports vs. blocking ones?
> 
> The async attribute for import means that we execute later script not waiting
> loading completion
>  in contradiction to preloading in import means that we start loading import
not
> waiting former script.

I guess that if we'd want to downgrade the priority of async imports we can do
that later on.

[Yoav Weiss, 2015-06-11 10:50:47]

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/htmlimports/import-cors-credentials.html (right):

https://codereview.chromium.org/976263002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/htmlimports/import-cors-credentials.html:9:
window.location = "resources/import-cors-credentials-body.html";
On 2015/06/11 04:27:43, yoichio wrote:
> On 2015/06/09 07:53:40, Yoav Weiss wrote:
> > Why is this change related to the code change?
> 
> Because we preload not waiting this script inserting cookie so tests fail.

OK, great :)

https://codereview.chromium.org/976263002/diff/140001/Source/core/html/parser...
File Source/core/html/parser/PreloadRequest.cpp (right):

https://codereview.chromium.org/976263002/diff/140001/Source/core/html/parser...
Source/core/html/parser/PreloadRequest.cpp:41: 
OK, so that corresponds to
http://w3c.github.io/webcomponents/spec/imports/#fetching-import step 3?

Mike, can you make sure that this makes sense?

[Mike West, 2015-06-12 07:33:03]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2015-06-12 07:33:04]

https://codereview.chromium.org/976263002/diff/140001/Source/core/html/parser...
File Source/core/html/parser/PreloadRequest.cpp (right):

https://codereview.chromium.org/976263002/diff/140001/Source/core/html/parser...
Source/core/html/parser/PreloadRequest.cpp:41: 
On 2015/06/11 at 10:50:47, Yoav Weiss wrote:
> OK, so that corresponds to
http://w3c.github.io/webcomponents/spec/imports/#fetching-import step 3?
> 
> Mike, can you make sure that this makes sense?

How does this deal with redirects? Otherwise it looks reasonable.

[yoichio, 2015-06-16 05:13:09]

> Why? This resource is not really needed for first load, and therefore adding
it
> early to the queue (to compete with other required resources) doesn't make
> sense.
> Same applies to external style sheets.
I see.

https://codereview.chromium.org/976263002/diff/140001/Source/core/html/parser...
File Source/core/html/parser/PreloadRequest.cpp (right):

https://codereview.chromium.org/976263002/diff/140001/Source/core/html/parser...
Source/core/html/parser/PreloadRequest.cpp:41: 
On 2015/06/12 07:33:04, Mike West wrote:
> On 2015/06/11 at 10:50:47, Yoav Weiss wrote:
> > OK, so that corresponds to
> http://w3c.github.io/webcomponents/spec/imports/#fetching-import step 3?
> > 
> > Mike, can you make sure that this makes sense?
> 
> How does this deal with redirects? Otherwise it looks reasonable.

CrossOriginAccessControl::handleRedirect also takes care of ridirected preload
requests.

http/tests/htmlimports/redirect.html tests redirect and now the all imports in
the test are preloaded.

[Mike West, 2015-06-16 06:50:17]

LGTM.

[Yoav Weiss, 2015-06-16 07:04:05]

On 2015/06/16 06:50:17, Mike West wrote:
> LGTM.

LGTM2

[yoichio, 2015-06-16 07:40:39]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2015-06-16 07:40:47]

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

[commit-bot: I haz the power, 2015-06-16 07:43:54]

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

[commit-bot: I haz the power, 2015-06-16 07:43:55]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59035)

[yoichio, 2015-06-17 13:05:57]

The CQ bit was checked by yoichio@chromium.org

[yoichio, 2015-06-17 13:05:57]

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/976263002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-17 13:06:15]

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

[commit-bot: I haz the power, 2015-06-17 13:50:35]

Committed patchset #3 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197260