Bug 95440

Created attachment 161873 [details] Patch Review would be appreciated.

Comment on attachment 161873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161873&action=review > LayoutTests/ChangeLog:4 > + Give image decoder a moment before testing img.complete > + https://bugs.webkit.org/show_bug.cgi?id=95440 Make it clear that this is about the tests. > LayoutTests/ChangeLog:15 > + > + Checking for img.complete before the decoding is finished is > + incorrect. We don't have a good way to know when it is finished, > + so use a timeout. > + > + Also fix the expected values of the tests, and remove "resource" class > + from the image ("resource" is invisible, meaning it's never actually > + decoded, see https://bugs.webkit.org/show_bug.cgi?id=28832) > + IT doesn't sounds like you are only talking about the tests here. You should make that more clear.

Created attachment 163335 [details] Patch

(In reply to comment #3) > > Make it clear that this is about the tests. > Thanks. I tried to clarify the changelog, HTH.

Comment on attachment 163335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163335&action=review > LayoutTests/ChangeLog:19 > + Remove extra line from expected results. You seem to be adding that instead > LayoutTests/ChangeLog:24 > + * canvas/philip/tests/2d.drawImage.broken-expected.txt: > + * canvas/philip/tests/2d.drawImage.broken.html: > + * canvas/philip/tests/2d.pattern.image.broken-expected.txt: > + * canvas/philip/tests/2d.pattern.image.broken.html: explain why the tests were passing before? wrongly? > LayoutTests/canvas/philip/tests/2d.drawImage.broken.html:20 > +// give decoder time to figure out the image is broken comments start with capital and ends with a punctuation mark > LayoutTests/canvas/philip/tests/2d.drawImage.broken.html:21 > +setTimeout(wrapFunction(function () { Write in the changelog why this shouldnt result in flakyness

Created attachment 163542 [details] Patch

This doesn't make any sense to me. img.complete should NOT change while script is running - in general nothing script-observable should change. I don't know what you are referring to when talking about "the decoder may not have...". The decoders in WebCore are synchronous, they can't spontaneously change state.

Comment on attachment 163542 [details] Patch This needs a better explanation at minimum. It seems that this is just adding a setTimeout() to tests for no reason in its current incarnation.

(In reply to comment #8) > This doesn't make any sense to me. img.complete should NOT change while script is running - in general nothing script-observable should change. The spec clearly disagrees, but I realise that doesn't mean implementation needs to... the comment about synchronous decoders is interesting -- I'll look closer at what's really happening then and report back here

Created attachment 163570 [details] test for img.complete behaviour Here's a test for img.complete behaviour, hopefully works straight from bugzilla. Results on chromium (not webkit master but a release): --- Log: * Testing img.complete on script startup decodable-visible: image.complete == false decodable-hidden: image.complete == false broken-visible: image.complete == false broken-hidden: image.complete == false * Testing img.complete after 5 secs... decodable-visible: image.complete == true decodable-hidden: image.complete == true broken-visible: image.complete == true broken-hidden: image.complete == true --- So the results do change while script is running -- because imageloader is still doing it's job (so my comment in the test is not very accurate -- the timeout is waiting for decoding results, but the images probably aren't even loaded at that point). The end results are also wrong, which is what bug 28832 is about: broken images should not be "complete". EFL MiniBrowser with the 28832 patch gives: --- Log: * Testing img.complete on script startup decodable-visible: image.complete == false decodable-hidden: image.complete == false broken-visible: image.complete == false broken-hidden: image.complete == false * Testing img.complete after 5 secs... decodable-visible: image.complete == true decodable-hidden: image.complete == true broken-visible: image.complete == false broken-hidden: image.complete == true --- Broken visible images are now "not complete". It's still saying broken hidden image is "complete", I would argue that's according to spec and we definitely wouldn't want to start decoding just for this anyway...

I was supposed to add info on what really happens after image loader finishes but I don't have that yet... The test result (with patch from 28832) is that on the onload handler for a broken visible image, img.complete _is_ true. This changes to false (the correct value) just a bit later. My original assumption was that this was because in onload the image had been loaded (fulfilling the spec requirements for "complete", if I read it correctly) but we didn't yet know it is not decodable. I may have assumed too much there or my patch could be wrong so I'll try to document what _really_ happens there -- James, if you have knowledge of this please share.

Looking at the sequence of events more carefully, this is what I think happens: * imageloader starts working * script starts (image.complete is always false because image is not loaded) * decoder starts getting data * onload event may happen here (valid images are now complete, but broken image may be incorrectly complete because decoding has not failed yet) * decoder may set failed=True here (but may also be earlier) It looks to me like my original reasoning is valid. I'll be happy if someone shows where I'm wrong though. The flakiness could be minimized by waiting for the onload-events but that's just not enough, a timeout is still needed because decoder might might decide it has failed later than onload.