Hide non-composited native scrollbars.

Hide non-composited native scrollbars.

This is a follow up to crrev.com/2175163005, which hides
composited native scrollbars.

BUG=617618
Committed: https://crrev.com/2b22b720242d5c9681a0b53cd56b542757ad7016
Cr-Commit-Position: refs/heads/master@{#412520}

[Eric Seckler, 2016-08-12 11:00:26]

eseckler@chromium.org changed reviewers:
+ skyostil@chromium.org

[Eric Seckler, 2016-08-12 11:00:27]

Hey Sami,

Do you think this is a sensible way to get rid of non-composited scrollbars? It
doesn't seem great to handle hidden scrollbars in three slightly different
places (mock, mobile, aura) - but it's not supported on mac, and mock uses
different ScrollbarTheme classes...

Eric

[Eric Seckler, 2016-08-12 11:03:10]

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

[commit-bot: I haz the power, 2016-08-12 11:04:00]

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

[Sami, 2016-08-12 11:12:27]

Are there still cases we don't handle after this patch?

This seems like the best we can do given the actual scroll bar implementations
are pretty different on each platform.

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

https://codereview.chromium.org/2239313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlayMock.h:40:
ScrollbarThemeOverlayMock(bool hidden = false)
I'd suggesting making this bool an enum instead so the call site logic becomes
obvious.

[Eric Seckler, 2016-08-12 11:30:23]

On 2016/08/12 11:12:27, Sami wrote:
> Are there still cases we don't handle after this patch?

Other than mac and custom scrollbars, I don't think so.

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

https://codereview.chromium.org/2239313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlayMock.h:40:
ScrollbarThemeOverlayMock(bool hidden = false)
On 2016/08/12 11:12:27, Sami wrote:
> I'd suggesting making this bool an enum instead so the call site logic becomes
> obvious.

Done :)

[Sami, 2016-08-12 11:33:25]

Thanks!


On 2016/08/12 11:30:23, Eric Seckler wrote:
> On 2016/08/12 11:12:27, Sami wrote:
> > Are there still cases we don't handle after this patch?
> 
> Other than mac and custom scrollbars, I don't think so.

Which path does mac use? Could we make it check this same flag?

[Eric Seckler, 2016-08-12 12:13:23]

eseckler@chromium.org changed reviewers:
+ pdr@chromium.org

[Eric Seckler, 2016-08-12 12:13:24]

r: pdr
cc: skobes, bokan

Hi Philip,

We weren't really sure who to send this patch to - Are you a good fit?

Thanks!
Eric

===
Hide non-composited native scrollbars.

This is a follow up to crrev.com/2175163005, which hides
composited native scrollbars.

BUG=617618
===

[Eric Seckler, 2016-08-12 12:13:35]

On 2016/08/12 11:33:25, Sami wrote:
> Which path does mac use? Could we make it check this same flag?

Mac uses a custom ScrollbarTheme (based on cocoa native scrollbars). I suspect
we can probably just replace this theme with the ScrollbarThemeOverlay that's
configured for hidden scrollbars (done). Though I'm not sure if we should add
a test for the non-mock case for the various platforms (layout tests seem to
always use mock scrollbars) - or how we would go about that. But since mock
bars work everywhere, I don't think there's a reason why this shouldn't work :)

[skobes, 2016-08-12 18:27:48]

On 2016/08/12 12:13:35, Eric Seckler wrote:
> On 2016/08/12 11:33:25, Sami wrote:
> > Which path does mac use? Could we make it check this same flag?
> 
> Mac uses a custom ScrollbarTheme (based on cocoa native scrollbars). I suspect
> we can probably just replace this theme with the ScrollbarThemeOverlay that's
> configured for hidden scrollbars (done).

This might work, but watch out for entanglements with ScrollAnimatorMac.

[skobes, 2016-08-12 18:29:53]

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

[skobes, 2016-08-12 18:29:54]

lgtm w/ nit

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

https://codereview.chromium.org/2239313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.cpp:311: if
(RuntimeEnabledFeatures::overlayScrollbarsEnabled() &&
RuntimeEnabledFeatures::hideScrollbarsEnabled()) {
Do we need to check overlayScrollbarsEnabled(), or is it implied by
hideScrollbarsEnabled()?

[Eric Seckler, 2016-08-15 08:49:51]

On 2016/08/12 18:27:48, skobes wrote:
> On 2016/08/12 12:13:35, Eric Seckler wrote:
> > On 2016/08/12 11:33:25, Sami wrote:
> > > Which path does mac use? Could we make it check this same flag?
> > 
> > Mac uses a custom ScrollbarTheme (based on cocoa native scrollbars). I
suspect
> > we can probably just replace this theme with the ScrollbarThemeOverlay
that's
> > configured for hidden scrollbars (done).
> 
> This might work, but watch out for entanglements with ScrollAnimatorMac.

Thanks Steve, you're right. There's a cast in ScrollAnimatorMac.mm that checks
only against isMockTheme() - and a similar one in web/mac/WebScrollbarTheme.mm.
So simply using the non-mocked hidden theme on Mac won't work. Instead, I'm now
always using a hidden mock theme. Since we're not showing the scrollbar anyway,
using the mock theme probably doesn't make a difference. Do you think that this
is a reasonable approach, or should we consider hiding within ScrollThemeMac
instead?

[skobes, 2016-08-15 17:13:33]

On 2016/08/15 08:49:51, Eric Seckler wrote:
> Thanks Steve, you're right. There's a cast in ScrollAnimatorMac.mm that checks
> only against isMockTheme() - and a similar one in
web/mac/WebScrollbarTheme.mm.
> So simply using the non-mocked hidden theme on Mac won't work. Instead, I'm
now
> always using a hidden mock theme. Since we're not showing the scrollbar
anyway,
> using the mock theme probably doesn't make a difference.

This sounds fine to me.  lgtm

[Eric Seckler, 2016-08-15 19:45:23]

eseckler@chromium.org changed reviewers:
+ sievers@chromium.org

[Eric Seckler, 2016-08-15 19:45:23]

+sievers for content/child/runtime_features.cc

Daniel, Philip - PTAL. Thanks!

[pdr., 2016-08-15 20:00:16]

On 2016/08/15 at 19:45:23, eseckler wrote:
> +sievers for content/child/runtime_features.cc
> 
> Daniel, Philip - PTAL. Thanks!

Can you update the comment in "src/ui/native_theme/native_theme_switches.cc" to
say that this does work on mac?

I tested this and found scrollbars still appear on
https://css-tricks.com/examples/WebKitScrollbars/. Is showing custom scrollbars
intended?

[Eric Seckler, 2016-08-15 20:28:46]

eseckler@chromium.org changed reviewers:
+ sky@chromium.org

[Eric Seckler, 2016-08-15 20:28:47]

+sky for ui/native_theme

On 2016/08/15 20:00:16, pdr. wrote:
> Can you update the comment in "src/ui/native_theme/native_theme_switches.cc"
to
> say that this does work on mac?
Done.

> I tested this and found scrollbars still appear on
> https://css-tricks.com/examples/WebKitScrollbars/. Is showing custom
scrollbars
> intended?
Yes, you're right. The flag only applies to native scrollbars. For our use case
(screenshots in headless chrome), that's fine. If it turns out that we need to
hide those as well, we can revisit this later.

[pdr., 2016-08-15 20:30:57]

On 2016/08/15 at 20:28:47, eseckler wrote:
> +sky for ui/native_theme
> 
> On 2016/08/15 20:00:16, pdr. wrote:
> > Can you update the comment in "src/ui/native_theme/native_theme_switches.cc"
to
> > say that this does work on mac?
> Done.
> 
> > I tested this and found scrollbars still appear on
> > https://css-tricks.com/examples/WebKitScrollbars/. Is showing custom
scrollbars
> > intended?
> Yes, you're right. The flag only applies to native scrollbars. For our use
case (screenshots in headless chrome), that's fine. If it turns out that we need
to hide those as well, we can revisit this later.

Thanks!

LGTM

[sky, 2016-08-15 23:40:40]

LGTM

[no sievers, 2016-08-16 20:37:52]

On 2016/08/15 19:45:23, Eric Seckler wrote:
> +sievers for content/child/runtime_features.cc
> 

lgtm

[Eric Seckler, 2016-08-17 09:48:55]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-08-17 09:48:55]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2239313002/#ps100001
(title: "update comment")

[commit-bot: I haz the power, 2016-08-17 09:49:05]

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-08-17 12:35:29]

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

[commit-bot: I haz the power, 2016-08-17 12:35:31]

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

[Eric Seckler, 2016-08-17 13:09:57]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2016-08-17 13:10:08]

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-08-17 14:33:36]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-17 14:37:45]

Description was changed from

==========
Hide non-composited native scrollbars.

This is a follow up to crrev.com/2175163005, which hides
composited native scrollbars.

BUG=617618
==========

to

==========
Hide non-composited native scrollbars.

This is a follow up to crrev.com/2175163005, which hides
composited native scrollbars.

BUG=617618

Committed: https://crrev.com/2b22b720242d5c9681a0b53cd56b542757ad7016
Cr-Commit-Position: refs/heads/master@{#412520}
==========

[commit-bot: I haz the power, 2016-08-17 14:37:46]

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