<rdar://problem/10155789>

Created attachment 115443 [details] Patch

Comment on attachment 115443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115443&action=review > Source/WebCore/rendering/FilterEffectRenderer.cpp:79 > + for (Vector<RefPtr<FilterOperation> >::const_iterator it = operations.operations().begin(); it != end; ++it) { We prefer index access for vectors, rather than using enumerators. > Source/WebCore/rendering/FilterEffectRenderer.cpp:90 > + float oneMinusAmount = clampTo(1.0 - narrowPrecisionToFloat(colorMatrixOperation->amount()), 0.0, 1.0); 1.0 -> 1, 0.0 -> 0 > Source/WebCore/rendering/FilterEffectRenderer.cpp:104 > + inputParameters.append(0.2126 + 0.7874 * oneMinusAmount); > + inputParameters.append(0.7152 - 0.7152 * oneMinusAmount); > + inputParameters.append(0.0722 - 0.0722 * oneMinusAmount); > + endMatrixRow(inputParameters); > + > + inputParameters.append(0.2126 - 0.2126 * oneMinusAmount); > + inputParameters.append(0.7152 + 0.2848 * oneMinusAmount); > + inputParameters.append(0.0722 - 0.0722 * oneMinusAmount); > + endMatrixRow(inputParameters); > + > + inputParameters.append(0.2126 - 0.2126 * oneMinusAmount); > + inputParameters.append(0.7152 - 0.7152 * oneMinusAmount); > + inputParameters.append(0.0722 + 0.9278 * oneMinusAmount); Would be nice to see a URL for the magic numbers. > Source/WebCore/rendering/FilterEffectRenderer.cpp:129 > + inputParameters.append(0.393 + 0.607 * oneMinusAmount); > + inputParameters.append(0.769 - 0.769 * oneMinusAmount); > + inputParameters.append(0.189 - 0.189 * oneMinusAmount); > + endMatrixRow(inputParameters); > + > + inputParameters.append(0.349 - 0.349 * oneMinusAmount); > + inputParameters.append(0.686 + 0.314 * oneMinusAmount); > + inputParameters.append(0.168 - 0.168 * oneMinusAmount); > + endMatrixRow(inputParameters); > + > + inputParameters.append(0.272 - 0.272 * oneMinusAmount); > + inputParameters.append(0.534 - 0.534 * oneMinusAmount); > + inputParameters.append(0.131 + 0.869 * oneMinusAmount); Ditto. > Source/WebCore/rendering/FilterEffectRenderer.cpp:192 > + float stdDeviationX = blurOperation->stdDeviationX().calcFloatValue(borderBox.width()); > + float stdDeviationY = blurOperation->stdDeviationY().calcFloatValue(borderBox.height()); Huh, why does the stdDev depend on box size? > Source/WebCore/rendering/FilterEffectRenderer.h:66 > + GraphicsContext* inputContext(); Can this be const? > Source/WebCore/rendering/FilterEffectRenderer.h:67 > + ImageBuffer* output() { return lastEffect()->asImageBuffer(); } Ditto. > Source/WebCore/rendering/FilterEffectRenderer.h:78 > + Vector<RefPtr<FilterEffect> >::const_iterator end = m_effects.end(); > + for (Vector<RefPtr<FilterEffect> >::const_iterator it = m_effects.begin(); it != end; ++it) { Use index access. > Source/WebCore/rendering/FilterEffectRenderer.h:96 > + Vector<RefPtr<FilterEffect> > m_effects; A typedef for Vector<RefPtr<FilterEffect> > may make the code easier to read. > LayoutTests/ChangeLog:62 > +2011-11-16 Dean Jackson <dino@apple.com> > + > + Need a short description and bug URL (OOPS!) > + > + Reviewed by NOBODY (OOPS!). > + Double changelog. > LayoutTests/css3/filters/effect-blur-expected.txt:2 > + RenderView at (0,0) size 800x600 You should make these all dumpAsText(true).

(In reply to comment #3) > (From update of attachment 115443 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115443&action=review > > > Source/WebCore/rendering/FilterEffectRenderer.cpp:79 > > + for (Vector<RefPtr<FilterOperation> >::const_iterator it = operations.operations().begin(); it != end; ++it) { > > We prefer index access for vectors, rather than using enumerators. Fixed. > > > Source/WebCore/rendering/FilterEffectRenderer.cpp:90 > > + float oneMinusAmount = clampTo(1.0 - narrowPrecisionToFloat(colorMatrixOperation->amount()), 0.0, 1.0); > > 1.0 -> 1, 0.0 -> 0 The problem with that was it then used an integer version of clampTo. > > Would be nice to see a URL for the magic numbers. Done. > > > Source/WebCore/rendering/FilterEffectRenderer.cpp:192 > > + float stdDeviationX = blurOperation->stdDeviationX().calcFloatValue(borderBox.width()); > > + float stdDeviationY = blurOperation->stdDeviationY().calcFloatValue(borderBox.height()); > > Huh, why does the stdDev depend on box size? Because it takes a length, which could be a %. I wonder if we should disallow this. > > Source/WebCore/rendering/FilterEffectRenderer.h:66 > > + GraphicsContext* inputContext(); > > Can this be const? > > > Source/WebCore/rendering/FilterEffectRenderer.h:67 > > + ImageBuffer* output() { return lastEffect()->asImageBuffer(); } > > Ditto. Unfortunately no, because they both access non-const functions in FilterEffect. > > > Source/WebCore/rendering/FilterEffectRenderer.h:78 > > + Vector<RefPtr<FilterEffect> >::const_iterator end = m_effects.end(); > > + for (Vector<RefPtr<FilterEffect> >::const_iterator it = m_effects.begin(); it != end; ++it) { > > Use index access. Done > > > Source/WebCore/rendering/FilterEffectRenderer.h:96 > > + Vector<RefPtr<FilterEffect> > m_effects; > > A typedef for Vector<RefPtr<FilterEffect> > may make the code easier to read. Done > > > LayoutTests/ChangeLog:62 > > +2011-11-16 Dean Jackson <dino@apple.com> > > + > > + Need a short description and bug URL (OOPS!) > > + > > + Reviewed by NOBODY (OOPS!). > > + > > Double changelog. > > > LayoutTests/css3/filters/effect-blur-expected.txt:2 > > + RenderView at (0,0) size 800x600 > > You should make these all dumpAsText(true). Done. The expected files are now empty. Is that ok?

(In reply to comment #4) > > > Source/WebCore/rendering/FilterEffectRenderer.cpp:192 > > > + float stdDeviationX = blurOperation->stdDeviationX().calcFloatValue(borderBox.width()); > > > + float stdDeviationY = blurOperation->stdDeviationY().calcFloatValue(borderBox.height()); > > > > Huh, why does the stdDev depend on box size? > > Because it takes a length, which could be a %. I wonder if we should disallow this. I think we should disallow it.