[webkit-reviews] review granted: [Bug 171720] [preload] Add media and type attribute support. : [Attachment 310889] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Yoav Weiss <yoav at yoav.ws>'s
request for review:
Bug 171720: [preload] Add media and type attribute support.
https://bugs.webkit.org/show_bug.cgi?id=171720

Attachment 310889: Patch

https://bugs.webkit.org/attachment.cgi?id=310889&action=review




--- Comment #34 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 310889
  --> https://bugs.webkit.org/attachment.cgi?id=310889
Patch

r=me.
I reviewed the tests more thoroughly. Nice to see some wpt ones!
There is a risk for flakiness on these tests and I am wondering whether we
could not improve on that and maybe on readability.
Some suggestions below.

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

> LayoutTests/ChangeLog:7
> +

ChangeLog should include listing TestExpectations changes.
Can you also add the explanations of why we are skipping them by default,
either here or in LayoutTests/TestExpectations, since we should be able at some
point to pass these tests in all platforms.

> LayoutTests/http/tests/preload/media-attribute.html:5
> +    var t = async_test('Makes sure that preloaded resources are not
downloaded when the media attribute is a mismatch.');

Why do we need to create the test here?
Can we do it in the lat script below instead?

> LayoutTests/http/tests/preload/media-attribute.html:7
> +<link rel=preload
href="http://127.0.0.1:8000/resources/square100.png?media_all" as=image
media="all">

http://127.0.0.1:8000 is probably unnecessary and we would need to remove it if
migrating to wpt.
But wait, isn't it the same as
LayoutTests/http/wpt/preload/media-attribute.html?

> LayoutTests/http/tests/preload/media-attribute.html:9
> +<script
src="http://127.0.0.1:8000/resources/slow-script.pl?delay=200"></script>

I am not sure this will not end up be flaky.
Can we use onload events on preload link elements instead of window.load?
If we put the link that will get preloaded at the end of the list, we can be
confident that other links will not be preloaded. We can of course wait a
little bit more to ensure that.

> LayoutTests/http/tests/preload/media-attribute.html:19
> +	       document.write('<img
src="http://127.0.0.1:8000/resources/square100.png?media_not_all">');

Would it be better if we were doing something like 
<img src="/resources/square100.png?media_all" onload="checkWasPreloaded();
loadNextImage();"> 
<img src="/resources/square100.png?media_not_all"
onload="checkWasNotPreloaded(); finish();">

We could create one async_test() to ensure we run everything and several
synchronous test() to better describe what is going on.

> LayoutTests/http/tests/preload/media-attribute.html:21
> +	       }), 100);

Is 100 meaningful? Why not 0?

> LayoutTests/http/wpt/preload/media-attribute-expected.txt:3
> +PASS Makes sure that preloaded resources are not downloaded when the media
attribute is a mismatch. 

Nice to see it in wpt!