[webkit-reviews] review denied: [Bug 82804] Support imageSmoothingEnabled : [Attachment 142344] Patch

[bugzilla-daemon at webkit.org]

Stephen White <senorblanco at chromium.org> has denied  review:
Bug 82804: Support imageSmoothingEnabled
https://bugs.webkit.org/show_bug.cgi?id=82804

Attachment 142344: Patch
https://bugs.webkit.org/attachment.cgi?id=142344&action=review

------- Additional Comments from Stephen White <senorblanco at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142344&action=review


> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2276
> +{

The usual WebKit way would be to do an early out if there's no change:

if (m_imageSmoothingEnabled == enabled)
  return;

That should also simplify some logic below, since you can omit both of the
secondary checks in the if clauses (I think).

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:235
> +	   attribute boolean imageSmoothingEnabled;

I think this should have the webkit prefix for now, since the spec is still
draft (someone correct me if I'm wrong).

>
LayoutTests/fast/canvas/canvas-check-imageSmoothingEnabled-works-webkit-bug-828
04.html:21
> +    function repaintTest()

Repaint tests have a specific meaning in LayoutTests (tests that invalidate a
portion of the document and test what is repainted); I'd just call this
"draw()" or something instead to avoid confusion.  Similarly, runRepaintTest()
-> runTests().

>
LayoutTests/fast/canvas/canvas-check-imageSmoothingEnabled-works-webkit-bug-828
04.html:77
> +</html>

If this is a pixel test, it should have expected pixel results for at least one
platform (as well as text results, even if they're empty).