Implement interest rects for synchronized paint.

Implement interest rects for synchronized paint.

BUG=536999
Committed: https://crrev.com/70a750f5e7427c3bed60e7c7adb231f68149f1a1
Cr-Commit-Position: refs/heads/master@{#353572}

[chrishtr, 2015-10-09 19:10:28]

chrishtr@chromium.org changed reviewers:
+ ajuma@chromium.org, wangxianzhu@chromium.org

[chrishtr, 2015-10-09 19:10:29]

Ali please review computeInterestRect for correctness.
Note that I still have not implemented the equivalent of
DisplayListRecordingSource::ExposesEnoughNewArea.

https://codereview.chromium.org/1393083003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/1393083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:71:
class PLATFORM_EXPORT CompositedLayerMapping final : public GraphicsLayerClient
{
This is needed to expose the class for testing across shared library boundaries.

[wkorman, 2015-10-09 19:46:30]

wkorman@chromium.org changed reviewers:
+ wkorman@chromium.org

[wkorman, 2015-10-09 19:46:32]

Reading to learn, minor self-edifying inquiries.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2477: if
(IntRect(interestRect) != LayoutRect::infiniteIntRect()) {
There's a LayoutRect::infiniteRect() that returns a LayoutRect and wouldn't need
IntRect(..) but maybe you need to do this for snapping and I see it's a testing
specific TODO anyway. Only mentioning since currently this will execute in main
path even outside tests.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2179:
// Now map the bounds to its visible content rect in screen space, including
applying clips along the way.
Where are the clips applied, is it basically just line 2185?

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2195:
localInterestRect.expand(IntRectOutsets(kPixelDistanceToRecord,
kPixelDistanceToRecord, kPixelDistanceToRecord, kPixelDistanceToRecord));
If we have no visible content rect, do we still need a kPixelDistanceToRecord
interest rect?

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:144:
fprintf(stderr, "\nTest:\n");
Need this and others still?

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.h (right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.h:45: virtual
void paintIfNeeded(GraphicsContext&) = 0;
Comment w/ some brief docs? Esp. re: this vs the other paint() methods. I sort
of feel this way about everything in the code base that says "xxxIfNeeded".

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h (right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h:71: virtual
void paintContentsIfNeeded(const GraphicsLayer*, GraphicsContext&,
GraphicsLayerPaintingPhase) const = 0;
Same re: docs/comment seem helpful.

[chrishtr, 2015-10-09 20:54:52]

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2477: if
(IntRect(interestRect) != LayoutRect::infiniteIntRect()) {
On 2015/10/09 at 19:46:31, wkorman wrote:
> There's a LayoutRect::infiniteRect() that returns a LayoutRect and wouldn't
need IntRect(..) but maybe you need to do this for snapping and I see it's a
testing specific TODO anyway. Only mentioning since currently this will execute
in main path even outside tests.

Done.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2179:
// Now map the bounds to its visible content rect in screen space, including
applying clips along the way.
On 2015/10/09 at 19:46:32, wkorman wrote:
> Where are the clips applied, is it basically just line 2185?

mapRectToPaintInvalidationBacking actually takes clips into account. It has a
bad name. Now is probably the time to fix the name as part of synchronized
paint.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2195:
localInterestRect.expand(IntRectOutsets(kPixelDistanceToRecord,
kPixelDistanceToRecord, kPixelDistanceToRecord, kPixelDistanceToRecord));
On 2015/10/09 at 19:46:32, wkorman wrote:
> If we have no visible content rect, do we still need a kPixelDistanceToRecord
interest rect?

Yes, or otherwise nothing will be recorded because it's empty. Also note that:
1. This matches what cc already does in
https://code.google.com/p/chromium/codesearch#chromium/src/cc/playback/displa...

2. It has known suboptimal behavior for some cases, such as the far-offscreen
example in the unittest, which should ideally not be painted at all.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:144:
fprintf(stderr, "\nTest:\n");
On 2015/10/09 at 19:46:32, wkorman wrote:
> Need this and others still?

Fixed. I think that's the only one.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.h (right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.h:45: virtual
void paintIfNeeded(GraphicsContext&) = 0;
On 2015/10/09 at 19:46:32, wkorman wrote:
> Comment w/ some brief docs? Esp. re: this vs the other paint() methods. I sort
of feel this way about everything in the code base that says "xxxIfNeeded".

Done.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h (right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h:71: virtual
void paintContentsIfNeeded(const GraphicsLayer*, GraphicsContext&,
GraphicsLayerPaintingPhase) const = 0;
On 2015/10/09 at 19:46:32, wkorman wrote:
> Same re: docs/comment seem helpful.

Done.

[ajuma, 2015-10-09 20:55:27]

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2198:
}
Do you have a sense of how efficient this approach is? On a 3d heavy page (e.g.
http://output.jsbin.com/giqafofe/1/quiet?JS_POSTER_CIRCLE), mapping each layer
to/from screen by iteratively mapping by each ancestor transform seems
potentially expensive.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:172:
}
Please also add a test where the transform to/from screen space is 3d.

[chrishtr, 2015-10-09 21:35:46]

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2198:
}
On 2015/10/09 at 20:55:27, ajuma wrote:
> Do you have a sense of how efficient this approach is? On a 3d heavy page
(e.g. http://output.jsbin.com/giqafofe/1/quiet?JS_POSTER_CIRCLE), mapping each
layer to/from screen by iteratively mapping by each ancestor transform seems
potentially expensive.

In that example it should be relatively cheap, because it is just walking up a
few layout tree nodes and aggregating a combined matrix. I expect it to be
sometimes/most of the time slower than cc though. In return for this short-term
overhead, we avoid a tree walk during paint invalidation, so I expect the total
frame time to be less. I intend to profile some pages once this code is in place
and working.

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/1393083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:172:
}
On 2015/10/09 at 20:55:27, ajuma wrote:
> Please also add a test where the transform to/from screen space is 3d.

Done.

[ajuma, 2015-10-09 22:03:55]

lgtm

https://codereview.chromium.org/1393083003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutScrollbar.cpp (right):

https://codereview.chromium.org/1393083003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutScrollbar.cpp:42: fprintf(stderr,
"custom scrollbar\n");
Leftover from debugging? :)

https://codereview.chromium.org/1393083003/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/1393083003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:115:
// It's rotated 90 degrees about the X axis, which means its visual content rect
is empty, and so the interest rect is the
Comment doesn't match test.

[chrishtr, 2015-10-09 22:28:19]

https://codereview.chromium.org/1393083003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutScrollbar.cpp (right):

https://codereview.chromium.org/1393083003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutScrollbar.cpp:42: fprintf(stderr,
"custom scrollbar\n");
On 2015/10/09 at 22:03:55, ajuma wrote:
> Leftover from debugging? :)

Oops, fixed.

https://codereview.chromium.org/1393083003/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/1393083003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:115:
// It's rotated 90 degrees about the X axis, which means its visual content rect
is empty, and so the interest rect is the
On 2015/10/09 at 22:03:55, ajuma wrote:
> Comment doesn't match test.

Fixed.

[chrishtr, 2015-10-09 22:28:24]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-10-09 22:28:25]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1393083003/#ps150001
(title: " ")

[commit-bot: I haz the power, 2015-10-09 22:30:38]

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

[commit-bot: I haz the power, 2015-10-10 00:51:53]

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

[commit-bot: I haz the power, 2015-10-10 00:51:54]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[chrishtr, 2015-10-10 00:59:06]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-10-10 00:59:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1393083003/#ps170001
(title: " ")

[commit-bot: I haz the power, 2015-10-10 01:00:39]

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

[commit-bot: I haz the power, 2015-10-10 02:37:56]

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

[commit-bot: I haz the power, 2015-10-10 02:37:57]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[chrishtr, 2015-10-12 17:41:48]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-10-12 17:41:49]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1393083003/#ps190001
(title: " ")

[commit-bot: I haz the power, 2015-10-12 17:42:46]

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

[commit-bot: I haz the power, 2015-10-12 19:24:06]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2015-10-12 19:25:15]

Patchset 11 (id:??) landed as
https://crrev.com/70a750f5e7427c3bed60e7c7adb231f68149f1a1
Cr-Commit-Position: refs/heads/master@{#353572}