Use recomputed interest rect only if it changed enough

Use recomputed interest rect only if it changed enough

Previously if there is any invalidation, we'll use recomputed interest
rect even if hasn't changed enough. This causes unnecessary interest
rect change and inconsistent interest rects between blink and cc.

Now use recomputed interest rect only if it changed enough.

We also need to save previous interest rect in GraphicsLayers instead
of in CompositedLayerMapping because one CompositedLayerMaping may have
multiple GraphicsLayers which have different interest rects.

BUG=553281
Committed: https://crrev.com/affad41e8c7d2f81f33ed31af4903b6fbdd05a73
Cr-Commit-Position: refs/heads/master@{#360116}

[Xianzhu, 2015-11-14 00:36:16]

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

[Xianzhu, 2015-11-14 00:36:17]

https://codereview.chromium.org/1441973003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1441973003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2277:
previousInterestRect.x(), previousInterestRect.y(),
previousInterestRect.width(), previousInterestRect.height());
The message indicates that there is performance penalty for blink's not
correctly predicting cc's next interest rect, but the paint result should be
correct because we'll repaint.

[enne (OOO), 2015-11-16 19:17:43]

What is "corrupted painting"?

If Blink is going to generate the recordings and the interest rects, the right
answer seems to be to tell cc::Layers what those interest rects are so they know
what the valid recorded region is.  I don't think it's worth trying to work
around two different systems fighting each other.

[chrishtr, 2015-11-16 19:22:11]

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

https://codereview.chromium.org/1441973003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:53:
do { \
Just use { } and skip the do and while keywords? Does that not work for some
reason?

https://codereview.chromium.org/1441973003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:319:
EXPECT_RECT_EQ(IntRect(0, 1400, 800, 8600),
rootScrollingLayer->previousInterestRect());
Why is it now taller than 600 + 4000 pixels?

https://codereview.chromium.org/1441973003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:327:
// Partial invalidation and repaint should not change previous paint interest
rect.
So at this point the value of computeInterestRect is not the same as the
interest rect stored in the  GraphicsLayer, but not different enough to change
the interest rect? This should be tested explicitly.

[chrishtr, 2015-11-16 19:23:01]

On 2015/11/16 at 19:17:43, enne wrote:
> What is "corrupted painting"?
> 
> If Blink is going to generate the recordings and the interest rects, the right
answer seems to be to tell cc::Layers what those interest rects are so they know
what the valid recorded region is.  I don't think it's worth trying to work
around two different systems fighting each other.

Agreed, that API change needs to be implemented. I can do that.

[jbroman, 2015-11-16 19:26:00]

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

https://codereview.chromium.org/1441973003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:53:
do { \
On 2015/11/16 at 19:22:11, chrishtr wrote:
> Just use { } and skip the do and while keywords? Does that not work for some
reason?

Drive-by: This is a common C pattern, useful because it will do the right thing
when it's the argument of a conditional, loop, etc.

Consider:

if (someCondition)
  EXPECT_RECT_EQ(expected, actual);

In this case, the version without do-while will only have the first statement
contained by the condition.
see, e.g.,
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Multi-statement_Macro

[Xianzhu, 2015-11-16 22:20:28]

Description was changed from

==========
Keep blink side and cc side interest rects in sync

If they are not in sync, the assumption of which tiles are invalid
at cc side and which are is painted at blink side will mismatch and
cause corrupted painting.

We tried to keep them in sync but failed when something at blink
side is invalidated. In the case we use the recomputed default
interest rect, while cc may still use the previous interest rect
if recomputed interest rect didn't change enough. We should use the
previous interest rect too if recomputed interest didn't change enough.

We also need to save previous interest rect in GraphicsLayers instead
of in CompositedLayerMapping because one CompositedLayerMaping may have
multiple GraphicsLayers which have different interest rects.

BUG=553281
==========

to

==========
Use recomputed interest rect only if it changed enough

Previously if there is any invalidation, we'll use recomputed interest
rect even if hasn't changed enough. This causes unnecessary interest
rect change and inconsistent interest rects between blink and cc.

Now use recomputed interest rect only if it changed enough.

We also need to save previous interest rect in GraphicsLayers instead
of in CompositedLayerMapping because one CompositedLayerMaping may have
multiple GraphicsLayers which have different interest rects.

BUG=553281
==========

[Xianzhu, 2015-11-16 22:21:39]

Updated the CL description. This CL and the change of cc interface about
interest rect can be independent.

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

https://codereview.chromium.org/1441973003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:319:
EXPECT_RECT_EQ(IntRect(0, 1400, 800, 8600),
rootScrollingLayer->previousInterestRect());
On 2015/11/16 19:22:11, chrishtr wrote:
> Why is it now taller than 600 + 4000 pixels?

(0,1400 800x8600) is the interest rect when the interest rect start to reach the
bottom. It's expanded from the view rect (0,5400 800x600) by 4000 pixels to the
top and the bottom.

When the view continues to scroll to the bottom, the new interest rects becomes
shorter. We'll still use (0,1400 800x8600) because it contains the new interest
rects.

https://codereview.chromium.org/1441973003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:327:
// Partial invalidation and repaint should not change previous paint interest
rect.
On 2015/11/16 19:22:11, chrishtr wrote:
> So at this point the value of computeInterestRect is not the same as the
> interest rect stored in the  GraphicsLayer, but not different enough to change
> the interest rect? This should be tested explicitly.

The test case is to cover the bug case: previously when there is invalidation,
we always use the recomputed interest rect despite how much the rect changed.

Split the test into 2 (InterestRectChangeOnScroll and
InterestRectShouldNotChangeOnInvalidation) to make it clearer.

[chrishtr, 2015-11-16 23:29:40]

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

https://codereview.chromium.org/1441973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2262:
IntRect previousInterestRect = graphicsLayer->previousInterestRect();
If there were a method computeInterestRect and
interestRectChangedEnoughToRepaint on GraphicsLayerClient, all this logic could
be moved into GraphicsLayer, then this method would not have to take an optional
interestRect argument and return a value.

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

https://codereview.chromium.org/1441973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:357:
// Paint invalidation and repaint should not change previous paint interest
rect.
Add a note that if the interest rect were computed, it would be [whatever it is]

[Xianzhu, 2015-11-17 00:42:11]

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

https://codereview.chromium.org/1441973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2262:
IntRect previousInterestRect = graphicsLayer->previousInterestRect();
On 2015/11/16 23:29:40, chrishtr wrote:
> If there were a method computeInterestRect and
> interestRectChangedEnoughToRepaint on GraphicsLayerClient, all this logic
could
> be moved into GraphicsLayer, then this method would not have to take an
optional
> interestRect argument and return a value.

Done.

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

https://codereview.chromium.org/1441973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:357:
// Paint invalidation and repaint should not change previous paint interest
rect.
On 2015/11/16 23:29:40, chrishtr wrote:
> Add a note that if the interest rect were computed, it would be [whatever it
is]

Added EXPECT_RECT_EQs for recomputed interest rects.

[chrishtr, 2015-11-17 00:47:54]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-17 00:47:54]

lgtm

[commit-bot: I haz the power, 2015-11-17 00:50:24]

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

[commit-bot: I haz the power, 2015-11-17 01:07:56]

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

[commit-bot: I haz the power, 2015-11-17 01:07:56]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Xianzhu, 2015-11-17 17:19:24]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-11-17 17:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1441973003/#ps80001
(title: "Fix release builds")

[commit-bot: I haz the power, 2015-11-17 17:20:18]

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

[commit-bot: I haz the power, 2015-11-17 18:19:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-17 18:20:05]

Patchset 5 (id:??) landed as
https://crrev.com/affad41e8c7d2f81f33ed31af4903b6fbdd05a73
Cr-Commit-Position: refs/heads/master@{#360116}