Implement Sebastien's overlay scrollbars for native UI (Views).

Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

high level overview:
1. thumb is a rectangle with a stroke
   a) when hovered, the thumb grows a little wider and darker.

2. thumb is hidden by default.
   a) Any time something happens that changes the size of the thumb, it
      appears (no animation). This includes scrolling or the contents
      changing size.
   b) The scrollbar also appears on mouse hover.
   c) Scrollbar fades out after a second when neither of the above are
      true (edge case: unless it's being dragged).

BUG=652520, 307091
Committed: https://crrev.com/862c7245d9b3f295df0db82f6e17c730d3f54825
Cr-Commit-Position: refs/heads/master@{#432358}

[Evan Stade, 2016-11-10 19:10:06]

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

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

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

[Evan Stade, 2016-11-10 19:31:21]

Description was changed from

==========
Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

BUG=652520,307091
==========

to

==========
Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

high level overview:
1. thumb is a rectangle with a stroke
   a) when hovered, the thumb grows a little wider and darker.

2. thumb is hidden by default.
   a) Any time something happens that changes the size of the thumb, it
      appears (no animation). This includes scrolling or the contents
      changing size.
   b) The scrollbar also appears on mouse hover.
   c) Scrollbar fades out after a second when neither of the above are
      true (edge case: unless it's being dragged).

BUG=652520,307091
==========

[Evan Stade, 2016-11-10 19:31:21]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Evan Stade, 2016-11-10 19:31:40]

Terry, can you take a first look before I ask for Sadrul's OWNER review?

https://codereview.chromium.org/2496643002/diff/1/ui/views/controls/scrollbar...
File ui/views/controls/scrollbar/base_scroll_bar_thumb.cc (right):

https://codereview.chromium.org/2496643002/diff/1/ui/views/controls/scrollbar...
ui/views/controls/scrollbar/base_scroll_bar_thumb.cc:37: SetSize(size);
the changes here are functionally idempotent; it's just shorter and easier to
read this way.

[tdanderson, 2016-11-10 20:50:20]

tdanderson@chromium.org changed reviewers:
+ sadrul@chromium.org

[tdanderson, 2016-11-10 20:50:22]

Took a first look, and handing over to Sadrul for OWNERS review.

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:20: // The thumb is slimmer
when unhovered by this amount.
Just reading this comment I'm not sure what "when unhovered by" means in terms
of a quantity. Can you clarify units here (pixels outside of border? seconds
since mouse exit?)

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:150:
settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(200));
nit: make 200 a constant

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.h (right):

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.h:47: // gfx::AnimationDelegate
overrides:
nit: just gfx::AnimationDelegate:

[Evan Stade, 2016-11-11 02:46:47]

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:20: // The thumb is slimmer
when unhovered by this amount.
On 2016/11/10 20:50:21, tdanderson wrote:
> Just reading this comment I'm not sure what "when unhovered by" means in terms
> of a quantity. Can you clarify units here (pixels outside of border? seconds
> since mouse exit?)

Done.

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:150:
settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(200));
On 2016/11/10 20:50:21, tdanderson wrote:
> nit: make 200 a constant

I can't see any purpose in making this a const variable. The code itself is
perfect documentation ("duration", "FromMilliseconds").

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.h (right):

https://codereview.chromium.org/2496643002/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.h:47: // gfx::AnimationDelegate
overrides:
On 2016/11/10 20:50:21, tdanderson wrote:
> nit: just gfx::AnimationDelegate:

guess I don't really care one way or another, but why?

[sadrul, 2016-11-12 03:01:11]

https://codereview.chromium.org/2496643002/diff/40001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/base_scroll_bar_thumb.h (right):

https://codereview.chromium.org/2496643002/diff/40001/ui/views/controls/scrol...
ui/views/controls/scrollbar/base_scroll_bar_thumb.h:62: virtual void
SetState(CustomButton::ButtonState state);
Can you introduce a separate OnStateChanged() or something like that, so the
subclass doesn't always need to chain up to the parent?

https://codereview.chromium.org/2496643002/diff/40001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2496643002/diff/40001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:103: hover_animation_.Show();
Since the thumb has a layer, can you just animate the opacity (and scale too, I
guess?) of the layer?

[Evan Stade, 2016-11-14 16:56:07]

https://codereview.chromium.org/2496643002/diff/40001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2496643002/diff/40001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:103: hover_animation_.Show();
On 2016/11/12 03:01:10, sadrul wrote:
> Since the thumb has a layer, can you just animate the opacity (and scale too,
I
> guess?) of the layer?

I've changed all animations to be applied to layers.

Implementation notes: I can't animate the scale because the stroke is always the
same width (1dp). Also it's difficult to manage two different opacity animations
(the one from 0 to 1 and the one from 0.3 to 0.5). So I added a layer to the bar
in addition to the thumb. The bar's layer clips the thumb while the thumb
translates off the outer edge (effectively growing/shrinking). The bar animates
from 0-1 opacity, the thumb animates from 0.3-0.5.

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/base_scroll_bar.cc (left):

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
ui/views/controls/scrollbar/base_scroll_bar.cc:41:
thumb_track_state_(CustomButton::STATE_NORMAL),
I noticed that this isn't actually used for anything. The thumb track state
doesn't affect appearance for this or any other scrollbar. The repeater is
affected by calling SetState, so that functionality had to be salvaged.

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
ui/views/controls/scrollbar/base_scroll_bar.cc:512: SchedulePaint();
there was a lot of extra painting for no reason... :/

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/base_scroll_bar_thumb.cc (right):

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
ui/views/controls/scrollbar/base_scroll_bar_thumb.cc:130: SchedulePaint();
no need to paint an overlay thumb

[Evan Stade, 2016-11-14 16:57:18]

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

[commit-bot: I haz the power, 2016-11-14 16:58:00]

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-14 17:34:38]

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

[commit-bot: I haz the power, 2016-11-14 17:34:39]

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

[sadrul, 2016-11-14 22:13:31]

The try-failures in views_unittests look relevant. Mind taking a look at those
to see what the problem is?

[varkha, 2016-11-15 00:31:30]

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

[varkha, 2016-11-15 00:31:30]

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:130:
layer()->SetOpacity(1.0f);
Do you need to stack |layer()| at the top (here or elsewhere)? Right now the
overlay scrollbars show below the sticky header rows if those get painted to
their own layer (which we may need to do given that some of the scroll contents
children views create controls with layers).

[Evan Stade, 2016-11-15 00:51:51]

On 2016/11/14 22:13:31, sadrul wrote:
> The try-failures in views_unittests look relevant. Mind taking a look at those
> to see what the problem is?

fixed

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:130:
layer()->SetOpacity(1.0f);
On 2016/11/15 00:31:30, varkha wrote:
> Do you need to stack |layer()| at the top (here or elsewhere)? Right now the
> overlay scrollbars show below the sticky header rows if those get painted to
> their own layer (which we may need to do given that some of the scroll
contents
> children views create controls with layers).

I don't think it's a problem because the scrollbars should come after the
viewport (their sibling) in ScrollView, meaning higher in Z-order.

[Evan Stade, 2016-11-15 00:51:58]

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

[commit-bot: I haz the power, 2016-11-15 00:53:06]

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

[varkha, 2016-11-15 01:33:29]

On 2016/11/15 00:51:51, Evan Stade wrote:
> On 2016/11/14 22:13:31, sadrul wrote:
> > The try-failures in views_unittests look relevant. Mind taking a look at
those
> > to see what the problem is?
> 
> fixed
> 
>
https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
> File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):
> 
>
https://codereview.chromium.org/2496643002/diff/80001/ui/views/controls/scrol...
> ui/views/controls/scrollbar/overlay_scroll_bar.cc:130:
> layer()->SetOpacity(1.0f);
> On 2016/11/15 00:31:30, varkha wrote:
> > Do you need to stack |layer()| at the top (here or elsewhere)? Right now the
> > overlay scrollbars show below the sticky header rows if those get painted to
> > their own layer (which we may need to do given that some of the scroll
> contents
> > children views create controls with layers).
> 
> I don't think it's a problem because the scrollbars should come after the
> viewport (their sibling) in ScrollView, meaning higher in Z-order.

You are right, the problem is not with the scrollbars but rather with the
header sticky rows (or even just with the way I tried to fix the header rows to
get them be on top of the controls added to the regular rows). That still needs
fixing despite https://codereview.chromium.org/2500493004/ (that CL only fixed
clipping by the detailed view topmost header but not by the sticky header rows).

[commit-bot: I haz the power, 2016-11-15 03:12:08]

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

[commit-bot: I haz the power, 2016-11-15 03:12:09]

Dry run: 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_...)

[Evan Stade, 2016-11-15 23:28:03]

ping?

[Evan Stade, 2016-11-15 23:28:23]

ah, nevermind, still some test failures..

[Evan Stade, 2016-11-15 23:28:24]

ah, nevermind, still some test failures..

[Evan Stade, 2016-11-15 23:54:46]

ok, tests should be good, ptal

[Evan Stade, 2016-11-15 23:54:51]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-15 23:55: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, 2016-11-15 23:55:28]

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

[commit-bot: I haz the power, 2016-11-15 23:55:29]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[sadrul, 2016-11-16 00:36:49]

lgtm

[Evan Stade, 2016-11-16 01:25:02]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-16 01:25:41]

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

[varkha, 2016-11-16 01:43:22]

lgtm

[commit-bot: I haz the power, 2016-11-16 02:34:11]

Description was changed from

==========
Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

high level overview:
1. thumb is a rectangle with a stroke
   a) when hovered, the thumb grows a little wider and darker.

2. thumb is hidden by default.
   a) Any time something happens that changes the size of the thumb, it
      appears (no animation). This includes scrolling or the contents
      changing size.
   b) The scrollbar also appears on mouse hover.
   c) Scrollbar fades out after a second when neither of the above are
      true (edge case: unless it's being dragged).

BUG=652520,307091
==========

to

==========
Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

high level overview:
1. thumb is a rectangle with a stroke
   a) when hovered, the thumb grows a little wider and darker.

2. thumb is hidden by default.
   a) Any time something happens that changes the size of the thumb, it
      appears (no animation). This includes scrolling or the contents
      changing size.
   b) The scrollbar also appears on mouse hover.
   c) Scrollbar fades out after a second when neither of the above are
      true (edge case: unless it's being dragged).

BUG=652520,307091
==========

[commit-bot: I haz the power, 2016-11-16 02:34:13]

Committed patchset #7 (id:120001)

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

Description was changed from

==========
Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

high level overview:
1. thumb is a rectangle with a stroke
   a) when hovered, the thumb grows a little wider and darker.

2. thumb is hidden by default.
   a) Any time something happens that changes the size of the thumb, it
      appears (no animation). This includes scrolling or the contents
      changing size.
   b) The scrollbar also appears on mouse hover.
   c) Scrollbar fades out after a second when neither of the above are
      true (edge case: unless it's being dragged).

BUG=652520,307091
==========

to

==========
Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

high level overview:
1. thumb is a rectangle with a stroke
   a) when hovered, the thumb grows a little wider and darker.

2. thumb is hidden by default.
   a) Any time something happens that changes the size of the thumb, it
      appears (no animation). This includes scrolling or the contents
      changing size.
   b) The scrollbar also appears on mouse hover.
   c) Scrollbar fades out after a second when neither of the above are
      true (edge case: unless it's being dragged).

BUG=652520,307091

Committed: https://crrev.com/862c7245d9b3f295df0db82f6e17c730d3f54825
Cr-Commit-Position: refs/heads/master@{#432358}
==========

[commit-bot: I haz the power, 2016-11-16 02:36:53]

Patchset 7 (id:??) landed as
https://crrev.com/862c7245d9b3f295df0db82f6e17c730d3f54825
Cr-Commit-Position: refs/heads/master@{#432358}

[findit-for-me, 2016-11-16 04:23:18]

FYI: Findit identified this CL at revision 432358 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[tapted, 2016-11-16 06:22:33]

On 2016/11/16 04:23:18, findit-for-me wrote:
> FYI: Findit identified this CL at revision 432358 as the culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

The failing tests were SystemTrayTest.NullDefaultViewIsNotRecorded, but it looks
like they cleared up in
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui...
with a revert ( https://codereview.chromium.org/2506913002 ). 

(context: I had a merge conflict just as
https://codereview.chromium.org/2454323002/ was going to land - since it doesn't
look like this CL needs reverting, I'll rebase and land my CL. )

[tapted, 2016-11-16 06:22:47]

Description was changed from

==========
Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

high level overview:
1. thumb is a rectangle with a stroke
   a) when hovered, the thumb grows a little wider and darker.

2. thumb is hidden by default.
   a) Any time something happens that changes the size of the thumb, it
      appears (no animation). This includes scrolling or the contents
      changing size.
   b) The scrollbar also appears on mouse hover.
   c) Scrollbar fades out after a second when neither of the above are
      true (edge case: unless it's being dragged).

BUG=652520,307091

Committed: https://crrev.com/862c7245d9b3f295df0db82f6e17c730d3f54825
Cr-Commit-Position: refs/heads/master@{#432358}
==========

to

==========
Implement Sebastien's overlay scrollbars for native UI (Views).

These are only used in two cros-specific surfaces (the system menu and
message center).

high level overview:
1. thumb is a rectangle with a stroke
   a) when hovered, the thumb grows a little wider and darker.

2. thumb is hidden by default.
   a) Any time something happens that changes the size of the thumb, it
      appears (no animation). This includes scrolling or the contents
      changing size.
   b) The scrollbar also appears on mouse hover.
   c) Scrollbar fades out after a second when neither of the above are
      true (edge case: unless it's being dragged).

BUG=652520,307091

Committed: https://crrev.com/862c7245d9b3f295df0db82f6e17c730d3f54825
Cr-Commit-Position: refs/heads/master@{#432358}
==========

[Evan Stade, 2016-11-16 18:32:00]

On 2016/11/16 06:22:33, tapted wrote:
> On 2016/11/16 04:23:18, findit-for-me wrote:
> > FYI: Findit identified this CL at revision 432358 as the culprit for
> > failures in the build cycles as shown on:
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> 
> The failing tests were SystemTrayTest.NullDefaultViewIsNotRecorded, but it
looks
> like they cleared up in
>
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui...
> with a revert ( https://codereview.chromium.org/2506913002 ). 
> 
> (context: I had a merge conflict just as
> https://codereview.chromium.org/2454323002/ was going to land - since it
doesn't
> look like this CL needs reverting, I'll rebase and land my CL. )

thanks!