[Webkit-unassigned] [Bug 171720] [preload] Add media and type attribute support.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=171720

youenn fablet <youennf at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #310889|review?                     |review+
              Flags|                            |

--- 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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170522/6f9cede8/attachment-0001.html>