This should be a P2 not a P1. 
BTW, I cannot repro this on ChromeOS btw on M65.

bokan@: Have you had a chance to look at the trace? From the description tt seems we have unnecessary impl side animation running e.g., for smooth scrolling or perhaps for scrollbar animation?

I haven't yet but I'll take a look today.

I also repro'd this on Mac.

There haven't been any recent changes here that I can think of.

Re: ChromeOS, since 65 we use compositor-only "paint-once" overlay scrollbars so that makes sense.

It's expected that wheel scrolling will cause animations for smooth scrolls. Animating the offset will cause a NeedsCommit so that the scroll can be synchronized to the main thread so that too is expected - that shouldn't on its own cause a repaint.

The ScrollbarTheme paint code doesn't actually get called repeatedly so I don't think we're actually repainting. I'll take a closer look at where the paint-flash is coming from tomorrow. 

So the flash happens because we call SetContentsNeedsDisplay on the scrollbar layer after a scroll offset changes.

#2 0x7fffee8724f2 cc::Layer::SetNeedsDisplayRect()
#3 0x7fffde66cdb1 cc::Layer::SetNeedsDisplay()
#4 0x7fffde8343e1 blink::GraphicsLayer::SetContentsNeedsDisplay()
#5 0x7fffde95fbcc blink::ScrollableArea::SetScrollbarNeedsPaintInvalidation()
#6 0x7fffde962441 blink::Scrollbar::SetNeedsPaintInvalidation()
#7 0x7fffde96279c blink::Scrollbar::OffsetDidChange()
#8 0x7fffde95e893 blink::ScrollableArea::ScrollOffsetChanged()
#9 0x7fffde95e59c blink::ScrollableArea::SetScrollOffset()
#10 0x7fffde960a32 blink::ScrollableArea::DidScroll()
#11 0x7fffe1bc1074 blink::PaintLayerScrollableArea::DidScroll()
#12 0x7fffe1ac8831 blink::ScrollingCoordinator::DidScroll()
<< COMPOSITOR COMMIT >>

This doesn't cause us to repaint any of the scrollbar parts (Scrollbar::{Thumb|Track}NeedsRepaint() are false) and it looks to me like scrollbars raster when they paint so all we need to do is cause a draw the scrollbar layer. Unfortunately my understanding of how CC turns layers to pixels isn't great here, is this call needed to ensure the layer gets drawn (i.e. AppendQuads gets called)? If so, show-paint-rects tracks the wrong thing for scrollbar layers. FWIW, removing this call from ScrollableArea::SetScrollbarNeedsPaintInvalidation didn't affect scrollbars, both from compositor based scrolls and Blink-based JS scrolls.

This will contribute to the frames damage rect which controls what we swap in partial swap. A property change also does the same, for the bounds of the layer. Quads are produced for the whole viewport regardless of the damage.

> Re: ChromeOS, since 65 we use compositor-only "paint-once" overlay scrollbars so that makes sense.

Do you know what is preventing us from doing the same on Windows & Mac?

> It's expected that wheel scrolling will cause animations for smooth scrolls. Animating the offset
> will cause a NeedsCommit so that the scroll can be synchronized to the main thread so that too is
> expected - that shouldn't on its own cause a repaint.
> 
> The ScrollbarTheme paint code doesn't actually get called repeatedly so I don't think we're
> actually repainting. I'll take a closer look at where the paint-flash is coming from tomorrow. 

I think our concern wasn't only that we're painting, but that there's significant main thread
execution running it's lifecycle, when we expect accelerated scrolling.  There's also tree Commit
+ Activation cost on the compositor thread.

#8 The root cause seems to be this bit of code:

void Scrollbar::SetNeedsPaintInvalidation(ScrollbarPart invalid_parts) {
  if (theme_.ShouldRepaintAllPartsOnInvalidation())
    invalid_parts = kAllParts;
  if (invalid_parts & ~kThumbPart)
    track_needs_repaint_ = true;
  if (invalid_parts & kThumbPart)
    thumb_needs_repaint_ = true;
  if (scrollable_area_)
    scrollable_area_->SetScrollbarNeedsPaintInvalidation(Orientation());
}

The last part invalidates the entire scrollbar even if invalid_parts is kNoPart.

Changing the condition to scrollable_area && invalid_parts != kNoPart seems to work as far as getting rid of commit (and activation) and the scrollbar thumb paint flashing, but there's still a lot of work on the main thread that seems unnecessary.

Deleted: trace_wikipedia_scrollbar_skip_invalidation.json.gz 1.2 MB

trace_wikipedia_scrollbar_skip_invalidation.json.gz 1.2 MB Download

#10 I suspect ChromeOS gets away with paint-once scrollbars because the UI there doesn't have top and bottom arrow buttons.

> Do you know what is preventing us from doing the same on Windows & Mac?

The scrollbars have different visual states - mouse hover, mouse down, etc (in addition to the buttons as sunnyps@ mentions). Mac is especially problematic as it has an elastic bounce effect, expansion on hover, etc.

Re#11: That's correct - I have a patch in progress that makes this exact fix (sorry, should have updated the bug) but have been backed up on other work so haven't had a chance to get the landed yet.

#13 I can help with landing this fix if you could tell me what kind of tests to write for this.

I put up my WIP which you can reuse: https://chromium-review.googlesource.com/c/chromium/src/+/1081291

Test is where I got sidetracked so I'm not sure. +pdr@,wangxianzhu@ who know better how we should test paint invalidation (or non-invalidation, in this case). 

The simplest way of testing this is to write a repaint layout test under LayoutTests/paint/invalidation/scroll. You can use LayoutTests/paint/invalidation/scroll/composited-add-resizer.html as a sample.

So I wrote a layout test to check that scrollbar produces no invalidations when no parts change. However, the same approach doesn't work for checking if we do produce invalidations when the parts change because of this:

  if (UseMockTheme() && !scrollbar.Enabled()) {
    state = WebThemeEngine::kStateDisabled;
  } else if (!UseMockTheme() &&
             ((check_min && (position <= 0)) ||
              (check_max && position >= scrollbar.Maximum()))) {
    state = WebThemeEngine::kStateDisabled;
  } else {
    if (part == scrollbar.PressedPart())
      state = WebThemeEngine::kStatePressed;
    else if (part == scrollbar.HoveredPart())
      state = WebThemeEngine::kStateHover;
  }

UseMockTheme returns true for layout tests, so the state of the part (say back button) remains the same. Any suggestions?

Here's the layout test for testing no invalidations which works as expected i.e. layout tree contains no paintInvalidations, but because the scrollbar part state doesn't change ever, it's not possible to write a test that produces paint invalidations with my change.

<!DOCTYPE html>
<script src=../resources/text-based-repaint.js></script>
<script>
function repaintTest() {
  document.getElementById('target').scrollTop = 200;
}

onload = function() {
  document.getElementById('target').scrollTop = 100;
  runRepaintAndPixelTest();
}
</script>
<div id='target' style="width: 100px; height: 100px; overflow: scroll; will-change: transform">
  <div id='content' style="width: 100px; height: 400px"></div>
</div>

You can turn off the mock theme in test with:

internals.settings.setMockScrollbarsEnabled(true);

But then it will depend on the platform on which it runs. e.g. ChromeOS doesn't have buttons.

Can we use the same theme (aura) on all platforms for layout tests?

(when mock scrollbars are enabled)

Unfortunately not, themes are compile-time decided. Maybe easiest to just disable the test on some/all-but-one platforms?

Mock scrollbar theme doesn't have parts (like ChromeOS) so we can't test
invalidations due to parts. Without mock scrollbars we use the platform
native theme (Aura on Windows and Linux), but we're short-circuiting the
part state logic in the Aura theme for layout tests. This exception for
layout tests was enabled a long time back in
https://codereview.chromium.org/41733004. Maybe it's not needed any more
given that we have a separate mock theme?