Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(20)

Issue 2835403002: Call ScrollableArea::ShowOverlayScrollbars for explicit scrolls only. (Closed)

Created:
2 years, 4 months ago by skobes
Modified:
2 years, 4 months ago
Reviewers:
bokan, pdr.
CC:
aelias_OOO_until_Jul13, blink-reviews, blink-reviews-frames_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, kinuko+watch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : fix VisualViewport #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -25 lines) Patch
M third_party/WebKit/Source/core/frame/FrameView.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/frame/VisualViewport.cpp View 1 1 chunk +6 lines, -2 lines 2 comments Download
M third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp View 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/scroll/Scrollbar.cpp View 1 chunk +5 lines, -2 lines 3 comments Download
M third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h View 1 chunk +0 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.h View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp View 1 chunk +0 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlayTest.cpp View 1 chunk +10 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp View 1 chunk +0 lines, -3 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 21 (13 generated)
skobes
2 years, 4 months ago (2017-04-25 20:34:38 UTC) #9
bokan
lgtm https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right): https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp#newcode551 third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:551: SetNeedsPaintInvalidation(skipPartsRepaint ? kNoPart : kAllParts); On the CC ...
2 years, 4 months ago (2017-04-25 23:21:18 UTC) #12
pdr.
https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Source/core/frame/VisualViewport.cpp File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Source/core/frame/VisualViewport.cpp#newcode642 third_party/WebKit/Source/core/frame/VisualViewport.cpp:642: if (IsExplicitScrollType(scroll_type)) { This will no longer call NotifyRootFrameViewport ...
2 years, 4 months ago (2017-04-26 00:28:32 UTC) #13
skobes
https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Source/core/frame/VisualViewport.cpp File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Source/core/frame/VisualViewport.cpp#newcode642 third_party/WebKit/Source/core/frame/VisualViewport.cpp:642: if (IsExplicitScrollType(scroll_type)) { On 2017/04/26 00:28:32, pdr. wrote: > ...
2 years, 4 months ago (2017-04-26 01:28:58 UTC) #14
pdr.
On 2017/04/26 at 01:28:58, skobes wrote: > https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Source/core/frame/VisualViewport.cpp > File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): > > https://codereview.chromium.org/2835403002/diff/20001/third_party/WebKit/Source/core/frame/VisualViewport.cpp#newcode642 ...
2 years, 4 months ago (2017-04-26 03:03:59 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2835403002/20001
2 years, 4 months ago (2017-04-26 06:07:27 UTC) #17
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/bd0fcfc50787bed5d42f1c8fab247234c1a6a20e
2 years, 4 months ago (2017-04-26 06:13:20 UTC) #20
bokan
2 years, 4 months ago (2017-04-26 11:51:10 UTC) #21
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698