[webkit-reviews] review denied: [Bug 112108] Implement CSSOM for CSS Variables : [Attachment 193223] Rebased onto ToT

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Alan Cutter
<alancutter at chromium.org>'s request for review:
Bug 112108: Implement CSSOM for CSS Variables
https://bugs.webkit.org/show_bug.cgi?id=112108

Attachment 193223: Rebased onto ToT
https://bugs.webkit.org/attachment.cgi?id=193223&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=193223&action=review


Large patch! :)

It looks like a lot of it is just tedious project adds and boilerplate, but
there are important bits that we probably don't want to lose sight of.

The bindings code will need to be reviewed by bindings experts (sam, ggaren for
JSC, haraken, abarth for V8), the CSSOM by the style experts (anttik, kling)

> Source/WebCore/ChangeLog:8
> +	   WIP. Fixing build issues with EFL and GTK. Rebased patch onto ToT
master. Still has unresolved matter of pointer to
PropertySetCSSStyleDeclaration object shared across JS thread not ref counted.

If it's a WIP, probably don't need to mark it for review yet :)

> Source/WebCore/bindings/js/JSCSSVariablesDeclarationCustom.h:31
> +#include "JSCSSVariablesDeclaration.h"

This is weird. Why do we need this file?

> Source/WebCore/css/StylePropertySet.cpp:752
> +	   if (testProperty.id() == CSSPropertyVariable && static_cast<const
CSSVariableValue*>(testProperty.value())->name() == name) {

This looks like a toCSSVariableValue helper.

> Source/WebCore/css/StylePropertySet.cpp:768
> +void StylePropertySet::getVariableNames(Vector<AtomicString>& names) const

here, get is not superfluous, you are actually not returning a value.

> Source/WebCore/css/StylePropertySet.cpp:777
> +const String& StylePropertySet::getVariableValue(const AtomicString& name)
const

here get is superfluous -> variableValue