Bug 144640

Summary:

Handle meta viewport in HTMLPreloadScanner

Product:

WebKit

Reporter:

Yoav Weiss <yoav>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dino, esprehn+autocc, gyuyoung.kim

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

Yoav Weiss

Reported 2015-05-05 14:50:05 PDT

Handle meta viewport in HTMLPreloadScanner

Attachments
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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Yoav Weiss

Comment 1 2015-05-05 15:06:59 PDT

Created attachment 252409 [details] Patch

Yoav Weiss

Comment 2 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.

Dean Jackson

Comment 3 2015-05-07 13:38:30 PDT

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 :)

Yoav Weiss

Comment 4 2015-05-07 14:25:10 PDT

Created attachment 252627 [details] Patch

Yoav Weiss

Comment 5 2015-05-07 14:27:14 PDT

Comment on attachment 252627 [details] Patch Thanks! Got rid of the doc writes :D

WebKit Commit Bot

Comment 6 2015-05-07 15:17:50 PDT

Comment on attachment 252627 [details] Patch Clearing flags on attachment: 252627 Committed r183951: <http://trac.webkit.org/changeset/183951>

WebKit Commit Bot

Comment 7 2015-05-07 15:17:54 PDT

All reviewed patches have been landed. Closing bug.

Darin Adler

Comment 8 2015-05-10 15:31:13 PDT

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?).

Yoav Weiss

Comment 9 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: ``` <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"> ```

Yoav Weiss

Comment 10 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?

Note You need to log in before you can comment on or make changes to this bug.