[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Makes Typed CSSOM Computed StylePropertyMap values reflect computed
style values instead of resolved style values (as are returned by
getComputedStyle) for some length-type properties (top, left, bottom,
right, width, & height).

For all other properties we get a value from
ComputedStyleCSSValueMapping::get, and convert it to a CSSStyleValue
using StyleValueFactory. These values may have undergone zoom-
adjustment and other alterations in ComputedStyleCSSValueMapping. I
may do a follow-up patch to add an option to
ComputedStyleCSSValueMapping::get for retrieving raw values.

We considered returning an CSSUnsupportedValue object (with cssText
from ComputedStyleCSSValueMapping) for unsupported properties. However
the custom paint code is already relying on this API, and changing to
CSSUnsupportedValues broke some of their layout tests. I've decided to
leave the CSSStyleValues in place for now as a best approximation to
true computed style.

BUG=648495
Review-Url: https://codereview.chromium.org/2403423002
Cr-Commit-Position: refs/heads/master@{#446731}
Committed: https://chromium.googlesource.com/chromium/src/+/738869d74b447a741a2e8a5cb95f5c49e12459f0

[rjwright, 2016-12-08 03:58:35]

Description was changed from

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Replaces https://codereview.chromium.org/2312293003, which was getting messy.

Currently only working for top, left, bottom, right, width, height.

I have removed the old code which returned resolved style values. This means
that for now we support fewer properties, and I have marked some layout tests as
expected failures. Will work on getting them passing again in follow-up patches.

I'm still not confident with the style recalc stuff, and I need to add more
property tests, and tests for pseudo-elements.

BUG=648495
==========

to

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Replaces https://codereview.chromium.org/2312293003, which was getting messy.

Currently only working for top, left, bottom, right, width, height. I can add
support for other length-type properties either in this patch or in a followup.

I have removed the old code which returned resolved style values. This means
that for now we support fewer properties, and I have marked some layout tests as
expected failures. Will work on getting them passing again in follow-up patches.

In a follow up patch I will add a flag to ComputedStyleCSSValyeMapping::get that
makes it return just the raw value off the ComputedStyle and not zoom adjusted
etc.

BUG=648495
==========

[rjwright, 2016-12-08 03:58:36]

rjwright@chromium.org changed reviewers:
+ meade@chromium.org, sashab@chromium.org

[rjwright, 2016-12-08 04:06:31]

The CQ bit was checked by rjwright@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meade_UTC10, 2016-12-08 23:22:15]

Minor comments only. Otherwise lgtm.

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html
(left):

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html:34:
}, 'Properties retrieved from ComputedStyleMap reflect the same values as from
ComputedStyle');
Is removing this test necessary? Could you also add an expectation instead?

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

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html:35:
pseudoComputedStyleMap = getComputedStyleMap(testElement, '::after');
Would it still work to put this statement outside the t.step functions?
So

document.onreadystatechange = function() {
  if (document.readyState == 'complete') {
    pseudoComputedStyleMap = getComputedStyleMap(testElement, '::after');
    t1.step(function() {
...

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h (left):

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:81: // CSSOM
functions. Don't make these public.
Do you know why? :o

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

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:97: switch
(propertyID) {
// TODO(rjwright): Generate this code?

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

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:1908: if (isPseudoElement())
What's this for? Could you please add why you needed to add this to the CL
description?

[rjwright, 2016-12-22 06:58:53]

On 2016/12/08 23:22:15, Eddy wrote:
> Minor comments only. Otherwise lgtm.
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html
> (left):
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html:34:
> }, 'Properties retrieved from ComputedStyleMap reflect the same values as from
> ComputedStyle');
> Is removing this test necessary? Could you also add an expectation instead?

This test isn't correct any more. We're not having the values match
getComputedStyle in any scenario, right?

> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
> File
> third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html
> (right):
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/typedcssom/computedstyle/pseudo-elements.html:35:
> pseudoComputedStyleMap = getComputedStyleMap(testElement, '::after');
> Would it still work to put this statement outside the t.step functions?
> So
> 
> document.onreadystatechange = function() {
>   if (document.readyState == 'complete') {
>     pseudoComputedStyleMap = getComputedStyleMap(testElement, '::after');
>     t1.step(function() {
> ...
> 

Yep :)

>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h (left):
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:81: // CSSOM
> functions. Don't make these public.
> Do you know why? :o
> 

Na, but Shane and Doug said they couldn't think of a good reason either

>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp
> (right):
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:97:
switch
> (propertyID) {
> // TODO(rjwright): Generate this code?
> 

Probably a good idea. Can I do it in a follow-up? Planning to do an immdiate
followup to add more properties anyway.

>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Element.cpp:1908: if (isPseudoElement())
> What's this for? Could you please add why you needed to add this to the CL
> description?

I can't remember, but I definitely didn't need it twice. That was a rebase
artifact. It's in master now so I just removed it from this CL.

[meade_UTC10, 2017-01-05 03:43:45]

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html
(left):

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html:34:
}, 'Properties retrieved from ComputedStyleMap reflect the same values as from
ComputedStyle');
On 2016/12/22 06:58:53, rjwright wrote:
> This test isn't correct any more. We're not having the values match
> getComputedStyle in any scenario, right?

Only some of the properties will be different right? We can write this test
differently to account for those ones.

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h (left):

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:81: // CSSOM
functions. Don't make these public.
> Na, but Shane and Doug said they couldn't think of a good reason either

FWIW from code spelunking it looks like a long time ago (2012) some people at
apple were trying to split CSSStyleDeclaration and the CSSOM implementation as
part of making shareable stylesheet data structures
https://bugs.webkit.org/show_bug.cgi?id=77299, however that project seems to
have been abandoned. It's probably fine to make these public - it's weird that
the subclasses have a different API to the superclasses.

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

https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:97: switch
(propertyID) {
> On 2016/12/22 06:58:53, rjwright wrote:
> Probably a good idea. Can I do it in a follow-up? Planning to do an immdiate
> followup to add more properties anyway.

Sorry, I meant just to add a TODO comment...

https://codereview.chromium.org/2403423002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h (right):

https://codereview.chromium.org/2403423002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:72: const
CSSValue* getPropertyCSSValueInternal(
It doesn't seem like the getPropertyCSSValueInternal method should be made
public. AFAICT getPropertyCSSValueInternal actually just calls
getPropertyCSSValue(CSSPropertyID), could you just use that instead?

[rjwright, 2017-01-06 08:08:40]

On 2017/01/05 03:43:45, Eddy wrote:
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html
> (left):
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html:34:
> }, 'Properties retrieved from ComputedStyleMap reflect the same values as from
> ComputedStyle');
> On 2016/12/22 06:58:53, rjwright wrote:
> > This test isn't correct any more. We're not having the values match
> > getComputedStyle in any scenario, right?
> 
> Only some of the properties will be different right? We can write this test
> differently to account for those ones.
> 

Overlap between computed style and resolved style is incidental though, so I
don't think it makes sense to test one against the other. Even unsupported
properties now pull their cssText from ComputedStyleCSSValueMapping, not
resolved style. I'm happy to replace it with something more appropriate if you
think we've lost some test coverage by deleting it. I'm just not sure we've lost
anything.

>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h (left):
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:81: // CSSOM
> functions. Don't make these public.
> > Na, but Shane and Doug said they couldn't think of a good reason either
> 
> FWIW from code spelunking it looks like a long time ago (2012) some people at
> apple were trying to split CSSStyleDeclaration and the CSSOM implementation as
> part of making shareable stylesheet data structures
> https://bugs.webkit.org/show_bug.cgi?id=77299, however that project seems to
> have been abandoned. It's probably fine to make these public - it's weird that
> the subclasses have a different API to the superclasses.
> 

Yeah, that was my conclusion too. I think you were involved in the initial
discussion and said the same thing then. It was a while ago.

>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp
> (right):
> 
>
https://codereview.chromium.org/2403423002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:97:
switch
> (propertyID) {
> > On 2016/12/22 06:58:53, rjwright wrote:
> > Probably a good idea. Can I do it in a follow-up? Planning to do an immdiate
> > followup to add more properties anyway.
> 
> Sorry, I meant just to add a TODO comment...
> 

done

>
https://codereview.chromium.org/2403423002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h (right):
> 
>
https://codereview.chromium.org/2403423002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:72: const
> CSSValue* getPropertyCSSValueInternal(
> It doesn't seem like the getPropertyCSSValueInternal method should be made
> public. AFAICT getPropertyCSSValueInternal actually just calls
> getPropertyCSSValue(CSSPropertyID), could you just use that instead?

done

[meade_UTC10, 2017-01-09 00:47:35]

lgtm

[sashab, 2017-01-09 02:19:04]

Definitely on the right track, only small things left.

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1870: # Regressed while Typed
CSSOM computed StylePropertyMap is converted to using ComputedStyle values. Will
fix in a subsequent patch (contact rjwright@)
These tests use outdated versions of ComptuedStyle for TypedOM. WIP fixing ...

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html
(right):

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html:22:

TODO ask eddy about exhaustive tests -- eg generator

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/typedcssom/computedstyle/left.html (right):

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/left.html:16: var t2 =
async_test("getAll for left returns a single value");
Even though its a bit longer, I think its clearer if you break these tests up,
if possible

In the worst case:
var t1 = async_test("...")
function t1_callback(computedStyleMap) {
  ...
}

var t2 = ...
function t2_callback ...

document.onready... = function() {
  ...
  computedStyleMap = ...
  t1_callback()
  t2_callback()
}

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/computedstyle/left.html:19:
if(document.readyState == 'complete') {
Space after if

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h (right):

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:66:
MutableStylePropertySet* copyPropertiesInSet(
Fix typo in CL description & wrap to 72 characters

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/CSSKeywordValue.h (right):

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/CSSKeywordValue.h:23: static
CSSKeywordValue* create(const AtomicString& keyword, ExceptionState&);
Can you use a default value for ExceptionState?

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

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:22:
CSSStyleValue* styleValueForLength(const Length& length) {
const CSSStyleValue&

Make return nullptr at end just be like return CSSKeywordValue::create("auto")

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:43: Node*
ComputedStylePropertyMap::node() const {
const Node*

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:69: if
(!node->inActiveDocument()) {
Make above if statement if (!node || !node->inActiveDocument())

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:88: if
(!node->inActiveDocument()) {
Make above if-statement ||

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:94: if
(!style)
Move this into above if statement too

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

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:33: Node*
node() const;
All these Node*s can be changed to const Node&s

Basically change in this class and in FilteredStylePropertyMap

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/css/cssom/FilteredComputedStylePropertyMapTest.cpp
(right):

https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/FilteredComputedStylePropertyMapTest.cpp:26:
Node* pageNode() { return m_page->document().documentElement(); }
Node* pageNode() const { ... }

Can this be const Node&?
At the very least - const Node*

[rjwright, 2017-01-09 04:53:33]

Description was changed from

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Replaces https://codereview.chromium.org/2312293003, which was getting messy.

Currently only working for top, left, bottom, right, width, height. I can add
support for other length-type properties either in this patch or in a followup.

I have removed the old code which returned resolved style values. This means
that for now we support fewer properties, and I have marked some layout tests as
expected failures. Will work on getting them passing again in follow-up patches.

In a follow up patch I will add a flag to ComputedStyleCSSValyeMapping::get that
makes it return just the raw value off the ComputedStyle and not zoom adjusted
etc.

BUG=648495
==========

to

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Makes Typed CSSOM Computed StylePropertyMap length values reflect computed style
values instead of resolved style values (as are returned by getComputedStyle).

Removes the old code which used resolved style values, and adds correct support
for top, left, bottom, right, width, & height. This means that, for now, we
support fewer properties; but what is supported is correct. More properties will
be added in subsequent patches.

Unsupported properties now yield a CSSUnsupportedValue object with cssText from
ComputedStyleCSSValueMapping. This cssText may have undergone zoom-adjustment
and other alterations in ComputedStyleCSSValueMapping. A follow up patch will
add an option to ComputedStyleCSSValueMapping::get for retrieving raw computed
style values.

BUG=648495
==========

[rjwright, 2017-01-09 04:54:07]

Description was changed from

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Makes Typed CSSOM Computed StylePropertyMap length values reflect computed style
values instead of resolved style values (as are returned by getComputedStyle).

Removes the old code which used resolved style values, and adds correct support
for top, left, bottom, right, width, & height. This means that, for now, we
support fewer properties; but what is supported is correct. More properties will
be added in subsequent patches.

Unsupported properties now yield a CSSUnsupportedValue object with cssText from
ComputedStyleCSSValueMapping. This cssText may have undergone zoom-adjustment
and other alterations in ComputedStyleCSSValueMapping. A follow up patch will
add an option to ComputedStyleCSSValueMapping::get for retrieving raw computed
style values.

BUG=648495
==========

to

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Makes Typed CSSOM Computed StylePropertyMap length values reflect 
computed style values instead of resolved style values (as are returned 
by getComputedStyle).

Removes the old code which used resolved style values, and adds correct 
support for top, left, bottom, right, width, & height. This means that, 
for now, we support fewer properties; but what is supported is correct. 
More properties will be added in subsequent patches.

Unsupported properties now yield a CSSUnsupportedValue object with 
cssText from ComputedStyleCSSValueMapping. This cssText may have 
undergone zoom-adjustment and other alterations in 
ComputedStyleCSSValueMapping. A follow up patch will add an option to 
ComputedStyleCSSValueMapping::get for retrieving raw computed style 
values.

BUG=648495
==========

[rjwright, 2017-01-16 10:17:53]

On 2017/01/09 02:19:04, sashab wrote:
> Definitely on the right track, only small things left.
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:1870: # Regressed while Typed
> CSSOM computed StylePropertyMap is converted to using ComputedStyle values.
Will
> fix in a subsequent patch (contact rjwright@)
> These tests use outdated versions of ComptuedStyle for TypedOM. WIP fixing ...
> 

done

>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html
> (right):
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html:22:
> 
> TODO ask eddy about exhaustive tests -- eg generator
> 

TODO

>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/typedcssom/computedstyle/left.html
(right):
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/typedcssom/computedstyle/left.html:16: var t2 =
> async_test("getAll for left returns a single value");
> Even though its a bit longer, I think its clearer if you break these tests up,
> if possible
> 
> In the worst case:
> var t1 = async_test("...")
> function t1_callback(computedStyleMap) {
>   ...
> }
> 
> var t2 = ...
> function t2_callback ...
> 
> document.onready... = function() {
>   ...
>   computedStyleMap = ...
>   t1_callback()
>   t2_callback()
> }
> 

Done

>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/typedcssom/computedstyle/left.html:19:
> if(document.readyState == 'complete') {
> Space after if
> 

Done

>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h (right):
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h:66:
> MutableStylePropertySet* copyPropertiesInSet(
> Fix typo in CL description & wrap to 72 characters

Done

> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/cssom/CSSKeywordValue.h (right):
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/CSSKeywordValue.h:23: static
> CSSKeywordValue* create(const AtomicString& keyword, ExceptionState&);
> Can you use a default value for ExceptionState?

The version that uses an ExceptionState is called from V8. It needs to dcheck
that it has a valid ExceptionState. Thus these can't be combined.

> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp
> (right):
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:22:
> CSSStyleValue* styleValueForLength(const Length& length) {
> const CSSStyleValue&
> 
> Make return nullptr at end just be like return CSSKeywordValue::create("auto")
> 

None of these pointer type changes seem practical to me. In each case the var
either has some non-const methods called on it, or it can be null, or both. If
you think it's really worth doing we should talk thru it in person.

>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:43:
Node*
> ComputedStylePropertyMap::node() const {
> const Node*
> 

See above

>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:69: if
> (!node->inActiveDocument()) {
> Make above if statement if (!node || !node->inActiveDocument())
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:88: if
> (!node->inActiveDocument()) {
> Make above if-statement ||
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:94: if
> (!style)
> Move this into above if statement too
> 

Done all 3

>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h
> (right):
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:33: Node*
> node() const;
> All these Node*s can be changed to const Node&s
> 
> Basically change in this class and in FilteredStylePropertyMap
> 

See above.

>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/core/css/cssom/FilteredComputedStylePropertyMapTest.cpp
> (right):
> 
>
https://codereview.chromium.org/2403423002/diff/200001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/css/cssom/FilteredComputedStylePropertyMapTest.cpp:26:
> Node* pageNode() { return m_page->document().documentElement(); }
> Node* pageNode() const { ... }
> 
> Can this be const Node&?
> At the very least - const Node*

See above.

[sashab, 2017-01-17 01:12:38]

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

https://codereview.chromium.org/2403423002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:52:
Member<Node> m_node;
Basically here's how I feel about passing around Node*:

- We should make it const in as many places as we can
- We should make it explicitly non-null in methods that cannot return a nullptr

Otherwise callers will make assumptions about the returned type (e.g. modify it,
or check if its null) when that can never be true.

If you'd rather do it in a follow up patch, then add a TODO here saying:
// TODO(rjwright): Make this Member<const Node>

And add a TODO above the node() method:
// TODO(rjwright): Make this return a Node&.

And add a DCHECK(node) to the constructor (since I'm pretty sure it can never be
null).

Technically this is bad design so like I said I'd much prefer if you did the
changes in this patch. Yes it's a nontrivial change but its really important to
the health and readability of this code. Alternatively you can upload a second
patch with this one as a dependent patchset, and we can land this one once both
are working. Either way I want to see progress on this.

[sashab, 2017-01-17 01:18:41]

Sorry, that came across a bit harsh. Didn't intend it that way. Happy to
discuss this in person. :)

On Tue, Jan 17, 2017 at 10:12 AM, <sashab@chromium.org> wrote:

>
> https://codereview.chromium.org/2403423002/diff/220001/
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h
> File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h
> (right):
>
> https://codereview.chromium.org/2403423002/diff/220001/
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h#
> newcode52
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:52:
> Member<Node> m_node;
> Basically here's how I feel about passing around Node*:
>
> - We should make it const in as many places as we can
> - We should make it explicitly non-null in methods that cannot return a
> nullptr
>
> Otherwise callers will make assumptions about the returned type (e.g.
> modify it, or check if its null) when that can never be true.
>
> If you'd rather do it in a follow up patch, then add a TODO here saying:
> // TODO(rjwright): Make this Member<const Node>
>
> And add a TODO above the node() method:
> // TODO(rjwright): Make this return a Node&.
>
> And add a DCHECK(node) to the constructor (since I'm pretty sure it can
> never be null).
>
> Technically this is bad design so like I said I'd much prefer if you did
> the changes in this patch. Yes it's a nontrivial change but its really
> important to the health and readability of this code. Alternatively you
> can upload a second patch with this one as a dependent patchset, and we
> can land this one once both are working. Either way I want to see
> progress on this.
>
> https://codereview.chromium.org/2403423002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sashab, 2017-01-17 01:18:41]

Sorry, that came across a bit harsh. Didn't intend it that way. Happy to
discuss this in person. :)

On Tue, Jan 17, 2017 at 10:12 AM, <sashab@chromium.org> wrote:

>
> https://codereview.chromium.org/2403423002/diff/220001/
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h
> File third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h
> (right):
>
> https://codereview.chromium.org/2403423002/diff/220001/
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h#
> newcode52
> third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:52:
> Member<Node> m_node;
> Basically here's how I feel about passing around Node*:
>
> - We should make it const in as many places as we can
> - We should make it explicitly non-null in methods that cannot return a
> nullptr
>
> Otherwise callers will make assumptions about the returned type (e.g.
> modify it, or check if its null) when that can never be true.
>
> If you'd rather do it in a follow up patch, then add a TODO here saying:
> // TODO(rjwright): Make this Member<const Node>
>
> And add a TODO above the node() method:
> // TODO(rjwright): Make this return a Node&.
>
> And add a DCHECK(node) to the constructor (since I'm pretty sure it can
> never be null).
>
> Technically this is bad design so like I said I'd much prefer if you did
> the changes in this patch. Yes it's a nontrivial change but its really
> important to the health and readability of this code. Alternatively you
> can upload a second patch with this one as a dependent patchset, and we
> can land this one once both are working. Either way I want to see
> progress on this.
>
> https://codereview.chromium.org/2403423002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[rjwright, 2017-01-24 00:18:39]

On 2017/01/17 01:18:41, sashab wrote:
> Sorry, that came across a bit harsh. Didn't intend it that way. Happy to
> discuss this in person. :)
>
> > third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h:52:
> > Member<Node> m_node;
> > Basically here's how I feel about passing around Node*:
> >
> > - We should make it const in as many places as we can
> > - We should make it explicitly non-null in methods that cannot return a
> > nullptr
> >
> > Otherwise callers will make assumptions about the returned type (e.g.
> > modify it, or check if its null) when that can never be true.
> >
> > If you'd rather do it in a follow up patch, then add a TODO here saying:
> > // TODO(rjwright): Make this Member<const Node>
> >
> > And add a TODO above the node() method:
> > // TODO(rjwright): Make this return a Node&.
> >
> > And add a DCHECK(node) to the constructor (since I'm pretty sure it can
> > never be null).
> >
> > Technically this is bad design so like I said I'd much prefer if you did
> > the changes in this patch. Yes it's a nontrivial change but its really
> > important to the health and readability of this code. Alternatively you
> > can upload a second patch with this one as a dependent patchset, and we
> > can land this one once both are working. Either way I want to see
> > progress on this.

I agree with the overall intention.

In the case of ComputedStylePropertyMap::node(), we have to call
ensureComputedStyle on the returned value. ensureComputedStyle will modify it. I
don't think m_node itself can be nullptr - IDL expects the caller to provide an
actual node. But ComputedStylePropertyMap::node() has to return something for
the case that there's a pseudo-element specifier which doesn't resolve to an
element.

In the case of CSSStyleValue* styleValueForLength, we can probably make it a
const ref. But I will have to change the create methods it uses, namely
CSSSimlpeLength and CSSKeywordValue, first. Currently those create methods can
return nullptr. If I'm going to change some of the Typed CSSOM create methods I
would rather change all of them at once so that we have a consistent style
throughout. I think that puts it outside the scope of this CL.

[sashab, 2017-01-24 02:27:00]

lgtm

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

https://codereview.chromium.org/2403423002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:70: node =
this->node();
Duplicate line; not needed

https://codereview.chromium.org/2403423002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:81: node =
this->node();
Not needed?

[rjwright, 2017-01-25 00:14:11]

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

https://codereview.chromium.org/2403423002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp:81: node =
this->node();
On 2017/01/24 02:27:00, sashab wrote:
> Not needed?

These are here because style resolution can change the value of an
element/pseudo-element-specifier pair.

[rjwright, 2017-01-25 01:04:57]

The CQ bit was checked by rjwright@chromium.org

[rjwright, 2017-01-25 01:04:57]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org,
sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2403423002/#ps260001
(title: "fix rebase derp")

[commit-bot: I haz the power, 2017-01-25 01:05:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 01:09:35]

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

[commit-bot: I haz the power, 2017-01-25 01:09:36]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rjwright, 2017-01-25 01:41:38]

The CQ bit was checked by rjwright@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 01:42:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 01:45:43]

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

[commit-bot: I haz the power, 2017-01-25 01:45:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[rjwright, 2017-01-25 02:27:36]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2017-01-25 02:28:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 02:31:39]

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

[commit-bot: I haz the power, 2017-01-25 02:31:41]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[rjwright, 2017-01-25 21:29:17]

The CQ bit was checked by rjwright@chromium.org

[rjwright, 2017-01-25 21:29:18]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org,
sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2403423002/#ps280001
(title: "squash and rebase")

[commit-bot: I haz the power, 2017-01-25 21:29:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 21:43:56]

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

[commit-bot: I haz the power, 2017-01-25 21:43:57]

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

[rjwright, 2017-01-25 22:05:05]

rjwright@chromium.org changed reviewers:
+ haraken@chromium.org

[rjwright, 2017-01-25 22:06:25]

Hey Haraken, I need an owner's review for the change in
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp.

Feel free to just check that, or do a full review. Whatever you prefer.

Thanks,

Renee.

[haraken, 2017-01-25 22:27:08]

LGTM

[rjwright, 2017-01-25 22:57:08]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2017-01-25 22:58:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 00:03:49]

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

[commit-bot: I haz the power, 2017-01-26 00:03:51]

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

[rjwright, 2017-01-26 23:24:40]

The CQ bit was checked by rjwright@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 23:25:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rjwright, 2017-01-26 23:40:37]

Description was changed from

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Makes Typed CSSOM Computed StylePropertyMap length values reflect 
computed style values instead of resolved style values (as are returned 
by getComputedStyle).

Removes the old code which used resolved style values, and adds correct 
support for top, left, bottom, right, width, & height. This means that, 
for now, we support fewer properties; but what is supported is correct. 
More properties will be added in subsequent patches.

Unsupported properties now yield a CSSUnsupportedValue object with 
cssText from ComputedStyleCSSValueMapping. This cssText may have 
undergone zoom-adjustment and other alterations in 
ComputedStyleCSSValueMapping. A follow up patch will add an option to 
ComputedStyleCSSValueMapping::get for retrieving raw computed style 
values.

BUG=648495
==========

to

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Makes Typed CSSOM Computed StylePropertyMap values reflect computed
style values instead of resolved style values (as are returned by
getComputedStyle) for some length-type properties (top, left, bottom,
right, width, & height).

For all other properties we get a value from
ComputedStyleCSSValueMapping::get, and convert it to a CSSStyleValue
using StyleValueFactory. These values may have undergone zoom-
adjustment and other alterations in ComputedStyleCSSValueMapping. I
may do a follow-up patch to add an option to
ComputedStyleCSSValueMapping::get for retrieving raw values.

We considered returning an CSSUnsupportedValue object (with cssText
from ComputedStyleCSSValueMapping) for unsupported properties. However
the custom paint code is already relying on this API, and changing to
CSSUnsupportedValues broke some of their layout tests. I've decided to
leave the CSSStyleValues in place for now as a best approximation to
true computed style.

BUG=648495
==========

[rjwright, 2017-01-26 23:44:27]

On 2017/01/26 00:03:51, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Eddy PTAL.

Changing unsupported property output for StyleValueFactory CSSStyleValues to
CSSUnsupportedValues broke some of the csspaint layout tests. I decided to
change it back to using StyleValueFactory for unsupported properties, rather
than risk breaking the custom paint implementation. The alternatative is to
alter the csspaint layout tests & get input from Ian that using
CSSUnsupportedValues is OK for him.

[commit-bot: I haz the power, 2017-01-27 03:08:01]

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

[commit-bot: I haz the power, 2017-01-27 03:08:02]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-01-27 06:18:49]

Seems fine to me.

[meade_UTC10, 2017-01-27 06:19:03]

lgtm

[rjwright, 2017-01-27 19:11:47]

The CQ bit was checked by rjwright@chromium.org

[rjwright, 2017-01-27 19:11:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2403423002/#ps300001
(title: "Use StyleValueFactory & ComputedStyleCSSValueMapping for unsupported")

[commit-bot: I haz the power, 2017-01-27 19:12:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 19:17:24]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1485544307427880,
"parent_rev": "bf35553cb93c48e324ff268383000d42018875da", "commit_rev":
"738869d74b447a741a2e8a5cb95f5c49e12459f0"}

[commit-bot: I haz the power, 2017-01-27 19:17:57]

Description was changed from

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Makes Typed CSSOM Computed StylePropertyMap values reflect computed
style values instead of resolved style values (as are returned by
getComputedStyle) for some length-type properties (top, left, bottom,
right, width, & height).

For all other properties we get a value from
ComputedStyleCSSValueMapping::get, and convert it to a CSSStyleValue
using StyleValueFactory. These values may have undergone zoom-
adjustment and other alterations in ComputedStyleCSSValueMapping. I
may do a follow-up patch to add an option to
ComputedStyleCSSValueMapping::get for retrieving raw values.

We considered returning an CSSUnsupportedValue object (with cssText
from ComputedStyleCSSValueMapping) for unsupported properties. However
the custom paint code is already relying on this API, and changing to
CSSUnsupportedValues broke some of their layout tests. I've decided to
leave the CSSStyleValues in place for now as a best approximation to
true computed style.

BUG=648495
==========

to

==========
[CSSTypedOM] Computed StylePropertyMap use ComputedStyle for Lengths

Makes Typed CSSOM Computed StylePropertyMap values reflect computed
style values instead of resolved style values (as are returned by
getComputedStyle) for some length-type properties (top, left, bottom,
right, width, & height).

For all other properties we get a value from
ComputedStyleCSSValueMapping::get, and convert it to a CSSStyleValue
using StyleValueFactory. These values may have undergone zoom-
adjustment and other alterations in ComputedStyleCSSValueMapping. I
may do a follow-up patch to add an option to
ComputedStyleCSSValueMapping::get for retrieving raw values.

We considered returning an CSSUnsupportedValue object (with cssText
from ComputedStyleCSSValueMapping) for unsupported properties. However
the custom paint code is already relying on this API, and changing to
CSSUnsupportedValues broke some of their layout tests. I've decided to
leave the CSSStyleValues in place for now as a best approximation to
true computed style.

BUG=648495

Review-Url: https://codereview.chromium.org/2403423002
Cr-Commit-Position: refs/heads/master@{#446731}
Committed:
https://chromium.googlesource.com/chromium/src/+/738869d74b447a741a2e8a5cb95f...
==========

[commit-bot: I haz the power, 2017-01-27 19:17:59]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/738869d74b447a741a2e8a5cb95f...