[Variables] Enable get/setProperty and similar APIs from the CSSOM

[Variables] Enable get/setProperty and similar APIs from the CSSOM

Add support for custom properties to the rule property set OM. My
approach adds an extra "if" to many of the standard code paths in
the interest of avoiding unnecessary code duplication, but if
we're really worried about performance, this could be changed.

There are clearly still parts of the code that assumes a property
ID should be enough to lookup an arbitrary property. These will
have to be burned down, but in the meantime I short-circuited
findCSSPropertyWithID for the custom property case.

BUG=551553
Committed: https://crrev.com/044c76e83b828c23e7e9f317d3db9304046dbb56
Cr-Commit-Position: refs/heads/master@{#361279}

[leviw_travelin_and_unemployed, 2015-11-06 02:17:55]

Description was changed from

==========
[Variables] Enable get/setProperty and similar APIs from the CSSOM

Turns out this goes deeper down the rabbit hole than expected...

Still a WIP. Needs some cleanup and to support getPropertyPriority, maybe
others.

BUG=551553
==========

to

==========
[Variables] Enable get/setProperty and similar APIs from the CSSOM

Turns out this goes deeper down the rabbit hole than expected...

Still a WIP. Needs some cleanup, to support getPropertyPriority, and tests.

BUG=551553
==========

[leviw_travelin_and_unemployed, 2015-11-07 00:50:39]

Description was changed from

==========
[Variables] Enable get/setProperty and similar APIs from the CSSOM

Turns out this goes deeper down the rabbit hole than expected...

Still a WIP. Needs some cleanup, to support getPropertyPriority, and tests.

BUG=551553
==========

to

==========
[Variables] Enable get/setProperty and similar APIs from the CSSOM

Add support for custom properties to the rule property set OM. My
approach adds an extra "if" to many of the standard code paths in
the interest of avoiding unnecessary code duplication, but if
we're really worried about performance, this could be changed.

There are clearly still parts of the code that assumes a property
ID should be enough to lookup an arbitrary property. These will
have to be burned down, but in the meantime I short-circuited
findCSSPropertyWithID for the custom property case.

BUG=551553
==========

[leviw_travelin_and_unemployed, 2015-11-07 00:50:39]

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

[leviw_travelin_and_unemployed, 2015-11-09 19:44:48]

ping

[Timothy Loh, 2015-11-09 19:55:54]

On 2015/11/09 19:44:48, leviw wrote:
> ping

test looks fine, I'm way too tired to actually review the code right now though
:-|

[leviw_travelin_and_unemployed, 2015-11-12 19:32:55]

On 2015/11/09 at 19:55:54, timloh wrote:
> On 2015/11/09 19:44:48, leviw wrote:
> > ping
> 
> test looks fine, I'm way too tired to actually review the code right now
though :-|

Friendly ping now that BlinkON is over.

[Timothy Loh, 2015-11-14 02:21:05]

Seems okay overall

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp
(right):

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp:227:
// TODO(leviw): This API doesn't support custom properties.
Just checking, the function we're in is for foo.style["left"]?

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:33:
bool CSSPropertyMetadata::isEnabledProperty(CSSPropertyID unresolvedProperty)
Where are we calling into here from? It looks like most of the entry points are
checking the RuntimeEnabledFeatures flag

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp:682:
exceptionState.throwDOMException(NoModificationAllowedError, "These styles are
computed, and therefore the '" + getPropertyNameString(id) + "' property is
read-only.");
this might do something weird for custom properties, we should probably test it.

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:100: void
setPropertyInternal(CSSPropertyID, const String& propertyName, const String&
value, bool important, ExceptionState&) override;
maybe customPropertyName here and everywhere else, since the variable is left
empty for regular properties

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValue.cpp:264: return
toCSSCustomPropertyDeclaration(this)->value()->tokenRange().serialize();
IMO the serialization logic here should be in CSSCustomPropertyDeclaration.cpp
so that this file won't technically need the dependency on CSSParserTokenRange.

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/PropertySetCSSStyleDeclaration.cpp:252: bool
changed = propertyID != CSSPropertyVariable ?
propertySet().removeProperty(propertyID, &result)
since it's a ternary, I'd make the comparison an equality check instead of an
inequality check and switch the order of the result

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:180: String
StylePropertySerializer::getCustomPropertyText(const PropertyValueForSerializer&
property, bool isNotFirstDecl) const
can this be static (not on the StylePropertySerializer class)?

[leviw_travelin_and_unemployed, 2015-11-17 01:25:07]

PTAL

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp
(right):

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp:227:
// TODO(leviw): This API doesn't support custom properties.
On 2015/11/14 at 02:21:05, Timothy Loh wrote:
> Just checking, the function we're in is for foo.style["left"]?

That's correct.

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:33:
bool CSSPropertyMetadata::isEnabledProperty(CSSPropertyID unresolvedProperty)
On 2015/11/14 at 02:21:05, Timothy Loh wrote:
> Where are we calling into here from? It looks like most of the entry points
are checking the RuntimeEnabledFeatures flag

That's right, and we check this when trying to set properties.

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp:682:
exceptionState.throwDOMException(NoModificationAllowedError, "These styles are
computed, and therefore the '" + getPropertyNameString(id) + "' property is
read-only.");
On 2015/11/14 at 02:21:05, Timothy Loh wrote:
> this might do something weird for custom properties, we should probably test
it.

Added a case for this to my test.

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:100: void
setPropertyInternal(CSSPropertyID, const String& propertyName, const String&
value, bool important, ExceptionState&) override;
On 2015/11/14 at 02:21:05, Timothy Loh wrote:
> maybe customPropertyName here and everywhere else, since the variable is left
empty for regular properties

SGTM.

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValue.cpp:264: return
toCSSCustomPropertyDeclaration(this)->value()->tokenRange().serialize();
On 2015/11/14 at 02:21:05, Timothy Loh wrote:
> IMO the serialization logic here should be in CSSCustomPropertyDeclaration.cpp
so that this file won't technically need the dependency on CSSParserTokenRange.

Done.

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/PropertySetCSSStyleDeclaration.cpp:252: bool
changed = propertyID != CSSPropertyVariable ?
propertySet().removeProperty(propertyID, &result)
On 2015/11/14 at 02:21:05, Timothy Loh wrote:
> since it's a ternary, I'd make the comparison an equality check instead of an
inequality check and switch the order of the result

Done.

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

https://codereview.chromium.org/1405293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:180: String
StylePropertySerializer::getCustomPropertyText(const PropertyValueForSerializer&
property, bool isNotFirstDecl) const
On 2015/11/14 at 02:21:05, Timothy Loh wrote:
> can this be static (not on the StylePropertySerializer class)?

Not if it wants to reference PropertyValueForSerializer. That's a private class.

[alancutter (OOO until 2018), 2015-11-17 07:10:42]

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/variables/custom-properties-in-object-model.html
(right):

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/variables/custom-properties-in-object-model.html:29:
'"NoModificationAllowedError: Failed to execute \'setProperty\' on
\'CSSStyleDeclaration\': These styles are computed, and therefore the
\'--error\' property is read-only."');
Does this pass? I don't think getPropertyNameString() works for custom
properties.

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:364:
result.append(getCustomPropertyText(property, numDecls++));
This doesn't seem to be tested. I assume this gets hit via reading
style.cssText.

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySet.cpp:564: 
The copy paste in this file is pretty unappealing. If instead of duplicating
StylePropertySet's interface for CSSPropertyID/AtomicString the parameter was
templated we would avoid code duplication and also avoid touching the callsites
to these methods. StylePropertySet.cpp can explicitly instantiate the
CSSPropertyID and AtomicString specialisations to avoid pushing implementations
up into StylePropertySet.h.

WDYT?

[leviw_travelin_and_unemployed, 2015-11-17 07:23:44]

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/variables/custom-properties-in-object-model.html
(right):

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/variables/custom-properties-in-object-model.html:29:
'"NoModificationAllowedError: Failed to execute \'setProperty\' on
\'CSSStyleDeclaration\': These styles are computed, and therefore the
\'--error\' property is read-only."');
On 2015/11/17 at 07:10:42, alancutter wrote:
> Does this pass? I don't think getPropertyNameString() works for custom
properties.

It does pass.

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySet.cpp:564: 
On 2015/11/17 at 07:10:42, alancutter wrote:
> The copy paste in this file is pretty unappealing. If instead of duplicating
StylePropertySet's interface for CSSPropertyID/AtomicString the parameter was
templated we would avoid code duplication and also avoid touching the callsites
to these methods. StylePropertySet.cpp can explicitly instantiate the
CSSPropertyID and AtomicString specialisations to avoid pushing implementations
up into StylePropertySet.h.
> 
> WDYT?

If that saved us code duplication I'd agree. I'm not entirely sure I understand
your suggestion, but if you're saying you'd prefer template specialization to
function overloading in this case, I'd disagree, but I'm not dogmatic about it.

[leviw_travelin_and_unemployed, 2015-11-18 17:14:39]

ping

[alancutter (OOO until 2018), 2015-11-18 21:22:51]

Shoot, I meant to send this yesterday, sorry.

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySet.cpp:564: 
On 2015/11/17 at 07:23:44, leviw wrote:
> On 2015/11/17 at 07:10:42, alancutter wrote:
> > The copy paste in this file is pretty unappealing. If instead of duplicating
StylePropertySet's interface for CSSPropertyID/AtomicString the parameter was
templated we would avoid code duplication and also avoid touching the callsites
to these methods. StylePropertySet.cpp can explicitly instantiate the
CSSPropertyID and AtomicString specialisations to avoid pushing implementations
up into StylePropertySet.h.
> > 
> > WDYT?
> 
> If that saved us code duplication I'd agree. I'm not entirely sure I
understand your suggestion, but if you're saying you'd prefer template
specialization to function overloading in this case, I'd disagree, but I'm not
dogmatic about it.

Explicit template specialisation isn't something I've done before so I tried it
out to be sure that it avoided code duplication.
I think it turned out okay, the differences between native and custom property
handling is captured in function overloads while everything else stays the same
only templated: https://codereview.chromium.org/1455053003
leviw, timloh, WDYT?

[leviw_travelin_and_unemployed, 2015-11-18 21:26:33]

On 2015/11/18 at 21:22:51, alancutter wrote:
> Shoot, I meant to send this yesterday, sorry.
> 
>
https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right):
> 
>
https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/StylePropertySet.cpp:564: 
> On 2015/11/17 at 07:23:44, leviw wrote:
> > On 2015/11/17 at 07:10:42, alancutter wrote:
> > > The copy paste in this file is pretty unappealing. If instead of
duplicating StylePropertySet's interface for CSSPropertyID/AtomicString the
parameter was templated we would avoid code duplication and also avoid touching
the callsites to these methods. StylePropertySet.cpp can explicitly instantiate
the CSSPropertyID and AtomicString specialisations to avoid pushing
implementations up into StylePropertySet.h.
> > > 
> > > WDYT?
> > 
> > If that saved us code duplication I'd agree. I'm not entirely sure I
understand your suggestion, but if you're saying you'd prefer template
specialization to function overloading in this case, I'd disagree, but I'm not
dogmatic about it.
> 
> Explicit template specialisation isn't something I've done before so I tried
it out to be sure that it avoided code duplication.
> I think it turned out okay, the differences between native and custom property
handling is captured in function overloads while everything else stays the same
only templated: https://codereview.chromium.org/1455053003
> leviw, timloh, WDYT?

I like that :) I was thinking about this only in the context of the method you
commented on, but I agree when you broaden it to the interface in general it's
better. I'll adopt it. Thanks!

[Timothy Loh, 2015-11-19 00:47:42]

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/variables/custom-properties-in-object-model.html
(right):

https://codereview.chromium.org/1405293012/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/variables/custom-properties-in-object-model.html:29:
'"NoModificationAllowedError: Failed to execute \'setProperty\' on
\'CSSStyleDeclaration\': These styles are computed, and therefore the
\'--error\' property is read-only."');
On 2015/11/17 07:23:44, leviw wrote:
> On 2015/11/17 at 07:10:42, alancutter wrote:
> > Does this pass? I don't think getPropertyNameString() works for custom
> properties.
> 
> It does pass.

I had a look, CSSComputedStyleDeclaration::setPropertyInternal seems unreachable
at this point. Maybe add a TODO to fix it up later.

[leviw_travelin_and_unemployed, 2015-11-20 06:21:11]

PTAL

[alancutter (OOO until 2018), 2015-11-23 05:24:00]

lgtm

https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):

https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:40:
enabledProperties->clear(0);
Is this ever hit? It doesn't look like we will get here from this patch. I think
it'd be best to leave this change until necessary.

https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/StylePropertySet.cpp:200: template
CORE_EXPORT PassRefPtrWillBeRawPtr<CSSValue>
StylePropertySet::getPropertyCSSValue<AtomicString>(AtomicString) const;
Which build configuration requires CORE_EXPORT?

[leviw_travelin_and_unemployed, 2015-11-23 18:13:06]

On 2015/11/23 at 05:24:00, alancutter wrote:
> lgtm
> 
>
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):
> 
>
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:40:
enabledProperties->clear(0);
> Is this ever hit? It doesn't look like we will get here from this patch. I
think it'd be best to leave this change until necessary.

It was in earlier iterations. Perhaps it isn't any longer? I'll double-check...

> 
>
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right):
> 
>
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/StylePropertySet.cpp:200: template
CORE_EXPORT PassRefPtrWillBeRawPtr<CSSValue>
StylePropertySet::getPropertyCSSValue<AtomicString>(AtomicString) const;
> Which build configuration requires CORE_EXPORT?

Most bot configurations do. See the output from patch #6 when for some reason
the bots needed more exports than my local linux build. Modules depends on
StylePropertySet, and is linked into another library in those configs.

[leviw_travelin_and_unemployed, 2015-11-23 20:59:20]

On 2015/11/23 at 18:13:06, leviw wrote:
> On 2015/11/23 at 05:24:00, alancutter wrote:
> > lgtm
> > 
> >
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> > File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):
> > 
> >
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> >
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:40:
enabledProperties->clear(0);
> > Is this ever hit? It doesn't look like we will get here from this patch. I
think it'd be best to leave this change until necessary.
> 
> It was in earlier iterations. Perhaps it isn't any longer? I'll
double-check...

STDERR: 1   0x12062cb3b WTF::BitArray<393u>::get(unsigned int) const
STDERR: 2   0x12062c569
blink::CSSPropertyMetadata::isEnabledProperty(blink::CSSPropertyID)
STDERR: 3   0x120c7c522 blink::StylePropertySerializer::asText() const
STDERR: 4   0x120c84372 blink::StylePropertySet::asText() const
STDERR: 5   0x120b94aa0 blink::CSSStyleRule::cssText() const
STDERR: 6   0x120141b86
blink::CSSRuleV8Internal::cssTextAttributeGetter(v8::FunctionCallbackInfo<v8::Value>
const&)
STDERR: 7   0x120141752
blink::CSSRuleV8Internal::cssTextAttributeGetterCallback(v8::FunctionCallbackInfo<v8::Value>
const&)
STDERR: 8   0x119eab2ee v8::internal::FunctionCallbackArguments::Call(void
(*)(v8::FunctionCallbackInfo<v8::Value> const&))
STDERR: 9   0x119ee0f44 v8::internal::MaybeHandle<v8::internal::Object>
v8::internal::HandleApiCallHelper<false>(v8::internal::Isolate*,
v8::internal::(anonymous
namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>&)
STDERR: 10  0x119ee08e8
v8::internal::Builtins::InvokeApiFunction(v8::internal::Handle<v8::internal::JSFunction>,
v8::internal::Handle<v8::internal::Object>, int,
v8::internal::Handle<v8::internal::Object>*)
STDERR: 11  0x11a16b75a v8::internal::Execution::Call(v8::internal::Isolate*,
v8::internal::Handle<v8::internal::Object>,
v8::internal::Handle<v8::internal::Object>, int,
v8::internal::Handle<v8::internal::Object>*)
STDERR: 12  0x11a29feaf
v8::internal::Object::GetPropertyWithAccessor(v8::internal::LookupIterator*,
v8::internal::LanguageMode)
STDERR: 13  0x11a29f102
v8::internal::Object::GetProperty(v8::internal::LookupIterator*,
v8::internal::LanguageMode)
STDERR: 14  0x11a22fabd
v8::internal::LoadIC::Load(v8::internal::Handle<v8::internal::Object>,
v8::internal::Handle<v8::internal::Name>)
STDERR: 15  0x11a2377fc v8::internal::Runtime_LoadIC_Miss(int,
v8::internal::Object**, v8::internal::Isolate*)

Still happens.

> 
> > 
> >
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right):
> > 
> >
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/css/StylePropertySet.cpp:200: template
CORE_EXPORT PassRefPtrWillBeRawPtr<CSSValue>
StylePropertySet::getPropertyCSSValue<AtomicString>(AtomicString) const;
> > Which build configuration requires CORE_EXPORT?
> 
> Most bot configurations do. See the output from patch #6 when for some reason
the bots needed more exports than my local linux build. Modules depends on
StylePropertySet, and is linked into another library in those configs.

[leviw_travelin_and_unemployed, 2015-11-23 21:03:07]

On 2015/11/23 at 20:59:20, leviw wrote:
> On 2015/11/23 at 18:13:06, leviw wrote:
> > On 2015/11/23 at 05:24:00, alancutter wrote:
> > > lgtm
> > > 
> > >
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> > > File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):
> > > 
> > >
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> > >
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:40:
enabledProperties->clear(0);
> > > Is this ever hit? It doesn't look like we will get here from this patch. I
think it'd be best to leave this change until necessary.
> > 
> > It was in earlier iterations. Perhaps it isn't any longer? I'll
double-check...
> 
> STDERR: 1   0x12062cb3b WTF::BitArray<393u>::get(unsigned int) const
> STDERR: 2   0x12062c569
blink::CSSPropertyMetadata::isEnabledProperty(blink::CSSPropertyID)
> STDERR: 3   0x120c7c522 blink::StylePropertySerializer::asText() const
> STDERR: 4   0x120c84372 blink::StylePropertySet::asText() const
> STDERR: 5   0x120b94aa0 blink::CSSStyleRule::cssText() const
> STDERR: 6   0x120141b86
blink::CSSRuleV8Internal::cssTextAttributeGetter(v8::FunctionCallbackInfo<v8::Value>
const&)
> STDERR: 7   0x120141752
blink::CSSRuleV8Internal::cssTextAttributeGetterCallback(v8::FunctionCallbackInfo<v8::Value>
const&)
> STDERR: 8   0x119eab2ee v8::internal::FunctionCallbackArguments::Call(void
(*)(v8::FunctionCallbackInfo<v8::Value> const&))
> STDERR: 9   0x119ee0f44 v8::internal::MaybeHandle<v8::internal::Object>
v8::internal::HandleApiCallHelper<false>(v8::internal::Isolate*,
v8::internal::(anonymous
namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>&)
> STDERR: 10  0x119ee08e8
v8::internal::Builtins::InvokeApiFunction(v8::internal::Handle<v8::internal::JSFunction>,
v8::internal::Handle<v8::internal::Object>, int,
v8::internal::Handle<v8::internal::Object>*)
> STDERR: 11  0x11a16b75a v8::internal::Execution::Call(v8::internal::Isolate*,
v8::internal::Handle<v8::internal::Object>,
v8::internal::Handle<v8::internal::Object>, int,
v8::internal::Handle<v8::internal::Object>*)
> STDERR: 12  0x11a29feaf
v8::internal::Object::GetPropertyWithAccessor(v8::internal::LookupIterator*,
v8::internal::LanguageMode)
> STDERR: 13  0x11a29f102
v8::internal::Object::GetProperty(v8::internal::LookupIterator*,
v8::internal::LanguageMode)
> STDERR: 14  0x11a22fabd
v8::internal::LoadIC::Load(v8::internal::Handle<v8::internal::Object>,
v8::internal::Handle<v8::internal::Name>)
> STDERR: 15  0x11a2377fc v8::internal::Runtime_LoadIC_Miss(int,
v8::internal::Object**, v8::internal::Isolate*)
> 
> Still happens.

With this in mind, let me know if we're good to land or if you have additional
comments on CSSPropertyMetadata.cpp.tmpl

[alancutter (OOO until 2018), 2015-11-23 21:55:33]

lgtm with nit.

We should wait for timloh's review before landing. Want us to tick the CQ box if
it's all good?

https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):

https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:40:
enabledProperties->clear(0);
We should ASSERT(CSSPropertyVariable == {{first_enum_value - 1}}).

[alancutter (OOO until 2018), 2015-11-23 22:20:47]

https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):

https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:40:
enabledProperties->clear(0);
On 2015/11/23 at 21:55:33, alancutter wrote:
> We should ASSERT(CSSPropertyVariable == {{first_enum_value - 1}}).

s/ASSERT/static_assert/

[leviw_travelin_and_unemployed, 2015-11-23 22:30:51]

On 2015/11/23 at 22:20:47, alancutter wrote:
>
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):
> 
>
https://codereview.chromium.org/1405293012/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:40:
enabledProperties->clear(0);
> On 2015/11/23 at 21:55:33, alancutter wrote:
> > We should ASSERT(CSSPropertyVariable == {{first_enum_value - 1}}).
> 
> s/ASSERT/static_assert/

Yeah, I used static_assert. CQ=? in the old-school parlance.

[Timothy Loh, 2015-11-23 23:49:30]

lgtm

[Timothy Loh, 2015-11-23 23:49:42]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2015-11-23 23:49:42]

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/1405293012/#ps160001
(title: "Use static_assert.")

[commit-bot: I haz the power, 2015-11-23 23:51:32]

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

[commit-bot: I haz the power, 2015-11-24 01:44:01]

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

[commit-bot: I haz the power, 2015-11-24 01:44:02]

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

[leviw_travelin_and_unemployed, 2015-11-24 01:46:51]

leviw@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org, mkwst@chromium.org

[leviw_travelin_and_unemployed, 2015-11-24 01:46:51]

Adding a few bindings owners: PTAL at V8CSSStyleDeclarationCustom.cpp and if you
approve, hit the CQ again.

[haraken, 2015-11-24 01:48:09]

On 2015/11/24 01:46:51, leviw wrote:
> Adding a few bindings owners: PTAL at V8CSSStyleDeclarationCustom.cpp and if
you
> approve, hit the CQ again.

bindings/ LGTM

[leviw_travelin_and_unemployed, 2015-11-24 01:48:37]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-11-24 01:48:37]

Thanks for the super quick review!

[commit-bot: I haz the power, 2015-11-24 01:51:54]

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

[commit-bot: I haz the power, 2015-11-24 03:47:32]

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

[commit-bot: I haz the power, 2015-11-24 03:47:33]

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

[leviw_travelin_and_unemployed, 2015-11-24 05:40:19]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2015-11-24 05:41:49]

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

[commit-bot: I haz the power, 2015-11-24 07:26:08]

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

[commit-bot: I haz the power, 2015-11-24 07:26:09]

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

[alancutter (OOO until 2018), 2015-11-24 07:26:31]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2015-11-24 07:28:01]

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

[commit-bot: I haz the power, 2015-11-24 07:58:52]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-11-24 07:59:48]

Patchset 9 (id:??) landed as
https://crrev.com/044c76e83b828c23e7e9f317d3db9304046dbb56
Cr-Commit-Position: refs/heads/master@{#361279}