[webkit-reviews] review denied: [Bug 171016] Add the imageframeready event to the DOM Element to reliably test the async image decoding : [Attachment 308788] 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 308788: Patch

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




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

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

> Source/WTF/wtf/FeatureDefines.h:769
> +#if !defined(ENABLE_IMAGE_FRAME_READY)
> +#define ENABLE_IMAGE_FRAME_READY 1
> +#endif

This turns it on unconditionally for all platforms. Is that the intent?

Also I think ENABLE_IMAGE_FRAME_READY should be ENABLE_IMAGE_FRAME_READY_EVENT

> Source/WebCore/html/HTMLAttributeNames.in:282
> +onimageframeready

Are we still adding "on" attributes for new event types? Please check with
cdumez.

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

This will fire on every frame of a GIF. Is that right?

> Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:150
> +#define WebKitImageFrameReadyEnabledPreferenceKey
@"WebKitImageFrameReadyEnabled"

I don't think you need any of the WK1/WK2 prefs.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:133
> +    void setImageFrameReadyEnabled(bool);

This should be enabled through InternalSettings (see setWebGL2Enabled), not
testRunner.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:144
> +    void display(bool noTrackRepaints);

It's sad that this has a negative name, and causes tests to fall into the
boolean trap ("display(false)").

I think it would be better to use an enum (see Internals.idl for examples).
Sadly the DRT bindings code will have to be written by hand.