Fix scrollbar damage accumulation issue

Fix scrollbar damage accumulation issue

Added ScrollableArea::addScrollbarDamage() to accumulate scrollbar
damage rects.

BUG=394833
TEST=fast/repaint/scrollbar-parts.html

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

[Julien - ping for review, 2014-07-23 20:46:23]

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

https://codereview.chromium.org/418483004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerScrollableArea.cpp:210:
box().setMayNeedPaintInvalidation(true);
I don't really like this change. If we need to generate invalidation for
scrollbars that approach in https://codereview.chromium.org/403593005/ (patchset
3) was better. Alternatively shouldCheckForPaintInvalidation should be patched.

https://codereview.chromium.org/418483004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerScrollableArea.cpp:210:
box().setMayNeedPaintInvalidation(true);
I don't really like this change. If we need to generate invalidation for
scrollbars that approach in https://codereview.chromium.org/403593005/ (patchset
3) was better. Alternatively shouldCheckForPaintInvalidation should be patched.

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:212: void addScrollbarDamage(Scrollbar*
scrollbar, const IntRect& rect)
As discussed, the code seems to assume a single invalidation rectangle so it's
not really addScrollbardDamage, more a setScrollbarDamage.

As we make this change, we should add some
ASSERT_RELEASE(m_horizontalBarDamage.isEmpty()) in the horizontalBarDamage
branch. Same for vertical scrollbars.

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:220: void resetScrollbarDamage()
I think we should remove this function.

All callers do:
- invalidate horizontal scrollbar.
- invalidate vertical scrollbar.
- resetScrollbarDamage.

That means that we should just have an API (invalidateScrollbars()) that takes
care of all the previous steps so that the callers doesn't have to take care of
it.

[Xianzhu, 2014-07-23 22:45:31]

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:212: void addScrollbarDamage(Scrollbar*
scrollbar, const IntRect& rect)
On 2014/07/23 20:46:23, Julien Chaffraix - PST wrote:
> As discussed, the code seems to assume a single invalidation rectangle so it's
> not really addScrollbardDamage, more a setScrollbarDamage.
> 
> As we make this change, we should add some
> ASSERT_RELEASE(m_horizontalBarDamage.isEmpty()) in the horizontalBarDamage
> branch. Same for vertical scrollbars.

Just find a case that a scrollbar may be invalidated several times with
different rects.

The call stack (reproduced on Linux by undefining THUMB_POSITION_AFFECTS_BUTTONS
in Scrollbar.cpp is):
  RenderLayerScrollableArea::invalidateScrollbarRect()
  Scrollbar::invalidateRect()
  ScrollbarTheme::invalidatePart() <-- Called 3 times
  ScrollbarTheme::invalidateParts(ForwardTrackPart | BackTrackPart | ThumbPart)
  Scrollbar::updateThumb()
  Scrollbar::updateThumbPosition()
  ...

[Xianzhu, 2014-07-23 23:55:01]

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:220: void resetScrollbarDamage()
On 2014/07/23 20:46:23, Julien Chaffraix - PST wrote:
> I think we should remove this function.
> 
> All callers do:
> - invalidate horizontal scrollbar.
> - invalidate vertical scrollbar.
> - resetScrollbarDamage.
> 
> That means that we should just have an API (invalidateScrollbars()) that takes
> care of all the previous steps so that the callers doesn't have to take care
of
> it.

That seems not feasible because the two callers (FrameView and RenderBox) have
different methods to invalidate rects. ScrollableArea doesn't (and should not)
know how to invalidate an area in its container.

In addition, I'd also like to make the damage flags be part of
paintInvalidationState, so that paintInvalidationStateIsDirty can check it.

[Xianzhu, 2014-07-24 00:09:36]

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/418483004/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:220: void resetScrollbarDamage()
There is also a case that we need to invalidate and reset the damages
separately: when view is doing full repaint, we should not invalidate the
damages, but need to reset them.

I think this is also a reason that Dan and I wanted to make the damages be a
part of paintInvalidationState.

[Xianzhu, 2014-07-24 00:33:48]

Because of reasons mentioned previously, I still keep resetScrollbarDamages().

PTAL.

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

https://codereview.chromium.org/418483004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerScrollableArea.cpp:210:
box().setMayNeedPaintInvalidation(true);
On 2014/07/23 20:46:23, Julien Chaffraix - PST wrote:
> I don't really like this change. If we need to generate invalidation for
> scrollbars that approach in https://codereview.chromium.org/403593005/
(patchset
> 3) was better. Alternatively shouldCheckForPaintInvalidation should be
patched.

Removed the above statement.

[Xianzhu, 2014-07-24 17:35:04]

Another case of multiple invalidateScrollbarRects in one frame cycle is:
scrollbar partial invalidation is exposed through PPAPI, so PPAPI client may
call the function multiple times.

[dsinclair, 2014-07-25 14:33:45]

This makes sense to me, jchaffraix, with the above explanation, are you happy
with this?

https://codereview.chromium.org/418483004/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/418483004/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:263: // For repaint after layout, stores
the damage to be repainted for the
Since repaint-after-layout is the only invalidation scheme, let's re-write this.
To just be:

Stores the paint invalidations for the scrollbars.

[Julien - ping for review, 2014-07-25 17:20:10]

lgtm

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/scrollbar-parts.html (right):

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scrollbar-parts.html:9: runRepaintTest();
While this works, I would argue to run it off the 'load' event as it matches the
other code and is more bullet proof if we change how runRepaintTest works.

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scrollbar-parts.html:13: should be properly repainted
when the contents change size. -->
We should dump this into the text output as we are doing a text dump.

[Xianzhu, 2014-07-25 17:28:25]

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/scrollbar-parts.html (right):

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scrollbar-parts.html:9: runRepaintTest();
On 2014/07/25 17:20:10, Julien Chaffraix - PST wrote:
> While this works, I would argue to run it off the 'load' event as it matches
the
> other code and is more bullet proof if we change how runRepaintTest works.

Done. Also just noticed this might cause flakiness if load is slow for a big
test (a frame may be generated during load).

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scrollbar-parts.html:13: should be properly repainted
when the contents change size. -->
On 2014/07/25 17:20:10, Julien Chaffraix - PST wrote:
> We should dump this into the text output as we are doing a text dump.

Currently text-based-repaint dump the layer tree only, not including text in the
test. Also the text causes platform rebaselines because of different text sizes
and I'd like to avoid that.

https://codereview.chromium.org/418483004/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/418483004/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:263: // For repaint after layout, stores
the damage to be repainted for the
On 2014/07/25 14:33:45, dsinclair wrote:
> Since repaint-after-layout is the only invalidation scheme, let's re-write
this.
> To just be:
> 
> Stores the paint invalidations for the scrollbars.

Done.

[Xianzhu, 2014-07-25 17:28:32]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-07-25 17:29:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/418483004/50001

[commit-bot: I haz the power, 2014-07-25 18:40:22]

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

[Xianzhu, 2014-07-25 19:00:55]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-07-25 19:00:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/418483004/70001

[commit-bot: I haz the power, 2014-07-25 20:09:15]

Change committed as 178952

[Julien - ping for review, 2014-07-25 20:24:44]

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/scrollbar-parts.html (right):

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scrollbar-parts.html:13: should be properly repainted
when the contents change size. -->
On 2014/07/25 17:28:25, Xianzhu wrote:
> On 2014/07/25 17:20:10, Julien Chaffraix - PST wrote:
> > We should dump this into the text output as we are doing a text dump.
> 
> Currently text-based-repaint dump the layer tree only, not including text in
the
> test. Also the text causes platform rebaselines because of different text
sizes
> and I'd like to avoid that.

I don't think that's a good argument. I am not concerned about the text output
here but someone trying to understand what's going wrong if this test output
changes. If you set the font and font size explicitly that should prevent the
font issue.

[Xianzhu, 2014-07-25 20:54:58]

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/scrollbar-parts.html (right):

https://codereview.chromium.org/418483004/diff/40001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scrollbar-parts.html:13: should be properly repainted
when the contents change size. -->
On 2014/07/25 20:24:44, Julien Chaffraix - PST wrote:
> On 2014/07/25 17:28:25, Xianzhu wrote:
> > On 2014/07/25 17:20:10, Julien Chaffraix - PST wrote:
> > > We should dump this into the text output as we are doing a text dump.
> > 
> > Currently text-based-repaint dump the layer tree only, not including text in
> the
> > test. Also the text causes platform rebaselines because of different text
> sizes
> > and I'd like to avoid that.
> 
> I don't think that's a good argument. I am not concerned about the text output
> here but someone trying to understand what's going wrong if this test output
> changes. If you set the font and font size explicitly that should prevent the
> font issue.

Setting font and font size won't work because the physical font metrics
differences, but 'div style="height: xxx"' should work. Will use the method in
later changes. Might also change existing tests to reduce platform rebaselines.