Split out CSSPrimitiveValue's PropertyID into CSSCustomIdentValue

Split out CSSPrimitiveValue's PropertyID into CSSCustomIdentValue

Move PropertyID CSSPrimitiveValues into CSSCustomIdentValue, to be
stored as an optimization for will-change and other properties (the spec
specifies these as custom identifiers, but since they represent
properties they can be stored as property IDs).

BUG=523893
Committed: https://crrev.com/2e9a05075d75ebd9fb67718a214f32bc10ac2782
Cr-Commit-Position: refs/heads/master@{#353664}

[sashab, 2015-09-25 04:44:56]

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

[sashab, 2015-09-25 04:44:56]

PTAL. Can merge with crrev.com/1306823004 if needed.

[Timothy Loh, 2015-09-25 05:44:16]

If we do this, then the patch moving the other string types out of
CSSPrimitiveValue should only move those out and not use the template class.

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSStringValues.cpp (right):

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStringValues.cpp:13: namespace {
..please read over your patches before sending them out

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSStringValues.h (right):

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStringValues.h:35: class
CSSCustomIdentValue : public CSSValue {
one class per header file

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStringValues.h:42: static
PassRefPtrWillBeRawPtr<CSSCustomIdentValue> create(CSSPropertyID propertyID)
I think this class would look better as:

{
public:
 static create(const String&) // no CSSPropertyID ctor!

 String value()
 bool isKnownPropertyID()
 CSSPropertyID valueAsPropertyID()

private:
 ensureParsedAsPropertyID();

 String m_string; // or atomicstring? might save a several KB on big web
properties
 mutable bool m_parsedAsPropertyID;
 mutable CSSPropertyID m_propertyId;
}

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStringValues.h:62: // TODO(sashab): Store
these in a union.
let's not do this

[sashab, 2015-09-28 03:37:03]

Updated both this patch and crrev.com/1306823004 as per feedback.

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSStringValues.h (right):

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStringValues.h:42: static
PassRefPtrWillBeRawPtr<CSSCustomIdentValue> create(CSSPropertyID propertyID)
On 2015/09/25 05:44:16, Timothy Loh wrote:
> I think this class would look better as:
> 
> {
> public:
>  static create(const String&) // no CSSPropertyID ctor!
> 
>  String value()
>  bool isKnownPropertyID()
>  CSSPropertyID valueAsPropertyID()
> 
> private:
>  ensureParsedAsPropertyID();
> 
>  String m_string; // or atomicstring? might save a several KB on big web
> properties
>  mutable bool m_parsedAsPropertyID;
>  mutable CSSPropertyID m_propertyId;
> }

Changing all the callsites in the parser to pass the unparsed string instead of
the property ID directly will take a while. Also, some places need the property
ID constructor (e.g.
ComputedStyle->rareNonInheritedData->m_willChange->m_properties is stored as a
list of property IDs). I think this should come in a later patch.

[alancutter (OOO until 2018), 2015-09-30 02:08:51]

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSStringValues.h (right):

https://codereview.chromium.org/1373433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStringValues.h:42: static
PassRefPtrWillBeRawPtr<CSSCustomIdentValue> create(CSSPropertyID propertyID)
On 2015/09/28 at 03:37:03, sashab wrote:
> On 2015/09/25 05:44:16, Timothy Loh wrote:
> > I think this class would look better as:
> > 
> > {
> > public:
> >  static create(const String&) // no CSSPropertyID ctor!
> > 
> >  String value()
> >  bool isKnownPropertyID()
> >  CSSPropertyID valueAsPropertyID()
> > 
> > private:
> >  ensureParsedAsPropertyID();
> > 
> >  String m_string; // or atomicstring? might save a several KB on big web
> > properties
> >  mutable bool m_parsedAsPropertyID;
> >  mutable CSSPropertyID m_propertyId;
> > }
> 
> Changing all the callsites in the parser to pass the unparsed string instead
of the property ID directly will take a while. Also, some places need the
property ID constructor (e.g.
ComputedStyle->rareNonInheritedData->m_willChange->m_properties is stored as a
list of property IDs). I think this should come in a later patch.

For now we can just use CSSPropertyInvalid to indicate that it's not a
CSSPropertyID and ASSERT(property != CSSPropertyInvalid) in the constructor.

[sashab, 2015-10-06 00:34:47]

Thanks alancutter -- feedback responded to. timloh ptal :)

[alancutter (OOO until 2018), 2015-10-07 02:05:10]

lgtm with nit.

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

https://codereview.chromium.org/1373433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSCustomIdentValue.h:24: // create(String)
method.
We won't want to convert to CSSPropertyIDs in every case e.g. animation-name,
instead the TODO should be to lazily parse the CSSPropertyID in
isKnownPropertyID().

[sashab, 2015-10-07 02:21:01]

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

[sashab, 2015-10-07 02:21:02]

Oh, don't need timloh as an owner on this one. Thanks alancutter :)

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

https://codereview.chromium.org/1373433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSCustomIdentValue.h:24: // create(String)
method.
On 2015/10/07 02:05:10, alancutter wrote:
> We won't want to convert to CSSPropertyIDs in every case e.g. animation-name,
> instead the TODO should be to lazily parse the CSSPropertyID in
> isKnownPropertyID().

Ahh yes, you're right. Thanks; done.

[sashab, 2015-10-07 03:21:52]

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

[sashab, 2015-10-07 03:21:52]

Err wait... Yes I do. Timloh please review. Lol

[sashab, 2015-10-09 05:06:19]

Ping timloh

[Timothy Loh, 2015-10-12 04:08:04]

On 2015/10/09 05:06:19, sashab wrote:
> Ping timloh

lgtm

[sashab, 2015-10-12 23:01:52]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-10-12 23:01:52]

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

[commit-bot: I haz the power, 2015-10-12 23:04:42]

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

[commit-bot: I haz the power, 2015-10-13 02:07:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-10-13 02:08:58]

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