Split CSSPrimitiveValue into CSSPrimitiveValue and CSSIdentifierValue

Split CSSPrimitiveValue into CSSPrimitiveValue and CSSIdentifierValue

Split CSSPrimitiveValue into two subtypes, CSSPrimitiveValue (for
numerical and calc values) and CSSIdentifierValue (for identifiers).
This is part of a larger effort to split up CSSPrimitiveValue.

This patch is meant to be entirely mechanical changes; if it passes all
tests with no behaviour changes, it has been implemented correctly. The
patch itself was mainly done with find-and-replaces to createKeyword()
and convertTo<>(), then fixing any other errors, sucha as those
resulting from irregular flow (e.g. if (!x.isPrimitiveValue) return;
in CSSToStyleMap) or similar.

The largest change has been to CSSPrimitiveValueMappings.h, since the
convertTo<> method has been moved to CSSIdentifierValue. Also, a
create(Length, zoom) method was added to CSSValue temporarily, and
callers will later change to switch between which create method
(CSSPrimitiveValue or CSSIdentifierValue) they wish to use.

BUG=523893

[sashab, 2016-09-19 05:29:45]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 05:29:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 06:19:26]

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

[commit-bot: I haz the power, 2016-09-19 06:19:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sashab, 2016-09-19 06:20:52]

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

[sashab, 2016-09-19 06:20:53]

Sorry this patch is so big :( Wanna discuss it in person?

[alancutter (OOO until 2018), 2016-09-19 06:33:44]

alancutter@chromium.org changed reviewers:
+ alancutter@chromium.org

[alancutter (OOO until 2018), 2016-09-19 06:33:45]

Yay. :D

[sashab, 2016-09-20 01:42:29]

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

[sashab, 2016-09-20 01:42:30]

Renee for initial review.

[Timothy Loh, 2016-09-21 07:55:06]

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2788: static
bool isGridTrackFixedSizedInternal(const CSSValue& value)
I'm not sure about this name change, both of these functions are only scoped to
this file so I would consider them both internal. Maybe isGridBreadthFixedSized
works better.

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2799: return
true;
Is this actually reached?

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.cpp
(right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.cpp:150: if
(!value || !value->isPrimitiveValue() || !value->isIdentifierValue())
This logic (and below in the file) is wrong since now the if condition will
always be false.

[rjwright, 2016-09-22 01:21:11]

It would be good for CSSIdentifierValue to have a name that reflects that it's
for keywords. To me "identifier" usually means variable name. As discussed,
calling it CSSKeywordValue creates a collision with the Typed CSSOM class
CSSKeywordValue. Maybe we can namespace Typed CSSOM.

Class-level code comments for CSSPrimitiveValue and CSSIdentifierValue would be
useful.

[rjwright, 2016-09-22 01:29:55]

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp
(right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/CSSColorInterpolationType.cpp:84: const
CSSIdentifierValue& ident = toCSSIdentifierValue(value);
Avoid abbreviated variable names. You've used "identifierValue" elsewhere.

[rjwright, 2016-09-22 06:12:50]

Check your newly-added TODOs and do the easy ones that don't make the CL too
much bigger.

LGTM

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSIdentifierValue.cpp (right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSIdentifierValue.cpp:47: // TODO(sashab):
Add a DCHECK_NE(valueID, CSSValueInvalid).
Want to add this now?

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSIdentifierValue.h (right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSIdentifierValue.h:16: // TODO(sashab):
Rename this to just create().
How come you don't do this in this patch? Does it make it too big?

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSIdentifierValue.h:19: // TODO(sashab):
Rename this to createFromPlatformValue().
Why wait?

[sashab, 2016-09-23 01:01:11]

Thanks renee. Added class-level comments to CSSPrimitiveValue and
CSSIdentifierValue, and CSSValue too, just explaining what they do. Also
explained why some of the TODOs are there.

timloh@, please review for OWNERs. :)

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSIdentifierValue.cpp (right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSIdentifierValue.cpp:47: // TODO(sashab):
Add a DCHECK_NE(valueID, CSSValueInvalid).
On 2016/09/22 at 06:12:50, rjwright wrote:
> Want to add this now?

It makes some tests fail :( clarified the todo :)

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSIdentifierValue.h (right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSIdentifierValue.h:16: // TODO(sashab):
Rename this to just create().
On 2016/09/22 at 06:12:50, rjwright wrote:
> How come you don't do this in this patch? Does it make it too big?

I have a style where I like to limit patches to just a single change. It helps
prevent rebasing problems and keeps the CL description meaningful. Also, this
patch is sort of 'the hard part', whereas a rename is relatively easy - if I
don't get around to it, someone else can easily do it. But I want to get the
hard part out of the way :)

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSIdentifierValue.h:19: // TODO(sashab):
Rename this to createFromPlatformValue().
On 2016/09/22 at 06:12:50, rjwright wrote:
> Why wait?

Same argument as above.

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2788: static
bool isGridTrackFixedSizedInternal(const CSSValue& value)
On 2016/09/21 at 07:55:06, Timothy Loh wrote:
> I'm not sure about this name change, both of these functions are only scoped
to this file so I would consider them both internal. Maybe
isGridBreadthFixedSized works better.

Nice! Done

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2799: return
true;
On 2016/09/21 at 07:55:06, Timothy Loh wrote:
> Is this actually reached?

Added NOTREACHED() and nothing failed :) So leaving that there.

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.cpp
(right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.cpp:150: if
(!value || !value->isPrimitiveValue() || !value->isIdentifierValue())
On 2016/09/21 at 07:55:06, Timothy Loh wrote:
> This logic (and below in the file) is wrong since now the if condition will
always be false.

Ack, I hate these confusing if-statements!! I want to write a patch to remove
them all. Fixed.

[rjwright, 2016-09-23 01:15:30]

Great class comments. Fair call on the to-dos; just checking.

[Timothy Loh, 2016-09-23 04:06:14]

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSIdentifierValue.h (right):

https://codereview.chromium.org/2346193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSIdentifierValue.h:16: // TODO(sashab):
Rename this to just create().
On 2016/09/23 01:01:10, sashab wrote:
> On 2016/09/22 at 06:12:50, rjwright wrote:
> > How come you don't do this in this patch? Does it make it too big?
> 
> I have a style where I like to limit patches to just a single change. It helps
> prevent rebasing problems and keeps the CL description meaningful. Also, this
> patch is sort of 'the hard part', whereas a rename is relatively easy - if I
> don't get around to it, someone else can easily do it. But I want to get the
> hard part out of the way :)

At least for this one you're already renaming from
CSSPrimitiveValue::createIdentifier to CSSIdentifierValue::createIdentifier, why
not just rename it to CSSIdentifierValue::create straight away instead of
touching all the lines twice? The other TODOs I'm fine with.

[sashab, 2016-09-26 01:55:26]

No prob, done and removed TODO for that :)

[sashab, 2016-09-26 01:55:35]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 01:55:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 02:51:15]

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

[commit-bot: I haz the power, 2016-09-26 02:51:18]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Timothy Loh, 2016-09-26 07:23:48]

Looks like this is still failing lots of tests

[sashab, 2016-09-27 01:01:23]

On 2016/09/26 at 07:23:48, timloh wrote:
> Looks like this is still failing lots of tests

Leave the failing bots to me. Does this patch look good to you or not?

[sashab, 2016-09-29 05:14:35]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 05:14:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 06:10:29]

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

[commit-bot: I haz the power, 2016-09-29 06:10:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2016-09-30 02:28:41]

We have to land this today, or the chromium reformat is going to make this
impossible to rebase.

[sashab, 2016-09-30 02:47:23]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 02:47:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 02:50:27]

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

[commit-bot: I haz the power, 2016-09-30 02:50:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sashab, 2016-09-30 03:01:15]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 03:01:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alancutter (OOO until 2018), 2016-09-30 04:25:54]

lgtm

https://codereview.chromium.org/2346193002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSContentDistributionValue.h (right):

https://codereview.chromium.org/2346193002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSContentDistributionValue.h:22: //
TODO(sashab): Make these result CSSValueIDs instead of CSSValues.
s/result/return/

[sashab, 2016-09-30 04:29:02]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 04:29:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 04:32:26]

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

[commit-bot: I haz the power, 2016-09-30 04:32:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Timothy Loh, 2016-09-30 05:44:36]

lgtm

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.h:61: // CSSPrimitiveValue
stores both numbers/lengths (e.g. 1, 10px, 4%) and calc()
4% isn't a length, also it stores other values like angles and so on. Maybe
'numeric data types' is a better description.

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.h:63: // TODO(sashab):
Split calc functionality out of CSSPrimitiveValue.
I think calc makes sense to be in CSSPrimtiveValue, what do you expect things to
look like if you do this? I don't think this TODO should be added right now.

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValue.h:37: // CSSValues are
property-independent; they can be created for any property, but
This sounds contradictory (and is probably wrong because
CSSCustomPropertyDeclaration objects store property information). Can we omit
the comments here for this patch since yuo want to get this in today and
bikeshed it later?

[sashab, 2016-09-30 06:45:48]

Thanks tim; lets get this patch landed! :)

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.h:61: // CSSPrimitiveValue
stores both numbers/lengths (e.g. 1, 10px, 4%) and calc()
On 2016/09/30 at 05:44:35, Timothy Loh wrote:
> 4% isn't a length, also it stores other values like angles and so on. Maybe
'numeric data types' is a better description.

Done

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.h:63: // TODO(sashab):
Split calc functionality out of CSSPrimitiveValue.
On 2016/09/30 at 05:44:35, Timothy Loh wrote:
> I think calc makes sense to be in CSSPrimtiveValue, what do you expect things
to look like if you do this? I don't think this TODO should be added right now.

Done

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/2346193002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValue.h:37: // CSSValues are
property-independent; they can be created for any property, but
On 2016/09/30 at 05:44:35, Timothy Loh wrote:
> This sounds contradictory (and is probably wrong because
CSSCustomPropertyDeclaration objects store property information). Can we omit
the comments here for this patch since yuo want to get this in today and
bikeshed it later?

Done, removed entire header comment in this file.

[sashab, 2016-09-30 06:51:04]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-09-30 06:51:05]

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

[commit-bot: I haz the power, 2016-09-30 06:51:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 07:01:50]

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

[commit-bot: I haz the power, 2016-09-30 07:01:52]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jochen (gone - plz use gerrit), 2016-09-30 12:31:30]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-09-30 12:31:31]

source/web lgtm

[sashab, 2016-09-30 12:33:38]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-09-30 12:33:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjwright@chromium.org, timloh@chromium.org, jochen@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2346193002/#ps180001
(title: "Yet another rebase")

[commit-bot: I haz the power, 2016-09-30 12:33:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 14:41:39]

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

[commit-bot: I haz the power, 2016-09-30 14:41:42]

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

Patch:       third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
Index: third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
diff --git a/third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
b/third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
index
48ecabd483dc1aa8b4f4a6acf92289df01a77a12..2080116b76ca5f9544574e8df62c14140ff024bb
100644
--- a/third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
+++ b/third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
@@ -32,12 +32,13 @@
 
 #include "core/CSSValueKeywords.h"
 #include "core/css/CSSCalculationValue.h"
+#include "core/css/CSSIdentifierValue.h"
 #include "core/css/CSSPrimitiveValue.h"
 #include "core/css/CSSReflectionDirection.h"
 #include "core/css/CSSToLengthConversionData.h"
 #include "core/style/ComputedStyleConstants.h"
-#include "core/style/SVGComputedStyleDefs.h"
 #include "core/style/LineClampValue.h"
+#include "core/style/SVGComputedStyleDefs.h"
 #include "platform/Length.h"
 #include "platform/ThemeTypes.h"
 #include "platform/fonts/FontDescription.h"
@@ -54,6 +55,7 @@
 
 namespace blink {
 
+// TODO(sashab): Move these to CSSPrimitiveValue.h.
 template<> inline short CSSPrimitiveValue::convertTo() const
 {
     ASSERT(isNumber());
@@ -103,29 +105,29 @@ template<> inline LineClampValue
CSSPrimitiveValue::convertTo() const
     return LineClampValue();
 }
 
-template<> inline CSSPrimitiveValue::CSSPrimitiveValue(CSSReflectionDirection
e)
-    : CSSValue(PrimitiveClass)
+// TODO(sashab): Move these to CSSIdentifierValueMappings.h, and update to use
+// the CSSValuePool.
+template<> inline CSSIdentifierValue::CSSIdentifierValue(CSSReflectionDirection
e)
+    : CSSValue(IdentifierClass)
 {
-    init(UnitType::ValueID);
     switch (e) {
     case ReflectionAbove:
-        m_value.valueID = CSSValueAbove;
+        m_valueID = CSSValueAbove;
         break;
     case ReflectionBelow:
-        m_value.valueID = CSSValueBelow;
+        m_valueID = CSSValueBelow;
         break;
     case ReflectionLeft:
-        m_value.valueID = CSSValueLeft;
+        m_valueID = CSSValueLeft;
         break;
     case ReflectionRight:
-        m_value.valueID = CSSValueRight;
+        m_valueID = CSSValueRight;
     }
 }
 
-template<> inline CSSReflectionDirection CSSPrimitiveValue::convertTo() const
+template<> inline CSSReflectionDirection CSSIdentifierValue::convertTo() const
 {
-    ASSERT(isValueID());
-    switch (m_value.valueID) {
+    switch (m_valueID) {
     case CSSValueAbove:
         return ReflectionAbove;
     case CSSValueBelow:
@@ -142,50 +144,45 @@ template<> inline CSSReflectionDirection
CSSPrimitiveValue::convertTo() const
     return ReflectionBelow;
 }
 
-template<> inline CSSPrimitiveValue::CSSPrimitiveValue(ColumnFill columnFill)
-    : CSSValue(PrimitiveClass)
+template<> inline CSSIdentifierValue::CSSIdentifierValue(ColumnFill columnFill)
+    : CSSValue(IdentifierClass)
 {
-    init(UnitType::ValueID);
     switch (columnFill) {
     case ColumnFillAuto:
-        m_value.valueID = CSSValueAuto;
+        m_valueID = CSSValueAuto;
         break;
     case ColumnFillBalance:
-        m_value.valueID = CSSValueBalance;
+        m_valueID = CSSValueBalance;
         break;
     }
 }
 
-template<> inline ColumnFill CSSPrimitiveValue::convertTo() const
+template<> inline ColumnFill CSSIdentifierValue::convertTo() const
 {
-    if (type() == UnitType::ValueID) {
-        if (m_value.valueID == CSSValueBalance)
-            return ColumnFillBalance;
-        if (m_value.valueID == CSSValueAuto)
-            return ColumnFillAuto;
-    }
+    if (m_valueID == CSSValueBalance)
+        return ColumnFillBalance;
+    if (m_valueID == CSSValueAuto)
+        return ColumnFillAuto;
     ASSERT_NOT_REACHED();
     return ColumnFillBalance;
 }
 
-template<> inline CSSPrimitiveValue::CSSPrimitiveValue(ColumnSpan columnSpan)
-    : CSSValue(PrimitiveClass)
+template<> inline CSSIdentifierValue::CSSIdentifierValue(ColumnSpan columnSpan)
+    : CSSValue(IdentifierClass)
 {
-    init(UnitType::ValueID);
     switch (columnSpan) {
     case ColumnSpanAll:
-        m_value.valueID = CSSValueAll;
+        m_valueID = CSSValueAll;
         break;
     case ColumnSpanNone:
-        m_value.valueID = CSSValueNone;
+        m_valueID = CSSValueNone;
         break;
     }
 }
 
-template<> inline ColumnSpan CSSPrimitiveValue::convertTo() const
+template<> inline ColumnSpan CSSIdentifierValue::convertTo() const
 {
-    ASSERT(isValueID());
-    switch (m_value.valueID) {
+    switch (m_valueID) {
     case CSSValueAll:
         return ColumnSpanAll;
     default:
@@ -197,24 +194,22 @@ template<> inline ColumnSpan
CSSPrimitiveValue::convertTo() const
 }
 
 
-template<> inline CSSPrimitiveValue::CSSPrimitiveValue(PrintColorAdjust value)
-    : CSSValue(PrimitiveClass)
+template<> inline CSSIdentifierValue::CSSIdentifierValue(PrintColorAdjust
value)
+    : CSSValue(IdentifierClass)
 {
-    init(UnitType::ValueID);
     switch (value) {
     case PrintColorAdjustExact:
-        m_value.valueID = CSSValueExact;
+        m_valueID = CSSValueExact;
         break;
     case PrintColorAdjustEconomy:
-        m_value.valueID = CSSValueEconomy;
+        m_valueID = CSSValueEconomy;
         break;
     }
 }
 
-template<> inline PrintColorAdjust CSSPrimitiveValue::convertTo() const
+template<> inline PrintColorAdjust CSSIdentifierValue::convertTo() const
 {
-    ASSERT(isValueID());
-    switch (m_value.valueID) {
+    switch (m_valueID) {
     case CSSValueEconomy:
         return PrintColorAdjustEconomy;
     case CSSValueExact:
@@ -228,99 +223,96 @@ template<> inline PrintColorAdjust
CSSPrimitiveValue::convertTo() const
 }
 
 
-template<> inline CSSPrimitiveValue::CSSPrimitiveValue(EBorderStyle e)
-    : CSSValue(PrimitiveClass)
+template<> inline CSSIdentifierValue::CSSIdentifierValue(EBorderStyle e)
+    : CSSValue(IdentifierClass)
 {
-    init(UnitType::ValueID);
     switch (e) {
     case BorderStyleNone:
-        m_value.valueID = CSSValueNone;
+        m_valueID = CSSValueNone;
         break;
     case BorderStyleHidden:
-        m_value.valueID = CSSValueHidden;
+        m_valueID = CSSValueHidden;
         break;
     case BorderStyleInset:
-        m_value.valueID = CSSValueInset;
+        m_valueID = CSSValueInset;
         break;
     case BorderStyleGroove:
-        m_value.valueID = CSSValueGroove;
+        m_valueID = CSSValueGroove;
         break;
     case BorderStyleRidge:
-        m_value.valueID = CSSValueRidge;
+        m_valueID = CSSValueRidge;
         break;
     case BorderStyleOutset:
-        m_value.valueID = CSSValueOutset;
+        m_valueID = CSSValueOutset;
         break;
     case BorderStyleDotted:
-        m_value.valueID = CSSValueDotted;
+        m_valueID = CSSValueDotted;
         break;
     case BorderStyleDashed:
-        m_value.valueID = CSSValueDashed;
+        m_valueID = CSSValueDashed;
         break;
     case BorderStyleSolid:
-        m_value.valueID = CSSValueSolid;
+        m_valueID = CSSValueSolid;
         break;
     case BorderStyleDouble:
-        m_value.valueID = CSSValueDouble;
+        m_valueID = CSSValueDouble;
         break;
     }
 }
 
-template<> inline EBorderStyle CSSPrimitiveValue::convertTo() const
+template<> inline EBorderStyle CSSIdentifierValue::convertTo() const
 {
-    ASSERT(isValueID());
-    if (m_value.valueID == CSSValueAuto) // Valid for CSS outline-style
+    if (m_valueID == CSSValueAuto) // Valid for CSS outline-style
         return BorderStyleDotted;
-    return (EBorderStyle)(m_value.valueID - CSSValueNone);
+    return (EBorderStyle)(m_valueID - CSSValueNone);
 }
 
-template<> inline OutlineIsAuto CSSPrimitiveValue::convertTo() const
+template<> inline OutlineIsAuto CSSIdentifierValue::convertTo() const
 {
-    if (m_value.valueID == CSSValueAuto)
+    if (m_valueID == CSSValueAuto)
         return OutlineIsAutoOn;
     return OutlineIsAutoOff;
 }
 
-template<> inline CSSPrimitiveValue::CSSPrimitiveValue(CompositeOperator e)
-    : CSSValue(PrimitiveClass)
+template<> inline CSSIdentifierValue::CSSIdentifierValue(CompositeOperator e)
+    : CSSValue(IdentifierClass)
 {
-    init(UnitType::ValueID);
     switch (e) {
     case CompositeClear:
-        m_value.valueID = CSSValueClear;
+        m_valueID = CSSValueClear;
         break;
     case CompositeCopy:
-        m_value.valueID = CSSValueCopy;
+        m_valueID = CSSValueCopy;
         break;
     case CompositeSourceOver:
-        m_value.valueID = CSSValueSourceOver;
+        m_valueID = CSSValueSourceOver;
         break;
     case CompositeSourceIn:
-        m_value.valueID = CSSValueSourceIn;
+        m_valueID = CSSValueSourceIn;
         break;
     case CompositeSourceOut:
-        m_value.valueID = CSSValueSourceOut;
+        m_valueID = CSSValueSourceOut;
         break;
     case CompositeSourceAtop:
-        m_value.valueID = CSSValueSourceAtop;
+        m_valueID = CSSValueSourceAtop;
         break;
     case CompositeDestinationOver:
-        m_value.valueID = CSSValueDestinationOver;
+        m_valueID = CSSValueDestinationOver;
         break;
     case CompositeDestinationIn:
-        m_value.valueID = CSSValueDestinationIn;
+        m_valueID = CSSValueDestinationIn;
         break;
     case CompositeDestinationOut:
-        m_value.valueID = CSSValueDestinationOut;
+        m_valueID = CSSValueDestinationOut;
         break;
     case CompositeDestinationAtop:
-        m_value.valueID = CSSValueDestinationAtop;
…
(message too large)

[sashab, 2016-09-30 14:50:38]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2016-09-30 14:50:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 14:59:32]

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

[commit-bot: I haz the power, 2016-09-30 14:59:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sashab, 2016-09-30 15:03:21]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2016-09-30 15:03:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 15:11:12]

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

[commit-bot: I haz the power, 2016-09-30 15:11:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[esprehn, 2016-09-30 19:33:05]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-09-30 19:33:05]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2016-09-30 19:33:08]

lgtm

https://codereview.chromium.org/2346193002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSIdentifierValue.h (right):

https://codereview.chromium.org/2346193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSIdentifierValue.h:17: class CORE_EXPORT
CSSIdentifierValue : public CSSValue {
final ?

[commit-bot: I haz the power, 2016-09-30 19:33:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 19:40:59]

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

[commit-bot: I haz the power, 2016-09-30 19:41:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[esprehn, 2016-09-30 20:01:54]

Looks like merge conflict on presubmit :P

[sashab, 2016-09-30 23:00:29]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-09-30 23:00:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
timloh@chromium.org, esprehn@chromium.org, rjwright@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2346193002/#ps200001
(title: "Rebase please work")

[commit-bot: I haz the power, 2016-09-30 23:01:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 00:05:13]

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

[commit-bot: I haz the power, 2016-10-01 00:05:15]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2016-10-01 02:45:11]

On 2016/10/01 00:05:15, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

As mentioned offline, I'm going to help land this patch.

[sashab, 2016-10-04 02:44:43]

Landed by dcheng:
https://crrev.com/90e5a7289784dc5a491a7f1e3a87f30a1eb0bbad

Thanks!