[webkit-reviews] review denied: [Bug 171016] Add the imageframeready event to the DOM Element to reliably test the async image decoding : [Attachment 308875] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 171016: Add the imageframeready event to the DOM Element to reliably test
the async image decoding
https://bugs.webkit.org/show_bug.cgi?id=171016

Attachment 308875: Patch

https://bugs.webkit.org/attachment.cgi?id=308875&action=review




--- Comment #31 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 308875
  --> https://bugs.webkit.org/attachment.cgi?id=308875
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308875&action=review

> Source/WebCore/ChangeLog:3
> +	   Add the imageframeready event to the DOM Element to reliably test
the async image decoding

"to the DOM Element" sounds weird. Maybe just "implement the imageframeready
event to reliably test..."

I'm looking at https://github.com/whatwg/html/issues/1920 and don't see a
specific proposal for the name of this event. Did you just make up
"imageframeready"? Should it just be "ready"?

> Source/WebCore/rendering/RenderElement.cpp:1512
> +    if (element() && image.image()->isBitmapImage())
> +	   element()->dispatchImageFrameReadyEvent();

This will dispatch the event for CSS background images as well as <img> which
is certainly not what the spec will say. Can we restrict it to RenderImage for
now?

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:117
> +void TestRunner::display(DisplayOptions* options)

It's ugly taking a pointer to an enum. Can't the caller handle the default
case, or use a std::optional?