Bug 117787 – [CSS] Add CSSVariablesMap interface

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Claudio Saavedra]

[CSS] Add CSSVariablesMap interface

[Claudio Saavedra]

Created attachment 204977 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204977 [details]
Patch

Attachment 204977 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/895530

[Build Bot]

Comment on attachment 204977 [details]
Patch

Attachment 204977 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/912277

[Early Warning System Bot]

Comment on attachment 204977 [details]
Patch

Attachment 204977 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/945148

[Early Warning System Bot]

Comment on attachment 204977 [details]
Patch

Attachment 204977 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/850452

[Chris Dumez]

Comment on attachment 204977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204977&action=review

> Source/WebCore/GNUmakefile.list.am:2361
> +	Source/WebCore/bindings/js/JSCSSVariablesMapCustom.cpp \

Needs to be added to Source/WebCore/UseJSC.cmake as well for ports using CMake.

> Source/WebCore/bindings/js/JSCSSVariablesMapCustom.h:1
> +/*

Why is this header needed? it is empty.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3069
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

Shouldn't this be: #if ENABLE(CSS_VARIABLES) ?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3072
> +    return "";

not implemented yet? Shouldn't we return String() for now?

> Source/WebCore/css/CSSStyleDeclaration.idl:49
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

using [Conditional=CSS_VARIABLES] idl extended attribute is preferable in IDL. Note that this could be in a partial interface to avoid cluttering CSSStyleDeclaration.idl and match the spec.

> Source/WebCore/css/CSSVariablesMap.cpp:36
> +CSSVariablesMap* CSSVariablesMap::create(CSSStyleDeclaration* styleDeclaration)

shouldn't this return a smart pointer?

> Source/WebCore/css/CSSVariablesMap.cpp:50
> +void CSSVariablesMap::ref()

why are we doing manual ref counting instead of subclassing RefCounted?

> Source/WebCore/css/CSSVariablesMap.cpp:69
> +    return get(variableName) != "";

get(variableName).isNull()? (if we return String() when missing)

> Source/WebCore/css/CSSVariablesMap.cpp:72
> +void CSSVariablesMap::set(const String& variableName, const String& variableValue, ExceptionCode& excCode)

excCode is usually just called ec.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:131
> +    m_variablesMap = CSSVariablesMap::create(this);

should probably be part of the initializer list. also it doesn't appear to be ever freed.

> Source/WebCore/css/StylePropertySet.cpp:1168
> +        return "";

Why do we return "" and not a null string?

[Claudio Saavedra]

Comment on attachment 204977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204977&action=review

>> Source/WebCore/bindings/js/JSCSSVariablesMapCustom.h:1
>> +/*
> 
> Why is this header needed? it is empty.

Not adding it causes one of the files in DerivedSources/ to fail compilation, as it expects this header (DerivedSources/WebCore/JSCSSVariablesMap.cpp). Not sure why.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3072
>> +    return "";
> 
> not implemented yet? Shouldn't we return String() for now?

If I understand correctly CSSComputedStyleDeclaration is not expected to have variables, hence the dummy implementation.

>> Source/WebCore/css/CSSStyleDeclaration.idl:49
>> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES
> 
> using [Conditional=CSS_VARIABLES] idl extended attribute is preferable in IDL. Note that this could be in a partial interface to avoid cluttering CSSStyleDeclaration.idl and match the spec.

I've tried using a partial interface (in a CSSStyleDeclarationVariablesMap) but then it expects a CSSStyleDeclaratioNVariablesMap.h header which I don't think we need.

[Build Bot]

Comment on attachment 204977 [details]
Patch

Attachment 204977 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/895570

[Chris Dumez]

Comment on attachment 204977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204977&action=review

>>> Source/WebCore/css/CSSStyleDeclaration.idl:49
>>> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES
>> 
>> using [Conditional=CSS_VARIABLES] idl extended attribute is preferable in IDL. Note that this could be in a partial interface to avoid cluttering CSSStyleDeclaration.idl and match the spec.
> 
> I've tried using a partial interface (in a CSSStyleDeclarationVariablesMap) but then it expects a CSSStyleDeclaratioNVariablesMap.h header which I don't think we need.

Right, this is because you are using [CustomNamedSetter] in the IDL. I need to check why we do it. Nevermind this comment.

[Chris Dumez]

(In reply to comment #9)
> (From update of attachment 204977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204977&action=review
> 
> >>> Source/WebCore/css/CSSStyleDeclaration.idl:49
> >>> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES
> >> 
> >> using [Conditional=CSS_VARIABLES] idl extended attribute is preferable in IDL. Note that this could be in a partial interface to avoid cluttering CSSStyleDeclaration.idl and match the spec.
> > 
> > I've tried using a partial interface (in a CSSStyleDeclarationVariablesMap) but then it expects a CSSStyleDeclaratioNVariablesMap.h header which I don't think we need.
> 
> Right, this is because you are using [CustomNamedSetter] in the IDL. I need to check why we do it. Nevermind this comment.

I will address this issue in the bindings generator via Bug 117795.

[Claudio Saavedra]

Comment on attachment 204977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204977&action=review

>> Source/WebCore/css/CSSVariablesMap.cpp:69
>> +    return get(variableName) != "";
> 
> get(variableName).isNull()? (if we return String() when missing)

Other methods also return the empty string (see String CSSComputedStyleDeclaration::getPropertyPriority(const String&)) for instance, so I thought that was accepted?

Also, please note that get() needs to return an empty string according to the specifications (which say the return value should be that of calling getProperty() for the associated CSSStyleDeclaration, with getProperty() defined with "Returns the empty string if the property has not been set.")

>> Source/WebCore/css/CSSVariablesMap.cpp:72
>> +void CSSVariablesMap::set(const String& variableName, const String& variableValue, ExceptionCode& excCode)
> 
> excCode is usually just called ec.

I am having second thoughts as to whether we need the exceptions. What's your opinion? The specifications don't mention them explicitly.

[Claudio Saavedra]

Created attachment 205000 [details]
Patch

[EFL EWS Bot]

Comment on attachment 205000 [details]
Patch

Attachment 205000 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/942235

[Chris Dumez]

(In reply to comment #11)
> (From update of attachment 204977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204977&action=review
> 
> >> Source/WebCore/css/CSSVariablesMap.cpp:69
> >> +    return get(variableName) != "";
> > 
> > get(variableName).isNull()? (if we return String() when missing)
> 
> Other methods also return the empty string (see String CSSComputedStyleDeclaration::getPropertyPriority(const String&)) for instance, so I thought that was accepted?

Ok, this is not my area so it may well be correct. I mostly wanted to review the bindings part. Someone else will need to look at the CSS part :)

[Build Bot]

Comment on attachment 205000 [details]
Patch

Attachment 205000 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/923531

[EFL EWS Bot]

Comment on attachment 205000 [details]
Patch

Attachment 205000 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/942326

[Build Bot]

Comment on attachment 205000 [details]
Patch

Attachment 205000 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/943385

[Build Bot]

Comment on attachment 205000 [details]
Patch

Attachment 205000 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/880401

New failing tests:
fast/css/variables/variables-map-2.html
fast/css/variables/variables-map.html

[Build Bot]

Created attachment 205052 [details]
Archive of layout-test-results from APPLE-EWS-3 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-3  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Claudio Saavedra]

Created attachment 205170 [details]
Patch

[Early Warning System Bot]

Comment on attachment 205170 [details]
Patch

Attachment 205170 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/961069

[Early Warning System Bot]

Comment on attachment 205170 [details]
Patch

Attachment 205170 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/884489

[EFL EWS Bot]

Comment on attachment 205170 [details]
Patch

Attachment 205170 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/933545

[EFL EWS Bot]

Comment on attachment 205170 [details]
Patch

Attachment 205170 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/858875

[Build Bot]

Comment on attachment 205170 [details]
Patch

Attachment 205170 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/921829

New failing tests:
fast/css/variables/variables-map-2.html
fast/css/variables/variables-map.html

[Build Bot]

Created attachment 205193 [details]
Archive of layout-test-results from APPLE-EWS-3 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-3  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Claudio Saavedra]

Created attachment 205297 [details]
Patch

[Early Warning System Bot]

Comment on attachment 205297 [details]
Patch

Attachment 205297 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/942583

[Early Warning System Bot]

Comment on attachment 205297 [details]
Patch

Attachment 205297 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/982193

[Claudio Saavedra]

Created attachment 205400 [details]
Patch

[Alan Cutter]

Comment on attachment 205400 [details]
Patch

You should use AtomicString for the variable names as that is what is used by the CSSVariableValue type.

View in context: https://bugs.webkit.org/attachment.cgi?id=205400&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3072
> +    return "";

You should be getting variables from
styledNode()->computedStyle(styledNode->isPseudoElement() ? NOPSEUDO : m_pseudoElementSpecifier)->variables() for computed styles (unless this has changed in WebKit).

> Source/WebCore/css/CSSComputedStyleDeclaration.h:116
> +    virtual void setVariable(const String&, const String&, ExceptionCode&) OVERRIDE { };

This should set the ExceptionCode to NO_MODIFICATION_ALLOWED_ERR, same for removeVariable in CSSComputedStyleDeclaration.

> Source/WebCore/css/CSSStyleDeclaration.h:79
> +    virtual String getVariableValue(const String& variableName) const = 0;

getVariableValue should be called just variableValue. WebKit style is to avoid prefixing getter names with "get" unless the value is returned via a parameter like in the case of getVariableNames. getVariableNames is an acceptable instance of prefixing the method name with "get".

> Source/WebCore/css/CSSStyleDeclaration.h:81
> +    virtual bool removeVariable(const String&variableName, ExceptionCode&) const = 0;

There should be a space after const String&.

> Source/WebCore/css/CSSStyleDeclarationVariablesMap.idl:22
> +    readonly attribute CSSVariablesMap var;

Could this be added to CSSStyleDeclaration.idl instead?

> Source/WebCore/css/CSSVariablesMap.cpp:36
> +PassRefPtr<CSSVariablesMap> CSSVariablesMap::create(CSSStyleDeclaration* styleDeclaration)

Is CSSVariablesMap.cpp necessary? Most functions seem small enough that they can live in the header.

> Source/WebCore/css/CSSVariablesMap.cpp:54
> +    return variablesVector.size();

It is not necessary to implement functions precisely as specified, as long as the behaviour matches the specification you can implement it however you want.
This should just count the variables rather than setting up a Vector of variable names for efficiency.

> Source/WebCore/css/CSSVariablesMap.cpp:64
> +        deleteFunction(variablesVector[i], ec);

Similar remarks here as for size(). This can be implemented in a more direct and efficient way with exactly the same external behaviour.

> Source/WebCore/css/CSSVariablesMap.cpp:74
> +    return get(variableName) != "";

I believe String has isEmpty().

> Source/WebCore/css/CSSVariablesMap.cpp:79
> +    m_parentStyleDeclaration->setVariable(variableName, variableValue, excCode);

Inconsistent naming: excCode vs ec.

> Source/WebCore/css/CSSVariablesMap.h:44
> +class CSSVariableValue;

Unused forward declarations/includes should be removed.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:133
> +}

IMO this constructor is small enough to remain in the header file for inlining.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:142
> +        StylePropertySet::PropertyReference reference = m_propertySet->propertyAt(i);

This line could be rewritten as
const StylePropertySet::PropertyReference& reference = m_propertySet->propertyAt(i);

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:223
> +    didMutate(PropertyChanged);

If a variable is set to the same value that it was already it should not be seen as a mutation.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:230
> +bool PropertySetCSSStyleDeclaration::removeVariable(const String& variableName, ExceptionCode& ec) const

This method should be handling CSS mutation like setVariable does. There should be a test for this behaviour.

> Source/WebCore/css/StylePropertySet.cpp:1184
> +    m_propertyVector.append(CSSProperty(CSSPropertyVariable, CSSVariableValue::create(variableName, value)));

I think setting a variable to the empty string should be the same as deleting it, the spec is not clear on this though...

> LayoutTests/fast/css/variables/variables-map-2.html:18
> +vars.set("width", "600px");

The name of this test file could be more descriptive e.g. "variables-map-set".

> LayoutTests/fast/css/variables/variables-map.html:16
> +<script src="variables-map.js"></script>

The Javascript code does not need to be in a separate file, it would be easier to debug a problem if the test code were in the one file.

[Chris Dumez]

Comment on attachment 205400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205400&action=review

>> Source/WebCore/css/CSSStyleDeclarationVariablesMap.idl:22
>> +    readonly attribute CSSVariablesMap var;
> 
> Could this be added to CSSStyleDeclaration.idl instead?

I think using partial interfaces avoids ending up with huge IDLs and also allows for splitting different features (in this case CSS variables).
There is no impact on the generated code and it matches the specification:
http://dev.w3.org/csswg/css-variables/#the-cssstyledeclaration-interface

Is there a policy I'm not aware of that we should avoid partial interfaces?

[Alan Cutter]

Comment on attachment 205400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205400&action=review

> Source/WebCore/css/StylePropertySet.cpp:1134
> +    ASSERT(CSSPropertyID != CSSPropertyVariable);

This will not compile, CSSPropertyID is a type name.

[Alan Cutter]

(In reply to comment #32)
> (From update of attachment 205400 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205400&action=review
> 
> >> Source/WebCore/css/CSSStyleDeclarationVariablesMap.idl:22
> >> +    readonly attribute CSSVariablesMap var;
> > 
> > Could this be added to CSSStyleDeclaration.idl instead?
> 
> I think using partial interfaces avoids ending up with huge IDLs and also allows for splitting different features (in this case CSS variables).
> There is no impact on the generated code and it matches the specification:
> http://dev.w3.org/csswg/css-variables/#the-cssstyledeclaration-interface
> 
> Is there a policy I'm not aware of that we should avoid partial interfaces?

This was just my personal judgement on maintainability. I think it would be less confusing if the interface for CSSStyleDeclaration were in a single file but I don't have a strong preference either way.

[Claudio Saavedra]

Created attachment 205615 [details]
Patch

[Claudio Saavedra]

Created attachment 205681 [details]
Patch

[Claudio Saavedra]

Created attachment 206057 [details]
Patch

[Claudio Saavedra]

Could anyone review this patch?

[Alan Cutter]

Comment on attachment 206057 [details]
Patch

One issue, otherwise LGTM. I am not a WebKit reviewer though so I can only give you a shadow review.


View in context: https://bugs.webkit.org/attachment.cgi?id=206057&action=review

> Source/WebCore/css/StylePropertySet.cpp:1194
> +}

Variables should be updated in place to preserve insertion order.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.14.4

[Claudio Saavedra]

Created attachment 206145 [details]
Patch

[Claudio Saavedra]

(In reply to comment #39)
> (From update of attachment 206057 [details])
> One issue, otherwise LGTM. I am not a WebKit reviewer though so I can only give you a shadow review.

Thanks for reviewing the patch, Alan!

[Chris Dumez]

Comment on attachment 206145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206145&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.h:121
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

#if ENABLE(CSS_VARIABLES) ?

> Source/WebCore/css/CSSStyleDeclaration.h:25
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto

> Source/WebCore/css/CSSStyleDeclaration.h:75
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto

> Source/WebCore/css/CSSVariablesMap.h:45
> +    static PassRefPtr<CSSVariablesMap> create(CSSStyleDeclaration* styleDeclaration) { return adoptRef(new CSSVariablesMap(styleDeclaration)); };

return statement should be on a new line.

> Source/WebCore/css/CSSVariablesMap.h:47
> +    virtual ~CSSVariablesMap() { };

Does not look like a need a virtual destructor. We can omit it or at least remove virtual.

> Source/WebCore/css/CSSVariablesMap.h:51
> +    String get(const String&);

Why not const ?

> Source/WebCore/css/CSSVariablesMap.h:52
> +    bool has(const String&);

Why not const?

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:27
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:184
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:42
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:49
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:58
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:71
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:101
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto.

> Source/WebCore/css/StylePropertySet.h:122
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto

> Source/WebCore/css/StylePropertySet.h:142
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

ditto

> Source/WebCore/css/StylePropertySet.h:232
> +#if defined(ENABLE_CSS_VARIABLES) && ENABLE_CSS_VARIABLES

Ditto

[Claudio Saavedra]

Comment on attachment 206145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206145&action=review

>> Source/WebCore/css/CSSVariablesMap.h:47
>> +    virtual ~CSSVariablesMap() { };
> 
> Does not look like a need a virtual destructor. We can omit it or at least remove virtual.

Somehow I can't build without the virtual destructor. Not sure why, but _ZTVN7WebCore15CSSVariablesMapE is expected (at least in the GTK+ port).

[Claudio Saavedra]

Comment on attachment 206145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206145&action=review

>> Source/WebCore/css/CSSVariablesMap.h:45
>> +    static PassRefPtr<CSSVariablesMap> create(CSSStyleDeclaration* styleDeclaration) { return adoptRef(new CSSVariablesMap(styleDeclaration)); };
> 
> return statement should be on a new line.

check-webkit-style disagrees:

Source/WebCore/css/CSSVariablesMap.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]

[Claudio Saavedra]

Created attachment 206165 [details]
Patch

[Alan Cutter]

Comment on attachment 206165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206165&action=review

> Source/WebCore/css/StylePropertySet.cpp:1184
> +}

This can use the existing function removePropertiesInSet().

CSSPropertyID variablesId = CSSPropertyVariable;
return removePropertiesInSet(&variablesId, 1);

[Claudio Saavedra]

No review for this? This is rotting here :/

[Andreas Kling]

CSS variables were yanked.