[webkit-reviews] review granted: [Bug 54800] SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in 'currentColor' handling : [Attachment 83522] Patch v2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 23 13:32:07 PST 2011
Dirk Schulze <krit at webkit.org> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 54800: SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in
'currentColor' handling
https://bugs.webkit.org/show_bug.cgi?id=54800
Attachment 83522: Patch v2
https://bugs.webkit.org/attachment.cgi?id=83522&action=review
------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83522&action=review
See comments below. Anyway r=me. Great fix!
> Source/WebCore/ChangeLog:27
> + It turns out the SVGPaint/SVGColor API is not implemented at all,
fix that. Though the API doesn't work as expected (yet?) as
> + we don't support mutable CSSValue derived objects for various
reasons (our CSSValues are shared, we'd need copy-on-write support
> + to make that work, and handle style invalidation, currently a
CSSValue is not tied to a RenderStyle in any way, making dynamic updates
impossible).
> + So you can basically grab a SVGPaint/SVGColor object through
getPropertyCSSValue(..), mutate it, that will all work as expected
> + but the change won't take effect (see CSSPrimitiveValue::setCssText,
for more reasons why this API is not implemented in WebKit).
The first sentence sounds like a demand, rephrase it please.
It seems to me that you're trying to fix two problems with one patch. Please
avoid this. The API is not part of the bug description. And together with the
color serialization fix you're fixing three bugs. Would make it much easier to
review if you split the patches. Also from a logical point of view. The size of
the patch grows from 58k to 136k.
> Source/WebCore/svg/SVGPaint.cpp:51
> + break;
> + case SVGPaint::SVG_PAINTTYPE_URI_RGBCOLOR:
> + case SVGPaint::SVG_PAINTTYPE_RGBCOLOR:
> + return SVGColor::SVG_COLORTYPE_RGBCOLOR;
> + case SVGPaint::SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR:
> + case SVGPaint::SVG_PAINTTYPE_RGBCOLOR_ICCCOLOR:
> + return SVGColor::SVG_COLORTYPE_RGBCOLOR_ICCCOLOR;
> + case SVGPaint::SVG_PAINTTYPE_URI_CURRENTCOLOR:
> + case SVGPaint::SVG_PAINTTYPE_CURRENTCOLOR:
> + return SVGColor::SVG_COLORTYPE_CURRENTCOLOR;
> + }
>
I'd replace the break with return SVGColor::SVG_COLORTYPE_UNKNOWN; and add a
ASSERT_NOT_REACHED after the switch.
> Source/WebCore/svg/SVGPaint.cpp:143
> + case SVG_PAINTTYPE_CURRENTCOLOR:
> + break;
> + case SVG_PAINTTYPE_NONE:
> return "none";
> - if (m_paintType == SVG_PAINTTYPE_CURRENTCOLOR)
> - return "currentColor";
> - if (m_paintType == SVG_PAINTTYPE_URI)
> + case SVG_PAINTTYPE_URI_NONE:
> + return makeString("url(" + m_uri + ") none");
> + case SVG_PAINTTYPE_URI_CURRENTCOLOR:
> + case SVG_PAINTTYPE_URI_RGBCOLOR:
> + case SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR:
> + return makeString("url(" + m_uri + ") ", SVGColor::cssText());
> + case SVG_PAINTTYPE_URI:
> return "url(" + m_uri + ")";
> + };
>
> return SVGColor::cssText();
Ditto.
> Source/WebCore/svg/SVGPaint.cpp:163
> + break;
> + case SVG_PAINTTYPE_URI_NONE:
> + case SVG_PAINTTYPE_URI_CURRENTCOLOR:
> + case SVG_PAINTTYPE_URI_RGBCOLOR:
> + case SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR:
> + case SVG_PAINTTYPE_URI:
> + return referenceId == SVGURIReference::getTarget(m_uri);
> + }
>
> - return referenceId == SVGURIReference::getTarget(m_uri);
> + return false;
Ditto.
> LayoutTests/svg/animations/script-tests/animate-color-fill-currentColor.js:36
> + // Opera doesn't support getPropertyCSSValue - no way to compare to
their SVGPaint/SVGColor objects :(
This test doesn't work on opera anyway, right?
More information about the webkit-reviews
mailing list