[webkit-reviews] review denied: [Bug 63558] Media element loads blocked by a resource load delegate or beforeload handler do not generate an error event : [Attachment 120072] Changes based on Eric's suggestions.

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Shadi <shadi at chromium.org>'s
request for review:
Bug 63558: Media element loads blocked by a resource load delegate or
beforeload handler do not generate an error event
https://bugs.webkit.org/show_bug.cgi?id=63558

Attachment 120072: Changes based on Eric's suggestions.
https://bugs.webkit.org/attachment.cgi?id=120072&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=120072&action=review


> LayoutTests/media/media-blocked-by-beforeload.html:22
> +	   var loadedmetadataFired = loadedFired = false

Nit: WebKit style is to declare each variable separately.

>>> LayoutTests/media/media-blocked-by-beforeload.html:35
>>> +		     checkEndTest();
>> 
>> Where is "checkEndTest"? What is it supposed to do?
> 
> Typo, this should be endTest();

How did this test work? Did you run it?

>>> LayoutTests/media/media-blocked-by-beforeload.html:112
>>> +		    endTest();
>> 
>> Isn't the theory that that we can't predict the order that 'load' and
'loadedmetadata' will fire? If so, the results will have
"EVENT('loadedmetadata')" when 'loadedmetadata' first first, but not if 'load'
first first.
> 
> I think that the window 'loaded' event can't be predictable. In addition, the
test results show that only the 'loaded' event happens at different points
while the 'loadedmetadata' always happens at the end of the test after the good
source is loaded.

Exactly, that is why the existing test sometimes fails - and it is still a
problem with your modifications. 

As I said above, if the 'load' event fires first the results will not have
"EVENT('loadedmetadata')". The comment at the top of this test is "Test to
ensure that a media file blocked by a beforeload handler generates an error and
does not block the document's 'load' event.", ergo it *must* not finish until
the 'load' event has fired.