Call ScrollableArea::ShowOverlayScrollbars for explicit scrolls only.

Call ScrollableArea::ShowOverlayScrollbars for explicit scrolls only.

We now call ShowOverlayScrollbars for user and programmatic scrolls, but not for
scroll anchoring scrolls or content area resizes.

In most cases the effects will not be observable until we update the show
triggers in the compositor (an upcoming patch).

BUG=606395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2835403002
Cr-Commit-Position: refs/heads/master@{#467252}
Committed: https://chromium.googlesource.com/chromium/src/+/bd0fcfc50787bed5d42f1c8fab247234c1a6a20e

[skobes, 2017-04-25 02:01:15]

Description was changed from

==========
Call ScrollableArea::ShowOverlayScrollbars for explicit scrolls only.

We now call ShowOverlayScrollbars for user and programmatic scrolls, but not for
scroll anchoring scrolls or content area resizes.

In most cases the effects will not be observable until we update the show
triggers in the compositor (an upcoming patch).

BUG=606395
==========

to

==========
Call ScrollableArea::ShowOverlayScrollbars for explicit scrolls only.

We now call ShowOverlayScrollbars for user and programmatic scrolls, but not for
scroll anchoring scrolls or content area resizes.

In most cases the effects will not be observable until we update the show
triggers in the compositor (an upcoming patch).

BUG=606395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[skobes, 2017-04-25 02:01:22]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 02:02:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 03:32:20]

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

[commit-bot: I haz the power, 2017-04-25 03:32:21]

Dry run: This issue passed the CQ dry run.

[skobes, 2017-04-25 18:37:53]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 18:38:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2017-04-25 20:34:38]

skobes@chromium.org changed reviewers:
+ bokan@chromium.org, pdr@chromium.org

[commit-bot: I haz the power, 2017-04-25 20:34:41]

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

[commit-bot: I haz the power, 2017-04-25 20:34:42]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-04-25 23:21:18]

lgtm

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:551:
SetNeedsPaintInvalidation(skipPartsRepaint ? kNoPart : kAllParts);
On the CC side (for Aura anyway) we only need to paint once since we draw a 9
patch resource. This will cause us to upload a new texture to CC everytime we
fade in, right? Though, maybe that's ok for now - I can't think of a way to keep
the main thread and impl sides working in a clean way.

[pdr., 2017-04-26 00:28:32]

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:642: if
(IsExplicitScrollType(scroll_type)) {
This will no longer call NotifyRootFrameViewport when the scroll type is
kClampingScroll. Is that intentional?

[skobes, 2017-04-26 01:28:58]

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:642: if
(IsExplicitScrollType(scroll_type)) {
On 2017/04/26 00:28:32, pdr. wrote:
> This will no longer call NotifyRootFrameViewport when the scroll type is
> kClampingScroll. Is that intentional?

Yes, I think it was a mistake to call it for kClampingScroll. 
NotifyRootFrameViewport exists to call ScrollAnchor::Clear which should only
occur after explicit scrolls.

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:551:
SetNeedsPaintInvalidation(skipPartsRepaint ? kNoPart : kAllParts);
On 2017/04/25 23:21:17, bokan wrote:
> On the CC side (for Aura anyway) we only need to paint once since we draw a 9
> patch resource. This will cause us to upload a new texture to CC everytime we
> fade in, right? Though, maybe that's ok for now - I can't think of a way to
keep
> the main thread and impl sides working in a clean way.

I think ideally we would not repurpose the enabled state for hiding.  The
problem is that painting while hidden stores a blank thumb resource, since
ScrollbarThemeOverlay::PaintThumb passes kStateDisabled.

If we made the part-repaints from cc ignore the hidden state (i.e. paint
resource as if visible) we could probably avoid the invalidations here and in
SetProportion.

[pdr., 2017-04-26 03:03:59]

On 2017/04/26 at 01:28:58, skobes wrote:
>
https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):
> 
>
https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/VisualViewport.cpp:642: if
(IsExplicitScrollType(scroll_type)) {
> On 2017/04/26 00:28:32, pdr. wrote:
> > This will no longer call NotifyRootFrameViewport when the scroll type is
> > kClampingScroll. Is that intentional?
> 
> Yes, I think it was a mistake to call it for kClampingScroll. 
NotifyRootFrameViewport exists to call ScrollAnchor::Clear which should only
occur after explicit scrolls.

LGTM

[skobes, 2017-04-26 06:07:06]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2017-04-26 06:07:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 06:13:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493186826508150,
"parent_rev": "d7c815081037864e1725f873a89bab9088270a59", "commit_rev":
"bd0fcfc50787bed5d42f1c8fab247234c1a6a20e"}

[commit-bot: I haz the power, 2017-04-26 06:13:19]

Description was changed from

==========
Call ScrollableArea::ShowOverlayScrollbars for explicit scrolls only.

We now call ShowOverlayScrollbars for user and programmatic scrolls, but not for
scroll anchoring scrolls or content area resizes.

In most cases the effects will not be observable until we update the show
triggers in the compositor (an upcoming patch).

BUG=606395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Call ScrollableArea::ShowOverlayScrollbars for explicit scrolls only.

We now call ShowOverlayScrollbars for user and programmatic scrolls, but not for
scroll anchoring scrolls or content area resizes.

In most cases the effects will not be observable until we update the show
triggers in the compositor (an upcoming patch).

BUG=606395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2835403002
Cr-Commit-Position: refs/heads/master@{#467252}
Committed:
https://chromium.googlesource.com/chromium/src/+/bd0fcfc50787bed5d42f1c8fab24...
==========

[commit-bot: I haz the power, 2017-04-26 06:13:20]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/bd0fcfc50787bed5d42f1c8fab24...

[bokan, 2017-04-26 11:51:10]

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:551:
SetNeedsPaintInvalidation(skipPartsRepaint ? kNoPart : kAllParts);
On 2017/04/26 01:28:58, skobes wrote:
> On 2017/04/25 23:21:17, bokan wrote:
> > On the CC side (for Aura anyway) we only need to paint once since we draw a
9
> > patch resource. This will cause us to upload a new texture to CC everytime
we
> > fade in, right? Though, maybe that's ok for now - I can't think of a way to
> keep
> > the main thread and impl sides working in a clean way.
> 
> I think ideally we would not repurpose the enabled state for hiding.  The
> problem is that painting while hidden stores a blank thumb resource, since
> ScrollbarThemeOverlay::PaintThumb passes kStateDisabled.
> 
> If we made the part-repaints from cc ignore the hidden state (i.e. paint
> resource as if visible) we could probably avoid the invalidations here and in
> SetProportion.

It's an unfortunate quirk of main-thread painting that it reuses enabled state
for hiding/showing overlays. The CC side hides using opacity without requiring a
repaint but if the main thread invalidates it we'll repaint on the next update.
Unfortunately, we have to disable the scrollbar from CC while its hidden so this
will cause us to repaint on fade in.