Bug 68475

Summary: Implement filter shorthands
Product: WebKit Reporter: Dean Jackson <dino>
Component: CSSAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, macpherson, simon.fraser, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 68473    
Bug Blocks: 68476    
Attachments:
Description Flags
Patch simon.fraser: review+

Description Dean Jackson 2011-09-20 14:22:28 PDT
I expect this to be the master bug for the implementation of:

grayscale
sepia
saturate
hue-rotate
invert
opacity
gamma
blur
sharpen
drop-shadow
Comment 1 Radar WebKit Bug Importer 2011-09-20 14:22:56 PDT
<rdar://problem/10155789>
Comment 2 Dean Jackson 2011-11-16 13:54:26 PST
Created attachment 115443 [details]
Patch
Comment 3 Simon Fraser (smfr) 2011-11-16 17:42:09 PST
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).
Comment 4 Dean Jackson 2011-11-16 23:28:08 PST
(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?
Comment 5 Dean Jackson 2011-11-16 23:28:25 PST
http://trac.webkit.org/changeset/100565
Comment 6 Simon Fraser (smfr) 2011-11-17 10:24:30 PST
(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.