Bug 69747 - FEComponentTransfer element doesn't support dynamic invalidation
Summary: FEComponentTransfer element doesn't support dynamic invalidation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Renata Hodovan
URL: http://granite.sru.edu/~ddailey/svg/f...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-10 02:38 PDT by Renata Hodovan
Modified: 2011-12-15 11:54 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (deleted)
2011-10-11 10:19 PDT, Renata Hodovan
zimmermann: review-
rhodovan.u-szeged: commit-queue-
Details | Formatted Diff | Diff
Propsed patch (deleted)
2011-10-12 06:43 PDT, Renata Hodovan
rhodovan.u-szeged: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (deleted)
2011-10-12 06:51 PDT, Renata Hodovan
zimmermann: review-
rhodovan.u-szeged: commit-queue-
Details | Formatted Diff | Diff
Propsed patch (deleted)
2011-10-13 08:40 PDT, Renata Hodovan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Renata Hodovan 2011-10-10 02:38:26 PDT
FEComponentTransfer element doesn't support dynamic invalidation
Comment 1 Renata Hodovan 2011-10-11 10:19:45 PDT
Created attachment 110532 [details]
Proposed patch
Comment 2 Renata Hodovan 2011-10-12 02:44:55 PDT
Chromium ews is yellow 'cos the new tests need plaform specific expecteds on chromium. The rest failing tests don't contain any SVG elements, so I guess they are unrelated or flaky.
Comment 3 Nikolas Zimmermann 2011-10-12 02:59:21 PDT
Comment on attachment 110532 [details]
Proposed patch

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

Tests look great, I checked each of them. I wish the changes would be more easily visible (did the filter apply really work?) I have to trust you all tests work manually fine and produce this result which is different to no filter applied :-)

r- because of abusing SVGElement to share code. Please rework this part, everything else is great.

> Source/WebCore/svg/SVGElement.cpp:141
> +        return;

Unnecessary.

> Source/WebCore/svg/SVGElement.h:120
> +    void invalidateParents();

This does not belong in SVGElement - it has a very generic name and leads to confusion. SVGElement should be kept as small as possible.
Why don't you introduce a free-function in eg. SVGFilterPrimitiveStandardAttributes.h or another filter-related class?
Comment 4 Renata Hodovan 2011-10-12 06:43:52 PDT
Created attachment 110679 [details]
Propsed patch

> Tests look great, I checked each of them. I wish the changes would be more easily visible (did the filter apply really work?) I have to trust you all tests work manually fine and produce this result which is different to no filter applied :-)
Yeah, tests work manually fine. Perhaps you don't see it on the pngs, because the second pricture tries to approach the input image (e.g. the second parameter set contains the default values) and it's printed to the png.

Furthermore I renamed invalidateParents() to invalidateFilterPrimitiveParent() and moved to SVGFilterPrimitveStandardAttributes. As we talked on IRC it's declared as a free function.
Comment 5 Renata Hodovan 2011-10-12 06:51:45 PDT
Created attachment 110680 [details]
Proposed patch

One unnecessary comment is removed.
Comment 6 Nikolas Zimmermann 2011-10-13 05:43:06 PDT
Comment on attachment 110680 [details]
Proposed patch

Looks great, r=me.
Comment 7 Nikolas Zimmermann 2011-10-13 05:44:09 PDT
Comment on attachment 110680 [details]
Proposed patch

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

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:174
> +{
> +    ContainerNode* parent = element->parentNode();

Ah forgot to say. You have to either add an early return if element can be null or add a ASSERT(element);
Comment 8 Nikolas Zimmermann 2011-10-13 05:50:03 PDT
Comment on attachment 110680 [details]
Proposed patch

Clearing patch flag hoping that cr-linux EWS will stop processing the patch over and over.
Comment 9 Nikolas Zimmermann 2011-10-13 08:33:50 PDT
Comment on attachment 110680 [details]
Proposed patch

Heh, obvious return missing in
+    if (attrName == SVGNames::inAttr)
+        invalidateFilterPrimitiveParent(this);
..
ASSERT_NOT_REACHED();
Comment 10 Renata Hodovan 2011-10-13 08:40:33 PDT
Created attachment 110851 [details]
Propsed patch

Missing returns are added both in SVGComponentTransferFunction and SVGFEMergeNodeElement.
FIXME about the missing svgAttrChanged() is also removed from SVGComponentTransferFunction.
Comment 11 Nikolas Zimmermann 2011-10-13 08:44:29 PDT
Comment on attachment 110851 [details]
Propsed patch

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

r=me, please fix this before landing:

> Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:144
> +    if (attrName == SVGNames::typeAttr
> +        || attrName == SVGNames::tableValuesAttr
> +        || attrName == SVGNames::slopeAttr
> +        || attrName == SVGNames::interceptAttr
> +        || attrName == SVGNames::amplitudeAttr
> +        || attrName == SVGNames::exponentAttr
> +        || attrName == SVGNames::offsetAttr) {

This is useless, isSupportedAttribute() checks the same, if that returned true, it's one of these attributes :-).

> Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:149
> +        invalidateFilterPrimitiveParent(this);
> +        return;
> +    }
> +
> +    ASSERT_NOT_REACHED();

Just make this: invalidateFilterPrimitiveParent(this);
Comment 12 Tim Horton 2011-12-15 11:54:14 PST
It looks to me like this was landed in http://trac.webkit.org/changeset/97369 and can be closed, no?