Bug 104207

Summary: Implement SVG2's buffered-rendering property
Product: WebKit Reporter: Philip Rogers <pdr>
Component: SVGAssignee: Philip Rogers <pdr>
Status: RESOLVED FIXED    
Severity: Normal CC: blambov, buildbot, dglazkov, d-r, eric, esprehn+autocc, fmalita, gtk-ews, krit, macpherson, menard, ojan.autocc, peter+ews, rniwa, schenney, webkit-ews, webkit.review.bot, xan.lopez, zimmermann
Priority: P2    
Version: 420+   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 113667    
Attachments:
Description Flags
Work in progress: add the buffered-rendering SVG2 property.
none
[SVG2] Add support for the buffered-rendering property
webkit-ews: commit-queue-
[SVG2] Add support for the buffered-rendering property
schenney: review-
Update per reviewer comments
schenney: review-, buildbot: commit-queue-
Updated per reviewer comments schenney: review+

Description Philip Rogers 2012-12-05 21:27:07 PST
This is a meta bug for implementing the buffered rendering hint from SVG2:
https://svgwg.org/svg2-draft/single-page.html#painting-BufferedRendering

The basic idea here is to cache static content. Feedback from many teams has indicated this is important for performance.
Comment 1 Dirk Schulze 2012-12-05 21:48:26 PST
We should be really careful with that. It has a high risk of regressions and the checking (if an update is needed), might make the implementation difficult to understand.

This is not a concern against a feature like that (I really believe we need something like background caching). But we should not be shy to propose another approach to the SVG WG if it makes more sense.
Comment 2 Dirk Schulze 2012-12-05 21:57:11 PST
We just discussed it on IRC. Once we have layers in SVG, this property could create a new layer. We would need to cache the layers, which is just possible with HW acceleration at the moment IIRC. Leaving the note here for future brain storming,
Comment 3 Nikolas Zimmermann 2012-12-20 05:32:31 PST
My example implementation of SVG using RenderLayers make use off that: an off-screen buffer associated with a RenderLayer if it's transformed (eg. translate animation). Once we fully integrate within the RenderLayer framework, we get this virtually "for free" (no need to take any special care outside of rendering/ for that).
Comment 4 Philip Rogers 2013-03-19 21:53:08 PDT
Created attachment 193982 [details]
Work in progress: add the buffered-rendering SVG2 property.

After the SVG image cache was removed, some users were saddened to learn that their scaled images would not be cached. I have been looking at implementing the SVG2 buffered-rendering property for a subset of the SVG renderers. The attached patch adds the property, but I need to decide what, where, and if we should actually cache.

For concreteness, lets talk about an svg image of a snowflake used many times under translation and rotation to create a snowing effect.

Even with layers, the snowing usecase will benefit from a hint that the snowflake image/subtree should be cached. For example: if the snowflake is complex, the rasterization time may be more expensive than simply stamping out a saved rasterized imagebuffer. This hint gives users the power to trade off memory vs performance even in purely declarative SVG files. Does this sound like a reasonable argument for adding paint-time caching of images as a first-pass?
Comment 5 Stephen Chenney 2013-03-21 12:20:57 PDT
Comment on attachment 193982 [details]
Work in progress: add the buffered-rendering SVG2 property.

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

> Source/WebCore/rendering/style/SVGRenderStyle.h:420
> +                // 19 bits unused

18 bits unused, because you're using 2.
Comment 6 Philip Rogers 2013-03-24 14:43:38 PDT
Created attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Unfortunately this patch will fail the style checker due to the enum values in SVGRenderStyleDefs.h. I think these failures can be ignored because the enum format in SVGRenderStyleDefs does not follow the WebKit code format.

Otherwise, this is ready for review! I put up a simple demo to show off our speedy cached rendering: http://philbit.com/bouncingTigers.html
Comment 7 WebKit Review Bot 2013-03-24 14:46:22 PDT
Attachment 194768 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/css/buffered-rendering-expected.txt', u'LayoutTests/svg/css/buffered-rendering.html', u'LayoutTests/svg/repaint/buffered-rendering-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-image.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/SVGCSSParser.cpp', u'Source/WebCore/css/SVGCSSPropertyNames.in', u'Source/WebCore/css/SVGCSSStyleSelector.cpp', u'Source/WebCore/css/SVGCSSValueKeywords.in', u'Source/WebCore/rendering/style/SVGRenderStyle.cpp', u'Source/WebCore/rendering/style/SVGRenderStyle.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h', u'Source/WebCore/rendering/svg/RenderSVGImage.cpp', u'Source/WebCore/rendering/svg/RenderSVGImage.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h', u'Source/WebCore/svg/SVGStyledElement.cpp', u'Source/WebCore/svg/svgattrs.in']" exit_code: 1
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Early Warning System Bot 2013-03-24 14:55:19 PDT
Comment on attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Attachment 194768 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17144674
Comment 9 Early Warning System Bot 2013-03-24 14:57:09 PDT
Comment on attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Attachment 194768 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17317054
Comment 10 kov's GTK+ EWS bot 2013-03-24 15:12:00 PDT
Comment on attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Attachment 194768 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17313047
Comment 11 WebKit Review Bot 2013-03-24 15:13:21 PDT
Comment on attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Attachment 194768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17288348
Comment 12 EFL EWS Bot 2013-03-24 15:16:01 PDT
Comment on attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Attachment 194768 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17289447
Comment 13 Peter Beverloo (cr-android ews) 2013-03-24 15:37:17 PDT
Comment on attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Attachment 194768 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17220899
Comment 14 Build Bot 2013-03-24 15:41:07 PDT
Comment on attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Attachment 194768 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17183984
Comment 15 WebKit Review Bot 2013-03-24 21:27:03 PDT
Comment on attachment 194768 [details]
[SVG2] Add support for the buffered-rendering property

Attachment 194768 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17323057
Comment 16 Philip Rogers 2013-03-25 09:51:38 PDT
Created attachment 194875 [details]
[SVG2] Add support for the buffered-rendering property

Updated to actually compile on platforms other than mac. Note that this is expected to still fail the style checker.
Comment 17 WebKit Review Bot 2013-03-25 09:59:18 PDT
Attachment 194875 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/css/buffered-rendering-expected.txt', u'LayoutTests/svg/css/buffered-rendering.html', u'LayoutTests/svg/repaint/buffered-rendering-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-image.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/SVGCSSParser.cpp', u'Source/WebCore/css/SVGCSSPropertyNames.in', u'Source/WebCore/css/SVGCSSStyleSelector.cpp', u'Source/WebCore/css/SVGCSSValueKeywords.in', u'Source/WebCore/rendering/style/SVGRenderStyle.cpp', u'Source/WebCore/rendering/style/SVGRenderStyle.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h', u'Source/WebCore/rendering/svg/RenderSVGImage.cpp', u'Source/WebCore/rendering/svg/RenderSVGImage.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h', u'Source/WebCore/svg/SVGStyledElement.cpp', u'Source/WebCore/svg/svgattrs.in']" exit_code: 1
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Stephen Chenney 2013-03-25 10:35:38 PDT
Comment on attachment 194875 [details]
[SVG2] Add support for the buffered-rendering property

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"
Comment 19 Florin Malita 2013-03-25 11:01:49 PDT
Comment on attachment 194875 [details]
[SVG2] Add support for the buffered-rendering property

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

Naming nits :P

> Source/WebCore/ChangeLog:19
> +            http://philbit.com/bouncingTigers.html

Would it be possible to add this as a manual test? I'm always wary of publishing links to personal content - they usually go stale sooner or later.

> Source/WebCore/rendering/svg/RenderSVGImage.cpp:153
> +    SVGImageElement* imageElement = static_cast<SVGImageElement*>(node());

I assume it's safe in this case, but I still cringe on static casts :)

> Source/WebCore/rendering/svg/RenderSVGImage.h:53
> +    void paintForeground(PaintInfo&);

"Foreground" seems too narrow as we're painting the full content (filters, markers, outline, etc). "paintImage"/"paintImageContent" maybe?

> Source/WebCore/rendering/svg/RenderSVGImage.h:73
> +    void invalidateBufferedForeground();

I don't feel too strongly about this since it's related to the attribute name, but technically we're "caching" not "buffering". Plus the "foreground" comment above :)

> Source/WebCore/rendering/svg/RenderSVGImage.h:88
> +    OwnPtr<ImageBuffer> m_bufferedForeground;

Ditto.

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:357
>> +    if (!imageBuffer)
> 
> this is now an else clause.

I would move this up, combine it with the top-level test and then you can just drop the previous conditional/else branch:

  if (!imageBuffer && !(imageBuffer = m_paintInfo->context->createCompatibleBuffer(expandedIntSize(boundingBox.size()), true)))
    return false;

  GraphicsContext* bufferedRenderingContext = imageBuffer->context();
  ...
Comment 20 Philip Rogers 2013-03-25 16:32:47 PDT
Created attachment 194942 [details]
Update per reviewer comments
Comment 21 Philip Rogers 2013-03-25 16:33:57 PDT
Comment on attachment 194875 [details]
[SVG2] Add support for the buffered-rendering property

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

>> Source/WebCore/ChangeLog:19
>> +            http://philbit.com/bouncingTigers.html
> 
> Would it be possible to add this as a manual test? I'm always wary of publishing links to personal content - they usually go stale sooner or later.

I agree this link will go stale at some point. This example is just a public version of an internal benchmark created by the Swiffy team that shows this patch makes a real improvement. I'm not sure this should be checked into the manualtest directory, and sadly this won't run on our perftest infrastructure :( Do you have another suggestion for where we could put it?

>> 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".

Done, and I've filed a bug about the extra WebCore line: https://bugs.webkit.org/show_bug.cgi?id=113221

>> Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87
>> +        BR_AUTO,
> 
> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]

@StyleQueue: it's like you don't even read my comments >:(

>> Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88
>> +        BR_DYNAMIC,
> 
> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]

Ditto!

>> Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89
>> +        BR_STATIC
> 
> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]

Ditto!!1

>> Source/WebCore/rendering/svg/RenderSVGImage.cpp:153
>> +    SVGImageElement* imageElement = static_cast<SVGImageElement*>(node());
> 
> I assume it's safe in this case, but I still cringe on static casts :)

I agree. Abhishek is making an automated pass through these soon.

To assuage your fears, I did check that RenderSVGImage can only be a renderer for SVGImageElements.

>> Source/WebCore/rendering/svg/RenderSVGImage.h:53
>> +    void paintForeground(PaintInfo&);
> 
> "Foreground" seems too narrow as we're painting the full content (filters, markers, outline, etc). "paintImage"/"paintImageContent" maybe?

All of these (other than outline) are considered "foreground" as far as the paint phases are concerned. See PaintPhase.h for the list of phases. This function is only intended for the foreground phase.

I would like to stick with this naming scheme because in a future patch I would like to add a similarly-named function to RenderSVGTransformableContainer to support buffered-rendering on <g>.

>> Source/WebCore/rendering/svg/RenderSVGImage.h:73
>> +    void invalidateBufferedForeground();
> 
> I don't feel too strongly about this since it's related to the attribute name, but technically we're "caching" not "buffering". Plus the "foreground" comment above :)

I think following the attribute naming scheme is best for readability, but, like you, I wish it was named cached-rendering.

>> 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.

I would like to call this function from RenderSVGTransformableContainer in the future in order to cache the foreground rendering of <g> elements. This function was written to be agnostic of the renderer, with the exception of the paintForeground(), which will require us to use something like (line 353):
if (m_object->isSVGImage())
    toRenderSVGImage(m_object)->paintForeground(bufferedInfo);
else if (m_object->isSVGContainer())
    toRenderSVGContainer(m_object)->paintForeground(bufferedInfo);

Does this context change your mind? If not, I'm happy to switch it.

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:346
>> +
> 
> Close this if here.

You and Florin were suggesting slightly different things here. I've cleaned this up, but could both of you please make sure you're happy here?

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

Done.
Comment 22 WebKit Review Bot 2013-03-25 16:34:23 PDT
Attachment 194942 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/css/buffered-rendering-expected.txt', u'LayoutTests/svg/css/buffered-rendering.html', u'LayoutTests/svg/repaint/buffered-rendering-dynamic-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-dynamic-image.html', u'LayoutTests/svg/repaint/buffered-rendering-static-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-static-image.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/SVGCSSParser.cpp', u'Source/WebCore/css/SVGCSSPropertyNames.in', u'Source/WebCore/css/SVGCSSStyleSelector.cpp', u'Source/WebCore/css/SVGCSSValueKeywords.in', u'Source/WebCore/rendering/style/SVGRenderStyle.cpp', u'Source/WebCore/rendering/style/SVGRenderStyle.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h', u'Source/WebCore/rendering/svg/RenderSVGImage.cpp', u'Source/WebCore/rendering/svg/RenderSVGImage.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h', u'Source/WebCore/svg/SVGStyledElement.cpp', u'Source/WebCore/svg/svgattrs.in']" exit_code: 1
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Build Bot 2013-03-25 17:24:52 PDT
Comment on attachment 194942 [details]
Update per reviewer comments

Attachment 194942 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17305197
Comment 24 Philip Rogers 2013-03-25 20:17:04 PDT
(In reply to comment #23)
> (From update of attachment 194942 [details])
> Attachment 194942 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-commit-queue.appspot.com/results/17305197

This is a real failure, I need to wrap the assignment in an additional set of parenthesis.
Comment 25 Build Bot 2013-03-25 22:21:58 PDT
Comment on attachment 194942 [details]
Update per reviewer comments

Attachment 194942 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17232784
Comment 26 Build Bot 2013-03-26 00:24:15 PDT
Comment on attachment 194942 [details]
Update per reviewer comments

Attachment 194942 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17241414
Comment 27 Stephen Chenney 2013-03-26 05:50:18 PDT
Comment on attachment 194942 [details]
Update per reviewer comments

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

Looks like I missed an issue earlier. Another pass required.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:339
> +        if (bufferSize != imageBuffer->internalSize())

Below you use expandedIntSize(boundingBox.size()), but here you compute something else to check the size against. This seems like a problem, as you'll get a different image size depending on which code path you take.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:345
> +        if (imageBuffer = m_paintInfo->context->createCompatibleBuffer(expandedIntSize(boundingBox.size()), true)) {

Above you include scale information in computing the size.

> LayoutTests/ChangeLog:58
> +        * platform/qt/css3/selectors3/xhtml/css3-modsel-14a7-expected.png:

This seems rather unexpected. Find/Replace error?
Comment 28 Florin Malita 2013-03-26 06:56:05 PDT
(In reply to comment #21)
> (From update of attachment 194875 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194875&action=review
> >> Source/WebCore/rendering/svg/RenderSVGImage.h:53
> >> +    void paintForeground(PaintInfo&);
> > 
> > "Foreground" seems too narrow as we're painting the full content (filters, markers, outline, etc). "paintImage"/"paintImageContent" maybe?
> 
> All of these (other than outline) are considered "foreground" as far as the paint phases are concerned. See PaintPhase.h for the list of phases. This function is only intended for the foreground phase.

The SVG layer patch introduces dedicated phases for masking and clipping also. 

I was interpreting the name wrong though: this is not "we're only painting the foreground of the SVG content" but "we're painting the foreground phase of RenderSVGImage", which makes sense.


> >> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:346
> >> +
> > 
> > Close this if here.
> 
> You and Florin were suggesting slightly different things here. I've cleaned this up, but could both of you please make sure you're happy here?

I think we're both suggesting to do away with nested conditional blocks :)

Can't find it in the style guide, but I seem to remember Dirk or Niko mentioned early returns are preferred:

  if (!ok)
    return false;

  <ok block>

instead of

  if (ok) {
    <ok block>
  } else
    return false;
Comment 29 Philip Rogers 2013-03-27 13:37:00 PDT
Comment on attachment 194942 [details]
Update per reviewer comments

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

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:339
>> +        if (bufferSize != imageBuffer->internalSize())
> 
> Below you use expandedIntSize(boundingBox.size()), but here you compute something else to check the size against. This seems like a problem, as you'll get a different image size depending on which code path you take.

These implementations end up being the same but certainly do look quite different. Above, I've extracted out the exact sizing code used by createCompatibleBuffer.

>> LayoutTests/ChangeLog:58
>> +        * platform/qt/css3/selectors3/xhtml/css3-modsel-14a7-expected.png:
> 
> This seems rather unexpected. Find/Replace error?

patch gremlins.
Comment 30 Philip Rogers 2013-03-28 22:40:53 PDT
Created attachment 195698 [details]
Updated per reviewer comments
Comment 31 WebKit Review Bot 2013-03-28 22:44:58 PDT
Attachment 195698 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/css/buffered-rendering-expected.txt', u'LayoutTests/svg/css/buffered-rendering.html', u'LayoutTests/svg/repaint/buffered-rendering-dynamic-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-dynamic-image.html', u'LayoutTests/svg/repaint/buffered-rendering-static-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-static-image.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/SVGCSSParser.cpp', u'Source/WebCore/css/SVGCSSPropertyNames.in', u'Source/WebCore/css/SVGCSSStyleSelector.cpp', u'Source/WebCore/css/SVGCSSValueKeywords.in', u'Source/WebCore/rendering/style/SVGRenderStyle.cpp', u'Source/WebCore/rendering/style/SVGRenderStyle.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h', u'Source/WebCore/rendering/svg/RenderSVGImage.cpp', u'Source/WebCore/rendering/svg/RenderSVGImage.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h', u'Source/WebCore/svg/SVGStyledElement.cpp', u'Source/WebCore/svg/svgattrs.in']" exit_code: 1
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Philip Rogers 2013-04-02 11:41:38 PDT
Landed manually with http://trac.webkit.org/changeset/147348