[webkit-reviews] review denied: [Bug 104207] Implement SVG2's buffered-rendering property : [Attachment 194875] [SVG2] Add support for the buffered-rendering property

[bugzilla-daemon at webkit.org]

Stephen Chenney <schenney at chromium.org> has denied Philip Rogers
<pdr at google.com>'s request for review:
Bug 104207: Implement SVG2's buffered-rendering property
https://bugs.webkit.org/show_bug.cgi?id=104207

Attachment 194875: [SVG2] Add support for the buffered-rendering property
https://bugs.webkit.org/attachment.cgi?id=194875&action=review

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


Minor stuff, really.

Please create two tests. Use "none" as the expected result setting for both
(i.e. both tests have the same expected result). Then have one test for dynamic
and one for static.

> Source/WebCore/ChangeLog:26
> +	   (WebCore):

ap has been telling people (and I agree) to remove the empty WebCore line, so
do that everywhere.

Also, add something along the lines of "All these files changed to support the
new property definition and handling".

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:328
> +bool SVGRenderingContext::bufferForeground(OwnPtr<ImageBuffer>& imageBuffer)


I would be happier if this were renamed "bufferImageElementForeground" to make
it clear it only works on images, to prevent confusion with later code that
enables it for more content.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:346
> +

Close this if here.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:347
> +	   if (imageBuffer) {

Move this out.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:357
> +    if (!imageBuffer)

this is now an else clause.

> LayoutTests/svg/css/buffered-rendering.html:11
> +description('Test that image accept different buffered rendering values');

"... that SVG Image accepts all buffered rendering values"