Partial implementation of CSSVariablesMap for CSS Variables CSSOM

Partial implementation of CSSVariablesMap for CSS Variables CSSOM

This is a partial implementation of the CSSVariablesMap specification.
This provides Javascript access to CSS Custom Properties (variables).

This implementation is missing the forEach() method which is to come in
a later patch.

Specification links:
http://dev.w3.org/csswg/css-variables/#the-CSSVariablesMap-interface
http://dev.w3.org/2006/webapi/WebIDL/#MapClass

BUG=231791
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154472

[alancutter (OOO until 2018), 2013-07-01 05:19:46]

Most of the code I'm touching has been modified by Apple folks. Would you be
able to review this patch Eric? :)

[arv (Not doing code reviews), 2013-07-01 13:59:46]

FYI

https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
File Source/core/css/CSSVariablesMap.idl (right):

https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
Source/core/css/CSSVariablesMap.idl:30: DOMString get(DOMString name);
This needs to return undefined for non existing keys.

[TreatReturnedNullStringAs=Undefined] DOMString get(DOMString name);

[alancutter (OOO until 2018), 2013-07-02 00:27:47]

Thanks for the reviews arv! I didn't mention it in the previous patch but your
insight has been most valuable. :)


(From previous patch)
> Source/core/css/CSSVariablesMap.idl:34: [RaisesException] void set(DOMString
> name, DOMString value);
> Why do these throw?

The CSSVariablesMap may be wrapping the variables in a
CSSComputedStyleDeclaration which must throw NO_MODIFICATION_ALLOWED_ERR if the
JS attempts to modify it.

https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
File Source/core/css/CSSVariablesMap.idl (right):

https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
Source/core/css/CSSVariablesMap.idl:30: DOMString get(DOMString name);
On 2013/07/01 13:59:46, arv wrote:
> This needs to return undefined for non existing keys.
> 
> [TreatReturnedNullStringAs=Undefined] DOMString get(DOMString name);

The get() method is defined as returning the value of getPropertyValue() on the
CSSStyleDeclaration which returns the empty string for unset properties.
http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleDeclaration

[arv (Not doing code reviews), 2013-07-02 13:20:57]

https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
File Source/core/css/CSSVariablesMap.idl (right):

https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
Source/core/css/CSSVariablesMap.idl:30: DOMString get(DOMString name);
On 2013/07/02 00:27:47, alancutter wrote:
> On 2013/07/01 13:59:46, arv wrote:
> > This needs to return undefined for non existing keys.
> > 
> > [TreatReturnedNullStringAs=Undefined] DOMString get(DOMString name);
> 
> The get() method is defined as returning the value of getPropertyValue() on
the
> CSSStyleDeclaration which returns the empty string for unset properties.
> http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleDeclaration

If we are striving to quack like a Map we should return undefined. Can the spec
for CSSVariableMap be fixed?

[arv (Not doing code reviews), 2013-07-02 13:31:09]

On 2013/07/02 13:20:57, arv wrote:
>
https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
> File Source/core/css/CSSVariablesMap.idl (right):
> 
>
https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
> Source/core/css/CSSVariablesMap.idl:30: DOMString get(DOMString name);
> On 2013/07/02 00:27:47, alancutter wrote:
> > On 2013/07/01 13:59:46, arv wrote:
> > > This needs to return undefined for non existing keys.
> > > 
> > > [TreatReturnedNullStringAs=Undefined] DOMString get(DOMString name);
> > 
> > The get() method is defined as returning the value of getPropertyValue() on
> the
> > CSSStyleDeclaration which returns the empty string for unset properties.
> > http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleDeclaration
> 
> If we are striving to quack like a Map we should return undefined. Can the
spec
> for CSSVariableMap be fixed?

The WebIDL [MapLike] clearly states that this is the intention:

http://dev.w3.org/2006/webapi/WebIDL/#es-map-get

[alancutter (OOO until 2018), 2013-07-03 00:36:21]

On 2013/07/02 13:31:09, arv wrote:
> On 2013/07/02 13:20:57, arv wrote:
> >
>
https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
> > File Source/core/css/CSSVariablesMap.idl (right):
> > 
> >
>
https://codereview.chromium.org/18311002/diff/1001/Source/core/css/CSSVariabl...
> > Source/core/css/CSSVariablesMap.idl:30: DOMString get(DOMString name);
> > On 2013/07/02 00:27:47, alancutter wrote:
> > > On 2013/07/01 13:59:46, arv wrote:
> > > > This needs to return undefined for non existing keys.
> > > > 
> > > > [TreatReturnedNullStringAs=Undefined] DOMString get(DOMString name);
> > > 
> > > The get() method is defined as returning the value of getPropertyValue()
on
> > the
> > > CSSStyleDeclaration which returns the empty string for unset properties.
> > > http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleDeclaration
> > 
> > If we are striving to quack like a Map we should return undefined. Can the
> spec
> > for CSSVariableMap be fixed?
> 
> The WebIDL [MapLike] clearly states that this is the intention:
> 
> http://dev.w3.org/2006/webapi/WebIDL/#es-map-get

Aha, thanks for bringing that up. I'll discuss it with Tab.

[alancutter (OOO until 2018), 2013-07-03 01:53:05]

> entries, values and keys: These do not depend on generators. You can implement
> these today in ES5. Here is some pseudo code for entries:
> 
> CSSVariablesMap.prototype.entries = function() {
>   var iterator = %GetVariablesIterator(this);
>   return {
>     next: function() {
>       if (iterator.atEnd())
>         return {value: undefined, done: true};
>       var key = iterator.key();
>       var value = iterator.value();
>       iterator.advance();
>       return {value: [key, value], done: false};
>     }
>   };
> };
> 
> It should be trivial to write a custom binding that does the above.

I have investigated this option and it appears that WebIDL does not support
returning callbacks.
http://dev.w3.org/2006/webapi/WebIDL/#dfn-platform-object

[esprehn, 2013-07-04 02:48:52]

Still working through this patch since it's big...

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:135: unsigned
StylePropertySet::variableCount() const
Why is this needed? Your getComputedStyle one looks at the size();

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.h:206: bool setVariableValue(const
AtomicString& name, const String& value, ExceptionCode&, bool important =
false);
StylePropertySet is an internal API, it shouldn't have anything related to
exceptions on it. You also ignore the value, so lets leave it off.

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:2342:
m_state.style()->setHasExplicitlyInheritedProperties();
This doesn't seem related to the patch?

[alancutter (OOO until 2018), 2013-07-04 06:18:14]

Thanks for the review esprehn. (:

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:135: unsigned
StylePropertySet::variableCount() const
On 2013/07/04 02:48:52, esprehn wrote:
> Why is this needed? Your getComputedStyle one looks at the size();

In the StylePropertySet the CSS Variable properties are in the same collection
as the other CSS properties so they'll need to be counted individually. In the
computed style the variables are stored in a HashMap separate from all the other
values.

Moving the CSS Variable properties in StylePropertySet into a separate HashMap
is certainly something I will look into doing but would be better as a different
patch.

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.h:206: bool setVariableValue(const
AtomicString& name, const String& value, ExceptionCode&, bool important =
false);
On 2013/07/04 02:48:52, esprehn wrote:
> StylePropertySet is an internal API, it shouldn't have anything related to
> exceptions on it. You also ignore the value, so lets leave it off.

Thanks for catching this, there's no reason ExceptionCode needs to be here.

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/18311002/diff/14001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:2342:
m_state.style()->setHasExplicitlyInheritedProperties();
On 2013/07/04 02:48:52, esprehn wrote:
> This doesn't seem related to the patch?

This fixes a bug which is only observable when CSSOM is introduced and  CSS
values are changed at runtime.
I could fix it in a separate patch but I would not be able to add test cases for
it. The tests that come with this patch test that the bug is fixed.

[esprehn, 2013-07-10 02:42:10]

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.h (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.h:106: virtual
PassRefPtr<CSSVariablesMap> var() OVERRIDE { return
CSSVariablesMap::create(this); }
This isn't correct, you need to keep it in a member variable. For example I can
do: s = getComputedStyle(...); s.var.expando = 1; assert (s.var.expando  == 1);
but since you return a brand new one every time expando will disappear.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSVariab...
File Source/core/css/CSSVariablesMap.h (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSVariab...
Source/core/css/CSSVariablesMap.h:70: bool deleteImpl(const AtomicString& name)
const
Just call it remove() ?

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSVariab...
File Source/core/css/CSSVariablesMap.idl (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSVariab...
Source/core/css/CSSVariablesMap.idl:33: [ImplementedAs=deleteImpl] boolean
delete(DOMString name);
=remove]

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/PropertyS...
File Source/core/css/PropertySetCSSStyleDeclaration.h (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/PropertyS...
Source/core/css/PropertySetCSSStyleDeclaration.h:72: virtual
PassRefPtr<CSSVariablesMap> var() OVERRIDE { return
CSSVariablesMap::create(this); }
This doesn't need to be virtual, all subclasses need it. Put var() on the
CSSStyleDeclaration.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:299: if
(static_cast<CSSVariableValue*>(cssValue)->value() == value)
Please add toCSSVariableValue() that does the cssValue->isVariableValue() ASSERT
internally. Just like our other ones. You shouldn't need to assert(cssValue)
here either, we'll just crash when you access value() anyway.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:575: return removed;
This can just be 

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

It'll be much better if you have a lot of variables too since you don't
constantly copy the stuff inside the vector.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:2376: // All properties become
dependent their parent style when they use variables.
-> dependent on

[alancutter (OOO until 2018), 2013-07-10 11:18:13]

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.h (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.h:106: virtual
PassRefPtr<CSSVariablesMap> var() OVERRIDE { return
CSSVariablesMap::create(this); }
On 2013/07/10 02:42:11, esprehn wrote:
> This isn't correct, you need to keep it in a member variable. For example I
can
> do: s = getComputedStyle(...); s.var.expando = 1; assert (s.var.expando  ==
1);
> but since you return a brand new one every time expando will disappear.
> 

CSSStyleDeclaration now has a RefPtr to CSSVariablesMap. The map now has a raw
pointer to CSSStyleDeclaration that gets nullified on style's destruction.

It's still possible to lose access to the same CSSVariablesMap JavaScript object
via the el.style.var pathway should the style declaration get deallocated during
the page's lifetime. This is perhaps an issue with style in general as any
expando declared on the style object would also be lost.

I had attempted a static mapping from CSSStyleDeclaration to CSSVariablesMap to
avoid adding extra weight onto the style declaration classes but this approach
is not as robust as the one you had suggested in IRC which is what I've
implemented here.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSVariab...
File Source/core/css/CSSVariablesMap.h (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/CSSVariab...
Source/core/css/CSSVariablesMap.h:70: bool deleteImpl(const AtomicString& name)
const
On 2013/07/10 02:42:11, esprehn wrote:
> Just call it remove() ? 

Done.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/PropertyS...
File Source/core/css/PropertySetCSSStyleDeclaration.h (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/PropertyS...
Source/core/css/PropertySetCSSStyleDeclaration.h:72: virtual
PassRefPtr<CSSVariablesMap> var() OVERRIDE { return
CSSVariablesMap::create(this); }
On 2013/07/10 02:42:11, esprehn wrote:
> This doesn't need to be virtual, all subclasses need it. Put var() on the
> CSSStyleDeclaration.

Done.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:299: if
(static_cast<CSSVariableValue*>(cssValue)->value() == value)
On 2013/07/10 02:42:11, esprehn wrote:
> Please add toCSSVariableValue() that does the cssValue->isVariableValue()
ASSERT
> internally. Just like our other ones. You shouldn't need to assert(cssValue)
> here either, we'll just crash when you access value() anyway.

Done.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:575: return removed;
On 2013/07/10 02:42:11, esprehn wrote:
> This can just be 
> 
> CSSPropertyID variableId = CSSPropertyVariable;
> return removePropertiesInSet(&variableId, 1);
> 
> It'll be much better if you have a lot of variables too since you don't
> constantly copy the stuff inside the vector.

Fantastic! Done.

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/18311002/diff/28001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:2376: // All properties become
dependent their parent style when they use variables.
On 2013/07/10 02:42:11, esprehn wrote:
> -> dependent on

Done.

[esprehn, 2013-07-17 14:06:11]

LGTM. It's sad to make these things use more memory but we can come up with a
fancy way around then if it turns out to be an issue.

[commit-bot: I haz the power, 2013-07-17 23:39:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/18311002/45001

[commit-bot: I haz the power, 2013-07-17 23:39:31]

Failed to apply patch for Source/core/css/CSSComputedStyleDeclaration.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/css/CSSComputedStyleDeclaration.cpp
  Hunk #1 FAILED at 26.
  Hunk #2 succeeded at 3003 (offset -6 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/css/CSSComputedStyleDeclaration.cpp.rej

Patch:       Source/core/css/CSSComputedStyleDeclaration.cpp
Index: Source/core/css/CSSComputedStyleDeclaration.cpp
diff --git a/Source/core/css/CSSComputedStyleDeclaration.cpp
b/Source/core/css/CSSComputedStyleDeclaration.cpp
index
32e1fa32e74331c3ee25009be5782afb06a8a1ff..6a7edf1ecba32bad714a90aea5ed1bca9c7e48fe
100644
--- a/Source/core/css/CSSComputedStyleDeclaration.cpp
+++ b/Source/core/css/CSSComputedStyleDeclaration.cpp
@@ -26,6 +26,7 @@
 
 #include "CSSPropertyNames.h"
 #include "FontFamilyNames.h"
+#include "RuntimeEnabledFeatures.h"
 #include "core/css/BasicShapeFunctions.h"
 #include "core/css/CSSArrayFunctionValue.h"
 #include "core/css/CSSAspectRatioValue.h"
@@ -3009,6 +3010,57 @@ void
CSSComputedStyleDeclaration::setPropertyInternal(CSSPropertyID, const Strin
     ec = NoModificationAllowedError;
 }
 
+const HashMap<AtomicString, String>* CSSComputedStyleDeclaration::variableMap()
const
+{
+    ASSERT(RuntimeEnabledFeatures::cssVariablesEnabled());
+    Node* styledNode = this->styledNode();
+    if (!styledNode)
+        return 0;
+    RefPtr<RenderStyle> style =
styledNode->computedStyle(styledNode->isPseudoElement() ? NOPSEUDO :
m_pseudoElementSpecifier);
+    if (!style)
+        return 0;
+    return style->variables();
+}
+
+unsigned CSSComputedStyleDeclaration::variableCount() const
+{
+    ASSERT(RuntimeEnabledFeatures::cssVariablesEnabled());
+    const HashMap<AtomicString, String>* variables = variableMap();
+    if (!variables)
+        return 0;
+    return variables->size();
+}
+
+String CSSComputedStyleDeclaration::variableValue(const AtomicString& name)
const
+{
+    ASSERT(RuntimeEnabledFeatures::cssVariablesEnabled());
+    const HashMap<AtomicString, String>* variables = variableMap();
+    if (!variables)
+        return emptyString();
+    HashMap<AtomicString, String>::const_iterator it = variables->find(name);
+    if (it == variables->end())
+        return emptyString();
+    return it->value;
+}
+
+void CSSComputedStyleDeclaration::setVariableValue(const AtomicString&, const
String&, ExceptionCode& ec)
+{
+    ASSERT(RuntimeEnabledFeatures::cssVariablesEnabled());
+    ec = NoModificationAllowedError;
+}
+
+bool CSSComputedStyleDeclaration::removeVariable(const AtomicString&)
+{
+    ASSERT(RuntimeEnabledFeatures::cssVariablesEnabled());
+    return false;
+}
+
+void CSSComputedStyleDeclaration::clearVariables(ExceptionCode& ec)
+{
+    ASSERT(RuntimeEnabledFeatures::cssVariablesEnabled());
+    ec = NoModificationAllowedError;
+}
+
 PassRefPtr<CSSValueList>
CSSComputedStyleDeclaration::getBackgroundShorthandValue() const
 {
     static const CSSPropertyID propertiesBeforeSlashSeperator[5] = {
CSSPropertyBackgroundColor, CSSPropertyBackgroundImage,

[commit-bot: I haz the power, 2013-07-18 00:28:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/18311002/52001

[commit-bot: I haz the power, 2013-07-18 02:29:37]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-07-18 06:38:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/18311002/52001

[commit-bot: I haz the power, 2013-07-18 07:24:01]

Change committed as 154472