69747 2011-10-10 02:38:26 -0700 FEComponentTransfer element doesn't support dynamic invalidation 2011-12-15 11:54:14 -0800 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED http://granite.sru.edu/~ddailey/svg/feComponentTransfer2.svg P2 Normal --- 1 rhodovan.u-szeged rhodovan.u-szeged krit simon.fraser thorton zimmermann oldest_to_newest 480891 0 rhodovan.u-szeged 2011-10-10 02:38:26 -0700 FEComponentTransfer element doesn't support dynamic invalidation 481708 1 110532 rhodovan.u-szeged 2011-10-11 10:19:45 -0700 Created attachment 110532 Proposed patch 482318 2 rhodovan.u-szeged 2011-10-12 02:44:55 -0700 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. 482322 3 110532 zimmermann 2011-10-12 02:59:21 -0700 Comment on attachment 110532 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? 482383 4 110679 rhodovan.u-szeged 2011-10-12 06:43:52 -0700 Created attachment 110679 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. 482388 5 110680 rhodovan.u-szeged 2011-10-12 06:51:45 -0700 Created attachment 110680 Proposed patch One unnecessary comment is removed. 483141 6 110680 zimmermann 2011-10-13 05:43:06 -0700 Comment on attachment 110680 Proposed patch Looks great, r=me. 483143 7 110680 zimmermann 2011-10-13 05:44:09 -0700 Comment on attachment 110680 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); 483145 8 110680 zimmermann 2011-10-13 05:50:03 -0700 Comment on attachment 110680 Proposed patch Clearing patch flag hoping that cr-linux EWS will stop processing the patch over and over. 483205 9 110680 zimmermann 2011-10-13 08:33:50 -0700 Comment on attachment 110680 Proposed patch Heh, obvious return missing in + if (attrName == SVGNames::inAttr) + invalidateFilterPrimitiveParent(this); .. ASSERT_NOT_REACHED(); 483207 10 110851 rhodovan.u-szeged 2011-10-13 08:40:33 -0700 Created attachment 110851 Propsed patch Missing returns are added both in SVGComponentTransferFunction and SVGFEMergeNodeElement. FIXME about the missing svgAttrChanged() is also removed from SVGComponentTransferFunction. 483210 11 110851 zimmermann 2011-10-13 08:44:29 -0700 Comment on attachment 110851 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); 522118 12 thorton 2011-12-15 11:54:14 -0800 It looks to me like this was landed in http://trac.webkit.org/changeset/97369 and can be closed, no? 110532 2011-10-11 10:19:45 -0700 2011-10-12 06:43:52 -0700 Proposed patch 0001-componentTransferElement-up.patch text/plain 0 rhodovan.u-szeged 110679 2011-10-12 06:43:52 -0700 2011-10-12 06:51:45 -0700 Propsed patch 0001-componetTransfer-free.patch text/plain 0 rhodovan.u-szeged 110680 2011-10-12 06:51:45 -0700 2011-10-13 08:40:33 -0700 Proposed patch 0001-componentTransfer-free.patch text/plain 0 rhodovan.u-szeged 110851 2011-10-13 08:40:33 -0700 2011-10-14 01:15:56 -0700 Propsed patch 0001-upped-component.patch text/plain 0 rhodovan.u-szeged