Handle meta viewport in HTMLPreloadScanner
Created attachment 252409 [details] Patch
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 on attachment 252409 [details] Patch 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 :)
Created attachment 252627 [details] Patch
Comment on attachment 252627 [details] Patch Thanks! Got rid of the doc writes :D
Comment on attachment 252627 [details] Patch Clearing flags on attachment: 252627 Committed r183951: <http://trac.webkit.org/changeset/183951>
All reviewed patches have been landed. Closing bug.
Comment on attachment 252627 [details] Patch 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?).
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: ``` <html><head> <script src="something.js">// This is a blocking script, so the preloader kicks in</script> <meta name=viewport content="SOME VIEWPORT CHANGING VALUE"> <body> <img srcset="400px.jpg 400w, 800px.jpg 800w"> ```
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?