[webkit-reviews] review granted: [Bug 185113] Fix color-filter to apply to SVG colors : [Attachment 339070] Patch

Sat Apr 28 11:49:22 PDT 2018

Dean Jackson <dino at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 185113: Fix color-filter to apply to SVG colors
https://bugs.webkit.org/show_bug.cgi?id=185113

Attachment 339070: Patch

https://bugs.webkit.org/attachment.cgi?id=339070&action=review

--- Comment #14 from Dean Jackson <dino at apple.com> ---
Comment on attachment 339070
  --> https://bugs.webkit.org/attachment.cgi?id=339070
Patch

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

It seems this new test doesn't pass.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:239
> +    for (Gradient::ColorStop stop : stops) {

Nice.

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:123
> +	   Color color =
renderer->style().colorByApplyingColorFilter(renderer->style().svgStyle().light
ingColor());

auto.

Also, is there any point of having the local variable?

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:205
> +    Color color =
renderer->style().colorByApplyingColorFilter(renderer->style().svgStyle().light
ingColor());

auto

> LayoutTests/css3/color-filters/svg/color-filter-inline-svg-expected.html:2
> +<html >

wat

> LayoutTests/css3/color-filters/svg/color-filter-inline-svg-expected.html:5
> +	   <link rel="author" title="Apple" href="http://www.apple.com/">

Is this preparing for WPT?

> LayoutTests/css3/color-filters/svg/color-filter-inline-svg-expected.html:7
> +	   <style type="text/css">

no need for type

> LayoutTests/css3/color-filters/svg/color-filter-inline-svg-expected.html:15
> +<svg viewBox="0 0 800 600" style="width: 800px; height: 600px">

Why isn't this just an SVG file? Why is it embedded in HTML?