Partial implementation of inline StylePropertyMap.

Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- makes StyleValue from retrievedCSSValue
from Element.inlineStyle.getPropertyCSSValue

Spec: https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
Committed: https://crrev.com/285b76a05c09b684956fab196db9c11d63d2a726
Cr-Commit-Position: refs/heads/master@{#386551}

[meade_UTC10, 2016-02-05 03:24:57]

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

[meade_UTC10, 2016-02-05 03:24:57]

Hey Tim, this isn't finished yet, but could you please take a look and let me
know if this is heading in the right direction?

Thanks!
Eddy

[Timothy Loh, 2016-02-09 07:29:32]

Direction seems fine. I think we should try and get some performance numbers for
e.g. setting transform on inline style as soon as that is hooked up.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSOMTypes.h (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSOMTypes.h:17: static bool
propertyCanTake(CSSPropertyID id, StyleValue::StyleValueType type)
I don't think the inputs here are sufficient (e.g. for keywords), maybe just
take a StyleValue?

Also probably a good idea to put any functions that we expect will grow to be
big into a cpp file.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:18: if
(!m_cleanStyles.get(propertyID)) {
Maybe better to move the duplicated logic here and in getAll to updateProperty
and rename to (e.g.) updatePropertyIfNeeded

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:27: if
(styleVector.size() < 1) {
There's probably an empty() or isEmpty() function to use here and everywhere
else. BTW I'm not sure this was the intention of the get/getAll split, it might
have been that this always returns the first value if there are any.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:54: for
(const auto key : m_styles.keys()) {
Using a "const auto" type does a copy of the value without telling that the type
is, so my default reaction is always "what's the actual type that we're
copying". Better to just write out the type here (and elsewhere in the patch).
You can use "for (const auto& x :" if the object has a copying cost and the type
is particularly long.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:63: if
(m_styles.contains(propertyID))
I think remove() does this check, so we don't need to explicitly do it (it costs
an extra hash lookup)

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:65:
append(propertyID, item, exceptionState);
doesn't this not work for non-lists? because append() checks
propertySupportsMultiple

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:113:
m_ownerElement->setInlineStyleProperty(propertyID, values[0]->toCSSValue());
Properties which accept list-of-T probably wouldn't work if you just had a T.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:126: if
(!m_styles.contains(propertyID)) {
Better to do something like below and avoid an extra hash lookup:

foo::iterator iterator = m_styles.find(propertyID);
if (iterator == m_styles.end())
    return;
m_styles.remove(iterator);
...

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:139: if
(cssValue && !cssValue->isValueList() && !cssValue->isValuePair()) {
I think <position> types get stored as pairs, might not want that here.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:144: }
missing a TODO to handle the other case(s)?

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:19: :
MutableStylePropertyMap()
no need to explicitly call ctors, this happens anyway.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:46: Element*
m_ownerElement;
Does this need to be a member?

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:47:
HeapHashMap<CSSPropertyID, HeapVector<Member<StyleValue>>> m_styles;
Probably a good idea to make using declarations for these (i.e. typedefs) if
we're saying them multiple times.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp:32: return
has(cssPropertyID(propertyName));
You probably meant to use propertyID here and all the other functions

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/StyleValue.cpp (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/StyleValue.cpp:16: if
(primitiveValue.isLength()) {
doesn't work for calcs btw, they return true for isLength but getDoubleValue()
shouldn't be called on them

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3467:
RefPtrWillBeRawPtr<CSSValue> cssValue =
cssValuePool().createIdentifierValue(identifier);
(btw at some point you'll want to look over your patch and undo the incidental
changes like this that arise from making and reverting changes)

[meade_UTC10, 2016-02-10 05:55:39]

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSOMTypes.h (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSOMTypes.h:17: static bool
propertyCanTake(CSSPropertyID id, StyleValue::StyleValueType type)
On 2016/02/09 07:29:32, Timothy Loh wrote:
> I don't think the inputs here are sufficient (e.g. for keywords), maybe just
> take a StyleValue?
> 
> Also probably a good idea to put any functions that we expect will grow to be
> big into a cpp file.

Keyword is also a StyleValueType.

Moved functions into the cpp file.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:18: if
(!m_cleanStyles.get(propertyID)) {
On 2016/02/09 07:29:32, Timothy Loh wrote:
> Maybe better to move the duplicated logic here and in getAll to updateProperty
> and rename to (e.g.) updatePropertyIfNeeded

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:27: if
(styleVector.size() < 1) {
On 2016/02/09 07:29:32, Timothy Loh wrote:
> There's probably an empty() or isEmpty() function to use here and everywhere
> else. BTW I'm not sure this was the intention of the get/getAll split, it
might
> have been that this always returns the first value if there are any.

Done. AFAIK it's supposed to return the first value if there is one or greater.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:54: for
(const auto key : m_styles.keys()) {
On 2016/02/09 07:29:32, Timothy Loh wrote:
> Using a "const auto" type does a copy of the value without telling that the
type
> is, so my default reaction is always "what's the actual type that we're
> copying". Better to just write out the type here (and elsewhere in the patch).
> You can use "for (const auto& x :" if the object has a copying cost and the
type
> is particularly long.

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:63: if
(m_styles.contains(propertyID))
On 2016/02/09 07:29:32, Timothy Loh wrote:
> I think remove() does this check, so we don't need to explicitly do it (it
costs
> an extra hash lookup)

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:65:
append(propertyID, item, exceptionState);
On 2016/02/09 07:29:32, Timothy Loh wrote:
> doesn't this not work for non-lists? because append() checks
> propertySupportsMultiple

As implemented, it only does that if it's a singlular StyleValue and it's not
the first value appended. So it does work, however append will always work the
first time for non-list values, which may be confusing. As implemented it's
convenient because I can avoid having similar code paths, but this probably
needs a short discussion.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:113:
m_ownerElement->setInlineStyleProperty(propertyID, values[0]->toCSSValue());
On 2016/02/09 07:29:32, Timothy Loh wrote:
> Properties which accept list-of-T probably wouldn't work if you just had a T.

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:126: if
(!m_styles.contains(propertyID)) {
On 2016/02/09 07:29:32, Timothy Loh wrote:
> Better to do something like below and avoid an extra hash lookup:
> 
> foo::iterator iterator = m_styles.find(propertyID);
> if (iterator == m_styles.end())
>     return;
> m_styles.remove(iterator);
> ...

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:139: if
(cssValue && !cssValue->isValueList() && !cssValue->isValuePair()) {
On 2016/02/09 07:29:32, Timothy Loh wrote:
> I think <position> types get stored as pairs, might not want that here.

hmm yeah. Removed.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:144: }
On 2016/02/09 07:29:32, Timothy Loh wrote:
> missing a TODO to handle the other case(s)?

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:19: :
MutableStylePropertyMap()
On 2016/02/09 07:29:32, Timothy Loh wrote:
> no need to explicitly call ctors, this happens anyway.

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:46: Element*
m_ownerElement;
On 2016/02/09 07:29:32, Timothy Loh wrote:
> Does this need to be a member?

It's used to actually set the styles on the element?

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:47:
HeapHashMap<CSSPropertyID, HeapVector<Member<StyleValue>>> m_styles;
On 2016/02/09 07:29:32, Timothy Loh wrote:
> Probably a good idea to make using declarations for these (i.e. typedefs) if
> we're saying them multiple times.

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp:32: return
has(cssPropertyID(propertyName));
On 2016/02/09 07:29:32, Timothy Loh wrote:
> You probably meant to use propertyID here and all the other functions

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/StyleValue.cpp (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/StyleValue.cpp:16: if
(primitiveValue.isLength()) {
On 2016/02/09 07:29:32, Timothy Loh wrote:
> doesn't work for calcs btw, they return true for isLength but getDoubleValue()
> shouldn't be called on them

Done.

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3467:
RefPtrWillBeRawPtr<CSSValue> cssValue =
cssValuePool().createIdentifierValue(identifier);
On 2016/02/09 07:29:32, Timothy Loh wrote:
> (btw at some point you'll want to look over your patch and undo the incidental
> changes like this that arise from making and reverting changes)

Acknowledged.

[Timothy Loh, 2016-02-10 06:01:32]

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:46: Element*
m_ownerElement;
On 2016/02/10 05:55:39, Eddy wrote:
> On 2016/02/09 07:29:32, Timothy Loh wrote:
> > Does this need to be a member?
> 
> It's used to actually set the styles on the element?

Err.. I meant Member here.

[meade_UTC10, 2016-02-10 23:45:18]

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:46: Element*
m_ownerElement;
On 2016/02/10 06:01:31, Timothy Loh wrote:
> On 2016/02/10 05:55:39, Eddy wrote:
> > On 2016/02/09 07:29:32, Timothy Loh wrote:
> > > Does this need to be a member?
> > 
> > It's used to actually set the styles on the element?
> 
> Err.. I meant Member here.

Oh right. Done.

[meade_UTC10, 2016-02-11 01:14:31]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

BUG=545318
==========

[meade_UTC10, 2016-02-11 02:44:11]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Cached StyleValues in the InlineStylePropertyMap with
invalidation to refresh them

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets cached StyleValue or makes StyleValue from retrieved
CSSValue from Element.inlineStyle.getPropertyCSSValue

- Started CSSOMTypes for getting which CSSPropertyIDs can take
which types of StyleValue.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

BUG=545318
==========

[Timothy Loh, 2016-02-15 04:41:53]

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSOMTypes.h (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSOMTypes.h:17: static bool
propertyCanTake(CSSPropertyID id, StyleValue::StyleValueType type)
On 2016/02/10 05:55:39, Eddy wrote:
> On 2016/02/09 07:29:32, Timothy Loh wrote:
> > I don't think the inputs here are sufficient (e.g. for keywords), maybe just
> > take a StyleValue?
> > 
> > Also probably a good idea to put any functions that we expect will grow to
be
> > big into a cpp file.
> 
> Keyword is also a StyleValueType.
> 
> Moved functions into the cpp file.

But we need to verify that we're passing in a sane keyword for a given property,
right? Similarly, some properties will accept lengths but not percentages (both
of which are represented by a LengthValue).

[meade_UTC10, 2016-02-16 04:26:51]

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSOMTypes.h (right):

https://codereview.chromium.org/1590193002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSOMTypes.h:17: static bool
propertyCanTake(CSSPropertyID id, StyleValue::StyleValueType type)
On 2016/02/15 04:41:53, Timothy Loh wrote:
> On 2016/02/10 05:55:39, Eddy wrote:
> > On 2016/02/09 07:29:32, Timothy Loh wrote:
> > > I don't think the inputs here are sufficient (e.g. for keywords), maybe
just
> > > take a StyleValue?
> > > 
> > > Also probably a good idea to put any functions that we expect will grow to
> be
> > > big into a cpp file.
> > 
> > Keyword is also a StyleValueType.
> > 
> > Moved functions into the cpp file.
> 
> But we need to verify that we're passing in a sane keyword for a given
property,
> right? Similarly, some properties will accept lengths but not percentages
(both
> of which are represented by a LengthValue).

Ah, I see. Done.

[dstockwell, 2016-02-25 02:55:08]

https://codereview.chromium.org/1590193002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLElement.idl (right):

https://codereview.chromium.org/1590193002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLElement.idl:60:
[RuntimeEnabled=CSSTypedOM] readonly attribute StylePropertyMap styleMap;
This should be defined in a partial interface.

[meade_UTC10, 2016-03-07 04:12:59]

On 2016/02/25 02:55:08, dstockwell wrote:
>
https://codereview.chromium.org/1590193002/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLElement.idl (right):
> 
>
https://codereview.chromium.org/1590193002/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLElement.idl:60:
> [RuntimeEnabled=CSSTypedOM] readonly attribute StylePropertyMap styleMap;
> This should be defined in a partial interface.

As discussed, it didn't seem worth it since I need to access ElementRareData in
Element, so it would end up only adding trivial files.

[meade_UTC10, 2016-03-22 04:52:11]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Cached StyleValues in the InlineStylePropertyMap with
invalidation to refresh them

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets cached StyleValue or makes StyleValue from retrieved
CSSValue from Element.inlineStyle.getPropertyCSSValue

- Started CSSOMTypes for getting which CSSPropertyIDs can take
which types of StyleValue.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Cached StyleValues in the InlineStylePropertyMap with
invalidation to refresh them

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets cached StyleValue or makes StyleValue from retrieved
CSSValue from Element.inlineStyle.getPropertyCSSValue

- Started CSSOMTypes for getting which CSSPropertyIDs can take
which types of StyleValue.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO: Iteration, custom properties

BUG=545318
==========

[meade_UTC10, 2016-03-22 04:52:23]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Cached StyleValues in the InlineStylePropertyMap with
invalidation to refresh them

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets cached StyleValue or makes StyleValue from retrieved
CSSValue from Element.inlineStyle.getPropertyCSSValue

- Started CSSOMTypes for getting which CSSPropertyIDs can take
which types of StyleValue.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO: Iteration, custom properties

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Cached StyleValues in the InlineStylePropertyMap with
invalidation to refresh them

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets cached StyleValue or makes StyleValue from retrieved
CSSValue from Element.inlineStyle.getPropertyCSSValue

- Started CSSOMTypes for getting which CSSPropertyIDs can take
which types of StyleValue.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

[meade_UTC10, 2016-03-22 04:53:04]

Ping Tim - now that the other CL is in, this is ready for proper review.

[Timothy Loh, 2016-03-23 03:45:09]

BTW braces aren't required for single line if statements, I think leaving them
out is more common than not but you can keep them if you prefer having them (I
personally prefer to avoid them).

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap.html
(right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap.html:16:
testElement.style.width = '10px';
Better to pick different values (10px, 20px, etc.). Otherwise if (e.g.) this
line didn't actually affect the styleMap value the assertion below would
probably still be fine.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap.html:28: });
Probably worth checking that nothing weird happens when we pass invalid data
(e.g. we clear a previously set value).

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap.html:45: //
Append
Let's split this up into separate tests. Maybe one for set/get, then separate
ones for append, delete, getAll, getProperties.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:68: if
(!item.isNull()) {
IMO clearer to not negate here and switch the order of the blocks.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:72: // Clear
it.
I don't think this is a useful comment.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:98: if
(!CSSPropertyMetadata::propertySupportsMultiple(propertyID)) {
!supportsMultiple

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:99:
exceptionState.throwTypeError("Property does not support multiple values");
need a return here?

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:111: } else
if (item.isNull()) {
IMO put this last (so the if structure matches the type name) and just make it

} else {
    ASSERT(item.isNull());
    return;
}

so anyone reading the code doesn't wonder if there's some case we didn't handle
here.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:184: if
(!m_styles.contains(propertyID)) {
Can probably make this a little more efficient in terms of hash lookups (does it
matter? I dunno..)

Something like:

..::iterator iterator = m_styles.find(propertyID);
if (!iterator)
    iterator = m_styles.set(propertyID, StyleValueVector())->storedValue;
return iterator->value;

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:30: void
set(CSSPropertyID, StyleValueOrStyleValueSequenceOrString& item,
ExceptionState&) override;
no need to give arguments names in declarations if they don't add any
information

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:51:
HashMap<CSSPropertyID, bool> m_cleanStyles;
Are we going to set the bool to false ever? Otherwise we can just use a HashSet.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/LengthValue.h (right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/LengthValue.h:33: static const int
kNumSupportedUnits = 15;
Unrelated, leave this out of the patch.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp (right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp:51:
exceptionState.throwTypeError("Invalid propertyName: " + propertyName);
Why do only some of these have exceptions? Are we going to add them to the other
functions later?

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3524: bool
Element::setInlineStyleProperty(CSSPropertyID propertyID, const
PassRefPtrWillBeRawPtr<CSSValue> value, bool important)
To save some copy paste and make this patch a little bit smaller, can we make a
separate change for this:

- Add this function (void return is probably sufficient?)
- Call this function from the two above functions (the non-string ones) and
change those to void return type.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3527:
ensureMutableInlineStyle().setProperty(propertyID, value);
Missing important argument

[meade_UTC10, 2016-03-29 06:27:37]

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap.html
(right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap.html:16:
testElement.style.width = '10px';
On 2016/03/23 03:45:08, Timothy Loh wrote:
> Better to pick different values (10px, 20px, etc.). Otherwise if (e.g.) this
> line didn't actually affect the styleMap value the assertion below would
> probably still be fine.

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap.html:28: });
On 2016/03/23 03:45:08, Timothy Loh wrote:
> Probably worth checking that nothing weird happens when we pass invalid data
> (e.g. we clear a previously set value).

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap.html:45: //
Append
On 2016/03/23 03:45:08, Timothy Loh wrote:
> Let's split this up into separate tests. Maybe one for set/get, then separate
> ones for append, delete, getAll, getProperties.

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:68: if
(!item.isNull()) {
On 2016/03/23 03:45:09, Timothy Loh wrote:
> IMO clearer to not negate here and switch the order of the blocks.

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:72: // Clear
it.
On 2016/03/23 03:45:08, Timothy Loh wrote:
> I don't think this is a useful comment.

Removed.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:98: if
(!CSSPropertyMetadata::propertySupportsMultiple(propertyID)) {
On 2016/03/23 03:45:09, Timothy Loh wrote:
> !supportsMultiple

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:99:
exceptionState.throwTypeError("Property does not support multiple values");
On 2016/03/23 03:45:09, Timothy Loh wrote:
> need a return here?

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:111: } else
if (item.isNull()) {
On 2016/03/23 03:45:09, Timothy Loh wrote:
> IMO put this last (so the if structure matches the type name) and just make it
> 
> } else {
>     ASSERT(item.isNull());
>     return;
> }
> 
> so anyone reading the code doesn't wonder if there's some case we didn't
handle
> here.

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:184: if
(!m_styles.contains(propertyID)) {
On 2016/03/23 03:45:09, Timothy Loh wrote:
> Can probably make this a little more efficient in terms of hash lookups (does
it
> matter? I dunno..)
> 
> Something like:
> 
> ..::iterator iterator = m_styles.find(propertyID);
> if (!iterator)
>     iterator = m_styles.set(propertyID, StyleValueVector())->storedValue;
> return iterator->value;

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:30: void
set(CSSPropertyID, StyleValueOrStyleValueSequenceOrString& item,
ExceptionState&) override;
On 2016/03/23 03:45:09, Timothy Loh wrote:
> no need to give arguments names in declarations if they don't add any
> information

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:51:
HashMap<CSSPropertyID, bool> m_cleanStyles;
On 2016/03/23 03:45:09, Timothy Loh wrote:
> Are we going to set the bool to false ever? Otherwise we can just use a
HashSet.

Good idea, done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/LengthValue.h (right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/LengthValue.h:33: static const int
kNumSupportedUnits = 15;
On 2016/03/23 03:45:09, Timothy Loh wrote:
> Unrelated, leave this out of the patch.

I don't remember what I was doing here :o removed

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp (right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp:51:
exceptionState.throwTypeError("Invalid propertyName: " + propertyName);
On 2016/03/23 03:45:09, Timothy Loh wrote:
> Why do only some of these have exceptions? Are we going to add them to the
other
> functions later?

Dunno. Probably assumed that setting something that doesn't exist is an error,
but you can just return null for getting something that doesn't exist. Added
exceptions.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3524: bool
Element::setInlineStyleProperty(CSSPropertyID propertyID, const
PassRefPtrWillBeRawPtr<CSSValue> value, bool important)
On 2016/03/23 03:45:09, Timothy Loh wrote:
> To save some copy paste and make this patch a little bit smaller, can we make
a
> separate change for this:
> 
> - Add this function (void return is probably sufficient?)
> - Call this function from the two above functions (the non-string ones) and
> change those to void return type.

Done.

https://codereview.chromium.org/1590193002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3527:
ensureMutableInlineStyle().setProperty(propertyID, value);
On 2016/03/23 03:45:09, Timothy Loh wrote:
> Missing important argument

Done.

[esprehn, 2016-03-29 22:13:41]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-03-29 22:13:42]

What does the spec say about the caching? I thought we were going to always
return a new value. Our caching strategy will be observable from script with the
object identity of StyleValue.

[meade_UTC10, 2016-03-30 00:54:00]

On 2016/03/29 22:13:42, esprehn wrote:
> What does the spec say about the caching? I thought we were going to always
> return a new value. Our caching strategy will be observable from script with
the
> object identity of StyleValue.

The spec doesn't say anything about caching, but since StyleValues are
immutable, I don't think it matters whether they are new copies of the object or
not.

[esprehn, 2016-03-30 00:56:16]

On 2016/03/30 at 00:54:00, meade wrote:
> On 2016/03/29 22:13:42, esprehn wrote:
> > What does the spec say about the caching? I thought we were going to always
> > return a new value. Our caching strategy will be observable from script with
the
> > object identity of StyleValue.
> 
> The spec doesn't say anything about caching, but since StyleValues are
immutable, I don't think it matters whether they are new copies of the object or
not.

It definitely matters, it's web observable if the object identity is the same.
Browsers need to agree on when new wrappers are created otherwise you get weird
and subtle bugs across browser in your apps.

[esprehn, 2016-03-30 01:02:01]

esprehn@chromium.org changed reviewers:
+ shans@chromium.org

[esprehn, 2016-03-30 01:02:02]

ex.

var width1 = element.styleMap.get("width");
doSomething(element);
var width2 = element.styleMap.get("width");

if (width1 == width2) {
  // do code here.
}

if doSomething would have invalidated the cache that code fails, if it didn't
that code passes.

I'm pretty sure we agreed to always return a fresh object in discussions with
shans@. If we're not going to do that we need to have the spec be super clear
about when the object identity will change. I'm not a fan of this though, it
means you get really weird and subtle bugs because "10px" is cached for a single
element, but not across elements, so the identity is especially unstable wrt. to
author expectations.

[meade_UTC10, 2016-03-30 05:14:15]

On 2016/03/30 01:02:02, esprehn wrote:
> ex.
> 
> var width1 = element.styleMap.get("width");
> doSomething(element);
> var width2 = element.styleMap.get("width");
> 
> if (width1 == width2) {
>   // do code here.
> }
> 
> if doSomething would have invalidated the cache that code fails, if it didn't
> that code passes.
> 
> I'm pretty sure we agreed to always return a fresh object in discussions with
> shans@. If we're not going to do that we need to have the spec be super clear
> about when the object identity will change. I'm not a fan of this though, it
> means you get really weird and subtle bugs because "10px" is cached for a
single
> element, but not across elements, so the identity is especially unstable wrt.
to
> author expectations.

Hmm, I see what you mean. Removed the caching.

[esprehn, 2016-03-31 05:04:51]

On 2016/03/30 at 05:14:15, meade wrote:
> On 2016/03/30 01:02:02, esprehn wrote:
> > ex.
> > 
> > var width1 = element.styleMap.get("width");
> > doSomething(element);
> > var width2 = element.styleMap.get("width");
> > 
> > if (width1 == width2) {
> >   // do code here.
> > }
> > 
> > if doSomething would have invalidated the cache that code fails, if it
didn't
> > that code passes.
> > 
> > I'm pretty sure we agreed to always return a fresh object in discussions
with
> > shans@. If we're not going to do that we need to have the spec be super
clear
> > about when the object identity will change. I'm not a fan of this though, it
> > means you get really weird and subtle bugs because "10px" is cached for a
single
> > element, but not across elements, so the identity is especially unstable
wrt. to
> > author expectations.
> 
> Hmm, I see what you mean. Removed the caching.

You might want to update your description. It still talks about caching. I don't
want to be your blocking reviewer here btw. You should get timloh@ and shans@ to
take a look. If they're happy I'm happy to look over it again, but don't block
on me. My drive by was mostly about the object identity thing. :)

[meade_UTC10, 2016-03-31 05:43:45]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Cached StyleValues in the InlineStylePropertyMap with
invalidation to refresh them

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets cached StyleValue or makes StyleValue from retrieved
CSSValue from Element.inlineStyle.getPropertyCSSValue

- Started CSSOMTypes for getting which CSSPropertyIDs can take
which types of StyleValue.

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets cached StyleValue or makes StyleValue from retrieved
CSSValue from Element.inlineStyle.getPropertyCSSValue


Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

[meade_UTC10, 2016-03-31 05:44:29]

On 2016/03/31 05:04:51, esprehn wrote:
> On 2016/03/30 at 05:14:15, meade wrote:
> > On 2016/03/30 01:02:02, esprehn wrote:
> > > ex.
> > > 
> > > var width1 = element.styleMap.get("width");
> > > doSomething(element);
> > > var width2 = element.styleMap.get("width");
> > > 
> > > if (width1 == width2) {
> > >   // do code here.
> > > }
> > > 
> > > if doSomething would have invalidated the cache that code fails, if it
> didn't
> > > that code passes.
> > > 
> > > I'm pretty sure we agreed to always return a fresh object in discussions
> with
> > > shans@. If we're not going to do that we need to have the spec be super
> clear
> > > about when the object identity will change. I'm not a fan of this though,
it
> > > means you get really weird and subtle bugs because "10px" is cached for a
> single
> > > element, but not across elements, so the identity is especially unstable
> wrt. to
> > > author expectations.
> > 
> > Hmm, I see what you mean. Removed the caching.
> 
> You might want to update your description. It still talks about caching. I
don't
> want to be your blocking reviewer here btw. You should get timloh@ and shans@
to
> take a look. If they're happy I'm happy to look over it again, but don't block
> on me. My drive by was mostly about the object identity thing. :)

Oops, done, thanks!

[Timothy Loh, 2016-04-05 07:55:04]

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:20:
ASSERT(propertyID != CSSPropertyInvalid);
I'm personally not a fan of these sorts of assertions since it ends up
suggesting some of the other cases might be valid (e.g. CSSPropertyVariable).

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:33:
RawPtr<CSSValue> cssValue =
m_ownerElement->ensureMutableInlineStyle().getPropertyCSSValue(propertyID);
just write CSSValue*, you shouldn't be writing RawPtr anywhere :-)

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:46: for
(auto& value : *toCSSValueList(cssValue.get())) {
does it work if you write for (CSSValue* value : ..) ?

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:48: if
(styleValue)
This is weird, is there some case where we end up only populating the vector
with some of the values?

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:110: // Null
is a no-op.
You're already handling this above

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:125:
CSSValueList* cssValueList = toCSSValueList(cssValue.get());
Let's be more defensive here. propertySupportsMultiple is not necessarily going
to be sufficient to guarantee a list. The properties transform and
transition-property can both take lists, or the value none (only by itself). The
content property can take a list or either of the values none and normal by
themselves.

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:133:
cssValueList->append(item.getAsStyleValue()->toCSSValue());
OK, although I'm slightly wary here since this is afaik the first time we're
making CSSValues mutable (normally CSSValueList::append is only used during
construction at parse-time)

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:148: // Null
is a no-op.
not an error?

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/MutableStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/MutableStylePropertyMap.h:15:
MutableStylePropertyMap() : StylePropertyMap() { }
MutableStylePropertyMap() = default;

[meade_UTC10, 2016-04-06 04:33:13]

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:20:
ASSERT(propertyID != CSSPropertyInvalid);
On 2016/04/05 07:55:03, Timothy Loh wrote:
> I'm personally not a fan of these sorts of assertions since it ends up
> suggesting some of the other cases might be valid (e.g. CSSPropertyVariable).

Sure, removed. In this case you'd just end up with nullptr if you passed in an
invalid propertyID anyway.

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:33:
RawPtr<CSSValue> cssValue =
m_ownerElement->ensureMutableInlineStyle().getPropertyCSSValue(propertyID);
On 2016/04/05 07:55:03, Timothy Loh wrote:
> just write CSSValue*, you shouldn't be writing RawPtr anywhere :-)

Done.

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:46: for
(auto& value : *toCSSValueList(cssValue.get())) {
On 2016/04/05 07:55:03, Timothy Loh wrote:
> does it work if you write for (CSSValue* value : ..) ?

Yep, done

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:48: if
(styleValue)
On 2016/04/05 07:55:03, Timothy Loh wrote:
> This is weird, is there some case where we end up only populating the vector
> with some of the values?

Yeah, good point. Returning empty list instead.

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:110: // Null
is a no-op.
On 2016/04/05 07:55:03, Timothy Loh wrote:
> You're already handling this above

Oops, copy pasta error. Fixed.

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:125:
CSSValueList* cssValueList = toCSSValueList(cssValue.get());
On 2016/04/05 07:55:04, Timothy Loh wrote:
> Let's be more defensive here. propertySupportsMultiple is not necessarily
going
> to be sufficient to guarantee a list. The properties transform and
> transition-property can both take lists, or the value none (only by itself).
The
> content property can take a list or either of the values none and normal by
> themselves.

Done.

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:133:
cssValueList->append(item.getAsStyleValue()->toCSSValue());
On 2016/04/05 07:55:03, Timothy Loh wrote:
> OK, although I'm slightly wary here since this is afaik the first time we're
> making CSSValues mutable (normally CSSValueList::append is only used during
> construction at parse-time)

Right. Would it be better to make a copy of it since I'm setting it into the
element below anyway?

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:148: // Null
is a no-op.
On 2016/04/05 07:55:03, Timothy Loh wrote:
> not an error?

Fixed.

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/MutableStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/MutableStylePropertyMap.h:15:
MutableStylePropertyMap() : StylePropertyMap() { }
On 2016/04/05 07:55:04, Timothy Loh wrote:
> MutableStylePropertyMap() = default;

Done.

[Timothy Loh, 2016-04-07 03:42:53]

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:20:
ASSERT(propertyID != CSSPropertyInvalid);
On 2016/04/06 04:33:13, Eddy wrote:
> On 2016/04/05 07:55:03, Timothy Loh wrote:
> > I'm personally not a fan of these sorts of assertions since it ends up
> > suggesting some of the other cases might be valid (e.g.
CSSPropertyVariable).
> 
> Sure, removed. In this case you'd just end up with nullptr if you passed in an
> invalid propertyID anyway.

I meant here and all the other functions too.

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:48: }
did you forget to append here? :s

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:127:
cssValueList = CSSValueList::createSpaceSeparated();
This isn't really what I meant. We can't just expect that we can put any value
in a value list. For now let's just bail out or something.

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:13: class
CSSValue;
fwd decl doesn't look used

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/StylePropertyMap.idl (right):

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/StylePropertyMap.idl:9:
[RaisesException] void append(DOMString property, (StyleValue or
sequence<StyleValue> or DOMString)? value);
Is this changing these to be optional (allow null)? Doesn't seem like the spec
has this.

[meade_UTC10, 2016-04-07 05:41:30]

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:20:
ASSERT(propertyID != CSSPropertyInvalid);
On 2016/04/07 03:42:53, Timothy Loh wrote:
> On 2016/04/06 04:33:13, Eddy wrote:
> > On 2016/04/05 07:55:03, Timothy Loh wrote:
> > > I'm personally not a fan of these sorts of assertions since it ends up
> > > suggesting some of the other cases might be valid (e.g.
> CSSPropertyVariable).
> > 
> > Sure, removed. In this case you'd just end up with nullptr if you passed in
an
> > invalid propertyID anyway.
> 
> I meant here and all the other functions too.

Done.

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:48: }
On 2016/04/07 03:42:53, Timothy Loh wrote:
> did you forget to append here? :s

argh, fixed.

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:127:
cssValueList = CSSValueList::createSpaceSeparated();
On 2016/04/07 03:42:53, Timothy Loh wrote:
> This isn't really what I meant. We can't just expect that we can put any value
> in a value list. For now let's just bail out or something.

Done.

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h (right):

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h:13: class
CSSValue;
On 2016/04/07 03:42:53, Timothy Loh wrote:
> fwd decl doesn't look used

Done.

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/StylePropertyMap.idl (right):

https://codereview.chromium.org/1590193002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/StylePropertyMap.idl:9:
[RaisesException] void append(DOMString property, (StyleValue or
sequence<StyleValue> or DOMString)? value);
On 2016/04/07 03:42:53, Timothy Loh wrote:
> Is this changing these to be optional (allow null)? Doesn't seem like the spec
> has this.

Ah, I think I was trying something out. Fixed.

[Timothy Loh, 2016-04-07 06:34:17]

almost there

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_append.html
(right):

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_append.html:1:
<!DOCTYPE html>
should probably test case sensitivity in some of the tests

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_delete.html
(right):

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_delete.html:1:
<!DOCTYPE html>
should test deleting after we added via stylemap too, and once we have any list
properties we should test deleting some of those.

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_getProperties.html
(right):

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_getProperties.html:1:
<!DOCTYPE html>
should have a test which has multiple properties in the returned array

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:116:
exceptionState.throwTypeError("Property is not already list valued");
return; ? otherwise you'll get a null pointer dereference very soon.

[meade_UTC10, 2016-04-07 07:15:36]

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_append.html
(right):

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_append.html:1:
<!DOCTYPE html>
On 2016/04/07 06:34:17, Timothy Loh wrote:
> should probably test case sensitivity in some of the tests

Done.

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_delete.html
(right):

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_delete.html:1:
<!DOCTYPE html>
On 2016/04/07 06:34:17, Timothy Loh wrote:
> should test deleting after we added via stylemap too, and once we have any
list
> properties we should test deleting some of those.

Done.

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_getProperties.html
(right):

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlineStylePropertyMap_getProperties.html:1:
<!DOCTYPE html>
On 2016/04/07 06:34:17, Timothy Loh wrote:
> should have a test which has multiple properties in the returned array

Done.

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1590193002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:116:
exceptionState.throwTypeError("Property is not already list valued");
On 2016/04/07 06:34:17, Timothy Loh wrote:
> return; ? otherwise you'll get a null pointer dereference very soon.

whoops, done.

[Timothy Loh, 2016-04-11 01:34:24]

lgtm, but fix up all the argument orders in the tests first

https://codereview.chromium.org/1590193002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_delete.html
(right):

https://codereview.chromium.org/1590193002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_delete.html:12:
assert_equals('80px', testElement.styleMap.get('width').cssString);
Arguments are backwards here and everywhere else, actual value should be first
and expected value should be second

https://codereview.chromium.org/1590193002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_getProperties.html
(right):

https://codereview.chromium.org/1590193002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_getProperties.html:43:
assert_true(result.indexOf('width') >= 0);
ok, the spec really ought to specify an ordering though.

[meade_UTC10, 2016-04-11 03:52:16]

https://codereview.chromium.org/1590193002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_delete.html
(right):

https://codereview.chromium.org/1590193002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_delete.html:12:
assert_equals('80px', testElement.styleMap.get('width').cssString);
On 2016/04/11 01:34:24, Timothy Loh wrote:
> Arguments are backwards here and everywhere else, actual value should be first
> and expected value should be second

Done.

https://codereview.chromium.org/1590193002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_getProperties.html
(right):

https://codereview.chromium.org/1590193002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_getProperties.html:43:
assert_true(result.indexOf('width') >= 0);
On 2016/04/11 01:34:24, Timothy Loh wrote:
> ok, the spec really ought to specify an ordering though.

https://github.com/w3c/css-houdini-drafts/issues/178

[meade_UTC10, 2016-04-12 01:59:25]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets cached StyleValue or makes StyleValue from retrieved
CSSValue from Element.inlineStyle.getPropertyCSSValue


Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets makes StyleValue from retrievedCSSValue 
from Element.inlineStyle.getPropertyCSSValue


Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

[meade_UTC10, 2016-04-12 01:59:56]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- gets makes StyleValue from retrievedCSSValue 
from Element.inlineStyle.getPropertyCSSValue


Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- makes StyleValue from retrievedCSSValue 
from Element.inlineStyle.getPropertyCSSValue


Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

[meade_UTC10, 2016-04-12 02:00:26]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2016-04-12 02:00:26]

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

[commit-bot: I haz the power, 2016-04-12 02:01:37]

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

[commit-bot: I haz the power, 2016-04-12 02:07:39]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- makes StyleValue from retrievedCSSValue 
from Element.inlineStyle.getPropertyCSSValue


Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- makes StyleValue from retrievedCSSValue 
from Element.inlineStyle.getPropertyCSSValue


Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

[commit-bot: I haz the power, 2016-04-12 02:07:40]

Committed patchset #33 (id:640001)

[commit-bot: I haz the power, 2016-04-12 02:08:35]

Description was changed from

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- makes StyleValue from retrievedCSSValue 
from Element.inlineStyle.getPropertyCSSValue


Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318
==========

to

==========
Partial implementation of inline StylePropertyMap.

- Implemented get, getAll, getProperties, set, append and remove

- Basic flow of types implemented:
set(styleValue) -> toCSSValue() -> sets in Element.setInlineStyle
get() <- makes StyleValue from retrievedCSSValue
from Element.inlineStyle.getPropertyCSSValue

Spec:
https://drafts.css-houdini.org/css-typed-om/#inline-stylepropertymap-objects

TODO after this CL: Iteration, custom properties

BUG=545318

Committed: https://crrev.com/285b76a05c09b684956fab196db9c11d63d2a726
Cr-Commit-Position: refs/heads/master@{#386551}
==========

[commit-bot: I haz the power, 2016-04-12 02:08:36]

Patchset 33 (id:??) landed as
https://crrev.com/285b76a05c09b684956fab196db9c11d63d2a726
Cr-Commit-Position: refs/heads/master@{#386551}