[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

--- Comment #35 from Yoav Weiss <yoav at yoav.ws> ---
(In reply to youenn fablet from comment #34)
> Comment on attachment 310889 [details]
> Patch
> 
> r=me.
> I reviewed the tests more thoroughly. Nice to see some wpt ones!

Thanks for reviewing! :)

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

Added

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

No reason. Moved
> 
> > 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?

git rm fail :/
Removed

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

While we could do that for the preloads which are loaded, we cannot do that for the ones that don't. preload with a non-matching media or type gets dropped silently, and doesn't trigger a load event...

> 
> > LayoutTests/http/tests/preload/media-attribute.html:21
> > +            }), 100);
> 
> Is 100 meaningful? Why not 0?

Nope, changed to 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/8d6f4570/attachment-0001.html>