Remove SVGPaint

Remove SVGPaint

Remove SVGPaint usage, instead use CSSValueList and CSSPrimitiveValue
for the fill/stroke CSS values.
This allows us to remove SVGPaint specific code in CSSValue.

Note that new result for svg/css/svg-attribute-parser-mode.svg matches
FireFox.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177550

[rwlbuis, 2014-06-30 22:11:02]

PTAL

[pdr, 2014-06-30 22:39:59]

I think this patch is correct but I'd like a second opinion from someone who is
more familiar with our css code. +cc leviw.

https://codereview.chromium.org/361543002/diff/40001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser.h (right):

https://codereview.chromium.org/361543002/diff/40001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser.h:97: static StyleColor
colorFromRGBColorString(const String&);
This doesn't feel like the right place for this function, but I don't have
another suggestion :/

https://codereview.chromium.org/361543002/diff/40001/Source/core/svg/SVGPaint...
File Source/core/svg/SVGPaint.cpp (left):

https://codereview.chromium.org/361543002/diff/40001/Source/core/svg/SVGPaint...
Source/core/svg/SVGPaint.cpp:1: /*
I thought we'd mark this file as deleted instead of modified. Maybe just a
Rietveld bug though.

https://codereview.chromium.org/361543002/diff/40001/Source/core/svg/SVGPaint.h
File Source/core/svg/SVGPaint.h (right):

https://codereview.chromium.org/361543002/diff/40001/Source/core/svg/SVGPaint...
Source/core/svg/SVGPaint.h:30: enum SVGPaintType {
I think we need a new home for this. CSSValue.h?

[leviw_travelin_and_unemployed, 2014-06-30 22:57:56]

I'm sadly not as familiar with the parsing bits as I wish I were. I can't catch
more than pdr already added, but I'm adding eseidel for his r s l g t m on the
CSS parsing bits.

[krit, 2014-07-01 06:01:55]

Reviewed the CSS parser part. Some comments before OKing the patch.

https://codereview.chromium.org/361543002/diff/60001/LayoutTests/svg/css/scri...
File LayoutTests/svg/css/script-tests/svg-attribute-parser-mode.js (right):

https://codereview.chromium.org/361543002/diff/60001/LayoutTests/svg/css/scri...
LayoutTests/svg/css/script-tests/svg-attribute-parser-mode.js:94:
shouldBeEqualToString("document.defaultView.getComputedStyle(rect,
null).stroke", "none");
This didn't work before? Wow.

https://codereview.chromium.org/361543002/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/361543002/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:602: if
(list->length() < 2)
Could you add an assert list->length() > 2? It isn't really a problem if it is
bigger than 2, but it is not expected either.

https://codereview.chromium.org/361543002/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:630: }
Don't you want to add an else with a return?

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/SVGCSSCo...
File Source/core/css/SVGCSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/SVGCSSCo...
Source/core/css/SVGCSSComputedStyleDeclaration.cpp:88:
RefPtrWillBeRawPtr<CSSValueList> values = CSSValueList::createSpaceSeparated();
This always creates a CSSValueList if color is involved. This is not the case
for parsing. Since getCSSPropertyValue doesn't exist anymore, this should be ok
though.

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1022: // FIXME: Rework css parser
so it is more SVG aware.
fill and stroke will have the same syntax as background shorthand (more or
less). -webkit-mask and background will accept paint servers/ mask operators as
well in the future. I don't think that this comment is correct. The CSS parser
must be reworked to support SVG resources in general.

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:8225: else if
(m_valueList->current()->id == CSSValueCurrentcolor)
You combine none and current color above, why not here? 

IMO, you could always create a space separated list, check for URI or none
first. In the "if", append URI/none to the list, check for m_valueList->next().
If m_valueList->next() false, return  the list (could be just the value as well
in which case the list will be destroyed again). After the "if" for none/URI,
you parse color, system color , currentColor and append it to the list if true.

This way you could avoid all the duplication.

However, the code is correct as it is currently. Just a suggestion to make it
shorter.

[fs, 2014-07-01 07:52:12]

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/SVGCSSCo...
File Source/core/css/SVGCSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/SVGCSSCo...
Source/core/css/SVGCSSComputedStyleDeclaration.cpp:85:
PassRefPtrWillBeRawPtr<CSSValue> adjustSVGPaintForCurrentColor(SVGPaintType
ptype, const String& url, const Color& color, const Color& currentColor)
Nit: Make static?

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1020: StyleColor
BisonCSSParser::colorFromRGBColorString(const String& colorString)
I suspect this method is better off being folded into it's callers. Could be
left for later of course.

[rwlbuis, 2014-07-01 14:23:09]

On 2014/07/01 07:52:12, fs wrote:
>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/SVGCSSCo...
> File Source/core/css/SVGCSSComputedStyleDeclaration.cpp (right):
> 
>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/SVGCSSCo...
> Source/core/css/SVGCSSComputedStyleDeclaration.cpp:85:
> PassRefPtrWillBeRawPtr<CSSValue> adjustSVGPaintForCurrentColor(SVGPaintType
> ptype, const String& url, const Color& color, const Color& currentColor)
> Nit: Make static?

Good point, fixed locally, will upload new one soon.

>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/B...
> File Source/core/css/parser/BisonCSSParser-in.cpp (right):
> 
>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/B...
> Source/core/css/parser/BisonCSSParser-in.cpp:1020: StyleColor
> BisonCSSParser::colorFromRGBColorString(const String& colorString)
> I suspect this method is better off being folded into it's callers. Could be
> left for later of course.

Right, all these helper methods seem a bit unfortunate but I guess we mostly
need them. But it does not seem so SVG related so I moved it out of svg/.

[rwlbuis, 2014-07-01 15:31:42]

On 2014/07/01 06:01:55, krit wrote:
> Reviewed the CSS parser part. Some comments before OKing the patch.
> 
>
https://codereview.chromium.org/361543002/diff/60001/LayoutTests/svg/css/scri...
> File LayoutTests/svg/css/script-tests/svg-attribute-parser-mode.js (right):
> 
>
https://codereview.chromium.org/361543002/diff/60001/LayoutTests/svg/css/scri...
> LayoutTests/svg/css/script-tests/svg-attribute-parser-mode.js:94:
> shouldBeEqualToString("document.defaultView.getComputedStyle(rect,
> null).stroke", "none");
> This didn't work before? Wow.

I was surprised too :)

>
https://codereview.chromium.org/361543002/diff/60001/Source/build/scripts/tem...
> File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/361543002/diff/60001/Source/build/scripts/tem...
> Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:602: if
> (list->length() < 2)
> Could you add an assert list->length() > 2? It isn't really a problem if it is
> bigger than 2, but it is not expected either.

Yes, that is better, fixed.

>
https://codereview.chromium.org/361543002/diff/60001/Source/build/scripts/tem...
> Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:630: }
> Don't you want to add an else with a return?

Fixed.

>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/SVGCSSCo...
> File Source/core/css/SVGCSSComputedStyleDeclaration.cpp (right):
> 
>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/SVGCSSCo...
> Source/core/css/SVGCSSComputedStyleDeclaration.cpp:88:
> RefPtrWillBeRawPtr<CSSValueList> values =
CSSValueList::createSpaceSeparated();
> This always creates a CSSValueList if color is involved. This is not the case
> for parsing. Since getCSSPropertyValue doesn't exist anymore, this should be
ok
> though.

I don't get the "if color is involved", it should only create it for >=
SVG_PAINTTYPE_URI_NONE.

>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/B...
> File Source/core/css/parser/BisonCSSParser-in.cpp (right):
> 
>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/B...
> Source/core/css/parser/BisonCSSParser-in.cpp:1022: // FIXME: Rework css parser
> so it is more SVG aware.
> fill and stroke will have the same syntax as background shorthand (more or
> less). -webkit-mask and background will accept paint servers/ mask operators
as
> well in the future. I don't think that this comment is correct. The CSS parser
> must be reworked to support SVG resources in general.

Ok, this patch is mostly a refactor, I think we should fix comments in separate
patches, unless it is really obvious.

>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/C...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/361543002/diff/60001/Source/core/css/parser/C...
> Source/core/css/parser/CSSPropertyParser.cpp:8225: else if
> (m_valueList->current()->id == CSSValueCurrentcolor)
> You combine none and current color above, why not here? 

You are right, fixed.

> IMO, you could always create a space separated list, check for URI or none
> first. In the "if", append URI/none to the list, check for
m_valueList->next().
> If m_valueList->next() false, return  the list (could be just the value as
well
> in which case the list will be destroyed again). After the "if" for none/URI,
> you parse color, system color , currentColor and append it to the list if
true.
> 
> This way you could avoid all the duplication.
> 
> However, the code is correct as it is currently. Just a suggestion to make it
> shorter.

Overruled! Just kidding, I'll have a try and see how this code looks....

[rwlbuis, 2014-07-01 18:45:35]

> > IMO, you could always create a space separated list, check for URI or none
> > first. In the "if", append URI/none to the list, check for
> m_valueList->next().
> > If m_valueList->next() false, return  the list (could be just the value as
> well
> > in which case the list will be destroyed again). After the "if" for
none/URI,
> > you parse color, system color , currentColor and append it to the list if
> true.
> > 
> > This way you could avoid all the duplication.
> > 
> > However, the code is correct as it is currently. Just a suggestion to make
it
> > shorter.
> 
> Overruled! Just kidding, I'll have a try and see how this code looks....

Ok I tried but I couldn't make it work and found it complicates the code in
/StyleBuilderFunctions.cpp.tmpl, so I'll go for the latest patch set.

[krit, 2014-07-03 04:57:21]

On 2014/07/01 18:45:35, rwlbuis wrote:
> > > IMO, you could always create a space separated list, check for URI or none
> > > first. In the "if", append URI/none to the list, check for
> > m_valueList->next().
> > > If m_valueList->next() false, return  the list (could be just the value as
> > well
> > > in which case the list will be destroyed again). After the "if" for
> none/URI,
> > > you parse color, system color , currentColor and append it to the list if
> > true.
> > > 
> > > This way you could avoid all the duplication.
> > > 
> > > However, the code is correct as it is currently. Just a suggestion to make
> it
> > > shorter.
> > 
> > Overruled! Just kidding, I'll have a try and see how this code looks....
> 
> Ok I tried but I couldn't make it work and found it complicates the code in
> /StyleBuilderFunctions.cpp.tmpl, so I'll go for the latest patch set.

I think it can be done more compact but you would need to change StyleBuilder as
well. I guess you didn't do that. However, since this code is "temporary", there
is no need to win a design or beauty contest. So fine with me.

[krit, 2014-07-03 05:00:32]

lgtm

LGTM

[davve, 2014-07-03 11:41:25]

Non-owner LGTM.

In the description: "This allows use " -> "This allows us"

Please also format the description to 72 columns or so to be a little more git
friendly.

Also, is there a bug reference you can add?

https://codereview.chromium.org/361543002/diff/80001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/361543002/diff/80001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1020: if
(BisonCSSParser::parseColor(color, colorString.stripWhiteSpace()))
If you're going to keep this method you may drop the BisonCSSParser:: prefix.

[rwlbuis, 2014-07-04 11:25:23]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-07-04 11:26:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob.buis@samsung.com/361543002/80001

[commit-bot: I haz the power, 2014-07-04 11:26:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-04 11:26:50]

Failed to apply patch for Source/core/animation/AnimatableSVGPaint.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/core/animation/AnimatableSVGPaint.cpp
  |diff --git a/Source/core/animation/AnimatableSVGPaint.cpp
b/Source/core/animation/AnimatableSVGPaint.cpp
  |index
94eca3cd5875c5ec4d48b503446c492ece0c67f4..d056dbe3d445e741b71ae684a764aa4d4510a7de
100644
  |--- a/Source/core/animation/AnimatableSVGPaint.cpp
  |+++ b/Source/core/animation/AnimatableSVGPaint.cpp
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       Source/core/animation/AnimatableSVGPaint.cpp
Index: Source/core/animation/AnimatableSVGPaint.cpp
diff --git a/Source/core/animation/AnimatableSVGPaint.cpp
b/Source/core/animation/AnimatableSVGPaint.cpp
index
94eca3cd5875c5ec4d48b503446c492ece0c67f4..d056dbe3d445e741b71ae684a764aa4d4510a7de
100644
--- a/Source/core/animation/AnimatableSVGPaint.cpp
+++ b/Source/core/animation/AnimatableSVGPaint.cpp
@@ -36,8 +36,8 @@ namespace WebCore {
 bool AnimatableSVGPaint::usesDefaultInterpolationWith(const AnimatableValue*
value) const
 {
     const AnimatableSVGPaint* svgPaint = toAnimatableSVGPaint(value);
-    return (paintType() != SVGPaint::SVG_PAINTTYPE_RGBCOLOR ||
svgPaint->paintType() != SVGPaint::SVG_PAINTTYPE_RGBCOLOR)
-        && (visitedLinkPaintType() != SVGPaint::SVG_PAINTTYPE_RGBCOLOR ||
svgPaint->visitedLinkPaintType() != SVGPaint::SVG_PAINTTYPE_RGBCOLOR);
+    return (paintType() != SVG_PAINTTYPE_RGBCOLOR || svgPaint->paintType() !=
SVG_PAINTTYPE_RGBCOLOR)
+        && (visitedLinkPaintType() != SVG_PAINTTYPE_RGBCOLOR ||
svgPaint->visitedLinkPaintType() != SVG_PAINTTYPE_RGBCOLOR);
 }
 
 PassRefPtrWillBeRawPtr<AnimatableValue> AnimatableSVGPaint::interpolateTo(const
AnimatableValue* value, double fraction) const

[rwlbuis, 2014-07-04 13:06:26]

On 2014/07/03 11:41:25, David Vest wrote:
> Non-owner LGTM.
> 
> In the description: "This allows use " -> "This allows us"

Right, fixed.

> Please also format the description to 72 columns or so to be a little more git
> friendly.

I tried my best :)

> Also, is there a bug reference you can add?

Sadly, I could not find a bug for this.

>
https://codereview.chromium.org/361543002/diff/80001/Source/core/css/parser/B...
> File Source/core/css/parser/BisonCSSParser-in.cpp (right):
> 
>
https://codereview.chromium.org/361543002/diff/80001/Source/core/css/parser/B...
> Source/core/css/parser/BisonCSSParser-in.cpp:1020: if
> (BisonCSSParser::parseColor(color, colorString.stripWhiteSpace()))
> If you're going to keep this method you may drop the BisonCSSParser:: prefix.

Fixed.

[rwlbuis, 2014-07-04 13:06:54]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-07-04 13:07:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob.buis@samsung.com/361543002/120001

[rwlbuis, 2014-07-04 22:13:24]

The CQ bit was unchecked by rob.buis@samsung.com

[rwlbuis, 2014-07-04 22:13:26]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-07-04 22:13:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob.buis@samsung.com/361543002/120001

[commit-bot: I haz the power, 2014-07-05 01:16:47]

Change committed as 177550