Issue 298723011: Make 'will-change: contents' suppress compositing in subtree

Issue 298723011: Make 'will-change: contents' suppress compositing in subtree (Closed)

Created:
6 years, 7 months ago by ajuma

Modified:
6 years, 6 months ago

Reviewers:
Ian Vollick, esprehn, abarth-chromium

CC:
blink-reviews, blink-reviews-rendering, zoltan1, blink-reviews-css, eae+blinkwatch, ed+blinkwatch_opera.com, leviw+renderwatch, abarth-chromium, dglazkov+blink, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, pdr., rune+blink, rwlbuis, nduca, Vangelis Kokkevis, ernstm

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Make 'will-change: contents' suppress compositing in subtree The contents of 'will-change: contents' elements are expected to change frequently (e.g. every frame). Consequently, the creation of composited layers within a 'will-change: contents' subtree should be avoided where possible. This CL suppresses compositing for animations and for will-change in descendants of elements with 'will-change: contents'. BUG=365885 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175584

Patch Set 1 #

Total comments: 6

Patch Set 2 : Address review comments #

Patch Set 3 : Rebased #

Patch Set 4 : Rebased #

Total comments: 2

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+96 lines, -1 line)			Patch
A	LayoutTests/compositing/will-change/will-change-contents-suppresses-compositing.html	View	1	1 chunk	+74 lines, -0 lines	0 comments	Download
A +	LayoutTests/compositing/will-change/will-change-contents-suppresses-compositing-expected.txt	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
M	Source/core/css/resolver/StyleBuilderCustom.cpp	View	1 2	3 chunks	+3 lines, -0 lines	0 comments	Download
M	Source/core/rendering/compositing/CompositingReasonFinder.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/compositing/CompositingReasonFinder.cpp	View	1 2 3	2 chunks	+10 lines, -2 lines	2 comments	Download
M	Source/core/rendering/style/RenderStyle.h	View	1 2	2 chunks	+2 lines, -0 lines	0 comments	Download
M	Source/core/rendering/style/StyleRareInheritedData.h	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	Source/core/rendering/style/StyleRareInheritedData.cpp	View		3 chunks	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

esprehn

Note that this is going to cause setting will-change to contents to force a recalcStyle ...

6 years, 7 months ago (2014-05-23 00:03:04 UTC) #2

ajuma

On 2014/05/23 00:03:04, esprehn wrote: > Note that this is going to cause setting will-change ...

6 years, 7 months ago (2014-05-23 00:50:36 UTC) #3

esprehn

I don't think that initial logic works, we don't go through the initial setter for ...

6 years, 7 months ago (2014-05-23 01:05:58 UTC) #4

ajuma

https://codereview.chromium.org/298723011/diff/1/LayoutTests/compositing/will-change/will-change-contents-suppresses-compositing.html File LayoutTests/compositing/will-change/will-change-contents-suppresses-compositing.html (right): https://codereview.chromium.org/298723011/diff/1/LayoutTests/compositing/will-change/will-change-contents-suppresses-compositing.html#newcode3 LayoutTests/compositing/will-change/will-change-contents-suppresses-compositing.html:3: <head> On 2014/05/23 01:05:58, esprehn wrote: > We normally ...

6 years, 7 months ago (2014-05-23 14:38:31 UTC) #5

Ian Vollick

On 2014/05/23 14:38:31, ajuma wrote: > https://codereview.chromium.org/298723011/diff/1/LayoutTests/compositing/will-change/will-change-contents-suppresses-compositing.html > File > LayoutTests/compositing/will-change/will-change-contents-suppresses-compositing.html > (right): > > ...

6 years, 7 months ago (2014-05-27 15:03:49 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/298723011/60001

6 years, 6 months ago (2014-06-05 15:09:17 UTC) #11

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...

6 years, 6 months ago (2014-06-05 16:31:10 UTC) #12

abarth-chromium

https://codereview.chromium.org/298723011/diff/60001/Source/core/rendering/compositing/CompositingReasonFinder.cpp File Source/core/rendering/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/298723011/diff/60001/Source/core/rendering/compositing/CompositingReasonFinder.cpp#newcode113 Source/core/rendering/compositing/CompositingReasonFinder.cpp:113: if (requiresCompositingForAnimation(renderer)) Why not pass in the RenderStyle given ...

6 years, 6 months ago (2014-06-06 17:18:29 UTC) #14

ajuma

6 years, 6 months ago (2014-06-06 17:50:10 UTC) #15

Message was sent while issue was closed.

https://codereview.chromium.org/298723011/diff/60001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/298723011/diff/60001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:113: if
(requiresCompositingForAnimation(renderer))
On 2014/06/06 17:18:29, abarth wrote:
> Why not pass in the RenderStyle given that these are style-determined reasons?

Good point. CL up to fix this: https://codereview.chromium.org/317013005/

Expand Messages | Collapse Messages