Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(690)

Issue 1896893004: Hook up style invalidation for CSS Paint API. (Closed)

Created:
4 years, 8 months ago by ikilpatrick
Modified:
4 years, 7 months ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, Eric Willigers, haraken, rjwright, rwlbuis, shans
Base URL:
https://chromium.googlesource.com/chromium/src.git@css-paint-register
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : fix component build. #

Total comments: 11

Patch Set 3 : rebase. #

Patch Set 4 : disable prefixes. #

Patch Set 5 : disallow -prefixed. #

Patch Set 6 : rebase #

Patch Set 7 : rebase. #

Patch Set 8 : rebase off HEAD #

Patch Set 9 : . #

Total comments: 8

Patch Set 10 : new pending image approach. #

Patch Set 11 : . #

Total comments: 5

Patch Set 12 : paint invalidation tests! #

Patch Set 13 : Vector -> OwnPtr<Vector> #

Patch Set 14 : Redo CSSParserToken::operator==() #

Total comments: 24

Patch Set 15 : address comments. #

Total comments: 6

Patch Set 16 : address comments. #

Patch Set 17 : rebase #

Patch Set 18 : rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+712 lines, -396 lines) Patch
A third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +41 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/csspaint/invalidation-background-image-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +107 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/csspaint/invalidation-border-image.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +43 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/csspaint/invalidation-border-image-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +107 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/csspaint/invalidation-content-image.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +40 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/csspaint/invalidation-content-image-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +107 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/csspaint/registerPaint.html View 1 2 3 2 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/csspaint/registerPaint-expected.txt View 1 2 3 1 chunk +4 lines, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/csspaint/resources/test-runner-invalidation-logging.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +96 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/make_css_property_names.py View 1 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/animation/css/CSSAnimationUpdate.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
D third_party/WebKit/Source/core/animation/css/CSSPropertyEquality.h View 1 chunk +0 lines, -23 lines 0 comments Download
D third_party/WebKit/Source/core/animation/css/CSSPropertyEquality.cpp View 1 chunk +0 lines, -340 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSPaintImageGenerator.h View 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSPaintValue.h View 1 2 3 4 5 6 7 8 9 1 chunk +10 lines, -1 line 0 comments Download
A + third_party/WebKit/Source/core/css/CSSPropertyEquality.h View 0 chunks +-1 lines, --1 lines 0 comments Download
A + third_party/WebKit/Source/core/css/CSSPropertyEquality.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +6 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/css/parser/CSSVariableParser.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/css/resolver/ElementStyleResources.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp View 1 2 3 4 5 6 7 8 9 10 6 chunks +16 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/core/style/ComputedStyle.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/ComputedStyle.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +47 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp View 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleImage.h View 3 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/StylePendingImage.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleRareNonInheritedData.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h View 5 chunks +8 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp View 1 2 3 4 5 1 chunk +5 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h View 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.cpp View 1 chunk +12 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp View 1 2 3 4 3 chunks +21 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/csspaint/README.md View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 42 (10 generated)
ikilpatrick
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/Source/core/css/CSSPaintValue.h File third_party/WebKit/Source/core/css/CSSPaintValue.h (right): ...
4 years, 8 months ago (2016-04-19 22:31:26 UTC) #4
alancutter (OOO until 2018)
On 2016/04/19 at 22:31:26, ikilpatrick wrote: > Note dependent on: https://codereview.chromium.org/1866623002#ps140001 > > alan: Does ...
4 years, 8 months ago (2016-04-20 01:16:04 UTC) #5
esprehn
Hmm I don't think you want a first paint concept. You want to just diff(oldInputs, ...
4 years, 8 months ago (2016-04-20 04:40:06 UTC) #6
ikilpatrick
On 2016/04/20 04:40:06, esprehn wrote: > Hmm I don't think you want a first paint ...
4 years, 8 months ago (2016-04-21 23:03:38 UTC) #7
ikilpatrick
Rebased of committed CSS Paint API. +rune/elliott it probably makes sense if one of you ...
4 years, 8 months ago (2016-04-27 00:45:31 UTC) #10
ikilpatrick
On 2016/04/27 00:45:31, ikilpatrick wrote: > Rebased of committed CSS Paint API. > > +rune/elliott ...
4 years, 8 months ago (2016-04-27 00:48:29 UTC) #11
rune
I've looked through and commented on the visual invalidation diff code. The direction looks good ...
4 years, 7 months ago (2016-04-27 11:34:34 UTC) #12
rune
Oops. Reviewed the wrong patch set, but the code I commented on hasn't changed, I ...
4 years, 7 months ago (2016-04-27 11:44:04 UTC) #13
chrishtr
How about checking that paint invalidations also happen, in addition to style invalidations? https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html File ...
4 years, 7 months ago (2016-04-27 19:52:41 UTC) #14
ikilpatrick
Chatted with esphren in person the other day, suggested making use the PendingImage codepath instead. ...
4 years, 7 months ago (2016-04-28 22:30:17 UTC) #15
chrishtr
On 2016/04/28 at 22:30:17, ikilpatrick wrote: > Chatted with esphren in person the other day, ...
4 years, 7 months ago (2016-04-29 00:24:16 UTC) #16
chrishtr
https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp (right): https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp#newcode78 third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:78: if (propertyID == CSSPropertyInvalid) { On 2016/04/28 at 22:30:16, ...
4 years, 7 months ago (2016-04-29 00:24:58 UTC) #17
chrishtr
https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html (right): https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html#newcode20 third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:20: { property: 'max-height', value: '160px' }, On 2016/04/28 at ...
4 years, 7 months ago (2016-04-29 00:25:34 UTC) #18
Timothy Loh
https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp (right): https://codereview.chromium.org/1896893004/diff/160001/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp#newcode78 third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:78: if (propertyID == CSSPropertyInvalid) { On 2016/04/28 22:30:16, ikilpatrick ...
4 years, 7 months ago (2016-04-29 01:20:25 UTC) #19
rune
https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Source/core/style/ComputedStyle.cpp File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Source/core/style/ComputedStyle.cpp#newcode794 third_party/WebKit/Source/core/style/ComputedStyle.cpp:794: if (thisVar->tokenRange().serialize() != otherVar->tokenRange().serialize()) Could you implement an operator==() ...
4 years, 7 months ago (2016-04-29 08:06:37 UTC) #20
ikilpatrick
@chris, updated tests to check in paint invalidations. @tim, what are your thoughts on operator==() ...
4 years, 7 months ago (2016-04-29 22:42:01 UTC) #22
chrishtr
lgtm
4 years, 7 months ago (2016-04-30 00:09:14 UTC) #23
rune
On 2016/04/29 22:42:01, ikilpatrick wrote: > https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h#newcode156 > third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:156: > Vector<Persistent<StyleImage>> m_paintImages; > On 2016/04/29 ...
4 years, 7 months ago (2016-05-02 14:20:32 UTC) #24
ikilpatrick
On 2016/05/02 14:20:32, rune wrote: > On 2016/04/29 22:42:01, ikilpatrick wrote: > > > > ...
4 years, 7 months ago (2016-05-02 23:24:49 UTC) #25
esprehn
If we're actually worried about the size of these things we should probably do some ...
4 years, 7 months ago (2016-05-03 03:06:30 UTC) #26
esprehn
If we're actually worried about the size of these things we should probably do some ...
4 years, 7 months ago (2016-05-03 03:06:30 UTC) #27
Timothy Loh
https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Source/core/style/ComputedStyle.cpp File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1896893004/diff/200001/third_party/WebKit/Source/core/style/ComputedStyle.cpp#newcode794 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: ...
4 years, 7 months ago (2016-05-03 08:01:39 UTC) #28
ikilpatrick
Ok, updated CSSParserToken::operator== to actually do a string comparison, instead of just ptr comparisons. Created ...
4 years, 7 months ago (2016-05-03 23:56:18 UTC) #29
Timothy Loh
Code change looks fine. I haven't looked at the tests yet. https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html (right): ...
4 years, 7 months ago (2016-05-09 07:07:07 UTC) #30
esprehn
https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html (right): https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html#newcode3 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/Source/core/css/parser/CSSParserToken.cpp File third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp ...
4 years, 7 months ago (2016-05-09 21:59:54 UTC) #31
ikilpatrick
https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html File third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html (right): https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html#newcode3 third_party/WebKit/LayoutTests/csspaint/invalidation-background-image.html:3: <head> On 2016/05/09 21:59:54, esprehn wrote: > leave out ...
4 years, 7 months ago (2016-05-10 20:39:38 UTC) #32
Timothy Loh
https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp File third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp (right): https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp#newcode152 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: ...
4 years, 7 months ago (2016-05-11 06:20:37 UTC) #33
ikilpatrick
https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp File third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp (right): https://codereview.chromium.org/1896893004/diff/260001/third_party/WebKit/Source/core/css/parser/CSSParserToken.cpp#newcode152 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 ...
4 years, 7 months ago (2016-05-11 23:59:12 UTC) #34
Timothy Loh
lgtm
4 years, 7 months ago (2016-05-12 07:14:59 UTC) #35
commit-bot: I haz the power
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
4 years, 7 months ago (2016-05-17 20:03:16 UTC) #38
commit-bot: I haz the power
Committed patchset #18 (id:340001)
4 years, 7 months ago (2016-05-17 20:57:24 UTC) #40
commit-bot: I haz the power
4 years, 7 months ago (2016-05-17 21:00:17 UTC) #42
Message was sent while issue was closed.
Patchset 18 (id:??) landed as
https://crrev.com/0ab7ac1ec67da157bc4ef5cbcb1b10530bf997ad
Cr-Commit-Position: refs/heads/master@{#394223}

Powered by Google App Engine
This is Rietveld 408576698