Disable overlay scrollbars to hide them on non-Mac.

Disable overlay scrollbars to hide them on non-Mac.

In https://codereview.chromium.org/2467693002/ I accidentally removed the code
that disabled/enabled scrollbars based on whether they're hidden. This was
because it caused an invalidation cycle on Mac but I had forgotten that the
reason for disabling scrollbars that are invisible is that painting code uses
this as a signal to "hide" the scrollbar.

This patch brings the code back but makes the behavior dependent on a flag in
ScrollbarTheme. This is needed since the signalling is flipped on Mac. On Mac,
painting code makes the scrollbars invisible which then sets the
scrollbarsHidden flag on the ScrollableArea. On Aura, a timer determines that
the scrollbars need to be hidden and disables them which causes an invalidation
and repaint.

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

Committed: https://crrev.com/4337e5dded29ebb7ef9401cb55e4effcf4f73b69
Cr-Commit-Position: refs/heads/master@{#433300}

[bokan, 2016-11-16 22:07:47]

Description was changed from

==========
Disable overlay scrollbars to hide them on non-Mac.

In https://codereview.chromium.org/2467693002/ I accidentally removed the code
that disabled/enabled scrollbars based on whether they're hidden. This was
because it caused an invalidation cycle on Mac but I had forgotten that the
reason for disabling scrollbars that are invisible is that painting code uses
this as a signal to "hide" the scrollbar.

This patch brings the code back but makes the behavior dependent on a flag in
ScrollbarTheme. This is needed since the signalling is flipped on Mac. On Mac,
painting code makes the scrollbars invisible which then sets the
scrollbarsHidden flag on the ScrollableArea. On Aura, a timer determines that
the scrollbars need to be hidden and disables them which causes an invalidation
and repaint.

BUG=307091
==========

to

==========
Disable overlay scrollbars to hide them on non-Mac.

In https://codereview.chromium.org/2467693002/ I accidentally removed the code
that disabled/enabled scrollbars based on whether they're hidden. This was
because it caused an invalidation cycle on Mac but I had forgotten that the
reason for disabling scrollbars that are invisible is that painting code uses
this as a signal to "hide" the scrollbar.

This patch brings the code back but makes the behavior dependent on a flag in
ScrollbarTheme. This is needed since the signalling is flipped on Mac. On Mac,
painting code makes the scrollbars invisible which then sets the
scrollbarsHidden flag on the ScrollableArea. On Aura, a timer determines that
the scrollbars need to be hidden and disables them which causes an invalidation
and repaint.

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

[bokan, 2016-11-16 22:10:47]

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

[commit-bot: I haz the power, 2016-11-16 22:11:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2016-11-16 22:17:08]

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

[bokan, 2016-11-16 22:17:09]

Hey Steve, ptal.

Its unfortunate that Mac scrollbars work the way they do. We definitely don't
want to add timers to Aura painting code since typically visibility is animated
on the compositor so this seems like the best compromise for now.

Long term, I'd like to see us do what Alexandre proposed in
https://chromiumcodereview.appspot.com/2421913002 by moving overlay scrollbars
totally into the compositor but that'll become easier with SPv2. For now I'd
like to get Aura overlays working.

[commit-bot: I haz the power, 2016-11-16 23:50:37]

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

[commit-bot: I haz the power, 2016-11-16 23:50:38]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bokan, 2016-11-17 13:48:30]

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

[commit-bot: I haz the power, 2016-11-17 13:48:44]

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, 2016-11-17 15:48:56]

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

[commit-bot: I haz the power, 2016-11-17 15:48:57]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-11-17 21:16:31]

lgtm w/ nits

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:894: crbug.com/417782
virtual/rootlayerscrolls/scrollbars/hidden-scrollbars-invisible.html [ Failure ]
Do you know why this fails?

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/scrollbars/hidden-scrollbars-invisible.html
(right):

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/scrollbars/hidden-scrollbars-invisible.html:18:
window.internals.settings.setOverlayScrollbarsEnabled(true);
nit: you don't need "window." (except on the line that checks for existence)

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlayMock.h
(right):

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlayMock.h:56: if
(!scrollbar.enabled())
Why isn't this in ScrollbarThemeOverlay?

[bokan, 2016-11-17 23:19:58]

Patchset #5 (id:80001) has been deleted

[bokan, 2016-11-17 23:22:26]

bokan@chromium.org changed reviewers:
+ jbroman@chromium.org

[bokan, 2016-11-17 23:22:27]

+jbroman@ for platform

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:894: crbug.com/417782
virtual/rootlayerscrolls/scrollbars/hidden-scrollbars-invisible.html [ Failure ]
On 2016/11/17 21:16:31, skobes wrote:
> Do you know why this fails?

I hadn't dug in deeply to find out the root cause but it looked like the clip is
applied in correctly to iframes for some reason. I assumed that was the reason
but on closer inspection there was more.

scrolling="no" on an iframe doesn't work. I disabled the scrollbars through
style so that did the trick. The only issue is now that it doesn't work because
the scrollingElement has overflow:auto which doesn't work but is fixed by the
follow up patch so I left this line in and removed it in the followup.

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/scrollbars/hidden-scrollbars-invisible.html
(right):

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/scrollbars/hidden-scrollbars-invisible.html:18:
window.internals.settings.setOverlayScrollbarsEnabled(true);
On 2016/11/17 21:16:31, skobes wrote:
> nit: you don't need "window." (except on the line that checks for existence)

Done.

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlayMock.h
(right):

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlayMock.h:56: if
(!scrollbar.enabled())
On 2016/11/17 21:16:31, skobes wrote:
> Why isn't this in ScrollbarThemeOverlay?

Not drawing a disabled scrollbar is normally handled by the painting code in the
ThemeEngine. In layout tests that's the MockWebThemeEngine which is unaware of
the difference between overlays and non-overlays so there's no way to do that
from there since non-overlays do draw disabled scrollbars.

[bokan, 2016-11-17 23:22:49]

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

[commit-bot: I haz the power, 2016-11-17 23:25:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2016-11-17 23:54:59]

lgtm

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2509843004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:894: crbug.com/417782
virtual/rootlayerscrolls/scrollbars/hidden-scrollbars-invisible.html [ Failure ]
On 2016/11/17 23:22:27, bokan wrote:
> On 2016/11/17 21:16:31, skobes wrote:
> > Do you know why this fails?
> 
> I hadn't dug in deeply to find out the root cause but it looked like the clip
is
> applied in correctly to iframes for some reason. I assumed that was the reason
> but on closer inspection there was more.
> 
> scrolling="no" on an iframe doesn't work. I disabled the scrollbars through
> style so that did the trick. The only issue is now that it doesn't work
because
> the scrollingElement has overflow:auto which doesn't work but is fixed by the
> follow up patch so I left this line in and removed it in the followup.

Ok, thanks for digging into it.

[bokan, 2016-11-18 00:06:24]

The CQ bit was unchecked by bokan@chromium.org

[bokan, 2016-11-18 00:06:28]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-11-18 00:07:51]

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, 2016-11-18 00:50:36]

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

[commit-bot: I haz the power, 2016-11-18 00:50:37]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2016-11-18 01:16:28]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-11-18 01:17:58]

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, 2016-11-18 01:28:52]

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

[commit-bot: I haz the power, 2016-11-18 01:28:54]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[bokan, 2016-11-18 01:39:31]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-11-18 01:41:04]

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, 2016-11-18 02:52:35]

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

[commit-bot: I haz the power, 2016-11-18 02:52:36]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jbroman, 2016-11-18 19:00:27]

platform rs lgtm with one nit

https://codereview.chromium.org/2509843004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):

https://codereview.chromium.org/2509843004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:79: virtual bool
disableInvisibleScrollbars() const { return true; }
nit: this name suggests that calling it disables the scrollbars. Could it be
named "disablesInvisibleScrollbars()" (like usesOverlayScrollbars above), or
"shouldDisableInvisibleScrollbars()", or something along those lines?

[bokan, 2016-11-18 19:09:00]

https://codereview.chromium.org/2509843004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):

https://codereview.chromium.org/2509843004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:79: virtual bool
disableInvisibleScrollbars() const { return true; }
On 2016/11/18 19:00:27, jbroman wrote:
> nit: this name suggests that calling it disables the scrollbars. Could it be
> named "disablesInvisibleScrollbars()" (like usesOverlayScrollbars above), or
> "shouldDisableInvisibleScrollbars()", or something along those lines?

Good point, done.

[bokan, 2016-11-18 19:09:05]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-11-18 19:09:06]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2509843004/#ps120001
(title: "Renamed to shouldDisableInvisibleScrollbars")

[commit-bot: I haz the power, 2016-11-18 19:10:21]

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, 2016-11-18 21:18:48]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-11-18 21:21:55]

Description was changed from

==========
Disable overlay scrollbars to hide them on non-Mac.

In https://codereview.chromium.org/2467693002/ I accidentally removed the code
that disabled/enabled scrollbars based on whether they're hidden. This was
because it caused an invalidation cycle on Mac but I had forgotten that the
reason for disabling scrollbars that are invisible is that painting code uses
this as a signal to "hide" the scrollbar.

This patch brings the code back but makes the behavior dependent on a flag in
ScrollbarTheme. This is needed since the signalling is flipped on Mac. On Mac,
painting code makes the scrollbars invisible which then sets the
scrollbarsHidden flag on the ScrollableArea. On Aura, a timer determines that
the scrollbars need to be hidden and disables them which causes an invalidation
and repaint.

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

to

==========
Disable overlay scrollbars to hide them on non-Mac.

In https://codereview.chromium.org/2467693002/ I accidentally removed the code
that disabled/enabled scrollbars based on whether they're hidden. This was
because it caused an invalidation cycle on Mac but I had forgotten that the
reason for disabling scrollbars that are invisible is that painting code uses
this as a signal to "hide" the scrollbar.

This patch brings the code back but makes the behavior dependent on a flag in
ScrollbarTheme. This is needed since the signalling is flipped on Mac. On Mac,
painting code makes the scrollbars invisible which then sets the
scrollbarsHidden flag on the ScrollableArea. On Aura, a timer determines that
the scrollbars need to be hidden and disables them which causes an invalidation
and repaint.

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

Committed: https://crrev.com/4337e5dded29ebb7ef9401cb55e4effcf4f73b69
Cr-Commit-Position: refs/heads/master@{#433300}
==========

[commit-bot: I haz the power, 2016-11-18 21:21:56]

Patchset 6 (id:??) landed as
https://crrev.com/4337e5dded29ebb7ef9401cb55e4effcf4f73b69
Cr-Commit-Position: refs/heads/master@{#433300}