[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 #37 from Yoav Weiss <yoav at yoav.ws> ---
(In reply to Yoav Weiss from comment #35)
> (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...
> 

What I could do is count the ones that trigger a load event, and once all of them fired, make sure that none of the ones that are not supposed to load have loaded.

-- 
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/b878921b/attachment.html>