[webkit-reviews] review denied: [Bug 72443] Implement url() filter function : [Attachment 148586] Fixed image results (minor pixel diffs due to intervening skia change)

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 72443: Implement url() filter function
https://bugs.webkit.org/show_bug.cgi?id=72443

Attachment 148586: Fixed image results (minor pixel diffs due to intervening
skia change)
https://bugs.webkit.org/attachment.cgi?id=148586&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=148586&action=review


Looking pretty good. Just some small points and questions.

> Source/WebCore/ChangeLog:5
> +	   Implement filter url() function.
> +	   https://bugs.webkit.org/show_bug.cgi?id=72443
> +

Please add some description of the implementation here, at least at a
high-level. I can read the details for each file below, but without much
context.

> Source/WebCore/ChangeLog:80
> +	   * platform/mac/ScrollbarThemeMac.mm:
> +	   Fix mac build for the GraphicsLayer/PlatformLayer change.

I guess you mean just include PlatformLayer.h?

> Source/WebCore/Target.pri:1687
> +    css/WebKitCSSSVGDocumentValue.h \
>      css/WebKitCSSShaderValue.h \

Shouldn't the new PlatformLayer.h be listed in this file too?

> Source/WebCore/WebCore.gypi:2585
> +	       'css/WebKitCSSSVGDocumentValue.cpp',
> +	       'css/WebKitCSSSVGDocumentValue.h',
>	       'css/WebKitCSSTransformValue.cpp',

ditto on PlatformLayer.h

> Source/WebCore/css/StyleResolver.cpp:1801
> +    // Start loading resources referenced by this style.
> +    loadPendingResources();
>      
> -#if ENABLE(CSS_SHADERS)
> -    // Start loading the shaders referenced by this style.
> -    loadPendingShaders();
> -#endif
> -
> +    // Start loading all pending resources referenced by this style.

This last comment is a duplicate.

> Source/WebCore/css/StyleResolver.cpp:5676
> +    if (!m_style->hasFilter() || m_pendingSVGDocuments.isEmpty())
> +	   return;

This might show me up as a moron, but shouldn't this be && ? If there is no
filter AND nothing pending? I understand the no filter bit. I assume this is
because the item would never have been put into the pending list if it was
already in the process of being loaded?

> Source/WebCore/css/WebKitCSSSVGDocumentValue.cpp:14
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE

I think you have the wrong license here.

> Source/WebCore/css/WebKitCSSSVGDocumentValue.h:15
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR

ditto here

> Source/WebCore/css/WebKitCSSSVGDocumentValue.h:49
> +    WebKitCSSSVGDocumentValue(const String& url);

Since you have inline functions above, any reason why you don't put the
constructor here as well? I have zero feelings about this - just asking. Same
with customCssText.

> Source/WebCore/platform/graphics/filters/FilterOperation.h:190
> +    void* m_data;

It's annoying we can't use a CachedSVGDocument here :(

> Source/WebCore/rendering/FilterEffectRenderer.cpp:129
> +    if (!document)
> +	   return 0;

Would this case be anything other than an error? Worth putting an ASSERT?

> Source/WebCore/rendering/FilterEffectRenderer.cpp:141
> +    // FIXME: Figure out what to do with SourceAlpha. Right now, we're
> +    // using the alpha of the original input layer, which is obviously
> +    // wrong. We should probably be extracting the alpha from the 
> +    // previousEffect, but this requires some more processing.  
> +    // This may need a spec clarification.

Good point. I'm not sure the input element alpha is definitely wrong, but this
is the first time we've had this in a chain.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:352
> +	       if (filterOperation->getOperationType() !=
FilterOperation::REFERENCE) {
>		   effect->inputEffects().append(previousEffect);
> -	       m_effects.append(effect);
> +		   m_effects.append(effect);

Why is this skipped for url()?

> Source/WebCore/rendering/RenderLayerFilterInfo.h:89
> +    void updateReferenceFilter(ReferenceFilterOperation*);

I don't see this used anywhere. Am I missing it?

> LayoutTests/css3/filters/effect-reference-ordering.html:15
> +<img style="-webkit-filter: url(#blurY);" src="resources/reference.png">
> +<img style="-webkit-filter: contrast(500%) url(#blurY);"
src="resources/reference.png">
> +<img style="-webkit-filter: url(#blurY) contrast(500%);"
src="resources/reference.png">

could we add another that's shorthand + url + shorthand?

> LayoutTests/css3/filters/resources/hueRotate.svg:5
> +    <filter id="MyFilter">
> +	 <feColorMatrix type="hueRotate" values="180"/>
> +    </filter>

Since we're replicating the filterBuilder functionality a bit, I think we also
need a test that has a filter with nested elements and non-linear input (named
inputs).

>
LayoutTests/platform/chromium-linux/css3/filters/effect-reference-expected.txt:
10
> +	       [feColorMatrix type="HUEROTATE" values="180.00"]
> +		 [SourceGraphic]

Hmmm... I wonder if we should decorate our filtered elements like this too, or
provide that option in WKTR?