Clear scrollbar damages in time

Clear scrollbar damages in time

When scrollbar is invalidated during layout, the damaged area is cached
so that the next invalidateTreeIfNeeded will pick up them and do real
invalidation.

However, when we do full view repaint we skip invalidation of scrollbar
damages, but leave the damage status not cleared.

Override RenderObject::clearPaintInvalidationState() and
RenderObject::paintInvalidationStateIsDirty() to handle clearing of
scrollbar damage status.

BUG=394833
TEST=fast/repaint/vertical-overflow-parent.html
TEST=fast/repaint/scrollbar-damage-and-full-viewport-repaint.html
R=jchaffraix@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179433

[Xianzhu, 2014-07-17 22:56:43]

This CL depends on https://codereview.chromium.org/398343003/.

Will update NeedsRebaselines later.

[Xianzhu, 2014-07-22 19:35:19]

This CL no longer depends on pending CLs.

PTAL.

[Xianzhu, 2014-07-22 19:47:57]

Split out the change of ScrollableArea. Will make another change to fix the
issues in it.

[dsinclair, 2014-07-22 19:55:22]

https://codereview.chromium.org/403593005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/403593005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1575: if (!view()->doingFullRepaint()) {
This is really confusing. I wonder if the resetScrollbarDamage shouldn't
actually be happening inside the clearPaintInvalidationState() method instead of
being done in here.

Then this chunk would stay after the invalidatePaintINeeded call.

[Xianzhu, 2014-07-23 00:18:13]

https://codereview.chromium.org/403593005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/403593005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1575: if (!view()->doingFullRepaint()) {
On 2014/07/22 19:55:22, dsinclair wrote:
> This is really confusing. I wonder if the resetScrollbarDamage shouldn't
> actually be happening inside the clearPaintInvalidationState() method instead
of
> being done in here.
> 
> Then this chunk would stay after the invalidatePaintINeeded call.

Done.

[dsinclair, 2014-07-23 03:06:34]

Thanks for the changes. This seems good to me, would like to get jchaffraix@ to
take a look as well.

Can you update the description? The last block is out of date with the current
code changes.

For the expectation changes, would it be possible to get a patch up without them
and some try results so we can see the differences? Are the removed
invalidations just due to us invalidating stale scrollbar information?

[Xianzhu, 2014-07-23 16:39:39]

On 2014/07/23 03:06:34, dsinclair wrote:
> Thanks for the changes. This seems good to me, would like to get jchaffraix@
to
> take a look as well.
> 
> Can you update the description? The last block is out of date with the current
> code changes.
> 

Done.

> For the expectation changes, would it be possible to get a patch up without
them
> and some try results so we can see the differences? Are the removed
> invalidations just due to us invalidating stale scrollbar information?

I'm running try bot with the expectation changes, so if they are not shown in
the results, the changes are correct.

I have verified that the removed invalidations were stale damages uncleared
since the last full viewport invalidation.

[Xianzhu, 2014-07-23 16:41:20]

Oh trybots don't work for this change because it depends on another pending
change (https://codereview.chromium.org/408273003/).

[Xianzhu, 2014-08-01 17:50:57]

The latest patch set no longer depends on
https://codereview.chromium.org/408273003/.

[Julien - ping for review, 2014-08-01 18:13:45]

https://codereview.chromium.org/403593005/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/403593005/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1614: if (enclosingLayer()) {
I missed this in our current code: we will end up invalidating / clearing the
same scrollbars several times as enclosingLayer can return the same RenderLayer
in a subtree.

It also potentially makes this algorithm supra-linear as we walk up the tree :-(

[Xianzhu, 2014-08-01 18:20:02]

https://codereview.chromium.org/403593005/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/403593005/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1614: if (enclosingLayer()) {
On 2014/08/01 18:13:45, Julien Chaffraix - PST wrote:
> I missed this in our current code: we will end up invalidating / clearing the
> same scrollbars several times as enclosingLayer can return the same
RenderLayer
> in a subtree.
> 
> It also potentially makes this algorithm supra-linear as we walk up the tree
:-(

Thanks for catching this. I think we should use hasLayer() here. Will try.

[Julien - ping for review, 2014-08-01 19:05:02]

lgtm

https://codereview.chromium.org/403593005/diff/140001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/scrollbar-damage-and-full-viewport-repaint.html
(right):

https://codereview.chromium.org/403593005/diff/140001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/scrollbar-damage-and-full-viewport-repaint.html:39:
</div>
Let's add a description of *what* we are testing.

https://codereview.chromium.org/403593005/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/403593005/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1600: if (hasLayer()) {
Better even:
if (RenderLayer* layer = this->layer()) {
   if (RenderLayerScrollableArea* area = layer->scrollableArea()) {

or even better: let's add a scrollableArea() function to RenderBox to that would
do it for us.

https://codereview.chromium.org/403593005/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1614: if (hasLayer()) {
Ditto.

https://codereview.chromium.org/403593005/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1623: if (hasLayer()) {
Ditto.

[Xianzhu, 2014-08-01 22:07:23]

All layout tests produced the same results as before changing enclosingLayer to
layer.

https://codereview.chromium.org/403593005/diff/140001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/scrollbar-damage-and-full-viewport-repaint.html
(right):

https://codereview.chromium.org/403593005/diff/140001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/scrollbar-damage-and-full-viewport-repaint.html:39:
</div>
On 2014/08/01 19:05:01, Julien Chaffraix - PST wrote:
> Let's add a description of *what* we are testing.

Done.

https://codereview.chromium.org/403593005/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/403593005/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1600: if (hasLayer()) {
On 2014/08/01 19:05:02, Julien Chaffraix - PST wrote:
> Better even:
> if (RenderLayer* layer = this->layer()) {
>    if (RenderLayerScrollableArea* area = layer->scrollableArea()) {
> 
> or even better: let's add a scrollableArea() function to RenderBox to that
would
> do it for us.

Just found we already have RenderLayerModelObject::scrollableArea(). Now using
it.

https://codereview.chromium.org/403593005/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1614: if (hasLayer()) {
On 2014/08/01 19:05:02, Julien Chaffraix - PST wrote:
> Ditto.

Done.

https://codereview.chromium.org/403593005/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1623: if (hasLayer()) {
On 2014/08/01 19:05:02, Julien Chaffraix - PST wrote:
> Ditto.

Done.

[Xianzhu, 2014-08-01 22:09:21]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-08-01 22:09:35]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/403593005/160001

[Xianzhu, 2014-08-01 23:40:14]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2014-08-01 23:40:20]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-08-01 23:40:35]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/403593005/160001

[commit-bot: I haz the power, 2014-08-02 00:58:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20239)

[commit-bot: I haz the power, 2014-08-02 01:57:20]

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

[commit-bot: I haz the power, 2014-08-02 01:57:21]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20247)

[Xianzhu, 2014-08-02 02:21:22]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-08-02 02:22:11]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/403593005/160001

[commit-bot: I haz the power, 2014-08-02 03:03:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20256)

[Xianzhu, 2014-08-02 03:23:09]

Committed patchset #9 manually as 179433 (presubmit successful).