Add flag to hide scrollbars completely.

Add flag to hide (native composited) scrollbars completely.

For headless chrome, we're interested in controlling the visibility of scrollbars in screenshots. As a first step, we would like to hide scrollbars completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes non-animated overlay scrollbars. When enabled, the scrollbars never appear and are effectively hidden completely. This affects only native composited scrollbars (a future patch will add support for hiding native non-composited scrollbars in Blink).

BUG=617618
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/025a020bd57294d44f907daef1badc5683e9220b
Cr-Commit-Position: refs/heads/master@{#410018}

[Eric Seckler, 2016-07-25 11:00:52]

Description was changed from

==========
Add flag to hide scrollbars completely.

BUG=
==========

to

==========
Add flag to hide scrollbars completely.

[future cc: chromium-reviews, mlamouri+watch-content@chromium.org,
creis+watch@chromium.org, nasko+codewatch@chromium.org, jam,
darin-cc@chromium.org, asvitkine+watch@chromium.org, piman+watch@chromium.org]

BUG=
==========

[Eric Seckler, 2016-07-25 11:01:07]

Description was changed from

==========
Add flag to hide scrollbars completely.

[future cc: chromium-reviews, mlamouri+watch-content@chromium.org,
creis+watch@chromium.org, nasko+codewatch@chromium.org, jam,
darin-cc@chromium.org, asvitkine+watch@chromium.org, piman+watch@chromium.org]

BUG=
==========

to

==========
Add flag to hide scrollbars completely.

[future cc: chromium-reviews, mlamouri+watch-content@chromium.org,
creis+watch@chromium.org, nasko+codewatch@chromium.org, jam,
darin-cc@chromium.org, asvitkine+watch@chromium.org, piman+watch@chromium.org]

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

[Eric Seckler, 2016-07-25 11:04:06]

Hey Sami,

I was playing around with hiding scrollbars for screenshots - this seemed like
the simplest way of effectively getting rid of them completely. WDYT?

To test it out, I added it as a flag, but maybe making this
DevTools-controllable is better?

Eric

[Sami, 2016-07-26 13:23:05]

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

[Sami, 2016-07-26 13:23:05]

This looks great. I think we can avoid adding this to about:flags since I doubt
many users would want to toggle it from the UI. A command line flag is good
enough for now.

A DevTools toggle would be nice too, although I'm not sure how feasible it is to
make this switch dynamically.

[Eric Seckler, 2016-08-01 11:04:38]

Patchset #2 (id:20001) has been deleted

[Eric Seckler, 2016-08-01 11:18:46]

Description was changed from

==========
Add flag to hide scrollbars completely.

[future cc: chromium-reviews, mlamouri+watch-content@chromium.org,
creis+watch@chromium.org, nasko+codewatch@chromium.org, jam,
darin-cc@chromium.org, asvitkine+watch@chromium.org, piman+watch@chromium.org]

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

to

==========
Add flag to hide scrollbars completely.

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

[Eric Seckler, 2016-08-01 11:27:35]

Description was changed from

==========
Add flag to hide scrollbars completely.

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

to

==========
Add flag to hide scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely.

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

[Eric Seckler, 2016-08-01 11:31:20]

eseckler@chromium.org changed reviewers:
+ jochen@chromium.org, sky@chromium.org, weiliangc@chromium.org

[Eric Seckler, 2016-08-01 11:31:21]

+weiliangc overall
+sky for ui/
+jochen for content/

Thanks!

[jochen (gone - plz use gerrit), 2016-08-01 12:17:07]

can you add a test please?

[Eric Seckler, 2016-08-01 18:02:02]

Description was changed from

==========
Add flag to hide scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely.

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

to

==========
Add flag to hide scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely.

BUG=617618
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Eric Seckler, 2016-08-01 18:17:56]

On 2016/08/01 12:17:07, jochen wrote:
> can you add a test please?

I added NO_ANIMATOR tests to the existing scroll animation LayerTreeHost tests.

However, there's a bug in the original test: a changed effect_tree_index is not
synced from the pending_tree to the active_tree, because the respective layer is
not marked as setNeedsPushProperties(). As a result, the layer points to the
wrong node in the tree. This bug didn't affect the original test, because that
wrong node also had the right (opacity) properties. However, in the added test,
it didn't.

I'm now manually marking the layer as setNeedsPushProperties() in the test. Yet,
we're not entirely sure if the test is the only place this situation may occur.
We were thinking of adding setNeedsPushProperties() to
LayerImpl::SetEffectTreeIndex(), but refrained from it for now, since that may
also have an effect on impl-to-main synchronization. WDYT?

[Eric Seckler, 2016-08-01 18:42:20]

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

[commit-bot: I haz the power, 2016-08-01 18:42:47]

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-08-01 19:42:19]

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

[commit-bot: I haz the power, 2016-08-01 19:42:20]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-08-02 15:27:24]

content/ lgtm

[weiliangc, 2016-08-02 17:38:42]

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

[weiliangc, 2016-08-02 17:38:43]

Don't think I'm the best person to review this. +skobes for overall review.

[skobes, 2016-08-02 17:55:56]

https://codereview.chromium.org/2175163005/diff/60001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2175163005/diff/60001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:414: // Android WebView uses
system scrollbars, so make ours invisible.
Update this comment since there is a new reason for us to be in this path.

https://codereview.chromium.org/2175163005/diff/60001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:459:
settings.scrollbar_animator = cc::LayerTreeSettings::NO_ANIMATOR;
Note that this implementation only hides native composited scrollbars.  It will
not hide custom scrollbars, or native scrollbars on non-composited scrollers. 
If you care about those cases you will need some Blink side plumbing.

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_switches.cc (right):

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.cc:16: // Hides scrollbars on Aura, Linux,
Android, ChromeOS. Does nothing on Mac.
What's the plan for Mac?  Or don't we care?

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.cc:17: const char kEnableHiddenScrollbar[]
= "enable-hidden-scrollbar";
Will there also be a disable flag?  If not, perhaps --hide-scrollbars is
clearer?

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.cc:39: bool IsHiddenScrollbarEnabled() {
"ShouldHideScrollbars"

[sky, 2016-08-02 20:30:24]

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_switches.h (right):

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.h:23: NATIVE_THEME_EXPORT bool
IsHiddenScrollbarEnabled();
AFAICT ui/native_theme doesn't handle this switch at all, it's handled in
content. Move the switch there?

[Eric Seckler, 2016-08-03 17:34:54]

Thanks for the comments! PTAL.

https://codereview.chromium.org/2175163005/diff/60001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2175163005/diff/60001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:414: // Android WebView uses
system scrollbars, so make ours invisible.
On 2016/08/02 17:55:55, skobes wrote:
> Update this comment since there is a new reason for us to be in this path.

Done.

https://codereview.chromium.org/2175163005/diff/60001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:459:
settings.scrollbar_animator = cc::LayerTreeSettings::NO_ANIMATOR;
On 2016/08/02 17:55:55, skobes wrote:
> Note that this implementation only hides native composited scrollbars.  It
will
> not hide custom scrollbars, or native scrollbars on non-composited scrollers. 
> If you care about those cases you will need some Blink side plumbing.

I think just supporting native composited scrollbars is a good first step. We'll
want to hide native non-composited ones next, but I'm happy to do that in a
later patch. Hiding custom scrollbars is only a nice-to-have.

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_switches.cc (right):

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.cc:16: // Hides scrollbars on Aura, Linux,
Android, ChromeOS. Does nothing on Mac.
On 2016/08/02 17:55:56, skobes wrote:
> What's the plan for Mac?  Or don't we care?

At the moment, we don't care and will look into this at a later time.

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.cc:17: const char kEnableHiddenScrollbar[]
= "enable-hidden-scrollbar";
On 2016/08/02 17:55:56, skobes wrote:
> Will there also be a disable flag?  If not, perhaps --hide-scrollbars is
> clearer?

Done. I was aiming for consistent naming, but you're right, it was awkward :)

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.cc:39: bool IsHiddenScrollbarEnabled() {
On 2016/08/02 17:55:56, skobes wrote:
> "ShouldHideScrollbars"

Done.

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_switches.h (right):

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.h:23: NATIVE_THEME_EXPORT bool
IsHiddenScrollbarEnabled();
On 2016/08/02 20:30:24, sky wrote:
> AFAICT ui/native_theme doesn't handle this switch at all, it's handled in
> content. Move the switch there?

It's used by IsOverlayScrollbarEnabled(), which is used in ui/native_theme.
Would it thus be ok to leave it here?

[skobes, 2016-08-03 19:13:08]

lgtm if sky is satsfied

[sky, 2016-08-03 23:44:04]

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_switches.h (right):

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.h:23: NATIVE_THEME_EXPORT bool
IsHiddenScrollbarEnabled();
On 2016/08/03 17:34:54, Eric Seckler wrote:
> On 2016/08/02 20:30:24, sky wrote:
> > AFAICT ui/native_theme doesn't handle this switch at all, it's handled in
> > content. Move the switch there?
> 
> It's used by IsOverlayScrollbarEnabled(), which is used in ui/native_theme.
> Would it thus be ok to leave it here?

Does it need to be? It seems like the call sites check ShouldHideScrollbars()
before checking IsOverlayScrollbarEnabled. Am I missing some calls that don't do
that?

https://codereview.chromium.org/2175163005/diff/80001/ui/native_theme/native_...
File ui/native_theme/native_theme_switches.cc (right):

https://codereview.chromium.org/2175163005/diff/80001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.cc:43: if
(command_line.HasSwitch(switches::kHideScrollbars))
No need for the conditional, return command_line.HasSwitch(...).

[Eric Seckler, 2016-08-04 07:31:58]

Description was changed from

==========
Add flag to hide scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely.

BUG=617618
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add flag to hide (native composited) scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely. This affects only native composited
scrollbars (a future patch will add support for hiding native non-composited
scrollbars in Blink).

BUG=617618
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Eric Seckler, 2016-08-04 08:07:25]

eseckler@chromium.org changed reviewers:
+ ajuma@chromium.org
- weiliangc@chromium.org

[Eric Seckler, 2016-08-04 08:07:26]

+ajuma for the test in cc/

Thanks! :)

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_switches.h (right):

https://codereview.chromium.org/2175163005/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.h:23: NATIVE_THEME_EXPORT bool
IsHiddenScrollbarEnabled();
On 2016/08/03 23:44:03, sky wrote:
> On 2016/08/03 17:34:54, Eric Seckler wrote:
> > On 2016/08/02 20:30:24, sky wrote:
> > > AFAICT ui/native_theme doesn't handle this switch at all, it's handled in
> > > content. Move the switch there?
> > 
> > It's used by IsOverlayScrollbarEnabled(), which is used in ui/native_theme.
> > Would it thus be ok to leave it here?
> 
> Does it need to be? It seems like the call sites check ShouldHideScrollbars()
> before checking IsOverlayScrollbarEnabled. Am I missing some calls that don't
do
> that?

Yeah, there are quite a few other callsites of IsOverlayScrollbarEnabled(), see
https://cs.chromium.org/search/?q=IsOverlayScrollbarEnabled&sq=package:chromi...
(e.g. in NativeThemeAura and other places in content/). We need to make sure
overlay scrollbars are enabled in order to hide them, otherwise the NO_ANIMATOR
setting in RWC doesn't have any effect.

https://codereview.chromium.org/2175163005/diff/80001/ui/native_theme/native_...
File ui/native_theme/native_theme_switches.cc (right):

https://codereview.chromium.org/2175163005/diff/80001/ui/native_theme/native_...
ui/native_theme/native_theme_switches.cc:43: if
(command_line.HasSwitch(switches::kHideScrollbars))
On 2016/08/03 23:44:03, sky wrote:
> No need for the conditional, return command_line.HasSwitch(...).

Done.

[sky, 2016-08-04 16:36:19]

Ok, LGTM

[ajuma, 2016-08-04 16:47:38]

On 2016/08/04 08:07:26, Eric Seckler wrote:
> +ajuma for the test in cc/

test in cc/ lgtm

[Eric Seckler, 2016-08-05 08:08:09]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-08-05 08:08:09]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2175163005/#ps100001
(title: "fix ShouldHideScrollbars().")

[commit-bot: I haz the power, 2016-08-05 08:08:29]

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-05 09:16:28]

Description was changed from

==========
Add flag to hide (native composited) scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely. This affects only native composited
scrollbars (a future patch will add support for hiding native non-composited
scrollbars in Blink).

BUG=617618
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add flag to hide (native composited) scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely. This affects only native composited
scrollbars (a future patch will add support for hiding native non-composited
scrollbars in Blink).

BUG=617618
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-05 09:16:29]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-08-05 09:18:41]

Description was changed from

==========
Add flag to hide (native composited) scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely. This affects only native composited
scrollbars (a future patch will add support for hiding native non-composited
scrollbars in Blink).

BUG=617618
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add flag to hide (native composited) scrollbars completely.

For headless chrome, we're interested in controlling the visibility of
scrollbars in screenshots. As a first step, we would like to hide scrollbars
completely. This patch adds a --enable-hidden-scrollbar flag, which utilizes
non-animated overlay scrollbars. When enabled, the scrollbars never appear and
are effectively hidden completely. This affects only native composited
scrollbars (a future patch will add support for hiding native non-composited
scrollbars in Blink).

BUG=617618
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/025a020bd57294d44f907daef1badc5683e9220b
Cr-Commit-Position: refs/heads/master@{#410018}
==========

[commit-bot: I haz the power, 2016-08-05 09:18:42]

Patchset 5 (id:??) landed as
https://crrev.com/025a020bd57294d44f907daef1badc5683e9220b
Cr-Commit-Position: refs/heads/master@{#410018}