Issue 498773007: Move some scroll invalidations to the paint invalidation phase

Issue 498773007: Move some scroll invalidations to the paint invalidation phase (Closed)

Created:
6 years, 4 months ago by Julien - ping for review

Modified:
6 years, 3 months ago

Reviewers:
leviw_travelin_and_unemployed, Ian Vollick, dsinclair, esprehn, abarth-chromium, wangxianzhu

CC:
blink-layers+watch_chromium.org, blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1

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

Project:
blink

Visibility:
Public.

More Reviews

Description

Move some scroll invalidations to the paint invalidation phase Scrolling used to generate immediate invalidations that are now postponed to the paint invalidation phase. In order to make this works, scrolling updates needs to to happen before paint invalidations. The test changes are either neutral (same amount of invalidations, except that we generate them one at a time) or compositing/repaint/fixed-pos-inside-composited-intermediate-layer is a progression. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181475

Patch Set 1 #

Patch Set 2 : Rebased and improved #

Total comments: 8

Patch Set 3 : Updated after the review comments #

Total comments: 2

Patch Set 4 : Patch updated after Dan's review. #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+32 lines, -92 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	LayoutTests/compositing/repaint/fixed-pos-inside-composited-intermediate-layer-expected.txt	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/compositing/repaint/fixed-pos-with-abs-pos-child-scroll-expected.txt	View		1 chunk	+4 lines, -2 lines	0 comments	Download
M	LayoutTests/compositing/repaint/fixed-pos-with-composited-child-expected.txt	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	LayoutTests/fast/repaint/fixed-scroll-simple-expected.txt	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	LayoutTests/platform/linux/fast/layers/scroll-descendant-with-cached-cliprects-expected.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/frame/FrameView.h	View	1 2 3	2 chunks	+0 lines, -2 lines	0 comments	Download
M	Source/core/frame/FrameView.cpp	View	1 2 3	7 chunks	+19 lines, -67 lines	0 comments	Download
M	Source/core/rendering/RenderLayerRepainter.h	View		1 chunk	+0 lines, -2 lines	0 comments	Download
M	Source/core/rendering/RenderLayerRepainter.cpp	View		1 chunk	+0 lines, -14 lines	0 comments	Download
M	Source/platform/scroll/ScrollView.h	View	1	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 19 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dsinclair

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameView.cpp#oldcode1284 Source/core/frame/FrameView.cpp:1284: InspectorInstrumentation::didScroll(page()); Does this still need to exist somewhere? https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameView.cpp ...

6 years, 3 months ago (2014-09-02 18:36:48 UTC) #4

esprehn

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameView.cpp#newcode1237 Source/core/frame/FrameView.cpp:1237: paintInvalidationRectIncludingNonCompositingDescendants(child); Instead of walking the whole tree recursively we ...

6 years, 3 months ago (2014-09-02 20:23:10 UTC) #8

Julien - ping for review

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameView.cpp#oldcode1284 Source/core/frame/FrameView.cpp:1284: InspectorInstrumentation::didScroll(page()); On 2014/09/02 at 18:36:48, dsinclair wrote: > Does ...

6 years, 3 months ago (2014-09-03 00:20:20 UTC) #11

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1284:
InspectorInstrumentation::didScroll(page());
On 2014/09/02 at 18:36:48, dsinclair wrote:
> Does this still need to exist somewhere?

I would believe it does, it's a bad merge :(

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1227: // FIXME: If we had a flag to force
invalidations in a whole subtree, we could get rid of this function.
On 2014/09/02 18:36:47, dsinclair wrote:
> crbug?

crbug.com/410097 (probably unneeded but at least it's better to have it than
not).

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1228: static void
paintInvalidationRectIncludingNonCompositingDescendants(const RenderLayer*
layer)
On 2014/09/02 18:36:48, dsinclair wrote:
> This method name doesn't seem to match up with what the method is doing. It
> isn't getting a paintInvalidationRect it's setting
> shouldDoFullPaintInvalidation.
> 
> setShouldDoFullPaintInvalidationIncludingNonCompositingDescendants ?

Fine with the alternative name.

https://codereview.chromium.org/498773007/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1237:
paintInvalidationRectIncludingNonCompositingDescendants(child);
On 2014/09/02 20:23:10, esprehn wrote:
> Instead of walking the whole tree recursively we should just have a bit that
> does a Force down the tree through the repaint invalidation tree walk. That's
> how style and compositing work and avoids this extra tree walk of all
> descendants.

The FIXME above exactly what covers this case :-)

I don't want to add this to the current patch as it's already complicated
enough. Also switching to using a new flag needs to cover
https://codereview.chromium.org/433603004 AFAICT so it adds too much complexity.

dsinclair

lgtm with nit. https://codereview.chromium.org/498773007/diff/40001/Source/core/frame/FrameView.h File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/498773007/diff/40001/Source/core/frame/FrameView.h#newcode389 Source/core/frame/FrameView.h:389: void updateFixedElementPaintInvalidationRectsAfterScroll(); Remove?

6 years, 3 months ago (2014-09-03 14:23:15 UTC) #12

Julien - ping for review

Updated patch for review. @abarth: I would like you to give a quick look at ...

6 years, 3 months ago (2014-09-03 22:37:45 UTC) #13

Julien - ping for review

This is blocking some other work so I need to land it.

6 years, 3 months ago (2014-09-05 17:11:20 UTC) #14

commit-bot: I haz the power

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

6 years, 3 months ago (2014-09-05 17:11:51 UTC) #16

commit-bot: I haz the power

Committed patchset #4 (id:60001) as 181475

6 years, 3 months ago (2014-09-05 17:16:53 UTC) #17

kouhei (in TOK)

A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/546713004/ by kouhei@chromium.org. ...

6 years, 3 months ago (2014-09-05 22:47:16 UTC) #18

kouhei (in TOK)

6 years, 3 months ago (2014-09-05 23:30:14 UTC) #19

Message was sent while issue was closed.

On 2014/09/05 22:47:16, kouhei wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/546713004/ by mailto:kouhei@chromium.org.
> 
> The reason for reverting is: The patch is likely the culprit for Mac IMAGE
> failures.
> Example:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@To....

Sorry. not reverting. More likely a skia roll.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages