Change Aura overlay scrollbars from solid color to painted scrollbars.

Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme. If the scroller is composited, these scrollbars will now be uploaded to
the compositor as PaintedScrollbarLayers rather than SolidColorScrollbarLayers.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single location in
ui/native_theme. I've also updated the values to match the latest spec.
Follow-up patches will make use of these values from the compositor.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/3d914bde3081131a655b467c1a7a232c7c065007
Cr-Commit-Position: refs/heads/master@{#426191}

[bokan, 2016-10-14 19:16:31]

Description was changed from

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single, central location. I've
also updated the values to the latest spec.

BUG=592098
==========

to

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single, central location. I've
also updated the values to the latest spec.

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

[bokan, 2016-10-14 19:16:43]

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

[commit-bot: I haz the power, 2016-10-14 19:17:10]

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

[bokan, 2016-10-14 19:17:22]

Description was changed from

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single, central location. I've
also updated the values to the latest spec.

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

to

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single, central location. I've
also updated the values to match the latest spec.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[bokan, 2016-10-14 19:23:03]

Description was changed from

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single, central location. I've
also updated the values to match the latest spec.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single, central location. I've
also updated the values to match the latest spec. Follow-up patches will make
use of these values from the compositor.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[bokan, 2016-10-14 19:24:48]

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

[bokan, 2016-10-14 19:24:49]

Hi Peter, are you the right person to look at this (at least the NativeTheme
bits)? If not, feel free to punt to someone else. Thanks.

[Peter Kasting, 2016-10-17 19:10:25]

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/materia...
File ui/native_theme/material_design_constants.h (right):

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/materia...
ui/native_theme/material_design_constants.h:6: #define
UI_NATIVE_THEME_MATERIAL_DESIGN_CONSTANTS_H_
You've named this file like it's a general-purpose set of constants, but these
all have to do with scrollbars.  Why not some kind of scrollbar_constants.h? 
I'd rather not tempt people in the future to create a sprawling file with
thousands of general-purpose UI constants in it.  See also next comment.

Honestly, I'd prefer not to have a separate file for constants at all, and just
define these in some other pre-existing header that has to do with scrollbars,
but perhaps that is not feasible.  If possible, though, it would be nice to at
least not put here anything that is only needed in one place -- for example, if
separate individual files handle things like painting (color, alpha), layout
(length, width), and animation (fade durations), I would keep these constants
separately scoped to those files.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/materia...
ui/native_theme/material_design_constants.h:12: const SkAlpha
kMDScrollbarAlphaNormal = 0x4D;
Why do the file name and these constant names all explicitly refer to material
design?  Now that we've shipped MD top chrome we're actively trying to remove
references to "material" as this is just The Way We Do Things and needs no
special name.  Are you trying to distinguish from some parallel set of non-MD
scrollbar constants?  Isn't the distinction here really "overlay vs.
non-overlay" rather than "MD vs. non-MD"?

So in general I'd prefer to avoid any reference to material/MD in names.

Also: All these should be constexpr.

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

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:49: SkAlpha
ThumbStrokeAlphaForState(NativeTheme::State state) {
Do we even need this function anymore?

It looks like the implementation could just be:

  DCHECK(IsOverlayScrollbarEnabled());
  return ThumbAlphaForState(state);

But in that case, why not have the caller just call ThumbAlphaForState()
directly?

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:210: const int kStrokeWidth =
kMDScrollbarStrokeWidth;
Nit: Can be constexpr

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:221: //
https://groups.google.com/forum/#!topic/skia-discuss/HGZcc3Z7w1Q
I'm concerned about this code.

Looking at the referenced discussion, I think the issue is with people trying to
draw on pixel-edge coordinates instead of stroking on pixel centers.  If, for
example, you want to stroke with width kStrokeWidth, and you want the stroke's
outer edge to be flush to the inside of |thumb_rect|, then the right thing to do
is basically:

  gfx::RectF stroke_rect(thumb_rect);
  constexpr float kStrokeHalfWidth = kStrokeWidth / 2.f;
  stroke_rect.Inset(kStrokeHalfWidth, kStrokeHalfWidth);
  canvas->drawRect(gfx::RectFToSkRect(stroke_rect)), paint;

This ensures you'll do the correct thing no matter the stroke width, whereas the
inset you've written basically only works if the stroke rect is exactly 1 (and
then probably only for scale factor 1).

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:263: if (!IsOverlayScrollbarEnabled())
Nit: This conditional effectively results in duplicated code for the "fall back
to parent" case.  It feels slightly less error-prone to just do:

  if (IsOverlayScrollbarEnabled()) {
    ... handle scrollbar constants ...
  }

  return NativeThemeBase::GetPartSize(part, state, extra);

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:264: return
NativeThemeBase::GetPartSize(part, state, extra);
Nit: 2 spaces

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:277: NOTREACHED();
Is NOTREACHED really correct here, or should we just return 0?  Basically, this
implies that calling code needs to do a runtime check for overlay scrollbars
before calling to get the size of these parts.  I can't decide whether that's a
good thing we want to enforce, or unnecessarily forces the caller to worry about
this.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:280: return
gfx::Size(kMDScrollbarMinimumLength + 2 * kMDScrollbarStrokeWidth,
Nit: You can factor this out to a constant above the switch and use it for both
these cases.

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

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_base.h:172: unsigned int scrollbar_width_;
Nit: unsigned int is banned.  I know you are just moving this code up, but try
to change this to int.

[bokan, 2016-10-17 23:21:04]

PTAL.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/materia...
File ui/native_theme/material_design_constants.h (right):

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/materia...
ui/native_theme/material_design_constants.h:6: #define
UI_NATIVE_THEME_MATERIAL_DESIGN_CONSTANTS_H_
On 2016/10/17 19:10:24, Peter Kasting wrote:
> You've named this file like it's a general-purpose set of constants, but these
> all have to do with scrollbars.  Why not some kind of scrollbar_constants.h? 
> I'd rather not tempt people in the future to create a sprawling file with
> thousands of general-purpose UI constants in it.  See also next comment.
> 
> Honestly, I'd prefer not to have a separate file for constants at all, and
just
> define these in some other pre-existing header that has to do with scrollbars,
> but perhaps that is not feasible.  If possible, though, it would be nice to at
> least not put here anything that is only needed in one place -- for example,
if
> separate individual files handle things like painting (color, alpha), layout
> (length, width), and animation (fade durations), I would keep these constants
> separately scoped to those files.

Ok, I've renamed the file to overlay_scrollbar_constants_aura.h and moved the
painting related properties into native_theme_aura.cc. Unfortunately, the
animation constants will be needed in both Blink and CC so I've kept them here.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/materia...
ui/native_theme/material_design_constants.h:12: const SkAlpha
kMDScrollbarAlphaNormal = 0x4D;
On 2016/10/17 19:10:24, Peter Kasting wrote:
> Why do the file name and these constant names all explicitly refer to material
> design?  Now that we've shipped MD top chrome we're actively trying to remove
> references to "material" as this is just The Way We Do Things and needs no
> special name.  Are you trying to distinguish from some parallel set of non-MD
> scrollbar constants?  Isn't the distinction here really "overlay vs.
> non-overlay" rather than "MD vs. non-MD"?
> 
> So in general I'd prefer to avoid any reference to material/MD in names.
Done.

> Also: All these should be constexpr.

Done except for SkColorSetRGB which isn't constexpr.

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

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:49: SkAlpha
ThumbStrokeAlphaForState(NativeTheme::State state) {
On 2016/10/17 19:10:25, Peter Kasting wrote:
> Do we even need this function anymore?
> 
> It looks like the implementation could just be:
> 
>   DCHECK(IsOverlayScrollbarEnabled());
>   return ThumbAlphaForState(state);
> 
> But in that case, why not have the caller just call ThumbAlphaForState()
> directly?

This function is now gone.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:210: const int kStrokeWidth =
kMDScrollbarStrokeWidth;
On 2016/10/17 19:10:25, Peter Kasting wrote:
> Nit: Can be constexpr

Done.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:221: //
https://groups.google.com/forum/#!topic/skia-discuss/HGZcc3Z7w1Q
On 2016/10/17 19:10:25, Peter Kasting wrote:
> I'm concerned about this code.
> 
> Looking at the referenced discussion, I think the issue is with people trying
to
> draw on pixel-edge coordinates instead of stroking on pixel centers.  If, for
> example, you want to stroke with width kStrokeWidth, and you want the stroke's
> outer edge to be flush to the inside of |thumb_rect|, then the right thing to
do
> is basically:
> 
>   gfx::RectF stroke_rect(thumb_rect);
>   constexpr float kStrokeHalfWidth = kStrokeWidth / 2.f;
>   stroke_rect.Inset(kStrokeHalfWidth, kStrokeHalfWidth);
>   canvas->drawRect(gfx::RectFToSkRect(stroke_rect)), paint;
> 
> This ensures you'll do the correct thing no matter the stroke width, whereas
the
> inset you've written basically only works if the stroke rect is exactly 1 (and
> then probably only for scale factor 1).

You're right, I misunderstood the discussion in the link (and Skia stroking more
generally). Thanks!

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:263: if (!IsOverlayScrollbarEnabled())
On 2016/10/17 19:10:24, Peter Kasting wrote:
> Nit: This conditional effectively results in duplicated code for the "fall
back
> to parent" case.  It feels slightly less error-prone to just do:
> 
>   if (IsOverlayScrollbarEnabled()) {
>     ... handle scrollbar constants ...
>   }
> 
>   return NativeThemeBase::GetPartSize(part, state, extra);

Done.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:264: return
NativeThemeBase::GetPartSize(part, state, extra);
On 2016/10/17 19:10:24, Peter Kasting wrote:
> Nit: 2 spaces

Done.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:277: NOTREACHED();
On 2016/10/17 19:10:25, Peter Kasting wrote:
> Is NOTREACHED really correct here, or should we just return 0?  Basically,
this
> implies that calling code needs to do a runtime check for overlay scrollbars
> before calling to get the size of these parts.  I can't decide whether that's
a
> good thing we want to enforce, or unnecessarily forces the caller to worry
about
> this.

I leaned towards forcing a runtime failure since I think asking for the
dimensions of one of these implies the calling code is doing something
incorrectly. This seems to be compatible with how the code works today. Also, we
already have a DCHECK for IsOverlayScrollbarEnabled() in the function above.

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:280: return
gfx::Size(kMDScrollbarMinimumLength + 2 * kMDScrollbarStrokeWidth,
On 2016/10/17 19:10:24, Peter Kasting wrote:
> Nit: You can factor this out to a constant above the switch and use it for
both
> these cases.

Done.

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

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_base.h:172: unsigned int scrollbar_width_;
On 2016/10/17 19:10:25, Peter Kasting wrote:
> Nit: unsigned int is banned.  I know you are just moving this code up, but try
> to change this to int.

Done. (also for button length below)

[bokan, 2016-10-17 23:21:15]

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

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

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

[Peter Kasting, 2016-10-18 00:43:37]

LGTM

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:37: const SkColor
kOverlayScrollbarThumbColor = SkColorSetRGB(0x0, 0x0, 0x0);
Nit: Use SK_ColorBLACK (and SK_ColorWHITE just below), and then these can be
constexpr as well.

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:215: // Inset the top and left edges for
filling in the stroke below.
Nit: Comment is inaccurate.

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:216: thumb_rect.Inset(kStrokeWidth,
kStrokeWidth, kStrokeWidth, kStrokeWidth);
Nit: Just the two-arg version will work.

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/overla...
File ui/native_theme/overlay_scrollbar_constants_aura.h (right):

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/overla...
ui/native_theme/overlay_scrollbar_constants_aura.h:17: constexpr int
kOverlayScrollbarFadeOutDurationMs = 200;
Nit: Does it make sense to do something like this?

constexpr base::TimeDelta kOverlayScrollbarFadeOutDelay =
    base::TimeDelta::FromMilliseconds(1000);
constexpr base::TimeDelta kOverlayScrollbarFadeOutDuration =
    base::TimeDelta::FromMilliseconds(200);

This is more verbose, but it prevents misuse of these numbers by people not
reading the unit names.

Of course, this would depend on also plumbing these as TimeDeltas, not ints,
through the rest of the system.

[commit-bot: I haz the power, 2016-10-18 01:24:07]

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

[commit-bot: I haz the power, 2016-10-18 01:24:08]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_TIMED_OUT, no build
URL)

[bokan, 2016-10-18 20:34:49]

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:37: const SkColor
kOverlayScrollbarThumbColor = SkColorSetRGB(0x0, 0x0, 0x0);
On 2016/10/18 00:43:36, Peter Kasting wrote:
> Nit: Use SK_ColorBLACK (and SK_ColorWHITE just below), and then these can be
> constexpr as well.

Done.

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:215: // Inset the top and left edges for
filling in the stroke below.
On 2016/10/18 00:43:36, Peter Kasting wrote:
> Nit: Comment is inaccurate.

Done.

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:216: thumb_rect.Inset(kStrokeWidth,
kStrokeWidth, kStrokeWidth, kStrokeWidth);
On 2016/10/18 00:43:36, Peter Kasting wrote:
> Nit: Just the two-arg version will work.

Done.

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/overla...
File ui/native_theme/overlay_scrollbar_constants_aura.h (right):

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/overla...
ui/native_theme/overlay_scrollbar_constants_aura.h:17: constexpr int
kOverlayScrollbarFadeOutDurationMs = 200;
On 2016/10/18 00:43:36, Peter Kasting wrote:
> Nit: Does it make sense to do something like this?
> 
> constexpr base::TimeDelta kOverlayScrollbarFadeOutDelay =
>     base::TimeDelta::FromMilliseconds(1000);
> constexpr base::TimeDelta kOverlayScrollbarFadeOutDuration =
>     base::TimeDelta::FromMilliseconds(200);
> 
> This is more verbose, but it prevents misuse of these numbers by people not
> reading the unit names.
> 
> Of course, this would depend on also plumbing these as TimeDeltas, not ints,
> through the rest of the system.

Done + plumbing

[bokan, 2016-10-18 20:36:32]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org, jbroman@chromium.org, wez@chromium.org

[bokan, 2016-10-18 20:36:33]

+wez@ for blimp
+jbroman@ for Source/platform
+aelias@ for cc

[aelias_OOO_until_Jul13, 2016-10-18 20:37:00]

aelias@chromium.org changed reviewers:
- jbroman@chromium.org, wez@chromium.org

[aelias_OOO_until_Jul13, 2016-10-18 20:37:00]

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/2421913002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:213:
canvas->drawRect(gfx::RectFToSkRect(stroke_rect), paint);
As I mentioned on http://crbug.com/304869#c36, I think this is moving in the
wrong direction.  You're switching Aura to perform raster on a non-raster
thread, and also causing CC to perform a texture upload and block activation on
scrollbars, and all this cost (and it's real cost, painted scrollbars were a
constant jank culprit in traces for the ~6 months Clank tried to use them) at no
actual value-added except for "code unification".  But you won't even get the
unification because Android is not interested in this bad approach.

I'm on board with the principle of unification but I'd like to do it the
following way instead:
1) Merge Android, Aura, and Mac overlay scrollbar code, without making them more
entangled with Blink/raster or otherwise sacrificing significant efficiency
(except that I am willing to tolerate switching Android to a singleton shared
ninepatch UIResource which would allow Aura hollow scrollbars and Mac
rounded-corner scrollbars, which I consider a negligible cost if done properly).
2) Relegate "Painted Scrollbars" to only be used for OS-system-call-mystery-meat
scrollbars and -webkit custom scrollbars, which is what their raster-heavy
design is for in the first place.

I don't think 1 and 2 have enough in common to be merged, but there is a lot to
be merged within category "1", which you are currently *un*-merging.

[jbroman, 2016-10-18 20:39:25]

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

[jbroman, 2016-10-18 20:39:26]

platform lgtm with nit

https://codereview.chromium.org/2421913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp (right):

https://codereview.chromium.org/2421913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp:133: int
thickness = Platform::current()
nit: this fetches the thickness every time, even though it's only used if
|theme| hasn't yet been initialized. Consider putting this inline where
|thickness| is, or if that doesn't fit well, extracting it to a function that
you can invoke there.

[aelias_OOO_until_Jul13, 2016-10-18 20:45:11]

aelias@chromium.org changed reviewers:
- jbroman@chromium.org

[aelias_OOO_until_Jul13, 2016-10-18 20:45:12]

OK, our messages crossed.  bokan@ made a good point in
http://crbug.com/304869#c38 about LCD text forcing Aura to support approach 2)
until SPv2 ships.  At which point there is no reason for Aura to also support
approach 1), for now, unless they really care about efficiency (and on desktops
my concerns might be more in the noise).  I guess it's unfortunate but correct
to leave Android as special snowflake for now, and perhaps go with the plan I
described above as a long-term one after the LCD text problem is architecturally
solved.  lgtm

[bokan, 2016-10-18 20:51:46]

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

[bokan, 2016-10-18 20:51:47]

sgtm, thanks.

+wez@ again for blimp

[bokan, 2016-10-18 21:22:54]

Description was changed from

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single, central location. I've
also updated the values to match the latest spec. Follow-up patches will make
use of these values from the compositor.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme. If the scroller is composited, these scrollbars will now be uploaded to
the compositor as PaintedScrollbarLayers rather than SolidColorScrollbarLayers.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single location in
ui/native_theme. I've also updated the values to match the latest spec.
Follow-up patches will make use of these values from the compositor.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[bokan, 2016-10-18 21:39:48]

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

https://codereview.chromium.org/2421913002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura.cc:277: NOTREACHED();
On 2016/10/17 19:10:25, Peter Kasting wrote:
> Is NOTREACHED really correct here, or should we just return 0?  Basically,
this
> implies that calling code needs to do a runtime check for overlay scrollbars
> before calling to get the size of these parts.  I can't decide whether that's
a
> good thing we want to enforce, or unnecessarily forces the caller to worry
about
> this.

Ok, I found a case where this did get tripped. When we create a message box that
creates a Views::ScrollView which asks for some of these dimensions, looks to me
like Views code isn't aware of layout scrollbars yet. I've removed this
NOTREACHED and just let the base method handle this so today's behavior remains
unchanged. I've added a TODO and will address this when I look at Views
specifically.

[Wez, 2016-10-18 21:41:51]

blimp/ LGTM

https://codereview.chromium.org/2421913002/diff/140001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_layer_tree_settings.cc (right):

https://codereview.chromium.org/2421913002/diff/140001/blimp/client/support/c...
blimp/client/support/compositor/blimp_layer_tree_settings.cc:66:
base::TimeDelta::FromMilliseconds(2000);
nit: FromSeconds(2)

[bokan, 2016-10-18 21:59:53]

https://codereview.chromium.org/2421913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp (right):

https://codereview.chromium.org/2421913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeAura.cpp:133: int
thickness = Platform::current()
On 2016/10/18 20:39:26, jbroman wrote:
> nit: this fetches the thickness every time, even though it's only used if
> |theme| hasn't yet been initialized. Consider putting this inline where
> |thickness| is, or if that doesn't fit well, extracting it to a function that
> you can invoke there.

Good catch, done.

https://codereview.chromium.org/2421913002/diff/140001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_layer_tree_settings.cc (right):

https://codereview.chromium.org/2421913002/diff/140001/blimp/client/support/c...
blimp/client/support/compositor/blimp_layer_tree_settings.cc:66:
base::TimeDelta::FromMilliseconds(2000);
On 2016/10/18 21:41:51, Wez wrote:
> nit: FromSeconds(2)

Done.

[bokan, 2016-10-18 22:37:01]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-10-18 22:37:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, aelias@chromium.org, jbroman@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2421913002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-18 22:37:31]

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-10-19 01:38:20]

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

[commit-bot: I haz the power, 2016-10-19 01:38:21]

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-10-19 02:15:23]

The CQ bit was checked by bokan@chromium.org

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

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-10-19 03:22:37]

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

[commit-bot: I haz the power, 2016-10-19 03:22:38]

Exceeded global retry quota

[bokan, 2016-10-19 10:30:30]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-10-19 10:31: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-10-19 10:40:46]

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

[commit-bot: I haz the power, 2016-10-19 10:40:48]

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-10-19 12:45:05]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-10-19 12:45:06]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
pkasting@chromium.org, jbroman@chromium.org, wez@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2421913002/#ps220001 (title: "Rebase")

[commit-bot: I haz the power, 2016-10-19 12:45:30]

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-10-19 15:08:23]

Description was changed from

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme. If the scroller is composited, these scrollbars will now be uploaded to
the compositor as PaintedScrollbarLayers rather than SolidColorScrollbarLayers.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single location in
ui/native_theme. I've also updated the values to match the latest spec.
Follow-up patches will make use of these values from the compositor.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme. If the scroller is composited, these scrollbars will now be uploaded to
the compositor as PaintedScrollbarLayers rather than SolidColorScrollbarLayers.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single location in
ui/native_theme. I've also updated the values to match the latest spec.
Follow-up patches will make use of these values from the compositor.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-19 15:08:24]

Committed patchset #12 (id:220001)

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

Description was changed from

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme. If the scroller is composited, these scrollbars will now be uploaded to
the compositor as PaintedScrollbarLayers rather than SolidColorScrollbarLayers.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single location in
ui/native_theme. I've also updated the values to match the latest spec.
Follow-up patches will make use of these values from the compositor.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Change Aura overlay scrollbars from solid color to painted scrollbars.

With this change, the Aura overlay scrollbars are now painted by the native
theme. If the scroller is composited, these scrollbars will now be uploaded to
the compositor as PaintedScrollbarLayers rather than SolidColorScrollbarLayers.

Additionally, many of the MD scrollbar constants and properties are scattered
around the code. This patch moves them into a single location in
ui/native_theme. I've also updated the values to match the latest spec.
Follow-up patches will make use of these values from the compositor.

BUG=592098, 652520
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/3d914bde3081131a655b467c1a7a232c7c065007
Cr-Commit-Position: refs/heads/master@{#426191}
==========

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

Patchset 12 (id:??) landed as
https://crrev.com/3d914bde3081131a655b467c1a7a232c7c065007
Cr-Commit-Position: refs/heads/master@{#426191}