Bug 144640 - Handle meta viewport in HTMLPreloadScanner
Summary: Handle meta viewport in HTMLPreloadScanner
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2015-05-05 14:50 PDT by Yoav Weiss
Modified: 2015-05-12 14:21 PDT (History)
4 users (show)

See Also:

Patch (8.95 KB, patch)
2015-05-05 15:06 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (9.10 KB, patch)
2015-05-07 14:25 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2015-05-05 14:50:05 PDT
Handle meta viewport in HTMLPreloadScanner
Comment 1 Yoav Weiss 2015-05-05 15:06:59 PDT
Created attachment 252409 [details]
Comment 2 Yoav Weiss 2015-05-05 15:10:29 PDT
In order to avoid cases of double download when a <meta name=viewport> tag is following a script and followed by <img srcset> which may be impacted by the viewport dimensions, I added <meta name=viewport> support to the HTMLPreloadScanner.
Comment 3 Dean Jackson 2015-05-07 13:38:30 PDT
Comment on attachment 252409 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=252409&action=review

> LayoutTests/fast/dom/HTMLMetaElement/meta-preloader.html:15
> +    if (actual != expected)
> +        document.writeln("<p>FAILURE: expected \"" + expected + "\", actual \"" + actual + "\"</p>");
> +    else
> +        document.writeln("<p>SUCCESS (value: \"" + actual + "\")</p>");

Yuck. I hate document.write :)
Comment 4 Yoav Weiss 2015-05-07 14:25:10 PDT
Created attachment 252627 [details]
Comment 5 Yoav Weiss 2015-05-07 14:27:14 PDT
Comment on attachment 252627 [details]

Thanks! Got rid of the doc writes :D
Comment 6 WebKit Commit Bot 2015-05-07 15:17:50 PDT
Comment on attachment 252627 [details]

Clearing flags on attachment: 252627

Committed r183951: <http://trac.webkit.org/changeset/183951>
Comment 7 WebKit Commit Bot 2015-05-07 15:17:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2015-05-10 15:31:13 PDT
Comment on attachment 252627 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=252627&action=review

I’m not happy with the testing strategy here.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:373
> +bool testPreloadScannerViewportSupport(Document* document)
> +{
> +    ASSERT(document);
> +    HTMLParserOptions options(*document);
> +    HTMLPreloadScanner scanner(options, document->url());
> +    HTMLResourcePreloader preloader(*document);
> +    scanner.appendToEnd(String("<meta name=viewport content='width=400'>"));
> +    scanner.scan(preloader, *document);
> +    return (document->viewportArguments().width == 400);
> +}

This is not how we expose our internals for testing. We don’t just embed individual tests in the code like this. Lets think about how to do this in a way that matches the rest of our strategy.

A test like this one would make sense in Tools/TestWebKitAPI/Tests/WebCore; there are plenty of others like that there.

Internals exposes things so they can be tested, it doesn’t expose tests already compiled in. We could expose something testable in internals that lets the JavaScript test machinery drive the preload scanner and check its results.

What we want even more is a test that shows the actual end-user effect of this is, testing the whole thing and not just a unit test. But I don’t understand what that effect is. What is the value of calling document.processViewport during preloading? Is it correct to do that at the time of preload scanning? Lets make it so the test machinery can test that real desired effect (earlier correct viewport maybe?).
Comment 9 Yoav Weiss 2015-05-11 01:59:00 PDT
When asking around in the WebKit contributor meeting, it was suggested that unit testing is possible to do in such a way. Now that I'm aware of /Tools/TestWebKitAPI/Tests/WebCore it does seems significantly better. I'll move the unit test there.

I tried to write tests that do show the end-user effect using LayoutTests, but couldn't find a way to turn on <meta name=viewport> awareness in tests, since it is a no-op on desktop.

If there's a way to that in layout tests, I'll be happy to add them as well.

And finally, the patch's goal is to avoid the preloader loading the wrong resource when the HTML is of the form:
<script src="something.js">// This is a blocking script, so the preloader kicks in</script>
<meta name=viewport content="SOME VIEWPORT CHANGING VALUE">
<img srcset="400px.jpg 400w, 800px.jpg 800w">
Comment 10 Yoav Weiss 2015-05-12 14:21:12 PDT
In order to create an HTMLPreloadScanner in the unit tests I need a Document there.
Is there a way to create a Document (or a mock) there?