Hook up style invalidation for CSS Paint API.

Hook up style invalidation for CSS Paint API.

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
Committed: https://crrev.com/0ab7ac1ec67da157bc4ef5cbcb1b10530bf997ad
Cr-Commit-Position: refs/heads/master@{#394223}

[ikilpatrick, 2016-04-19 21:33:33]

Description was changed from

==========
Hook up style invalidation for CSS Paint API.

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

to

==========
Hook up style invalidation for CSS Paint API.

***NOTE dependent on***
https://codereview.chromium.org/1866623002#ps140001

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

[ikilpatrick, 2016-04-19 21:34:46]

Description was changed from

==========
Hook up style invalidation for CSS Paint API.

***NOTE dependent on***
https://codereview.chromium.org/1866623002#ps140001

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

to

==========
Hook up style invalidation for CSS Paint API.

***NOTE DEPENDENT ON***
https://codereview.chromium.org/1866623002#ps140001

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

[ikilpatrick, 2016-04-19 21:34:47]

ikilpatrick@chromium.org changed reviewers:
+ alancutter@chromium.org, chrishtr@chromium.org, esprehn@chromium.org

[ikilpatrick, 2016-04-19 22:31:26]

Note dependent on: https://codereview.chromium.org/1866623002#ps140001

alan: Does moving CSSPropertyEquality make sense here?

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintValue.h (right):

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintValue.h:40: DCHECK(m_generator);
This is potentially bad, the m_generator doesn't exist until paint() is called
(as it needs the Document to look up the paint worklet).

In ComputedStyle this could be protected against by a needsFirstPaint() check,
i.e.

// CSSPaintValue.h
bool needsFirstPaint() { return !m_generator; }

// ComputedStyle.h
bool ComputedStyle::diffNeedsPaintInvalidationObjectForPaint(styleImage)
{
  if (paintValue->needsFirstPaint())
    return true;
}

I.e. a paint function which has never been painted is always invalid.

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:764: const StyleImage*
image = borderImage().image();
Should this block be if-guarded with:
1) if (RuntimeEnabledFeatures::CSSPaintAPI)?
2) if (hasSomePaintImageOnStyle())?

for hasSomePaintImageOnStyle(), this bool would be set in StyleResolverState?,
and all image calls would have to proxy through that?

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleImage.h (right):

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleImage.h:80: ALWAYS_INLINE bool
isPaintImage() const { return m_isPaintImage; }
This could be removed in favour of:

s/styleImage->isPaintImage()/styleImage->cssValue()->isPaintValue()/g

[alancutter (OOO until 2018), 2016-04-20 01:16:04]

On 2016/04/19 at 22:31:26, ikilpatrick wrote:
> Note dependent on: https://codereview.chromium.org/1866623002#ps140001
> 
> alan: Does moving CSSPropertyEquality make sense here?

Yes. There's a bunch of style code under core/animation just because it's only
used by animation code, it's fine by me if it moves into core/css should it be
useful elsewhere.

[esprehn, 2016-04-20 04:40:06]

Hmm I don't think you want a first paint concept. You want to just
diff(oldInputs, newInputs) oldInputs should be null when we're on the first
resolve.

[ikilpatrick, 2016-04-21 23:03:38]

On 2016/04/20 04:40:06, esprehn wrote:
> Hmm I don't think you want a first paint concept. You want to just
> diff(oldInputs, newInputs) oldInputs should be null when we're on the first
> resolve.

Made oldInputs be null on first paint.

Also made -prefixed-property console.warn.

@alan I've started working on https://codereview.chromium.org/1910263003/ if you
want to have a first look at it. Might be good to have a chat about it. Will try
and catch you during SYD hours.

[ikilpatrick, 2016-04-27 00:41:31]

Description was changed from

==========
Hook up style invalidation for CSS Paint API.

***NOTE DEPENDENT ON***
https://codereview.chromium.org/1866623002#ps140001

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

to

==========
Hook up style invalidation for CSS Paint API.

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

[ikilpatrick, 2016-04-27 00:45:30]

ikilpatrick@chromium.org changed reviewers:
+ rune@opera.com

[ikilpatrick, 2016-04-27 00:45:31]

Rebased of committed CSS Paint API.

+rune/elliott it probably makes sense if one of you checks the style invalidate
code before others review.

alan, I've sent out https://codereview.chromium.org/1910263003 which is a first
pass of auto-generating CSSPropertyEquality.

[ikilpatrick, 2016-04-27 00:48:29]

On 2016/04/27 00:45:31, ikilpatrick wrote:
> Rebased of committed CSS Paint API.
> 
> +rune/elliott it probably makes sense if one of you checks the style
invalidate
> code before others review.
> 
> alan, I've sent out https://codereview.chromium.org/1910263003 which is a
first
> pass of auto-generating CSSPropertyEquality.

rune, if you are catching up, also take a look at my comments in original
message:
https://codereview.chromium.org/1896893004/#msg4
^-^

[rune, 2016-04-27 11:34:34]

I've looked through and commented on the visual invalidation diff code.

The direction looks good as long as the performance impact is fine.

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:764: const StyleImage*
image = borderImage().image();
On 2016/04/19 22:31:25, ikilpatrick wrote:
> Should this block be if-guarded with:
> 1) if (RuntimeEnabledFeatures::CSSPaintAPI)?
> 2) if (hasSomePaintImageOnStyle())?
> 
> for hasSomePaintImageOnStyle(), this bool would be set in StyleResolverState?,
> and all image calls would have to proxy through that?

Something like that. I would check the performance impact first to see how big a
problem this is. Anyway, I think that I would have put the added code into a
separate method.

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:765: if (image &&
image->isPaintImage() && diffNeedsPaintInvalidationObjectForPaintImage(image,
other))
So the assumption here is that the image pointers are equal (possibly both
nullptr), and the sizes did not change, or otherwise this method would have
returned true for visuallyEqual above? The code here is to check if a repaint
would produce a different result due to property changes?

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:796: CSSVariableData*
otherVar = other.variables() ? other.variables()->getVariable(property) :
nullptr;
The variable is resolved at this point, I guess? (in case you have --x:
var(--y))

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:797: if (thisVar !=
otherVar)
A pointer comparison. Is that correct?

If one of them is nullptr, there is no concept of a initial value for css paint
which could be optimized further? For instance if there is a default shape for a
painter like "circle" so that "--paint-shape: circle" gives the same result as
not specifying --paint-shape at all?

[rune, 2016-04-27 11:44:04]

Oops. Reviewed the wrong patch set, but the code I commented on hasn't changed,
I think.

[chrishtr, 2016-04-27 19:52:41]

How about checking that paint invalidations also happen, in addition to style
invalidations?

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html
(right):

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:20: {
property: 'max-height', value: '160px' },
It doesn't look like these tests check the new value of the property?

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:78: if
(propertyID == CSSPropertyInvalid) {
It seems weird that custom properties are identified as CSSPropertyInvalid.

[ikilpatrick, 2016-04-28 22:30:17]

Chatted with esphren in person the other day, suggested making use the
PendingImage codepath instead. Much cleaner :)

@chris re: paint invalidation tests - do you mean using something like
runPaintInvalidationTest to check the actual paint invalidations?

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:764: const StyleImage*
image = borderImage().image();
On 2016/04/27 11:34:34, rune wrote:
> On 2016/04/19 22:31:25, ikilpatrick wrote:
> > Should this block be if-guarded with:
> > 1) if (RuntimeEnabledFeatures::CSSPaintAPI)?
> > 2) if (hasSomePaintImageOnStyle())?
> > 
> > for hasSomePaintImageOnStyle(), this bool would be set in
StyleResolverState?,
> > and all image calls would have to proxy through that?
> 
> Something like that. I would check the performance impact first to see how big
a
> problem this is. Anyway, I think that I would have put the added code into a
> separate method.

Changed this quite a bit now after chat with elliott. Ran it quickly against
some perftests to sanity check.

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:765: if (image &&
image->isPaintImage() && diffNeedsPaintInvalidationObjectForPaintImage(image,
other))
On 2016/04/27 11:34:34, rune wrote:
> So the assumption here is that the image pointers are equal (possibly both
> nullptr), and the sizes did not change, or otherwise this method would have
> returned true for visuallyEqual above? The code here is to check if a repaint
> would produce a different result due to property changes?

Yup thats correct.

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:796: CSSVariableData*
otherVar = other.variables() ? other.variables()->getVariable(property) :
nullptr;
On 2016/04/27 11:34:34, rune wrote:
> The variable is resolved at this point, I guess? (in case you have --x:
> var(--y))

Yup, the variables are resolved with each other at this point, added test case
for this.

https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:797: if (thisVar !=
otherVar)
On 2016/04/27 11:34:34, rune wrote:
> A pointer comparison. Is that correct?
> 
> If one of them is nullptr, there is no concept of a initial value for css
paint
> which could be optimized further? For instance if there is a default shape for
a
> painter like "circle" so that "--paint-shape: circle" gives the same result as
> not specifying --paint-shape at all?

Oh, yeah completely not correct. Sorry about that.

This now serializes the CSSVariableData and does a string compare on that.

Internally CSSVariableData stores a list of CSSParserTokens, but doing a compare
on the tokens is not correct as they do a pointer compare internally, see:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html
(right):

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:20: {
property: 'max-height', value: '160px' },
On 2016/04/27 19:52:40, chrishtr wrote:
> It doesn't look like these tests check the new value of the property?

Need the TypedOM to plumb through the values of the new property. Will modify
these / create new once we have that.

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:78: if
(propertyID == CSSPropertyInvalid) {
On 2016/04/27 19:52:40, chrishtr wrote:
> It seems weird that custom properties are identified as CSSPropertyInvalid.

Right, not sure of the history here. Would have to ask timloh@.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[chrishtr, 2016-04-29 00:24:16]

On 2016/04/28 at 22:30:17, ikilpatrick wrote:
> Chatted with esphren in person the other day, suggested making use the
PendingImage codepath instead. Much cleaner :)
> 
> @chris re: paint invalidation tests - do you mean using something like
runPaintInvalidationTest to check the actual paint invalidations?

Checking the paint invalidation rects, yes. I think you should also have a
couple of reftests to make sure the final output is correct.

One way to check paint invalidation is runPaintInvalidationTest. Another is the
approach in https://codereview.chromium.org/1931513003

> 
>
https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):
> 
>
https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/style/ComputedStyle.cpp:764: const StyleImage*
image = borderImage().image();
> On 2016/04/27 11:34:34, rune wrote:
> > On 2016/04/19 22:31:25, ikilpatrick wrote:
> > > Should this block be if-guarded with:
> > > 1) if (RuntimeEnabledFeatures::CSSPaintAPI)?
> > > 2) if (hasSomePaintImageOnStyle())?
> > > 
> > > for hasSomePaintImageOnStyle(), this bool would be set in
StyleResolverState?,
> > > and all image calls would have to proxy through that?
> > 
> > Something like that. I would check the performance impact first to see how
big a
> > problem this is. Anyway, I think that I would have put the added code into a
> > separate method.
> 
> Changed this quite a bit now after chat with elliott. Ran it quickly against
some perftests to sanity check.
> 
>
https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/style/ComputedStyle.cpp:765: if (image &&
image->isPaintImage() && diffNeedsPaintInvalidationObjectForPaintImage(image,
other))
> On 2016/04/27 11:34:34, rune wrote:
> > So the assumption here is that the image pointers are equal (possibly both
> > nullptr), and the sizes did not change, or otherwise this method would have
> > returned true for visuallyEqual above? The code here is to check if a
repaint
> > would produce a different result due to property changes?
> 
> Yup thats correct.
> 
>
https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/style/ComputedStyle.cpp:796: CSSVariableData*
otherVar = other.variables() ? other.variables()->getVariable(property) :
nullptr;
> On 2016/04/27 11:34:34, rune wrote:
> > The variable is resolved at this point, I guess? (in case you have --x:
> > var(--y))
> 
> Yup, the variables are resolved with each other at this point, added test case
for this.
> 
>
https://codereview.chromium.org/1896893004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/style/ComputedStyle.cpp:797: if (thisVar !=
otherVar)
> On 2016/04/27 11:34:34, rune wrote:
> > A pointer comparison. Is that correct?
> > 
> > If one of them is nullptr, there is no concept of a initial value for css
paint
> > which could be optimized further? For instance if there is a default shape
for a
> > painter like "circle" so that "--paint-shape: circle" gives the same result
as
> > not specifying --paint-shape at all?
> 
> Oh, yeah completely not correct. Sorry about that.
> 
> This now serializes the CSSVariableData and does a string compare on that.
> 
> Internally CSSVariableData stores a list of CSSParserTokens, but doing a
compare on the tokens is not correct as they do a pointer compare internally,
see:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
>
https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html
(right):
> 
>
https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:20:
{ property: 'max-height', value: '160px' },
> On 2016/04/27 19:52:40, chrishtr wrote:
> > It doesn't look like these tests check the new value of the property?
> 
> Need the TypedOM to plumb through the values of the new property. Will modify
these / create new once we have that.
> 
>
https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):
> 
>
https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:78: if
(propertyID == CSSPropertyInvalid) {
> On 2016/04/27 19:52:40, chrishtr wrote:
> > It seems weird that custom properties are identified as CSSPropertyInvalid.
> 
> Right, not sure of the history here. Would have to ask timloh@.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[chrishtr, 2016-04-29 00:24:58]

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:78: if
(propertyID == CSSPropertyInvalid) {
On 2016/04/28 at 22:30:16, ikilpatrick wrote:
> On 2016/04/27 19:52:40, chrishtr wrote:
> > It seems weird that custom properties are identified as CSSPropertyInvalid.
> 
> Right, not sure of the history here. Would have to ask timloh@.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Ok. Please ask him about it.

[chrishtr, 2016-04-29 00:25:34]

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html
(right):

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:20: {
property: 'max-height', value: '160px' },
On 2016/04/28 at 22:30:16, ikilpatrick wrote:
> On 2016/04/27 19:52:40, chrishtr wrote:
> > It doesn't look like these tests check the new value of the property?
> 
> Need the TypedOM to plumb through the values of the new property. Will modify
these / create new once we have that.

Ok please file a bug to remind yourself to do it?

[Timothy Loh, 2016-04-29 01:20:25]

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:78: if
(propertyID == CSSPropertyInvalid) {
On 2016/04/28 22:30:16, ikilpatrick wrote:
> On 2016/04/27 19:52:40, chrishtr wrote:
> > It seems weird that custom properties are identified as CSSPropertyInvalid.
> 
> Right, not sure of the history here. Would have to ask timloh@.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I don't recall exactly. I think it was so that while custom properties were
being implemented, callsites which weren't yet updated to handle custom
properties would just not support them (if we made it return CSSPropertyVariable
then they might error in weird or unsafe ways).

At this point I don't see any reason to not have the isValidVariableName call
inside cssPropertyID/unresolvedCSSPropertyID. I made a keep note to fix it up at
some point (just need to check all the call sites and make sure they handle the
updated function correctly).

[rune, 2016-04-29 08:06:37]

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:794: if
(thisVar->tokenRange().serialize() != otherVar->tokenRange().serialize())
Could you implement an operator==() on CSSTokenRange instead? If whitespace
tokens are preserved (and they're never significant in declarations?), make a
comparison method that skips whitespaces?

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:156:
Vector<Persistent<StyleImage>> m_paintImages;
It's a bit unfortunate that we store these in the computed style as they're only
needed right after style resolution for diffing. I realize the
StyleResolverState is gone before we do the diff, otherwise that could be a
place to store them. Is the rationale that looking up the known image properties
in the diff like you did before is more expensive? Or that this is just cleaner?

[ikilpatrick, 2016-04-29 22:41:59]

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

[ikilpatrick, 2016-04-29 22:42:01]

@chris, updated tests to check in paint invalidations.

@tim, what are your thoughts on operator==() on CSSParserTokenRange? or better
way to do that :).

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html
(right):

https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:20: {
property: 'max-height', value: '160px' },
On 2016/04/29 00:25:33, chrishtr wrote:
> On 2016/04/28 at 22:30:16, ikilpatrick wrote:
> > On 2016/04/27 19:52:40, chrishtr wrote:
> > > It doesn't look like these tests check the new value of the property?
> > 
> > Need the TypedOM to plumb through the values of the new property. Will
modify
> these / create new once we have that.
> 
> Ok please file a bug to remind yourself to do it?

Done: https://bugs.chromium.org/p/chromium/issues/detail?id=608082

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:794: if
(thisVar->tokenRange().serialize() != otherVar->tokenRange().serialize())
On 2016/04/29 08:06:37, rune wrote:
> Could you implement an operator==() on CSSTokenRange instead? If whitespace
> tokens are preserved (and they're never significant in declarations?), make a
> comparison method that skips whitespaces?

+timloh to see what he thinks.

Tim what do you think of operator==() on CSSTokenRange?

I also just checked ant operator==() on CSSParserToken doesn't seemed to be used
anywhere? So could also change to to do a deep equality check instead of just
the pointer comparison it does at the moment. Tim thoughts?

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:156:
Vector<Persistent<StyleImage>> m_paintImages;
On 2016/04/29 08:06:37, rune wrote:
> It's a bit unfortunate that we store these in the computed style as they're
only
> needed right after style resolution for diffing. I realize the
> StyleResolverState is gone before we do the diff, otherwise that could be a
> place to store them. Is the rationale that looking up the known image
properties
> in the diff like you did before is more expensive? Or that this is just
cleaner?

Both, but primarily (slightly) less expensive. Now we just have an empty vector
check, as opposed to walking through the list of all images.

It's not a big gain so can switch back if you like? But seemed better this way.

[chrishtr, 2016-04-30 00:09:14]

lgtm

[rune, 2016-05-02 14:20:32]

On 2016/04/29 22:42:01, ikilpatrick wrote:

>
https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:156:
> Vector<Persistent<StyleImage>> m_paintImages;
> On 2016/04/29 08:06:37, rune wrote:
> > It's a bit unfortunate that we store these in the computed style as they're
> only
> > needed right after style resolution for diffing. I realize the
> > StyleResolverState is gone before we do the diff, otherwise that could be a
> > place to store them. Is the rationale that looking up the known image
> properties
> > in the diff like you did before is more expensive? Or that this is just
> cleaner?
> 
> Both, but primarily (slightly) less expensive. Now we just have an empty
vector
> check, as opposed to walking through the list of all images.
> 
> It's not a big gain so can switch back if you like? But seemed better this
way.

If think I'm fine with either, but it adds at least 16 bytes (I think it was for
an empty Vector?) to every rare-non-inherited-data, which can be quite a few on
some sites IIRC. I guess putting it there was based on your discussion with
Elliott?

[ikilpatrick, 2016-05-02 23:24:49]

On 2016/05/02 14:20:32, rune wrote:
> On 2016/04/29 22:42:01, ikilpatrick wrote:
> 
> >
>
https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:156:
> > Vector<Persistent<StyleImage>> m_paintImages;
> > On 2016/04/29 08:06:37, rune wrote:
> > > It's a bit unfortunate that we store these in the computed style as
they're
> > only
> > > needed right after style resolution for diffing. I realize the
> > > StyleResolverState is gone before we do the diff, otherwise that could be
a
> > > place to store them. Is the rationale that looking up the known image
> > properties
> > > in the diff like you did before is more expensive? Or that this is just
> > cleaner?
> > 
> > Both, but primarily (slightly) less expensive. Now we just have an empty
> vector
> > check, as opposed to walking through the list of all images.
> > 
> > It's not a big gain so can switch back if you like? But seemed better this
> way.
> 
> If think I'm fine with either, but it adds at least 16 bytes (I think it was
for
> an empty Vector?) to every rare-non-inherited-data, which can be quite a few
on
> some sites IIRC. I guess putting it there was based on your discussion with
> Elliott?

Yup that's right.

Just did a simple thing to get it down to 8 bytes by wrapping it in an OwnPtr.
This could also be a Persistent<HeapVector<StyleImage>> but I looks like
persistent is 16 bytes.

[esprehn, 2016-05-03 03:06:30]

If we're actually worried about the size of these things we should probably
do some general refactoring of the rare objects. People pile in features to
them all the time. :)

That said an OwnPtr seems fine.

-- 
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.

[esprehn, 2016-05-03 03:06:30]

If we're actually worried about the size of these things we should probably
do some general refactoring of the rare objects. People pile in features to
them all the time. :)

That said an OwnPtr seems fine.

-- 
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.

[Timothy Loh, 2016-05-03 08:01:39]

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:794: if
(thisVar->tokenRange().serialize() != otherVar->tokenRange().serialize())
On 2016/04/29 22:42:01, ikilpatrick wrote:
> On 2016/04/29 08:06:37, rune wrote:
> > Could you implement an operator==() on CSSTokenRange instead? If whitespace
> > tokens are preserved (and they're never significant in declarations?), make
a
> > comparison method that skips whitespaces?
> 
> +timloh to see what he thinks.
> 
> Tim what do you think of operator==() on CSSTokenRange?
> 
> I also just checked ant operator==() on CSSParserToken doesn't seemed to be
used
> anywhere? So could also change to to do a deep equality check instead of just
> the pointer comparison it does at the moment. Tim thoughts?

It should be used to in style diffing at the moment to avoid subtree recalcs.

StyleRareInheritedData::operator==
StyleVariableData::operator==
CSSVariableData::operator==

[ikilpatrick, 2016-05-03 23:56:18]

Ok, updated CSSParserToken::operator== to actually do a string comparison,
instead of just ptr comparisons.
Created a simple unit test for this.

It looks like this was only used in:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
So this seems safe.

[Timothy Loh, 2016-05-09 07:07:07]

Code change looks fine. I haven't looked at the tests yet.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html
(right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:24:
// TODO(ikilpatrick): Should test shorthands and direction aware properties.
We only have legacy (prefixed) direction aware properties at the moment.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp:152: if
(m_valueLength != other.m_valueLength)
I don't think this change should be part of this patch, since currently it
already mostly works. A custom property value should compare equal to itself
currently, so I think this only fixes the case where you explicitly set a custom
property value to the same as what it currently is.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:779: return true;
I don't understand why this is true instead of false

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:782: if
(!CSSPropertyEquality::propertiesEqual(propertyID, *this, other))
For now, I think we should just return true if either isInterpolableProperty or
isShorthandProperty are true, since propertiesEqual handles neither of these yet
(well probably you'll want to handle shorthands here). In particular, I don't
think the checked in code should ever be in a state where we can hit ASSERTs.

[esprehn, 2016-05-09 21:59:54]

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html
(right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:3:
<head>
leave out html and body

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp:157: equal((const
LChar*) m_valueDataCharRaw, (const LChar*) other.m_valueDataCharRaw,
m_valueLength) :
static_cast, don't do C style casts in blink.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserTokenTest.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserTokenTest.cpp:14:
result.init(string);
we should just add explicit constructors to CSSParserString, having to call
init() and add wrapper methods is weird.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:764: if
(Vector<Persistent<StyleImage>>* paintImages =
rareNonInheritedData->m_paintImages.get()) {
you can just write

if (rareNonInheritedData->m_paintImages)

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:782: if
(!CSSPropertyEquality::propertiesEqual(propertyID, *this, other))
Please add TODOs

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:787: CSSVariableData*
thisVar = variables() ? variables()->getVariable(property) : nullptr;
you can early out this before the loop too.

if (static_cast<bool>(variables()) != static_cast<bool>(other.variables()))
  return true;
if (variables()) {
  for (...) {
    ...
  }
}

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:156:
OwnPtr<Vector<Persistent<StyleImage>>> m_paintImages;
use unique_ptr and WTF::WrapUnique in new code.

[ikilpatrick, 2016-05-10 20:39:38]

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html
(right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:3:
<head>
On 2016/05/09 21:59:54, esprehn wrote:
> leave out html and body

Done.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:24:
// TODO(ikilpatrick): Should test shorthands and direction aware properties.
On 2016/05/09 07:07:07, Timothy Loh wrote:
> We only have legacy (prefixed) direction aware properties at the moment.

Done.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp:152: if
(m_valueLength != other.m_valueLength)
On 2016/05/09 07:07:07, Timothy Loh wrote:
> I don't think this change should be part of this patch, since currently it
> already mostly works. A custom property value should compare equal to itself
> currently, so I think this only fixes the case where you explicitly set a
custom
> property value to the same as what it currently is.

I think that case is pretty important, at least at first. While we do allow this
in the spec, I'd like to make this rational for folks who are playing with this
initially.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp:157: equal((const
LChar*) m_valueDataCharRaw, (const LChar*) other.m_valueDataCharRaw,
m_valueLength) :
On 2016/05/09 21:59:54, esprehn wrote:
> static_cast, don't do C style casts in blink.

Done.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserTokenTest.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserTokenTest.cpp:14:
result.init(string);
On 2016/05/09 21:59:54, esprehn wrote:
> we should just add explicit constructors to CSSParserString, having to call
> init() and add wrapper methods is weird.

Acknowledged.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:764: if
(Vector<Persistent<StyleImage>>* paintImages =
rareNonInheritedData->m_paintImages.get()) {
On 2016/05/09 21:59:54, esprehn wrote:
> you can just write
> 
> if (rareNonInheritedData->m_paintImages)

Done.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:779: return true;
On 2016/05/09 07:07:07, Timothy Loh wrote:
> I don't understand why this is true instead of false

Added note:

This is because at style-diff time the CSSPaintValue may not have a backing
CSSPaintImageGenerator yet, we use null 
(creating a CSSPaintImageGenerator requires a document).

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:782: if
(!CSSPropertyEquality::propertiesEqual(propertyID, *this, other))
On 2016/05/09 21:59:54, esprehn wrote:
> Please add TODOs

Tim, did you mean this check? Don't need isShorthandProperty as they are caught
by isInterpolablePropertyCheck.

Added TODO to remove once we have full coverage in CSSPropertyEquality

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:787: CSSVariableData*
thisVar = variables() ? variables()->getVariable(property) : nullptr;
On 2016/05/09 21:59:54, esprehn wrote:
> you can early out this before the loop too.
> 
> if (static_cast<bool>(variables()) != static_cast<bool>(other.variables()))
>   return true;
> if (variables()) {
>   for (...) {
>     ...
>   }
> }

Not strictly correct as you could have:
this->variables = { '--bar': '2' }
other->variables = null;
value->customInvalidationProperties() = [ '--foo' ];

Added check if they are both null.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:156:
OwnPtr<Vector<Persistent<StyleImage>>> m_paintImages;
On 2016/05/09 21:59:54, esprehn wrote:
> use unique_ptr and WTF::WrapUnique in new code.

Done.

[Timothy Loh, 2016-05-11 06:20:37]

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp:152: if
(m_valueLength != other.m_valueLength)
On 2016/05/10 20:39:37, ikilpatrick wrote:
> On 2016/05/09 07:07:07, Timothy Loh wrote:
> > I don't think this change should be part of this patch, since currently it
> > already mostly works. A custom property value should compare equal to itself
> > currently, so I think this only fixes the case where you explicitly set a
> custom
> > property value to the same as what it currently is.
> 
> I think that case is pretty important, at least at first. While we do allow
this
> in the spec, I'd like to make this rational for folks who are playing with
this
> initially.

I don't get why this case is important, but regardless I think we should pull
this out into a separate change.

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:782: if
(!CSSPropertyEquality::propertiesEqual(propertyID, *this, other))
On 2016/05/10 20:39:38, ikilpatrick wrote:
> On 2016/05/09 21:59:54, esprehn wrote:
> > Please add TODOs
> 
> Tim, did you mean this check? Don't need isShorthandProperty as they are
caught
> by isInterpolablePropertyCheck.
> 
> Added TODO to remove once we have full coverage in CSSPropertyEquality

looks fine

https://codereview.chromium.org/1896893004/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js
(right):

https://codereview.chromium.org/1896893004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js:3:
// Registers an async_test per test case, which:
promise_test?

https://codereview.chromium.org/1896893004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js:43:
const testName = test.property + ': ' + (test.prevValue || 'unset') + ' => ' +
test.property + ': ' + (test.value || 'unset');
unset isn't really the same as what you're doing (it would override the
declarations in div {}), maybe worth rewording slightly

https://codereview.chromium.org/1896893004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js:76:
assert_equals(!!test.noInvalidation, !paintInvalidations);
Argument order is reversed in all the assert calls

    function assert_equals(actual, expected, description)

[ikilpatrick, 2016-05-11 23:59:12]

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp:152: if
(m_valueLength != other.m_valueLength)
On 2016/05/11 06:20:37, Timothy Loh wrote:
> On 2016/05/10 20:39:37, ikilpatrick wrote:
> > On 2016/05/09 07:07:07, Timothy Loh wrote:
> > > I don't think this change should be part of this patch, since currently it
> > > already mostly works. A custom property value should compare equal to
itself
> > > currently, so I think this only fixes the case where you explicitly set a
> > custom
> > > property value to the same as what it currently is.
> > 
> > I think that case is pretty important, at least at first. While we do allow
> this
> > in the spec, I'd like to make this rational for folks who are playing with
> this
> > initially.
> 
> I don't get why this case is important, but regardless I think we should pull
> this out into a separate change.

Done.

https://codereview.chromium.org/1896893004/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js
(right):

https://codereview.chromium.org/1896893004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js:3:
// Registers an async_test per test case, which:
On 2016/05/11 06:20:37, Timothy Loh wrote:
> promise_test?

Done.

https://codereview.chromium.org/1896893004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js:43:
const testName = test.property + ': ' + (test.prevValue || 'unset') + ' => ' +
test.property + ': ' + (test.value || 'unset');
On 2016/05/11 06:20:37, Timothy Loh wrote:
> unset isn't really the same as what you're doing (it would override the
> declarations in div {}), maybe worth rewording slightly

Changed to [inline not set]

https://codereview.chromium.org/1896893004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js:76:
assert_equals(!!test.noInvalidation, !paintInvalidations);
On 2016/05/11 06:20:37, Timothy Loh wrote:
> Argument order is reversed in all the assert calls
> 
>     function assert_equals(actual, expected, description)

Done.

[Timothy Loh, 2016-05-12 07:14:59]

lgtm

[ikilpatrick, 2016-05-17 20:02:21]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-05-17 20:02:21]

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

[commit-bot: I haz the power, 2016-05-17 20:03:16]

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

[commit-bot: I haz the power, 2016-05-17 20:57:22]

Description was changed from

==========
Hook up style invalidation for CSS Paint API.

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

to

==========
Hook up style invalidation for CSS Paint API.

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

[commit-bot: I haz the power, 2016-05-17 20:57:24]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-05-17 21:00:15]

Description was changed from

==========
Hook up style invalidation for CSS Paint API.

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252
==========

to

==========
Hook up style invalidation for CSS Paint API.

This introduces the plumbing required invalidating a custom paint
function.

This breaks the style invalidation into two distinct sets.
 - If the property is a valid native property the registerPaint function
   will convert it to a CSSPropertyID.
 - If it is a CSS variable (custom property) it will convert to an
   AtomicString.

The invalidation check occurs in ComputedStyle::diffNeedsPaintInvalidationObject

This relies on the incomplete behaviour of CSSPropertyEquality which
needs to be auto-generated (more).

BUG=578252

Committed: https://crrev.com/0ab7ac1ec67da157bc4ef5cbcb1b10530bf997ad
Cr-Commit-Position: refs/heads/master@{#394223}
==========

[commit-bot: I haz the power, 2016-05-17 21:00:17]

Patchset 18 (id:??) landed as
https://crrev.com/0ab7ac1ec67da157bc4ef5cbcb1b10530bf997ad
Cr-Commit-Position: refs/heads/master@{#394223}