Split out String, URI and CustomIdent from CSSPrimitiveValue

Split out String, URI and CustomIdent from CSSPrimitiveValue

Remove String, URI and CustomIdent from CSSPrimitiveValue and moved them
into 3 new classes: CSSStringValue, CSSURIValue and CSSCustomIdentValue.

BUG=523893
Committed: https://crrev.com/10f96710fa357e0699f6ae62ac29b2c3bc1b3441
Cr-Commit-Position: refs/heads/master@{#352795}

[sashab, 2015-08-26 07:54:06]

sashab@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2015-08-31 06:32:47]

https://codereview.chromium.org/1306823004/diff/20001/Source/core/css/CSSStri...
File Source/core/css/CSSStringValue.h (right):

https://codereview.chromium.org/1306823004/diff/20001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:50: StringImpl* m_string;
..just store a String :\

[Timothy Loh, 2015-09-07 03:44:03]

timloh@chromium.org changed reviewers:
+ alancutter@chromium.org, dstockwell@chromium.org

[Timothy Loh, 2015-09-07 03:44:04]

This doesn't look like an improvement to me (esp. in the StyleBuilder code).
dstockwell/alancutter, WDYT?

[alancutter (OOO until 2018), 2015-09-07 05:00:34]

On 2015/09/07 at 03:44:04, timloh wrote:
> This doesn't look like an improvement to me (esp. in the StyleBuilder code).
dstockwell/alancutter, WDYT?

I agree the StyleBuilder code is a little ugly, it would be neater if we split
it up further into CSSStringValue, CSSIdentValue and CSSURIValue.

[Timothy Loh, 2015-09-07 05:14:42]

On 2015/09/07 05:00:34, alancutter wrote:
> On 2015/09/07 at 03:44:04, timloh wrote:
> > This doesn't look like an improvement to me (esp. in the StyleBuilder code).
> dstockwell/alancutter, WDYT?
> 
> I agree the StyleBuilder code is a little ugly, it would be neater if we split
> it up further into CSSStringValue, CSSIdentValue and CSSURIValue.

Maybe make a templated CSSStringValue class and use using declarations to define
the different actual classes?

[Timothy Loh, 2015-09-10 07:36:51]

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
File Source/core/css/CSSStringValue.h (right):

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:17: enum StringSerializationType {
This.. isn't really a serialization type, <string>s, <custom-ident>s and <url>s
are all technically different types.

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:30: return adoptRefWillBeNoop(new
CSSStringValueBase(str, StringClass));
This logic would be better in the ctor

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:40: String getStringValue() const { return
m_string; }
value() maybe?

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:42: String customCSSText() const
This should be in a .cpp file

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:79: DEFINE_CSS_VALUE_TYPE_CASTS(CSSIdentValue,
isIdentValue());
CSSCustomIdentValue and similar elsewhere

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/FontFac...
File Source/core/css/FontFace.cpp (right):

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/FontFac...
Source/core/css/FontFace.cpp:284: } else if (familyValue->isPrimitiveValue() &&
toCSSPrimitiveValue(familyValue)->isValueID()) {
This check probably isn't needed (and would be wrong if it were, since the fail
case is returning false).

[alancutter (OOO until 2018), 2015-09-17 02:59:30]

Much neater now that they're split up. The template approach is a little crazy
but doesn't extend beyond being an implementation detail of the 3 classes. lgtm
after logic fix.

https://codereview.chromium.org/1306823004/diff/120001/Source/core/css/CSSStr...
File Source/core/css/CSSStringValueBase.h (right):

https://codereview.chromium.org/1306823004/diff/120001/Source/core/css/CSSStr...
Source/core/css/CSSStringValueBase.h:39: : CSSValue(static_cast<ClassType>(T))
Is the static_cast necessary?

https://codereview.chromium.org/1306823004/diff/120001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1306823004/diff/120001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderCustom.cpp:744: if
(!item->isPrimitiveValue() && !item->isStringValue())
Shouldn't this be ||?

[sashab, 2015-09-17 04:19:49]

Responded to both sets of comments. :)

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
File Source/core/css/CSSStringValue.h (right):

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:17: enum StringSerializationType {
On 2015/09/10 07:36:51, Timothy Loh wrote:
> This.. isn't really a serialization type, <string>s, <custom-ident>s and
<url>s
> are all technically different types.

Used CSSValue's types, but had to make CSSValue::classType public to make this
easier. Not sure how you feel about this.

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:30: return adoptRefWillBeNoop(new
CSSStringValueBase(str, StringClass));
On 2015/09/10 07:36:51, Timothy Loh wrote:
> This logic would be better in the ctor

Just used CSSValue's types and got rid of this

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:40: String getStringValue() const { return
m_string; }
On 2015/09/10 07:36:51, Timothy Loh wrote:
> value() maybe?

Oh yeah, I meant to do this. Done.

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:42: String customCSSText() const
On 2015/09/10 07:36:51, Timothy Loh wrote:
> This should be in a .cpp file

Used specializations instead of the switch statement and put it in a cpp file.

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/CSSStri...
Source/core/css/CSSStringValue.h:79: DEFINE_CSS_VALUE_TYPE_CASTS(CSSIdentValue,
isIdentValue());
On 2015/09/10 07:36:51, Timothy Loh wrote:
> CSSCustomIdentValue and similar elsewhere

Done. Also renamed this file to CSSStringValueBase to be clearer.

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/FontFac...
File Source/core/css/FontFace.cpp (right):

https://codereview.chromium.org/1306823004/diff/60001/Source/core/css/FontFac...
Source/core/css/FontFace.cpp:284: } else if (familyValue->isPrimitiveValue() &&
toCSSPrimitiveValue(familyValue)->isValueID()) {
On 2015/09/10 07:36:51, Timothy Loh wrote:
> This check probably isn't needed (and would be wrong if it were, since the
fail
> case is returning false).

Fair enough, now it will ASSERT fail if its not a primitive value, same as
before.

https://codereview.chromium.org/1306823004/diff/120001/Source/core/css/CSSStr...
File Source/core/css/CSSStringValueBase.h (right):

https://codereview.chromium.org/1306823004/diff/120001/Source/core/css/CSSStr...
Source/core/css/CSSStringValueBase.h:39: : CSSValue(static_cast<ClassType>(T))
On 2015/09/17 02:59:29, alancutter wrote:
> Is the static_cast necessary?

Yes, but turns out you can add types to template arguments! :/ Made the template
argument a CSSValue::ClassType instead of an int.

https://codereview.chromium.org/1306823004/diff/120001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1306823004/diff/120001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderCustom.cpp:744: if
(!item->isPrimitiveValue() && !item->isStringValue())
On 2015/09/17 02:59:29, alancutter wrote:
> Shouldn't this be ||?

It's a bit confusing, but it should be &&. "If the item is not a primitive value
and is not a string value..."

Changed it to !(... || ...) to make it easier to read.

EDIT: Moved it into the if/else statement below.

[Timothy Loh, 2015-09-17 05:35:01]

Let's use a separate enum instead of making CSSValue's type enum public

https://codereview.chromium.org/1306823004/diff/140001/Source/core/css/CSSStr...
File Source/core/css/CSSStringValueBase.h (right):

https://codereview.chromium.org/1306823004/diff/140001/Source/core/css/CSSStr...
Source/core/css/CSSStringValueBase.h:16: class CORE_EXPORT CSSStringValueBase :
public CSSValue {
does this need to CORE_EXPORT?

[sashab, 2015-09-21 01:13:27]

Latest patch uses separate enum; ptal timloh.

https://codereview.chromium.org/1306823004/diff/140001/Source/core/css/CSSStr...
File Source/core/css/CSSStringValueBase.h (right):

https://codereview.chromium.org/1306823004/diff/140001/Source/core/css/CSSStr...
Source/core/css/CSSStringValueBase.h:16: class CORE_EXPORT CSSStringValueBase :
public CSSValue {
On 2015/09/17 05:35:01, Timothy Loh wrote:
> does this need to CORE_EXPORT?

Nope; removed.

[sashab, 2015-09-30 01:55:43]

Ping timloh

[Timothy Loh, 2015-10-02 05:35:25]

ok lgtm

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSStringValue.h (right):

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSStringValue.h:8: #include
"core/CoreExport.h"
not needed?

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSURIValue.h (right):

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSURIValue.h:8: #include "core/CoreExport.h"
not needed?

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:491:
ASSERT(namedGridLineValue->isCustomIdentValue());
no need to assert, the conversionbelow checks

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:783:
state.style()->setLocale(nullAtom);
Probably more sensible to assert the id is auto instead of checking it.

[Timothy Loh, 2015-10-02 05:35:37]

please update the patch description

[sashab, 2015-10-02 06:07:54]

Thanks Tim!! :-)

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSStringValue.h (right):

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSStringValue.h:8: #include
"core/CoreExport.h"
On 2015/10/02 05:35:25, Timothy Loh wrote:
> not needed?

Done.

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSURIValue.h (right):

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSURIValue.h:8: #include "core/CoreExport.h"
On 2015/10/02 05:35:25, Timothy Loh wrote:
> not needed?

Done.

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:491:
ASSERT(namedGridLineValue->isCustomIdentValue());
On 2015/10/02 05:35:25, Timothy Loh wrote:
> no need to assert, the conversionbelow checks

Good point, thanks. :)

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1306823004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:783:
state.style()->setLocale(nullAtom);
On 2015/10/02 05:35:25, Timothy Loh wrote:
> Probably more sensible to assert the id is auto instead of checking it.

Ahh good call, I did this everywhere else but didn't here. Thanks!

[sashab, 2015-10-02 06:14:02]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-10-02 06:14:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1306823004/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-02 06:14:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306823004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306823004/280001

[commit-bot: I haz the power, 2015-10-02 06:19:49]

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

[commit-bot: I haz the power, 2015-10-02 06:19:49]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sashab, 2015-10-05 23:34:28]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2015-10-05 23:35:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306823004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306823004/280001

[sashab, 2015-10-07 03:09:31]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-10-07 03:09:31]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1306823004/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-07 03:11:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306823004/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306823004/300001

[commit-bot: I haz the power, 2015-10-07 04:46:36]

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

[commit-bot: I haz the power, 2015-10-07 04:46:36]

Failed to apply patch for
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:7
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp' with
conflicts.
  U third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp

Patch:       third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
Index: third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
index
e746827912449a437188a22e750c36422013e339..d72462d2f16121f9117bb6c7a54a40f8e887a933
100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -7,8 +7,11 @@
 
 #include "core/StylePropertyShorthand.h"
 #include "core/css/CSSCalculationValue.h"
+#include "core/css/CSSCustomIdentValue.h"
 #include "core/css/CSSFontFaceSrcValue.h"
 #include "core/css/CSSFontFeatureValue.h"
+#include "core/css/CSSStringValue.h"
+#include "core/css/CSSURIValue.h"
 #include "core/css/CSSUnicodeRangeValue.h"
 #include "core/css/CSSValuePool.h"
 #include "core/css/parser/CSSParserFastPaths.h"
@@ -81,18 +84,18 @@ static PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
consumeIdent(CSSParserTokenRang
     return
cssValuePool().createIdentifierValue(range.consumeIncludingWhitespace().id());
 }
 
-static PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
consumeCustomIdent(CSSParserTokenRange& range)
+static PassRefPtrWillBeRawPtr<CSSCustomIdentValue>
consumeCustomIdent(CSSParserTokenRange& range)
 {
     if (range.peek().type() != IdentToken)
         return nullptr;
-    return
cssValuePool().createValue(range.consumeIncludingWhitespace().value(),
CSSPrimitiveValue::UnitType::CustomIdentifier);
+    return
CSSCustomIdentValue::create(range.consumeIncludingWhitespace().value());
 }
 
-static PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
consumeString(CSSParserTokenRange& range)
+static PassRefPtrWillBeRawPtr<CSSStringValue>
consumeString(CSSParserTokenRange& range)
 {
     if (range.peek().type() != StringToken)
         return nullptr;
-    return
cssValuePool().createValue(range.consumeIncludingWhitespace().value(),
CSSPrimitiveValue::UnitType::String);
+    return CSSStringValue::create(range.consumeIncludingWhitespace().value());
 }
 
 static String consumeUrl(CSSParserTokenRange& range)
@@ -394,7 +397,7 @@ static PassRefPtrWillBeRawPtr<CSSValue>
consumeFontFeatureSettings(CSSParserToke
     return settings.release();
 }
 
-static PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
consumePage(CSSParserTokenRange& range)
+static PassRefPtrWillBeRawPtr<CSSValue> consumePage(CSSParserTokenRange& range)
 {
     if (range.peek().id() == CSSValueAuto)
         return consumeIdent(range);
@@ -407,7 +410,7 @@ static PassRefPtrWillBeRawPtr<CSSValue>
consumeQuotes(CSSParserTokenRange& range
         return consumeIdent(range);
     RefPtrWillBeRawPtr<CSSValueList> values =
CSSValueList::createSpaceSeparated();
     while (!range.atEnd()) {
-        RefPtrWillBeRawPtr<CSSValue> parsedValue = consumeString(range);
+        RefPtrWillBeRawPtr<CSSStringValue> parsedValue = consumeString(range);
         if (!parsedValue)
             return nullptr;
         values->append(parsedValue.release());

[commit-bot: I haz the power, 2015-10-07 04:47:11]

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

[sashab, 2015-10-07 06:11:58]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-10-07 06:11:59]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1306823004/#ps340001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-07 06:13:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306823004/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306823004/340001

[commit-bot: I haz the power, 2015-10-07 07:37:39]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-10-07 07:38:34]

Patchset 18 (id:??) landed as
https://crrev.com/10f96710fa357e0699f6ae62ac29b2c3bc1b3441
Cr-Commit-Position: refs/heads/master@{#352795}