Implement ComputedStylePropertyMap for Typed OM.

Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window.

Implements the getAll and getProperties.
Moved has() up into the base class from InlineStylePropertyMap.

Not implemented: iteration

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

[meade_UTC10, 2016-03-31 05:05:25]

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

[Timothy Loh, 2016-03-31 06:59:25]

I just skimmed it really briefly (since I still need to look at the dependent
one first).

- Description needs to provide some context. At least a couple of sentences
about what the class you're implementing does, mention what spec it's from, and
so on.
- Needs a test that this works with pseudo-elements
- We probably can't simply re-use the ComputedStyleCSSValueMapping completely or
at least need to rework it a little. The spec says "getComputedStyleMap
represents computed style, not resolved style", so we don't want all the logic
for resolving width and so on.

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

https://codereview.chromium.org/1849653003/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:37: return
(!cssValue->isValueList() || toCSSValueList(cssValue)->length() > 0);
CSSValueLists are never empty (if you're setting empty CSSValueLists, something
is probably going to break)

https://codereview.chromium.org/1849653003/diff/70001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/WindowGetComputedStyle.h (right):

https://codereview.chromium.org/1849653003/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/WindowGetComputedStyle.h:20:
ASSERT(elt);
Not sure about this, what happens if you use null? We should test that. Also
please don't use abbreviated names in Blink, just write element and
pseudoElement.

[meade_UTC10, 2016-04-04 01:27:06]

Description was changed from

==========
Implement ComputedStylePropertyMap.

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

to

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window (note spec says document, but needs updating; see
https://github.com/w3c/css-houdini-drafts/issues/81)

Implements getAll, has and getProperties
Moves some of the common functionality of InlineStylePropertyMap up
into StylePropertyMap (get, cssValueToStyleVector,
typedef StyleValueVector).

Not implemented: iteration

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

[meade_UTC10, 2016-04-04 01:27:23]

Description was changed from

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window (note spec says document, but needs updating; see
https://github.com/w3c/css-houdini-drafts/issues/81)

Implements getAll, has and getProperties
Moves some of the common functionality of InlineStylePropertyMap up
into StylePropertyMap (get, cssValueToStyleVector,
typedef StyleValueVector).

Not implemented: iteration

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

to

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window (note spec says document, but needs updating; see
https://github.com/w3c/css-houdini-drafts/issues/81)

Implements getAll, has and getProperties
Moves some of the common functionality of InlineStylePropertyMap up
into StylePropertyMap (get, cssValueToStyleVector,
typedef StyleValueVector).

Not implemented: iteration

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

[meade_UTC10, 2016-04-04 03:32:41]

Updated description and added test

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

https://codereview.chromium.org/1849653003/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:37: return
(!cssValue->isValueList() || toCSSValueList(cssValue)->length() > 0);
On 2016/03/31 06:59:25, Timothy Loh wrote:
> CSSValueLists are never empty (if you're setting empty CSSValueLists,
something
> is probably going to break)

Acknowledged. Seems best to look at that case though, just in case something
weird is going on. Would it be better to add an assert instead?

https://codereview.chromium.org/1849653003/diff/70001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/WindowGetComputedStyle.h (right):

https://codereview.chromium.org/1849653003/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/WindowGetComputedStyle.h:20:
ASSERT(elt);
On 2016/03/31 06:59:25, Timothy Loh wrote:
> Not sure about this, what happens if you use null? We should test that. Also
> please don't use abbreviated names in Blink, just write element and
> pseudoElement.

Done. FWIW, this was pretty much copied from here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

In this case (and the linked one) the generated code type checks that the
Element argument is actually an argument and throws a JS exception, so it
shouldn't get here if element is null.

[Timothy Loh, 2016-04-15 04:16:08]

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

https://codereview.chromium.org/1849653003/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:37: return
(!cssValue->isValueList() || toCSSValueList(cssValue)->length() > 0);
On 2016/04/04 03:32:41, Eddy wrote:
> On 2016/03/31 06:59:25, Timothy Loh wrote:
> > CSSValueLists are never empty (if you're setting empty CSSValueLists,
> something
> > is probably going to break)
> 
> Acknowledged. Seems best to look at that case though, just in case something
> weird is going on. Would it be better to add an assert instead?

Why would this function care about whether the value is a CSSValueList at all
though?

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html
(right):

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html:1:
<head>
no need for head/body, and please use standards mode unless you're explicitly
testing quirks mode.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html:6:
<style type='text/css'>
no need for type=..

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html:9:
width: 20px;
width not needed?

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:1: //
Copyright 2016 the chromium authors. All rights reserved.
some capitalization got lost here

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:14:
ASSERT(propertyID != CSSPropertyInvalid);
let's remove these as discussed onthe other patch

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h
(right):

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:1: //
Copyright 2016 the chromium authors. All rights reserved.
capitalisation

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:13: class
CSSStyleDeclaration;
probably not needed since you include CSSComputedStyleDeclaration?

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:14: class
Element;
unused?

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

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp:85:
StylePropertyMap::StyleValueVector
StylePropertyMap::cssValueToStyleValueVector(CSSPropertyID propertyID, const
CSSValue& cssValue)
The part of the CL which just moves code around should be a separate CL, since
it doesn't need the rest of the change to go along with it. Smaller CLs are
easier/quicker to review than larger CLs.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp:96: for (const
Member<CSSValue> value : *toCSSValueList(&cssValue)) {
Wasn't this CSSValue* previously?

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/WindowGetComputedStyle.h (right):

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/WindowGetComputedStyle.h:10: #include
"core/dom/Document.h"
do we need Document and Allocator?

[meade_UTC10, 2016-04-27 05:53:56]

I'll move the code that moved into a new CL, stay tuned...

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html
(right):

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html:1:
<head>
On 2016/04/15 04:16:07, Timothy Loh wrote:
> no need for head/body, and please use standards mode unless you're explicitly
> testing quirks mode.

Done.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html:6:
<style type='text/css'>
On 2016/04/15 04:16:07, Timothy Loh wrote:
> no need for type=..

Done.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html:9:
width: 20px;
On 2016/04/15 04:16:07, Timothy Loh wrote:
> width not needed?

Done.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:1: //
Copyright 2016 the chromium authors. All rights reserved.
On 2016/04/15 04:16:07, Timothy Loh wrote:
> some capitalization got lost here

Done.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:14:
ASSERT(propertyID != CSSPropertyInvalid);
On 2016/04/15 04:16:07, Timothy Loh wrote:
> let's remove these as discussed onthe other patch

Done.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h
(right):

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:1: //
Copyright 2016 the chromium authors. All rights reserved.
On 2016/04/15 04:16:07, Timothy Loh wrote:
> capitalisation

Done.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:13: class
CSSStyleDeclaration;
On 2016/04/15 04:16:07, Timothy Loh wrote:
> probably not needed since you include CSSComputedStyleDeclaration?

Removed.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:14: class
Element;
On 2016/04/15 04:16:07, Timothy Loh wrote:
> unused?

Removed.

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/WindowGetComputedStyle.h (right):

https://codereview.chromium.org/1849653003/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/WindowGetComputedStyle.h:10: #include
"core/dom/Document.h"
On 2016/04/15 04:16:08, Timothy Loh wrote:
> do we need Document and Allocator?

We dont...copy pasta error. Fixed.

[meade_UTC10, 2016-05-09 04:51:16]

Ping, now that the other CL is submitted.

[Timothy Loh, 2016-05-13 02:08:23]

lgtm, but fix up the below comment

https://codereview.chromium.org/1849653003/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1849653003/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:24: return
(!cssValue->isValueList() || toCSSValueList(cssValue)->length() > 0);
Commenting on this again - let's just change it to return cssValue. I think it's
actively misleading to do this sort of check.

This function should also be tested, at least have a couple cases where it
returns true and a couple cases where it returns false (in particular, comes
through here and returns false, since unknown properties names will fail
earlier). Some properties like max-zoom will return false here, but probably a
better test is setting something like "border: yellow; border-top-color: blue"
and checking border (I assume this is also invalid in typed OM?)

[Timothy Loh, 2016-05-13 02:11:16]

Description needs to be updated too

[meade_UTC10, 2016-05-13 05:05:23]

Description was changed from

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window (note spec says document, but needs updating; see
https://github.com/w3c/css-houdini-drafts/issues/81)

Implements getAll, has and getProperties
Moves some of the common functionality of InlineStylePropertyMap up
into StylePropertyMap (get, cssValueToStyleVector,
typedef StyleValueVector).

Not implemented: iteration

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

to

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window.

Implements the getAll and getProperties.
Moved has() up into the base class from InlineStylePropertyMap.

Not implemented: iteration

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

[meade_UTC10, 2016-05-13 05:05:32]

https://codereview.chromium.org/1849653003/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1849653003/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:24: return
(!cssValue->isValueList() || toCSSValueList(cssValue)->length() > 0);
On 2016/05/13 02:08:23, Timothy Loh wrote:
> Commenting on this again - let's just change it to return cssValue. I think
it's
> actively misleading to do this sort of check.
> 
> This function should also be tested, at least have a couple cases where it
> returns true and a couple cases where it returns false (in particular, comes
> through here and returns false, since unknown properties names will fail
> earlier). Some properties like max-zoom will return false here, but probably a
> better test is setting something like "border: yellow; border-top-color: blue"
> and checking border (I assume this is also invalid in typed OM?)

I changed this to use the base class version instead - in the other CL we
decided to return false if we couldn't represent a value in typed OM. 

I added some basic tests, but you can't use shorthands in typed OM, so your
colour example doesn't work as a testcase. Any other ideas?

[Timothy Loh, 2016-05-13 05:07:45]

On 2016/05/13 05:05:32, Eddy wrote:
> I changed this to use the base class version instead - in the other CL we
> decided to return false if we couldn't represent a value in typed OM. 
> 
> I added some basic tests, but you can't use shorthands in typed OM, so your
> colour example doesn't work as a testcase. Any other ideas?

I don't think any non-shorthands should return false (aside from stuff like
this-is-not-a-property and max-zoom, which is not a CSS property but in
CSSProperties.in)

[meade_UTC10, 2016-05-13 06:36:05]

On 2016/05/13 05:07:45, Timothy Loh wrote:
> On 2016/05/13 05:05:32, Eddy wrote:
> > I changed this to use the base class version instead - in the other CL we
> > decided to return false if we couldn't represent a value in typed OM. 
> > 
> > I added some basic tests, but you can't use shorthands in typed OM, so your
> > colour example doesn't work as a testcase. Any other ideas?
> 
> I don't think any non-shorthands should return false (aside from stuff like
> this-is-not-a-property and max-zoom, which is not a CSS property but in
> CSSProperties.in)

Hmm ok. Then I'm not sure the best way to test this with the current set of
supported properties and values (which are only things that return simple
lengths).

[meade_UTC10, 2016-06-03 03:14:16]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2016-06-03 03:14:17]

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/1849653003/#ps350001
(title: "Sync to HEAD and replace ASSERT with DCHECK")

[commit-bot: I haz the power, 2016-06-03 03:14:41]

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

[commit-bot: I haz the power, 2016-06-03 04:30:37]

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

[commit-bot: I haz the power, 2016-06-03 04:30:40]

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

[shans, 2016-06-06 00:34:31]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2016-06-06 00:34:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1849653003/350001

[commit-bot: I haz the power, 2016-06-06 01:54:45]

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

[commit-bot: I haz the power, 2016-06-06 01:54:46]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[meade_UTC10, 2016-06-06 06:52:04]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2016-06-06 06:52:04]

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/1849653003/#ps390001
(title: "Update LayoutTests")

[commit-bot: I haz the power, 2016-06-06 06:52:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1849653003/390001

[commit-bot: I haz the power, 2016-06-06 08:15:05]

Description was changed from

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window.

Implements the getAll and getProperties.
Moved has() up into the base class from InlineStylePropertyMap.

Not implemented: iteration

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

to

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window.

Implements the getAll and getProperties.
Moved has() up into the base class from InlineStylePropertyMap.

Not implemented: iteration

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

[commit-bot: I haz the power, 2016-06-06 08:15:11]

Committed patchset #21 (id:390001)

[commit-bot: I haz the power, 2016-06-06 08:16:04]

Description was changed from

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window.

Implements the getAll and getProperties.
Moved has() up into the base class from InlineStylePropertyMap.

Not implemented: iteration

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

to

==========
Implement ComputedStylePropertyMap for Typed OM.

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

Uses a  partial IDL interface to attach a getComputedStyleMap method to
window.

Implements the getAll and getProperties.
Moved has() up into the base class from InlineStylePropertyMap.

Not implemented: iteration

BUG=545318

Committed: https://crrev.com/adbd6389292d7443b39dab6accb1581db3788465
Cr-Commit-Position: refs/heads/master@{#397992}
==========

[commit-bot: I haz the power, 2016-06-06 08:16:06]

Patchset 21 (id:??) landed as
https://crrev.com/adbd6389292d7443b39dab6accb1581db3788465
Cr-Commit-Position: refs/heads/master@{#397992}