[webkit-reviews] review denied: [Bug 113405] Respect image-rendering setting for determing image-rendering quality : [Attachment 198489] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 18 13:15:42 PDT 2013
Benjamin Poulain <benjamin at webkit.org> has denied Allan Sandfeld Jensen
<allan.jensen at digia.com>'s request for review:
Bug 113405: Respect image-rendering setting for determing image-rendering
quality
https://bugs.webkit.org/show_bug.cgi?id=113405
Attachment 198489: Patch
https://bugs.webkit.org/attachment.cgi?id=198489&action=review
------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=198489&action=review
I like the new direction this is taking.
Some comments bellow:
> Source/WebCore/rendering/style/RenderStyleConstants.h:484
> ImageRenderingAuto, ImageRenderingOptimizeSpeed,
ImageRenderingOptimizeQuality,
> - ImageRenderingCrispEdges, ImageRenderingPixelated, ImageRenderingSmooth
> + ImageRenderingCrispEdges
I think it would be better to either split the enum on one value per line, or
put them all on the same line.
> Source/WebCore/rendering/style/RenderStyleConstants.h:485
> + // For CSS4 Images we will also need ImageRenderingPixelated and
ImageRenderingSmooth
I think we should not have this comment. There is no way to keep those kind of
comments in sync with spec updates.
> LayoutTests/ChangeLog:12
> + * fast/css/image-rendering-pre-css4.html: Added.
The name of the test is not good enough in my opinion.
I think you should also have the following coverage:
-Tests for CSSOM. Both checking the style and computed style when using
image-rendering.
-A ref test verifying "-webkit-crisp-edges" and "-webkit-optimize-contrast"
render the exact same thing.
Look for the current coverage of image-rendering, as it should be a good start.
More information about the webkit-reviews
mailing list