Make IsSolidColor() a property on CC scrollbar layers.

Make IsSolidColor() a property on CC scrollbar layers.

We need a mechanism to allow scrollbar layers to be "solid color" on a
per-scrollbar-layer basis, rather than the current all-on-none situation
controlled by a global parameter. This CL allows (1) forcing all scrollbar
layers to be solid-color via a global parameter, or (2) marking individual
scrollbar layers as solid-color via a constructor paremeter.

Existing unit tests for solid-color scrollbar layers have been parameterized to
test against both mechanisms.

BUG=none

[wjmaclean, 2013-06-21 15:54:34]

Please take a look and let me know if this seems sane. I made the settings
parameter "force" as suggested by aelias@, and modified the existing unit tests
for solid color scrollbars to test both for settings()-driven and
per-scrollbar-parameter based cases.

[aelias_OOO_until_Jul13, 2013-06-21 23:34:34]

Do you have any plans to expose the solid color setting to Blink?

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer.cc#...
cc/layers/scrollbar_layer.cc:69: (layer_tree_host() &&
Please DCHECK(layer_tree_host()) instead.

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer_imp...
File cc/layers/scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer_imp...
cc/layers/scrollbar_layer_impl.cc:161: (layer_tree_impl() &&
DCHECK(layer_tree_impl()).  (Or you could just get rid of the branch on this
side by pushing the value of IsSolidColor() in PushPropertiesTo()).

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer_uni...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer_uni...
cc/layers/scrollbar_layer_unittest.cc:34: bool reverse_order, bool
is_solid_color = false) {
Default params aren't allowed by the style guide.

[wjmaclean, 2013-06-24 14:06:41]

Addressed comments, PTAL.

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer.cc#...
cc/layers/scrollbar_layer.cc:69: (layer_tree_host() &&
On 2013/06/21 23:34:35, aelias wrote:
> Please DCHECK(layer_tree_host()) instead.

Actually, the only reason it's there is that there are unit tests that don't set
layer tree host (so they'll crash without this).

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer_imp...
File cc/layers/scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer_imp...
cc/layers/scrollbar_layer_impl.cc:161: (layer_tree_impl() &&
On 2013/06/21 23:34:35, aelias wrote:
> DCHECK(layer_tree_impl()).  (Or you could just get rid of the branch on this
> side by pushing the value of IsSolidColor() in PushPropertiesTo()).

I thought about pushing it as a property, though I wasn't sure if we'd ever want
to distinguish (at some later time) between a layer that was created "solid"
from one that just picks up the property.

I'll change it so we push the value over.

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer_uni...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer_uni...
cc/layers/scrollbar_layer_unittest.cc:34: bool reverse_order, bool
is_solid_color = false) {
On 2013/06/21 23:34:35, aelias wrote:
> Default params aren't allowed by the style guide.

Removed.

[wjmaclean, 2013-06-24 14:18:08]

On 2013/06/21 23:34:34, aelias wrote:
> Do you have any plans to expose the solid color setting to Blink?

No ... the Scrollbar layers will now be created in CC and attached to the
structure created in Blink, so there's no need for Blink to see this.

[aelias_OOO_until_Jul13, 2013-06-25 06:54:57]

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer.cc#...
cc/layers/scrollbar_layer.cc:69: (layer_tree_host() &&
On 2013/06/24 14:06:41, wjmaclean wrote:
> On 2013/06/21 23:34:35, aelias wrote:
> > Please DCHECK(layer_tree_host()) instead.
> 
> Actually, the only reason it's there is that there are unit tests that don't
set
> layer tree host (so they'll crash without this).

Can we fix the unit tests instead?  I feel this production change is slightly
dangerous.  There's a risk of creating a texture in the interval before the
layer_tree_host is available.

[wjmaclean, 2013-06-25 13:50:53]

On 2013/06/25 06:54:57, aelias wrote:
> https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer.cc
> File cc/layers/scrollbar_layer.cc (right):
> 
>
https://codereview.chromium.org/17550008/diff/1/cc/layers/scrollbar_layer.cc#...
> cc/layers/scrollbar_layer.cc:69: (layer_tree_host() &&
> On 2013/06/24 14:06:41, wjmaclean wrote:
> > On 2013/06/21 23:34:35, aelias wrote:
> > > Please DCHECK(layer_tree_host()) instead.
> > 
> > Actually, the only reason it's there is that there are unit tests that don't
> set
> > layer tree host (so they'll crash without this).
> 
> Can we fix the unit tests instead?  I feel this production change is slightly
> dangerous.  There's a risk of creating a texture in the interval before the
> layer_tree_host is available.

I assume you mean to avoid regressions sneaking in? Done.

[wjmaclean, 2013-06-25 13:51:21]

PTAL.

[aelias_OOO_until_Jul13, 2013-06-26 17:06:48]

lgtm (although you'll also need jamesr@'s approval for OWNERS)

[enne (OOO), 2013-06-26 17:39:05]

https://codereview.chromium.org/17550008/diff/13001/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/17550008/diff/13001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer.cc:68: DCHECK(layer_tree_host());
This seems dangerous.  cc::Layers can be detached from their host at any point.

What about having WebScrollbarLayerImpl check the command line for this force
flag and pass that to the ScrollbarLayer constructor, in the same way that we do
for making WebContentLayerImpl decide to be backed by ContentLayer vs.
PictureLayer?

https://codereview.chromium.org/17550008/diff/13001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/17550008/diff/13001/cc/trees/layer_tree_impl....
cc/trees/layer_tree_impl.cc:226: // FIXME: This logic will need to change when
we re-factor RootScrollLayer()
FIXME => TODO(wjmaclean)

[wjmaclean, 2013-06-26 18:16:00]

https://codereview.chromium.org/17550008/diff/13001/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/17550008/diff/13001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer.cc:68: DCHECK(layer_tree_host());
On 2013/06/26 17:39:05, enne wrote:
> This seems dangerous.  cc::Layers can be detached from their host at any
point.
> 
> What about having WebScrollbarLayerImpl check the command line for this force
> flag and pass that to the ScrollbarLayer constructor, in the same way that we
do
> for making WebContentLayerImpl decide to be backed by ContentLayer vs.
> PictureLayer?

There isn't a command line flag for force_solid_color_scrollbars. It only exists
as a layer tree setting, meaning that the value of this could (theoretically)
differ between RenderWidgetCompositor instances.

My original design tried to get around this by retaining a distinction between
(1) the is_solid_color_ value the scrollbar was created with, and (2) the
settings of the currently attached layer_tree_host_, and or-ing them to
determine what to do.

WebScrollbarLayerImpl's default to false then unless overriden by the settings,
and ScrollbarLayers created to be pinch-inner-viewport scrollbars have this
value set at construction time.

Could we go back to that?

I'm assuming that WebScrollbarLayerImpl shouldn't know about which compositor
instance will use it until it's attached to a tree, is that right?

https://codereview.chromium.org/17550008/diff/13001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/17550008/diff/13001/cc/trees/layer_tree_impl....
cc/trees/layer_tree_impl.cc:226: // FIXME: This logic will need to change when
we re-factor RootScrollLayer()
On 2013/06/26 17:39:05, enne wrote:
> FIXME => TODO(wjmaclean)

Done.

[enne (OOO), 2013-06-26 20:02:10]

https://codereview.chromium.org/17550008/diff/13001/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/17550008/diff/13001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer.cc:68: DCHECK(layer_tree_host());
On 2013/06/26 18:16:00, wjmaclean wrote:
> On 2013/06/26 17:39:05, enne wrote:
> > This seems dangerous.  cc::Layers can be detached from their host at any
> point.
> > 
> > What about having WebScrollbarLayerImpl check the command line for this
force
> > flag and pass that to the ScrollbarLayer constructor, in the same way that
we
> do
> > for making WebContentLayerImpl decide to be backed by ContentLayer vs.
> > PictureLayer?
> 
> There isn't a command line flag for force_solid_color_scrollbars. It only
exists
> as a layer tree setting, meaning that the value of this could (theoretically)
> differ between RenderWidgetCompositor instances.

Right now it can't differ, and is always true on Android:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/g...

I'm not sure allowing it to differ per compositor is a feature we should
consider supporting.  I suspect moving a ScrollbarLayer between hosts with
different solid color scrollbars probably would behave quite poorly.

[aelias_OOO_until_Jul13, 2013-06-26 23:06:20]

A new command line sounds fine to me.  It can be appended in
content/browser/android/content_startup_flags.cc

[wjmaclean, 2013-06-27 13:15:51]

On 2013/06/26 23:06:20, aelias wrote:
> A new command line sounds fine to me.  It can be appended in
> content/browser/android/content_startup_flags.cc

OK, I'll plumb this and post a cl shortly.

[wjmaclean, 2013-06-27 17:54:02]

On 2013/06/27 13:15:51, wjmaclean wrote:
> On 2013/06/26 23:06:20, aelias wrote:
> > A new command line sounds fine to me.  It can be appended in
> > content/browser/android/content_startup_flags.cc

I've added this, and revised the rest accordingly (aelias@, please look at the
changes in RenderWidgetCompositor, I wasn't sure exactly which Android settings
could be grouped under the --force-solid-color-scrollbar flag).

PTAL

[enne (OOO), 2013-06-27 18:01:54]

lgtm.  Thanks for the changes.  That's a lot cleaner.

https://codereview.chromium.org/17550008/diff/24001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/17550008/diff/24001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc:17: static bool
forceSolidColorScrollbars() {
style nit: this is Chromium code, so the 'f' should be capitalized. 
setScrollLayer is named as it is because it's an override from WebKit.

[wjmaclean, 2013-06-27 18:10:31]

Thanks. I'll wait to make sure it clears the Android bots before I try to submit
it (and I get aelias@'s feedback as well).

https://codereview.chromium.org/17550008/diff/24001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/17550008/diff/24001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc:17: static bool
forceSolidColorScrollbars() {
On 2013/06/27 18:01:54, enne wrote:
> style nit: this is Chromium code, so the 'f' should be capitalized. 
> setScrollLayer is named as it is because it's an override from WebKit.

I'll fix this. I wasn't sure what to do, as the similar helper function in
WebContentLayerImpl had a lowercase letter for the start of the name.

[wjmaclean, 2013-06-28 13:16:20]

+Daniel Sievers for changes to 	
content/browser/android/content_startup_flags.cc

[wjmaclean, 2013-06-28 16:12:14]

-Daniel Sievers (unavailable)
+Marcus Bulach for content/browser/android changes

[bulach, 2013-06-28 16:59:09]

lgtm for content/browser/android

[aelias_OOO_until_Jul13, 2013-06-28 17:31:59]

https://codereview.chromium.org/17550008/diff/24001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/17550008/diff/24001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:281: } else if
(settings.use_pinch_virtual_viewport) {
I don't think these conditionals make a lot of sense, they are overly specific
to the current needs.  (For example, Android will eventually turn on
settings.use_pinch_virtual_viewport).

How about adding two more flags, one --solid-color-scrollbar-color=(whatever
color encoding makes sense) and --solid-color-scrollbar-thickness=?  (I was
already planning to add a color flag to change it in AndroidWebView
configuration anyway.)

[wjmaclean, 2013-06-28 17:34:17]

On 2013/06/28 17:31:59, aelias wrote:
>
https://codereview.chromium.org/17550008/diff/24001/content/renderer/gpu/rend...
> File content/renderer/gpu/render_widget_compositor.cc (right):
> 
>
https://codereview.chromium.org/17550008/diff/24001/content/renderer/gpu/rend...
> content/renderer/gpu/render_widget_compositor.cc:281: } else if
> (settings.use_pinch_virtual_viewport) {
> I don't think these conditionals make a lot of sense, they are overly specific
> to the current needs.  (For example, Android will eventually turn on
> settings.use_pinch_virtual_viewport).

Yeah, I figured when Android was ready to start using it we'd make it more
generic.
 
> How about adding two more flags, one --solid-color-scrollbar-color=(whatever
> color encoding makes sense) and --solid-color-scrollbar-thickness=?  (I was
> already planning to add a color flag to change it in AndroidWebView
> configuration anyway.)

OK, I'll look into this.

[wjmaclean, 2013-06-28 19:48:22]

I've added command line flags for color/thickness of the solid color scrollbars.

aelias@ and buchas@ - does this still lgt m?

[bulach, 2013-06-28 19:55:47]

still lgtm, but one suggestion below:

https://codereview.chromium.org/17550008/diff/38001/content/browser/android/c...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/17550008/diff/38001/content/browser/android/c...
content/browser/android/content_startup_flags.cc:67:
thickness_string.append("=3");
nit: maybe extract these as named constants up above?
something like:

namespace {
const char kSolidColorScrollbarThicknessValue[] = "=3";
const char kSolidColorScrollbarColorValue[] = "=0x80808080";
}  // namespace

[wjmaclean, 2013-06-28 20:08:20]

On 2013/06/28 19:55:47, bulach wrote:
> still lgtm, but one suggestion below:
> 
>
https://codereview.chromium.org/17550008/diff/38001/content/browser/android/c...
> File content/browser/android/content_startup_flags.cc (right):
> 
>
https://codereview.chromium.org/17550008/diff/38001/content/browser/android/c...
> content/browser/android/content_startup_flags.cc:67:
> thickness_string.append("=3");
> nit: maybe extract these as named constants up above?
> something like:
> 
> namespace {
> const char kSolidColorScrollbarThicknessValue[] = "=3";
> const char kSolidColorScrollbarColorValue[] = "=0x80808080";
> }  // namespace

Good idea, thanks!

[aelias_OOO_until_Jul13, 2013-06-28 22:07:57]

lgtm provided you fix the problems below and double-check the color setting
works before submitting.

https://codereview.chromium.org/17550008/diff/39023/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/17550008/diff/39023/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:80: static SkColor
StringToSkColor(const std::string& str, SkColor& color) {
Did you mean to change the return value to 'bool'?

https://codereview.chromium.org/17550008/diff/39023/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:89: color =
SkColorSetARGB(values[0], values[1], values[2], values[3]);
Should be "return true;" after this?

[wjmaclean, 2013-06-29 01:00:37]

Fixing and verifying now, new patch in a moment.

https://codereview.chromium.org/17550008/diff/39023/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/17550008/diff/39023/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:80: static SkColor
StringToSkColor(const std::string& str, SkColor& color) {
On 2013/06/28 22:07:58, aelias wrote:
> Did you mean to change the return value to 'bool'?

Gah! Yes, this should be bool!! I forgot SkColor resolves to int ...

https://codereview.chromium.org/17550008/diff/39023/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:89: color =
SkColorSetARGB(values[0], values[1], values[2], values[3]);
On 2013/06/28 22:07:58, aelias wrote:
> Should be "return true;" after this?

Yes, fixed.

[jochen (gone - plz use gerrit), 2013-06-30 08:13:07]

just some nits.

You really want James to look at CC related changes, not me

https://codereview.chromium.org/17550008/diff/51001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/17550008/diff/51001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:80: static bool
StringToSkColor(const std::string& str, SkColor& color) {
nit. move into the anonymous namespace instead of making this static

https://codereview.chromium.org/17550008/diff/51001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:300: if (base::StringToInt(str,
&thickness))
what happens if the thickness is negative or zero?

https://codereview.chromium.org/17550008/diff/51001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/17550008/diff/51001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc:31:
ForceSolidColorScrollbars()))) {}
why not just invoking cc::switches::ForceSolidColorScrollbars() directly?

[wjmaclean, 2013-07-02 13:50:30]

jamesr@ ... can you please take a look?

https://codereview.chromium.org/17550008/diff/51001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/17550008/diff/51001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:80: static bool
StringToSkColor(const std::string& str, SkColor& color) {
On 2013/06/30 08:13:07, jochen wrote:
> nit. move into the anonymous namespace instead of making this static

Done.

https://codereview.chromium.org/17550008/diff/51001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:300: if (base::StringToInt(str,
&thickness))
On 2013/06/30 08:13:07, jochen wrote:
> what happens if the thickness is negative or zero?

Done.

Change so that if the user enters a negative thickness, they get the default
value for thickness (i.e. as if the flag was never specified).

https://codereview.chromium.org/17550008/diff/51001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/17550008/diff/51001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc:31:
ForceSolidColorScrollbars()))) {}
On 2013/06/30 08:13:07, jochen wrote:
> why not just invoking cc::switches::ForceSolidColorScrollbars() directly?

Done.

[jamesr, 2013-07-03 21:26:24]

https://codereview.chromium.org/17550008/diff/56001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/17550008/diff/56001/cc/base/switches.cc#newco...
cc/base/switches.cc:134: // Allow forcing of solid-color scrollbars
what does it mean to "allow forcing"?  Does the flag force solid color
scrollbars on or not?

https://codereview.chromium.org/17550008/diff/56001/cc/base/switches.cc#newco...
cc/base/switches.cc:144: bool ForceSolidColorScrollbars() {
the function name looks like it's doing something, not querying a value. can you
come up with a name that reads better? i expect typically calling code to look
like

if (Something()) { .... }

so you want the function name to make that line read naturally as

"if solid color scrollbars are forced on then ...."

as is, the line would be

if (ForceSolidColorScrollbars()) { .... }

which looks like the body of the if() is causing something to happen, not
querying a value

https://codereview.chromium.org/17550008/diff/56001/cc/layers/scrollbar_layer.h
File cc/layers/scrollbar_layer.h (right):

https://codereview.chromium.org/17550008/diff/56001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer.h:27: bool is_solid_color);
this being a bool makes callsites really hard to read.  without reading this
header, it's hard to tell what the difference between
cc::ScrollbarLayer::Create(..., false) and (..., true) means. can you make this
an enum like Orientation?

https://codereview.chromium.org/17550008/diff/56001/cc/layers/scrollbar_layer...
File cc/layers/scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/17550008/diff/56001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer_impl.cc:86: !is_solid_color())
no need to use a getter here, just access the bool

https://codereview.chromium.org/17550008/diff/56001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer_impl.cc:106: if (is_solid_color()) {
ditto

https://codereview.chromium.org/17550008/diff/56001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer_impl.cc:252: if (is_solid_color()) {
ditto

https://codereview.chromium.org/17550008/diff/56001/cc/layers/scrollbar_layer...
File cc/layers/scrollbar_layer_impl.h (right):

https://codereview.chromium.org/17550008/diff/56001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer_impl.h:44: void set_is_solid_color(bool
is_solid_color);
inline these if you're going to use hacker_style names. also, don't squeeze this
in between the Orientation() and CurrentPos()/Maximum() functions, it breaks up
the flow of the header for no good reason

[wjmaclean, 2013-07-04 17:17:32]

I propose that the approach in https://codereview.chromium.org/18341009/ is
superior to, and addresses the same issues as this one, and that we should close
this issue.