Implement pinch-zoom scaling for main-frame scrollbars and pinch-zoom overlay scrollbars.

Implement pinch-zoom scaling for main-frame scrollbars and pinch-zoom overlay scrollbars.

BUG=165842
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188500

[wjmaclean, 2012-12-13 02:17:50]

Hi - Here's a first draft of the pinch-zoom scrollbars CL.

Nat, I understand you're re-factoring pinch-zoom code, so I guess you'll want to
see this.

Enne - I'll clean up the style in the new files before I land (d'oh, coded in WK
style by default), so don't worry about wasting time on WK vs. Chromium style.

How does the approach look? I kept all the mechanics on the impl-side, though I
did have to hang a dummy node on the main-thread side to get the
ScrollbarLayerImpls attached. There's some mind-numbing boiler-plate (which can
be reduced in a future re-factoring cl, but I didn't want to mess too much with
the GeometryFixedThumb classes in this CL).

Overall, it does seems to achieve the desired effect.

If at all possible, I'd like to try and land a revised version of this for M25.

[enne (OOO), 2012-12-13 17:43:37]

At a high level, this patch needs some design massage.  Please encapsulate as
much complexity as possible in layer subclasses behind existing layer update
APIs, rather than pushing that complexity up into the LTHI or even in the
scrollbar manager.  These scrollbar layers are getting a special manual
treatment that increase complexity in a way that I don't think they merit.

Also, tests.  You're creating resources, so this needs a lost context test.  You
could create a test that inserts these layers and verifies that the scrollbar
values return the right things for certain layer configurations.

I'll be honest and say I don't think this can be landed by the m25 branch cut,
since Nat and I are both super busy and today is the first time we've seen this
code.

https://codereview.chromium.org/11550035/diff/2002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11550035/diff/2002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:394: // TODO(wjmaclean): make sure this is revised to
work with multiple LayerTreeImpls.
Whoa, why is the PinchZoomScrollbarsManager on the tree and not the host? It
seems to me that the layers from the main thread side have no state, so there's
no reason to have them on the pending tree.

Also, if you create them here and don't sync, then you don't need to do anything
on the layer side.  You can just create them here and hand them to the manager. 
(Or remove them from the previous tree and insert them in the next active tree
during tree swap.)

https://codereview.chromium.org/11550035/diff/2002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:904: PinchZoomScrollbarsManager* pinchZoomScrollbars
= activeTree()->pinch_zoom_scrollbars_manager();
I would like nearly all the changes in this file to go away.  This adds way too
much complexity to the already overwhelming LTHI.  We already have abstractions
for updating layers.  You could give the manager an LTHI pointer or the layers
themselves could ask for their LTHI and then get the right device scale /
viewport / etc.

The pinch zoom scrollbar manager doesn't need to be special cased any time one
of these values changed.  You should just look them up when you need them.

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbar_la...
File cc/pinch_zoom_scrollbar_layer.h (right):

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbar_la...
cc/pinch_zoom_scrollbar_layer.h:13: class PinchZoomScrollbarLayer : public Layer
{
I'm really not seeing what this is for.  Can you just store the id of the layer
you want to attach the layer impls to rather than creating layers in the tree? 
That would eliminate these classes and 90% of the code that you've added in LTH.

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
File cc/pinch_zoom_scrollbars_manager.h (right):

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
cc/pinch_zoom_scrollbars_manager.h:99: class PZScrollbarGeometry : public
ScrollbarGeometryFixedThumb {
Why are you using FixedThumb?

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
cc/pinch_zoom_scrollbars_manager.h:138: ScrollbarLayerImpl*
m_pinchZoomScrollbarVertical;
This class is way too smart and the layers are way too dumb.  Why do you
duplicate everything here? Why not just put everything once on the layer? It'd
actually be pretty useful to have something that inherits from
ScrollbarLayerImpl but can paint its scrollbar on the impl thread.

[wjmaclean, 2012-12-13 18:26:50]

https://codereview.chromium.org/11550035/diff/2002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11550035/diff/2002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:394: // TODO(wjmaclean): make sure this is revised to
work with multiple LayerTreeImpls.
On 2012/12/13 17:43:38, enne wrote:
> Whoa, why is the PinchZoomScrollbarsManager on the tree and not the host? It
> seems to me that the layers from the main thread side have no state, so
there's
> no reason to have them on the pending tree.
> 
> Also, if you create them here and don't sync, then you don't need to do
anything
> on the layer side.  You can just create them here and hand them to the
manager. 
> (Or remove them from the previous tree and insert them in the next active tree
> during tree swap.)

OK, I will re-do with the manager on LTHI. I considered both options, but as I
was unaware of how the tree-swap mechanics would look I wasn't sure how easily
I'd be able to do the remove/insert.

https://codereview.chromium.org/11550035/diff/2002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:904: PinchZoomScrollbarsManager* pinchZoomScrollbars
= activeTree()->pinch_zoom_scrollbars_manager();
On 2012/12/13 17:43:38, enne wrote:
> I would like nearly all the changes in this file to go away.  This adds way
too
> much complexity to the already overwhelming LTHI.  We already have
abstractions
> for updating layers.  You could give the manager an LTHI pointer or the layers
> themselves could ask for their LTHI and then get the right device scale /
> viewport / etc.

OK, I'll look into doing this. I had hoped this would minimize the number of
calls required to transfer the information.

> The pinch zoom scrollbar manager doesn't need to be special cased any time one
> of these values changed.  You should just look them up when you need them.

OK.

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbar_la...
File cc/pinch_zoom_scrollbar_layer.h (right):

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbar_la...
cc/pinch_zoom_scrollbar_layer.h:13: class PinchZoomScrollbarLayer : public Layer
{
On 2012/12/13 17:43:38, enne wrote:
> I'm really not seeing what this is for.  Can you just store the id of the
layer
> you want to attach the layer impls to rather than creating layers in the tree?

> That would eliminate these classes and 90% of the code that you've added in
LTH.

How do I get a valid layer ID to use when creating my SLI's?

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
File cc/pinch_zoom_scrollbars_manager.h (right):

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
cc/pinch_zoom_scrollbars_manager.h:99: class PZScrollbarGeometry : public
ScrollbarGeometryFixedThumb {
On 2012/12/13 17:43:38, enne wrote:
> Why are you using FixedThumb?

Primarily because ScrollbarLayerImpl requires this type, and I was trying to
minimize the changes. I'll look into deriving from SLI to achieve this with less
overhead.

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
cc/pinch_zoom_scrollbars_manager.h:138: ScrollbarLayerImpl*
m_pinchZoomScrollbarVertical;
On 2012/12/13 17:43:38, enne wrote:
> This class is way too smart and the layers are way too dumb.  Why do you
> duplicate everything here? Why not just put everything once on the layer? It'd
> actually be pretty useful to have something that inherits from
> ScrollbarLayerImpl but can paint its scrollbar on the impl thread.

OK, I'll create a derived class to do it.

[enne (OOO), 2012-12-13 18:57:42]

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
File cc/pinch_zoom_scrollbars_manager.h (right):

https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
cc/pinch_zoom_scrollbars_manager.h:99: class PZScrollbarGeometry : public
ScrollbarGeometryFixedThumb {
On 2012/12/13 18:26:50, wjmaclean wrote:
> On 2012/12/13 17:43:38, enne wrote:
> > Why are you using FixedThumb?
> 
> Primarily because ScrollbarLayerImpl requires this type, and I was trying to
> minimize the changes. I'll look into deriving from SLI to achieve this with
less
> overhead.

Ah, just curious.  FixedThumb is there because ScrollbarLayerImpl can't
regenerate content, so it needs to keep the thumb at a fixed size.  It just
didn't seem like that restriction was needed here.

[wjmaclean, 2012-12-13 19:25:48]

On 2012/12/13 18:57:42, enne wrote:
>
https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
> File cc/pinch_zoom_scrollbars_manager.h (right):
> 
>
https://codereview.chromium.org/11550035/diff/2002/cc/pinch_zoom_scrollbars_m...
> cc/pinch_zoom_scrollbars_manager.h:99: class PZScrollbarGeometry : public
> ScrollbarGeometryFixedThumb {
> On 2012/12/13 18:26:50, wjmaclean wrote:
> > On 2012/12/13 17:43:38, enne wrote:
> > > Why are you using FixedThumb?
> > 
> > Primarily because ScrollbarLayerImpl requires this type, and I was trying to
> > minimize the changes. I'll look into deriving from SLI to achieve this with
> less
> > overhead.
> 
> Ah, just curious.  FixedThumb is there because ScrollbarLayerImpl can't
> regenerate content, so it needs to keep the thumb at a fixed size.  It just
> didn't seem like that restriction was needed here.

Indeed. It's sort of a fault I have, always trying to change as little as
possible around me when adding something, thus I stuck with what was there ;-)

[wjmaclean, 2012-12-21 21:27:26]

Just uploading work in progress (for safe keeping). I won't get back to this
until after the holidays, so not looking for a review at this time.

[wjmaclean, 2013-01-03 18:28:29]

I'm currently working on unit tests to go with this, but would like some initial
feedback on the (revised) approach.

I will break this up into at least two, perhaps three, CLs for landing (external
layer id generation, applying transform to children of root, pz scrollbars).

The style in this CL is a bit of a combination of Chromium/WK ... I'm aware of
this and will sort it out in individual patches before landing.

[jamesr, 2013-01-04 18:54:12]

Why would you want to create+destroy new layers on the impl thread?  Do you not
know ahead of time that you'll need these?  How many do you expect to create?

[wjmaclean, 2013-01-04 19:34:12]

On 2013/01/04 18:54:12, jamesr wrote:
> Why would you want to create+destroy new layers on the impl thread?

To avoid needing to have a layer object on the main-thread side that does
nothing [since PZ scrollbars have all their functionality on the impl-side].

> Do you not know ahead of time that you'll need these?
> How many do you expect to create?

We do know in advance, we'll need exactly two. They may not always be visible,
but the layers are created and in place for when zooming occurs. The layers will
re-create themselves in the event they get destroyed (we have to give up
ownership to attach them to the active impl-tree, so we can't guarantee what
happens to them).

I tried keep thing simpler by just having two and swapping them between the two
trees, as this reduces resource allocation, but there is additional overhead to
do the swapping.

[wjmaclean, 2013-01-11 21:31:43]

Here's a new version with simplified lifetime management: basically, we just
keep pushing lightweight wrapper layers into the impl tree on each sync.

To avoid recreating (and re-painting) the scrollbars unnecessarily, the layers
have clients that hold the state, including the textures. Thus destruction of a
layer doesn't necessarily require repainting. The PinchZoomViewport owns the
clients.

I've included the code to apply implTransform  to the root's children just to
make the patch complete, but it should land separately.

I've avoided trying to generate layer IDs (as per jamesr's suggestions), but I
also wanted to do it impl-side only as per enne's request, so have hard coded
two layer IDs as negative values, for now at least.

I'd be interested in feedback on the approach.

[aelias_OOO_until_Jul13, 2013-01-15 20:12:36]

CC doesn't have a concept of impl-side only layer.  That really doesn't fit with
CC's architecture and is likely to create problems, since a lot of layer-tree
walking code assumes the impl tree is a clone of the main-thread tree.  I agree
with James that you should create a non-impl layer class corresponding to your
impl layer.  It's fine if that class  contains very little code.

[Ian Vollick, 2013-01-15 20:33:48]

I wonder if it would be useful to add the concept of an impl-only layer?

Thoughts on how this might be approached:
 - impl-only layers have negative ids so as not to conflict with ordinary
layers.
 - impl-only layers are registered with the LTHI (like anim controllers)
 - impl-only layers would be able to add themselves to a LayerTreeImpl 
 - after a commit the LHTI will ensure that the impl-only layers are in the
tree. (If possible. If their desired home doesn't exist, they'll be orphaned.)
 - The LTHI will NOT manage the lifetime of these layers. Whoever registers them
will be responsible for unregistering them and destroying them.

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/damage_tracker.cc
File cc/damage_tracker.cc (right):

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/damage_tracker....
cc/damage_tracker.cc:165: DCHECK(layerID > 0 || layerID == -1 || layerID == -2);
I'll make a more detailed proposal about how to handle layer id's and impl-only
layers later, but if we were to go with my approach, I think DCHECK(layerID)
would be more appropriate here.

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/layer_impl.cc#n...
cc/layer_impl.cc:56: DCHECK(m_layerId > 0 || m_layerId == -1 || m_layerId ==
-2);
Same here and anywhere else this may crop up.

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/layer_tree_host...
cc/layer_tree_host.cc:279: DCHECK(children.size() <= implChildren.size());
I think we need to be more explicit about impl-only layers here. We should be
able to DCHECK that the number of children exactly equals (implChildren.size() -
num-impl-only-layers).

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/layer_tree_host...
cc/layer_tree_host.cc:334:
hostImpl->pinchZoomViewport().AddPinchZoomScrollbarsToTree(syncTree);
I don't like that this function is getting littered with code to add special
layers. I think that if we were to make impl-only layers a more official thing,
we could replace both the hud and ping zoom scrollbar stuff here with a call to
addImplOnlyLayers(dataRequiredByThisFunction).

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/layer_tree_impl...
cc/layer_tree_impl.cc:121:
layerImpl->setImplTransform(layer_tree_host_impl_->implTransform());
Please add a comment here that the implTransform is a generalization of page
scale for pinch zoom that includes translation, and that pinch zoom is, by
design, supposed to affect the scale of the main frame's scrollbars so we need
to apply it to all descendants. It's also worth noting here that you can't just
apply it to the root since the root makes assumptions about never being
translated.

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/pinch_zoom_scro...
File cc/pinch_zoom_scrollbar_layer_impl.h (right):

https://chromiumcodereview.appspot.com/11550035/diff/26001/cc/pinch_zoom_scro...
cc/pinch_zoom_scrollbar_layer_impl.h:19: // wrapper that can be cheaply created
and discarded.
We should design a system that allows impl-only layers to persist. Keeping the
state alive so that we can regenerate the layers cheaply is unfortunate, all
this stuff should live in PZSI.

[aelias_OOO_until_Jul13, 2013-01-15 20:57:56]

It looks like this feature falls into the category of fancy impl-side behavior
somewhat like tedchoc@'s URL-bar hiding change.  I think we need more design
discussion about what to do here before banging out more code on this patch. 
wjmaclean@ is coming to MTV next week so we should talk it over.

I don't think an impl-only layer is necessarily a fundamental need of this
patch.  It seems the real need is to have impl-side control over positions and
other settings.  But the layer itself could still be created on the main thread.

[wjmaclean, 2013-01-15 21:03:03]

On 2013/01/15 20:57:56, aelias wrote:
> It looks like this feature falls into the category of fancy impl-side behavior
> somewhat like tedchoc@'s URL-bar hiding change.  I think we need more design
> discussion about what to do here before banging out more code on this patch. 
> wjmaclean@ is coming to MTV next week so we should talk it over.
> 
> I don't think an impl-only layer is necessarily a fundamental need of this
> patch.  It seems the real need is to have impl-side control over positions and
> other settings.  But the layer itself could still be created on the main
thread.

Certainly. An earlier version of this patch did use a main-thread layer to
coordinate, though it had other issues. If we can get consensus on a high-level
design that is acceptable to all that would help me.

It would be best to meet early in the week if possible (Tue) as I'd like to get
this figured out well before the M26 deadline.

[jamesr, 2013-01-16 04:22:06]

If you can use a main-thread layer, I'd say do it.

[wjmaclean, 2013-02-06 18:02:47]

Here's a re-vamped pinch-zoom scrollbar framework (this CL will need to be
broken up before landing, but I want feedback on the approach if possible
first).

[aelias_OOO_until_Jul13, 2013-02-06 19:02:45]

https://codereview.chromium.org/11550035/diff/33001/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11550035/diff/33001/cc/layer_impl.cc#newcode923
cc/layer_impl.cc:923: // know the offset of the desktop compatibility viewport.
I gather you're trying to make the scrollbars visible near bottom or right
fixed-pos elements?  In the meantime, you could get something landable by
expanding the viewport by (1 / pageScaleFactor) and then clamp to edges, not
worrying too much about precise y scroll offset.

https://codereview.chromium.org/11550035/diff/33001/cc/scrollbar_layer.h
File cc/scrollbar_layer.h (right):

https://codereview.chromium.org/11550035/diff/33001/cc/scrollbar_layer.h#newc...
cc/scrollbar_layer.h:83: static bool s_usesOverlayScrollbars;
Please avoid statics.

[wjmaclean, 2013-02-06 19:40:12]

BTW, patch set #6 relies on https://bugs.webkit.org/show_bug.cgi?id=109064 to
plumb through scrollbar defaults from WebKit.

[wjmaclean, 2013-02-26 15:19:59]

James - this has been revised based on our recent discussion, PTAL?

[jamesr, 2013-02-26 20:48:46]

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:336: m_pinchZoomScrollbarHorizontal ?
m_pinchZoomScrollbarHorizontal->id() : 0);
we use -1 as an invalid id most of the time

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:359: Layer* rootScrollLayer = this->rootScrollLayer();
I can't find any uses of this local in this function

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:365: m_pinchZoomScrollbarHorizontal->setNeedsDisplay();
Most of these properties are the same every time, can't you just set them once
at initialization and have that be it?  The only thing that can change is the
bounds/position.  You should set these only when the layout viewport size
changes, not every commit

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:367: m_pinchZoomScrollbarVertical->setBounds(gfx::Size(10,
size.height() - 10));
What are these "10"s for?  Are they supposed to be trackWidth or some other
magical value?  If the latter, please put them in a constant with a descriptive
name

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:393: bool
LayerTreeHost::rootScrollLayerUsesOverlayScrollbars() const
This function doesn't seem helpful.  The decision over whether to use pinch zoom
scrollbars or not should be up to the embedder, they can set this however they
like.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:430: if (rootScrollLayer() &&
!rootScrollLayerUsesOverlayScrollbars()) {
This should check a setting to determine whether to make pinch zoom scrollbars
or not

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:433: if (!m_pinchZoomScrollbarHorizontal) {
This is way too much code to add to ::willCommit() and the horizontal and
vertical cases are nearly identical.  Please refactor into a helper function or
two.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:706: Layer* LayerTreeHost::rootScrollLayer() const
I think this function can completely go away

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:450:
activeTree()->SetPinchZoomScrollbarsVisibility();
Why is this different for the pinch zoom scrollbars vs any other type of
scrollbar that has opacity animations?

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:534: float opacity = pinchZoomScrollbarShouldDraw ? 0.5 :
0;
0.5 seems like another magical value - where does it come from?

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar.h
File cc/pinch_zoom_scrollbar.h (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar.h...
cc/pinch_zoom_scrollbar.h:20: virtual bool isOverlay() const OVERRIDE;
document what interface this is overriding.

Don't use OVERRIDE when the declaration of the interface is in a different repo
(i.e. WebKit) from the override

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar.h...
cc/pinch_zoom_scrollbar.h:37: static const int kTrackWidth = 10;
It's not good practice to put a constant's definition in a header like this
since many translation units will include this header. To declare a static
constant, put the declaration in the header but put the definition (and value)
in the .cc to define storage

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
File cc/pinch_zoom_scrollbar_geometry.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.cc:12: return
static_cast<WebScrollbarThemeGeometry*>(new PinchZoomScrollbarGeometry());
Chromium style line wraps at 80 columns, do that here

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
File cc/pinch_zoom_scrollbar_geometry.h (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:11: namespace WebKit {
Code in cc/ should be in namespace cc::

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:17: const int kTrackWidth = 10;
Another duplicate?

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:19: virtual WebScrollbarThemeGeometry*
clone() const OVERRIDE;
Same comments as above re OVERRIDE and comment

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:20: virtual int thumbPosition(WebScrollbar*)
OVERRIDE;
In chromium style all parameters have names

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:24: virtual bool hasButtons(WebScrollbar*)
OVERRIDE { return false; }
In chromium style, all overrides with code (even if it's just a return
statement) should go in the .cc

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:51: virtual void splitTrack(
newline between functions

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_p...
File cc/pinch_zoom_scrollbar_painter.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_p...
cc/pinch_zoom_scrollbar_painter.cc:14: void
PinchZoomScrollbarPainter::PaintScrollbarBackground(SkCanvas*, const gfx::Rect&)
{
line wrap

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_p...
File cc/pinch_zoom_scrollbar_painter.h (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_p...
cc/pinch_zoom_scrollbar_painter.h:18: virtual void
PaintScrollbarBackground(SkCanvas*, const gfx::Rect&) OVERRIDE;
parameters need names

[wjmaclean, 2013-03-01 15:30:32]

I've uploaded a new patch, PTAL.

The current patch will need to be broken up for landing, since the changes to
the animation framework will need to be reviewed and landed separately, but I've
included them here for now to get feedback (Vollick gave me guidance for the
animation changes).

Also, we'll still need to create a flag --enable-pinch-zoom-scrollbars I
suppose, and make the default in settings to be 'false' (and not 'true', as in
this version).

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:336: m_pinchZoomScrollbarHorizontal ?
m_pinchZoomScrollbarHorizontal->id() : 0);
On 2013/02/26 20:48:46, jamesr wrote:
> we use -1 as an invalid id most of the time

Fixed.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:359: Layer* rootScrollLayer = this->rootScrollLayer();
On 2013/02/26 20:48:46, jamesr wrote:
> I can't find any uses of this local in this function

Ooops, deleted this, but must have inadvertently restored it in the editor.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:365: m_pinchZoomScrollbarHorizontal->setNeedsDisplay();
On 2013/02/26 20:48:46, jamesr wrote:
> Most of these properties are the same every time, can't you just set them once
> at initialization and have that be it?  The only thing that can change is the
> bounds/position.  You should set these only when the layout viewport size
> changes, not every commit

Fixed.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:367: m_pinchZoomScrollbarVertical->setBounds(gfx::Size(10,
size.height() - 10));
On 2013/02/26 20:48:46, jamesr wrote:
> What are these "10"s for?  Are they supposed to be trackWidth or some other
> magical value?  If the latter, please put them in a constant with a
descriptive
> name

Fixed.

Trackwidth. I had them set correctly for horizontal, but missed them here.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:393: bool
LayerTreeHost::rootScrollLayerUsesOverlayScrollbars() const
On 2013/02/26 20:48:46, jamesr wrote:
> This function doesn't seem helpful.  The decision over whether to use pinch
zoom
> scrollbars or not should be up to the embedder, they can set this however they
> like.

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:430: if (rootScrollLayer() &&
!rootScrollLayerUsesOverlayScrollbars()) {
On 2013/02/26 20:48:46, jamesr wrote:
> This should check a setting to determine whether to make pinch zoom scrollbars
> or not

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:433: if (!m_pinchZoomScrollbarHorizontal) {
On 2013/02/26 20:48:46, jamesr wrote:
> This is way too much code to add to ::willCommit() and the horizontal and
> vertical cases are nearly identical.  Please refactor into a helper function
or
> two.

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:706: Layer* LayerTreeHost::rootScrollLayer() const
On 2013/02/26 20:48:46, jamesr wrote:
> I think this function can completely go away

It's still used by PinchZoomScrollbar.

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:450:
activeTree()->SetPinchZoomScrollbarsVisibility();
On 2013/02/26 20:48:46, jamesr wrote:
> Why is this different for the pinch zoom scrollbars vs any other type of
> scrollbar that has opacity animations?

Because I had thought in the initial CL we would do this simply, and not via
animations. I've added animations now, so this will be gone in the next patch.

The existing animator for scrollbars requires that the scrollbars be managed by
a layerImpl, which is not the case for PZ scrollbars.

The new patch animates the scrollbars using vollick@'s animation framework,
which I think should replace the current ScrollbarAnimationController eventually
(soon, even).

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:534: float opacity = pinchZoomScrollbarShouldDraw ? 0.5 :
0;
On 2013/02/26 20:48:46, jamesr wrote:
> 0.5 seems like another magical value - where does it come from?

Fixed.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar.h
File cc/pinch_zoom_scrollbar.h (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar.h...
cc/pinch_zoom_scrollbar.h:20: virtual bool isOverlay() const OVERRIDE;
On 2013/02/26 20:48:46, jamesr wrote:
> document what interface this is overriding.
> 
> Don't use OVERRIDE when the declaration of the interface is in a different
repo
> (i.e. WebKit) from the override

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar.h...
cc/pinch_zoom_scrollbar.h:37: static const int kTrackWidth = 10;
On 2013/02/26 20:48:46, jamesr wrote:
> It's not good practice to put a constant's definition in a header like this
> since many translation units will include this header. To declare a static
> constant, put the declaration in the header but put the definition (and value)
> in the .cc to define storage

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
File cc/pinch_zoom_scrollbar_geometry.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.cc:12: return
static_cast<WebScrollbarThemeGeometry*>(new PinchZoomScrollbarGeometry());
On 2013/02/26 20:48:46, jamesr wrote:
> Chromium style line wraps at 80 columns, do that here

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
File cc/pinch_zoom_scrollbar_geometry.h (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:11: namespace WebKit {
On 2013/02/26 20:48:46, jamesr wrote:
> Code in cc/ should be in namespace cc::

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:17: const int kTrackWidth = 10;
On 2013/02/26 20:48:46, jamesr wrote:
> Another duplicate?

Fixed.

I forgot I had two. I removed the one in PinchZoomScrollbar, since this should
really reside in the geometry class.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:19: virtual WebScrollbarThemeGeometry*
clone() const OVERRIDE;
On 2013/02/26 20:48:46, jamesr wrote:
> Same comments as above re OVERRIDE and comment

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:20: virtual int thumbPosition(WebScrollbar*)
OVERRIDE;
On 2013/02/26 20:48:46, jamesr wrote:
> In chromium style all parameters have names

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:24: virtual bool hasButtons(WebScrollbar*)
OVERRIDE { return false; }
On 2013/02/26 20:48:46, jamesr wrote:
> In chromium style, all overrides with code (even if it's just a return
> statement) should go in the .cc

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.h:51: virtual void splitTrack(
On 2013/02/26 20:48:46, jamesr wrote:
> newline between functions

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_p...
File cc/pinch_zoom_scrollbar_painter.cc (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_p...
cc/pinch_zoom_scrollbar_painter.cc:14: void
PinchZoomScrollbarPainter::PaintScrollbarBackground(SkCanvas*, const gfx::Rect&)
{
On 2013/02/26 20:48:46, jamesr wrote:
> line wrap

Done.

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_p...
File cc/pinch_zoom_scrollbar_painter.h (right):

https://codereview.chromium.org/11550035/diff/45001/cc/pinch_zoom_scrollbar_p...
cc/pinch_zoom_scrollbar_painter.h:18: virtual void
PaintScrollbarBackground(SkCanvas*, const gfx::Rect&) OVERRIDE;
On 2013/02/26 20:48:46, jamesr wrote:
> parameters need names

Done.

[jamesr, 2013-03-02 03:19:13]

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:373: new PinchZoomScrollbar(orientation,
owner)).PassAs<WebKit::WebScrollbar>(),
doing make_scoped_ptr(...).PassAs<T> is just extra typing. The point of
make_scoped_ptr() is to infer the type, but if you have to just override that
with PassAs<> it's not saving you anything. scoped_ptr<T>(...).Pass() is shorter

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:378: scrollbarLayer->setOpacity(0.01);
//PinchZoomScrollbar::kDefaultOpacity);
This doesn't make sense. If it's drawable, why is the opacity so low?  Why not
just make it not drawable?  Why the reference to the constant in a comment?

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.h#newc...
cc/layer_tree_host.h:158: Layer* rootScrollLayer() const;
Do you still need this?

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:347: UpdatePinchZoomScrollbarsIfNeeded(true /* force
update */);
If you need a comment for a bool parameter, that probably means you should just
use an enum

[wjmaclean, 2013-03-04 15:53:10]

I think I've addressed your comments, PTAL?

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:373: new PinchZoomScrollbar(orientation,
owner)).PassAs<WebKit::WebScrollbar>(),
On 2013/03/02 03:19:14, jamesr wrote:
> doing make_scoped_ptr(...).PassAs<T> is just extra typing. The point of
> make_scoped_ptr() is to infer the type, but if you have to just override that
> with PassAs<> it's not saving you anything. scoped_ptr<T>(...).Pass() is
shorter

Done.

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:378: scrollbarLayer->setOpacity(0.01);
//PinchZoomScrollbar::kDefaultOpacity);
On 2013/03/02 03:19:14, jamesr wrote:
> This doesn't make sense. If it's drawable, why is the opacity so low?  Why not
> just make it not drawable?  Why the reference to the constant in a comment?

If I make it not drawable, then later when we use an impl-side animation to fade
in/out, nothing happens because the scrollbar textures have never been painted
in the first paint. I don't think the animation framework can do this, and since
we're starting the animation on the impl-thread I don't have access. Calling
setDrawsContent(true) on the impl-thread isn't enough.

A better value here is 0 for opacity. The value in comments was left over from
an experiment and has been removed.

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.h#newc...
cc/layer_tree_host.h:158: Layer* rootScrollLayer() const;
On 2013/03/02 03:19:14, jamesr wrote:
> Do you still need this?

Yes, I believe so. In order for a PZ scrollbar to paint, its ScrollbarLayer
calls the PinchZoomScrollbar object as part of determining the thumb geometry,
and since this information isn't coming from WebKit, the PinchZoomScrollbar
needs to be able to pull it from the root scroll layer.

This function allows a caller external to LTH to get access to
findFirstScrollableLayer(), though I suppose we could expose that instead, or
possible have PinchZoomScrollbar replicate LTH's logic for doing this (though I
suspect that would be harder to maintain).

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:347: UpdatePinchZoomScrollbarsIfNeeded(true /* force
update */);
On 2013/03/02 03:19:14, jamesr wrote:
> If you need a comment for a bool parameter, that probably means you should
just
> use an enum

Done.

[jamesr, 2013-03-04 18:30:56]

>
https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc#new...
> cc/layer_tree_host.cc:378: scrollbarLayer->setOpacity(0.01);
> //PinchZoomScrollbar::kDefaultOpacity);
> On 2013/03/02 03:19:14, jamesr wrote:
> > This doesn't make sense. If it's drawable, why is the opacity so low?  Why
not
> > just make it not drawable?  Why the reference to the constant in a comment?
> 
> If I make it not drawable, then later when we use an impl-side animation to
fade
> in/out, nothing happens because the scrollbar textures have never been painted
> in the first paint. I don't think the animation framework can do this, and
since
> we're starting the animation on the impl-thread I don't have access. Calling
> setDrawsContent(true) on the impl-thread isn't enough.

The animation system knows how to handle layers that have 0 opacity (on the main
thread) but have an animated opacity on the impl thread.  Can you use that
mechanism?

[wjmaclean, 2013-03-05 14:32:54]

On 2013/03/04 18:30:56, jamesr wrote:
> >
>
https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc#new...
> > cc/layer_tree_host.cc:378: scrollbarLayer->setOpacity(0.01);
> > //PinchZoomScrollbar::kDefaultOpacity);
> > On 2013/03/02 03:19:14, jamesr wrote:
> > > This doesn't make sense. If it's drawable, why is the opacity so low?  Why
> not
> > > just make it not drawable?  Why the reference to the constant in a
comment?
> > 
> > If I make it not drawable, then later when we use an impl-side animation to
> fade
> > in/out, nothing happens because the scrollbar textures have never been
painted
> > in the first paint. I don't think the animation framework can do this, and
> since
> > we're starting the animation on the impl-thread I don't have access. Calling
> > setDrawsContent(true) on the impl-thread isn't enough.
> 
> The animation system knows how to handle layers that have 0 opacity (on the
main
> thread) but have an animated opacity on the impl thread.  Can you use that
> mechanism?

I assume you're referring to the subtreeShouldBeSkipped() functions in 
layer_tree_host_common? Those functions only refer to the layers' (main & impl)
opacity property, and prevent main-thread opacities from incorrectly
over-writing opacities during an animation. But, it the layer is not marked as
drawable in the first place, then the scrollbar is never drawn even while
animating. setIsDrawable(true) is required before the layer will be painted.

Now, the good news is we aren't doing extra paints - when the main thread layer
has zero opacity it doesn't get painted either, and the additions to the
animation framework to pass opacity values back to the main thread during
animations forces the PZ scrollbars to be painted, but only when needed.

[wjmaclean, 2013-03-06 16:39:44]

On 2013/03/05 14:32:54, wjmaclean wrote:
> On 2013/03/04 18:30:56, jamesr wrote:
> > >
> >
>
https://codereview.chromium.org/11550035/diff/51001/cc/layer_tree_host.cc#new...
> > > cc/layer_tree_host.cc:378: scrollbarLayer->setOpacity(0.01);
> > > //PinchZoomScrollbar::kDefaultOpacity);
> > > On 2013/03/02 03:19:14, jamesr wrote:
> > > > This doesn't make sense. If it's drawable, why is the opacity so low? 
Why
> > not
> > > > just make it not drawable?  Why the reference to the constant in a
> comment?
> > > 
> > > If I make it not drawable, then later when we use an impl-side animation
to
> > fade
> > > in/out, nothing happens because the scrollbar textures have never been
> painted
> > > in the first paint. I don't think the animation framework can do this, and
> > since
> > > we're starting the animation on the impl-thread I don't have access.
Calling
> > > setDrawsContent(true) on the impl-thread isn't enough.
> > 
> > The animation system knows how to handle layers that have 0 opacity (on the
> main
> > thread) but have an animated opacity on the impl thread.  Can you use that
> > mechanism?
> 
> I assume you're referring to the subtreeShouldBeSkipped() functions in 
> layer_tree_host_common? Those functions only refer to the layers' (main &
impl)
> opacity property, and prevent main-thread opacities from incorrectly
> over-writing opacities during an animation. But, it the layer is not marked as
> drawable in the first place, then the scrollbar is never drawn even while
> animating. setIsDrawable(true) is required before the layer will be painted.
> 
> Now, the good news is we aren't doing extra paints - when the main thread
layer
> has zero opacity it doesn't get painted either, and the additions to the
> animation framework to pass opacity values back to the main thread during
> animations forces the PZ scrollbars to be painted, but only when needed.

Are we good with this approach? I'd like to land this fairly soon if possible
(with some tests added of course).

[enne (OOO), 2013-03-06 19:32:58]

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:360: m_pinchZoomScrollbarHorizontal->setNeedsDisplay();
setBounds should take care of this if anything changes.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:386: if (!m_pinchZoomScrollbarHorizontal) {
It's always the case that you have both pinch zoom scrollbars or neither, right?
Just asking, since this way these conditionals are structured it looks like it
could be the case that you could enter into this function having only one or the
other.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:426: // Setup pinch-zoom scrollbars if required.
This comment doesn't really add anything.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.h#newc...
cc/layer_tree_host.h:157: Layer* rootScrollLayer() const;
const functions should not return non-const pointers.  Can you return a const
Layer*?

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1199: if (activeTree()->HasPinchZoomScrollbars())
Can LayerTreeHostImpl be less aware of pinch zoom scrollbars and instead just
have an API on LayerTreeImpl like DidBeginScroll(), DidUpdateScroll(),
DidEndScroll()? This would encapsulate the pinch zoom scrollbar updates and
animations inside of LayerTreeImpl and LayerTreeImpl would no longer need to
expose so many accessors and updaters.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:347:
UpdatePinchZoomScrollbarsIfNeeded(UPDATE_MAIN_THREAD_SCROLL);
Can you explain this more clearly? Why would the scrollbars not be up to date? 
If there's no currently scrolling layer, why would they even need to be?

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:523: root_scroll_layer != CurrentlyScrollingLayer()))
What does the currently scrolling layer have to do with updating the scrollbars
with changes from the root scroll layer?

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:560: MakePinchZoomFadeAnimation(0.01,
PinchZoomScrollbar::kDefaultOpacity));
Shouldn't you start the fade animation from the current opacity value? What if
you fade in during the middle of a fade out?

At any rate, 0.01 looks odd.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h
File cc/layer_tree_impl.h (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:199: enum PinchZoomScrollbarUpdateReason {
What's the purpose of this enum? Are you just emulating the updateDrawProperties
update reason? Is updating 6 properties so expensive that you have to guard
against doing it too often?

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:226: int pinch_zoom_scrollbar_horizontal_layer_id;
style nit: member variables end with trailing underscores.

https://codereview.chromium.org/11550035/diff/58001/cc/pinch_zoom_scrollbar.cc
File cc/pinch_zoom_scrollbar.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:14: const float PinchZoomScrollbar::kFadeDuration =
0.5; // Seconds.
Instead of a comment, you could just bake this into the constant name, e.g.
kFadeDurationInSeconds.

https://codereview.chromium.org/11550035/diff/58001/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:72: size = rootScrollLayer->contentBounds();
Can you convince me that contentBounds is the right choice for totalSize and
maximum? I thought scroll positions were all in layer space and thus should be
using bounds().  See also: LayerImpl::updateScrollPositions()

https://codereview.chromium.org/11550035/diff/58001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:118: // Pinch-zoom scrollbars will have a scrollLayerId
of -1, but they are
Can you add a named constant to cc::Layer (since it manages layer ids) instead
of just using -1?

[jamesr, 2013-03-06 19:38:12]

On 2013/03/06 16:39:44, wjmaclean wrote:
> 
> Are we good with this approach? I'd like to land this fairly soon if possible
> (with some tests added of course).

I thought you planned to separate out the animation bits before landing this.

[wjmaclean, 2013-03-06 19:43:16]

On 2013/03/06 19:38:12, jamesr wrote:
> On 2013/03/06 16:39:44, wjmaclean wrote:
> > 
> > Are we good with this approach? I'd like to land this fairly soon if
possible
> > (with some tests added of course).
> 
> I thought you planned to separate out the animation bits before landing this.

I am: to clarify, I'm asking if the approach is OK before I start to write
tests. The animation parts are currently under review at
https://codereview.chromium.org/12453010/.

I will address Enne's comments and put up a new CL without the animation stuff,
and we'll try to finish up that way - does that sound OK?

[wjmaclean, 2013-03-06 22:36:53]

PTAL.

I'll write tests tomorrow morning if this is OK.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:360: m_pinchZoomScrollbarHorizontal->setNeedsDisplay();
On 2013/03/06 19:32:58, enne wrote:
> setBounds should take care of this if anything changes.

Removed.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:386: if (!m_pinchZoomScrollbarHorizontal) {
On 2013/03/06 19:32:58, enne wrote:
> It's always the case that you have both pinch zoom scrollbars or neither,
right?
> Just asking, since this way these conditionals are structured it looks like it
> could be the case that you could enter into this function having only one or
the
> other.

If nothing is wrong, then yes, we have both or neither. I guess I was just
programming for the worst case here, but have changed the condition to re-create
both if either is missing. That way it's always clear we expect to have both.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:426: // Setup pinch-zoom scrollbars if required.
On 2013/03/06 19:32:58, enne wrote:
> This comment doesn't really add anything.

Done.

It used to mean more before the last re-factor I guess.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host.h#newc...
cc/layer_tree_host.h:157: Layer* rootScrollLayer() const;
On 2013/03/06 19:32:58, enne wrote:
> const functions should not return non-const pointers.  Can you return a const
> Layer*?

Done.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1199: if (activeTree()->HasPinchZoomScrollbars())
On 2013/03/06 19:32:58, enne wrote:
> Can LayerTreeHostImpl be less aware of pinch zoom scrollbars and instead just
> have an API on LayerTreeImpl like DidBeginScroll(), DidUpdateScroll(),
> DidEndScroll()? This would encapsulate the pinch zoom scrollbar updates and
> animations inside of LayerTreeImpl and LayerTreeImpl would no longer need to
> expose so many accessors and updaters.

Sounds like a good idea! Done.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:347:
UpdatePinchZoomScrollbarsIfNeeded(UPDATE_MAIN_THREAD_SCROLL);
On 2013/03/06 19:32:58, enne wrote:
> Can you explain this more clearly? Why would the scrollbars not be up to date?

> If there's no currently scrolling layer, why would they even need to be?

Main-thread scrolls just update properties on the main thread, and never call
any of the scroll methods in LayerTreeHostImpl. Since PZ scrollbars are attached
to the LTI, and not a LayerImpl, then we need to update these properties after
each commit, and at a time when the max scroll offset has been called. This
seems to be the right place.

Comment revised.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:523: root_scroll_layer != CurrentlyScrollingLayer()))
On 2013/03/06 19:32:58, enne wrote:
> What does the currently scrolling layer have to do with updating the
scrollbars
> with changes from the root scroll layer?

I had intended not to update the PZ scrollbars if some layer other than the
rootScrollLayer was scrolling.

This was removed based on a suggestion elsewhere in your comments.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:560: MakePinchZoomFadeAnimation(0.01,
PinchZoomScrollbar::kDefaultOpacity));
On 2013/03/06 19:32:58, enne wrote:
> Shouldn't you start the fade animation from the current opacity value? What if
> you fade in during the middle of a fade out?
> 
> At any rate, 0.01 looks odd.

Agreed, but we need the start opacity to be non-zero or else the scrollbars
never paint.

I'm revising to handle pz scrollbar animations interrupting one another, so will
handle your comment about the start opacity (in part) with the revision.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h
File cc/layer_tree_impl.h (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:199: enum PinchZoomScrollbarUpdateReason {
On 2013/03/06 19:32:58, enne wrote:
> What's the purpose of this enum? Are you just emulating the
updateDrawProperties
> update reason? Is updating 6 properties so expensive that you have to guard
> against doing it too often?

I was hoping that it might reduce the number of paint()'s issue to the PZ
scrollbars, though I can't really say that it helps much. It's gone.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:226: int pinch_zoom_scrollbar_horizontal_layer_id;
On 2013/03/06 19:32:58, enne wrote:
> style nit: member variables end with trailing underscores.

Thanks, missed that!

Done.

https://codereview.chromium.org/11550035/diff/58001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:118: // Pinch-zoom scrollbars will have a scrollLayerId
of -1, but they are
On 2013/03/06 19:32:58, enne wrote:
> Can you add a named constant to cc::Layer (since it manages layer ids) instead
> of just using -1?

Done.

[enne (OOO), 2013-03-06 22:45:38]

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:560: MakePinchZoomFadeAnimation(0.01,
PinchZoomScrollbar::kDefaultOpacity));
On 2013/03/06 22:36:53, wjmaclean wrote:
> On 2013/03/06 19:32:58, enne wrote:
> > Shouldn't you start the fade animation from the current opacity value? What
if
> > you fade in during the middle of a fade out?
> > 
> > At any rate, 0.01 looks odd.
> 
> Agreed, but we need the start opacity to be non-zero or else the scrollbars
> never paint.
> 
> I'm revising to handle pz scrollbar animations interrupting one another, so
will
> handle your comment about the start opacity (in part) with the revision.

This is an impl-only animation.  How does that interact with painting?

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h
File cc/layer_tree_impl.h (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:199: enum PinchZoomScrollbarUpdateReason {
On 2013/03/06 22:36:53, wjmaclean wrote:
> On 2013/03/06 19:32:58, enne wrote:
> > What's the purpose of this enum? Are you just emulating the
> updateDrawProperties
> > update reason? Is updating 6 properties so expensive that you have to guard
> > against doing it too often?
> 
> I was hoping that it might reduce the number of paint()'s issue to the PZ
> scrollbars, though I can't really say that it helps much. It's gone.

Paints? Isn't all the painting done on the main thread?

[wjmaclean, 2013-03-07 22:57:39]

PTAL, I've added tests; please let me know if there are any remaining concerns.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:560: MakePinchZoomFadeAnimation(0.01,
PinchZoomScrollbar::kDefaultOpacity));
On 2013/03/06 22:45:38, enne wrote:
> On 2013/03/06 22:36:53, wjmaclean wrote:
> > On 2013/03/06 19:32:58, enne wrote:
> > > Shouldn't you start the fade animation from the current opacity value?
What
> if
> > > you fade in during the middle of a fade out?
> > > 
> > > At any rate, 0.01 looks odd.
> > 
> > Agreed, but we need the start opacity to be non-zero or else the scrollbars
> > never paint.
> > 
> > I'm revising to handle pz scrollbar animations interrupting one another, so
> will
> > handle your comment about the start opacity (in part) with the revision.
> 
> This is an impl-only animation.  How does that interact with painting?

Since the PZ scrollbar starts with opacity 0, it doesn't get painted until the
first time we do a fade-in animation. It seems if the animation doesn't start at
a non-zero value that it never gets painted.

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h
File cc/layer_tree_impl.h (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:199: enum PinchZoomScrollbarUpdateReason {
On 2013/03/06 22:45:38, enne wrote:
> On 2013/03/06 22:36:53, wjmaclean wrote:
> > On 2013/03/06 19:32:58, enne wrote:
> > > What's the purpose of this enum? Are you just emulating the
> > updateDrawProperties
> > > update reason? Is updating 6 properties so expensive that you have to
guard
> > > against doing it too often?
> > 
> > I was hoping that it might reduce the number of paint()'s issue to the PZ
> > scrollbars, though I can't really say that it helps much. It's gone.
> 
> Paints? Isn't all the painting done on the main thread?

Yes, but I was concerned about updates on the impl-side parameters possibly
making their way back and causing re-paints on the main thread ... perhaps I was
being overly cautious.

https://codereview.chromium.org/11550035/diff/58001/cc/pinch_zoom_scrollbar.cc
File cc/pinch_zoom_scrollbar.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:14: const float PinchZoomScrollbar::kFadeDuration =
0.5; // Seconds.
On 2013/03/06 19:32:58, enne wrote:
> Instead of a comment, you could just bake this into the constant name, e.g.
> kFadeDurationInSeconds.

Sorry, missed this in the latest patch, but have added it now to the upcoming
patch.

https://codereview.chromium.org/11550035/diff/58001/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:72: size = rootScrollLayer->contentBounds();
On 2013/03/06 19:32:58, enne wrote:
> Can you convince me that contentBounds is the right choice for totalSize and
> maximum? I thought scroll positions were all in layer space and thus should be
> using bounds().  See also: LayerImpl::updateScrollPositions()

On the main thread size, bounds() does not change when you zoom in, yet the
scrollbars need to update based on the pageScaleFactor. Using contentBounds
makes this easier (I don't need to export PSF from LTH), and I make sure that
all the rects produced in PinchZoomScrollbarGeometry are in bounds coordinates.

The rootScrollLayer scrollOffset already seems to have PSF baked in. I could
might be able to get away with using bounds if (1) the PSF was available to
re-scale the scrollOffset, and (2) the rootScrollLayer maxScrollOffset was
guaranteed to be > scrollOffset (at present this doesn't seem to be true always
- in fact it's sometimes zero when scrollOffset is quite large, making me think
it's perhaps only reliable on the impl-side).

[enne (OOO), 2013-03-07 23:41:17]

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h
File cc/layer_tree_impl.h (right):

https://codereview.chromium.org/11550035/diff/58001/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:199: enum PinchZoomScrollbarUpdateReason {
On 2013/03/07 22:57:39, wjmaclean wrote:
> On 2013/03/06 22:45:38, enne wrote:
> > On 2013/03/06 22:36:53, wjmaclean wrote:
> > > On 2013/03/06 19:32:58, enne wrote:
> > > > What's the purpose of this enum? Are you just emulating the
> > > updateDrawProperties
> > > > update reason? Is updating 6 properties so expensive that you have to
> guard
> > > > against doing it too often?
> > > 
> > > I was hoping that it might reduce the number of paint()'s issue to the PZ
> > > scrollbars, though I can't really say that it helps much. It's gone.
> > 
> > Paints? Isn't all the painting done on the main thread?
> 
> Yes, but I was concerned about updates on the impl-side parameters possibly
> making their way back and causing re-paints on the main thread ... perhaps I
was
> being overly cautious.

I think you're being overly cautious.  I don't see how UpdatePinchZoomScrollbars
could cause a repaint.

https://codereview.chromium.org/11550035/diff/58001/cc/pinch_zoom_scrollbar.cc
File cc/pinch_zoom_scrollbar.cc (right):

https://codereview.chromium.org/11550035/diff/58001/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:72: size = rootScrollLayer->contentBounds();
On 2013/03/07 22:57:39, wjmaclean wrote:
> On 2013/03/06 19:32:58, enne wrote:
> > Can you convince me that contentBounds is the right choice for totalSize and
> > maximum? I thought scroll positions were all in layer space and thus should
be
> > using bounds().  See also: LayerImpl::updateScrollPositions()
> 
> On the main thread size, bounds() does not change when you zoom in, yet the
> scrollbars need to update based on the pageScaleFactor. Using contentBounds
> makes this easier (I don't need to export PSF from LTH), and I make sure that
> all the rects produced in PinchZoomScrollbarGeometry are in bounds
coordinates.

Aren't the PinchZoomScrollbars overlay scrollbars and thus are sized to the
viewport? Why would you need need to update them based on page scale factor?

> The rootScrollLayer scrollOffset already seems to have PSF baked in.

This should only happen if boundsContainPageScale(), although I feel like that
should always be false these days.  That doesn't mean that you should use
contentBounds.

My understanding is that scroll offset / delta / sent delta are always in layer
space and not content space.  For another example, see:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layer_tree_host...

https://codereview.chromium.org/11550035/diff/71001/cc/pinch_zoom_scrollbar.cc
File cc/pinch_zoom_scrollbar.cc (right):

https://codereview.chromium.org/11550035/diff/71001/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:74: size = owner_->layoutViewportSize();
This if statement doesn't make any sense.  You can't mix contentBounds and
layoutViewportSize.  Perhaps the totalSize should be zero if there's no root
scroll layer?

[enne (OOO), 2013-03-08 00:00:50]

Ok, after some conversation with danakj, I see why you want to use contentBounds
for the scrollbar size / offsets / etc, as you want them to reflect the pinched
size and not the original size.  That makes sense; sorry for the
misunderstanding.

However, I would like to be less confused about the mixed use of
layoutViewPortSize and contentBounds (e.g. PinchZoomScrollbar::maximum()).  It
seems like if you are going to use contentBounds of the root scroll layer then
that value is going to include DSF and PSF.  However, layoutViewportSize
contains neither of these things, so I am not sure how you can subtract one from
the other and get what you're looking for out of it.  Am I also mistaken here?

[wjmaclean, 2013-03-08 15:28:56]

PTAL
I've modified to account for device scale factor, and I think this may help
address (in part) your concerns about mixing contentBounds().

I'm viewing it like this: both contentBounds() and layoutViewportSize() are in
layout pixels, once adjusted for d.s.f.. The fact that contentBounds() also has
p.s.f. shouldn't be a problem, since it still indicates how many pixels the
content has/requires, and layoutViewportSize says how much space to we have to
display them. The latter shouldn't change on a pinch zoom (only on window
resize), but the former is expected to change, and should affect the maximum
scroll offset.

Does that help?

I've tested both link and desktop builds with different scale factors, and it
works well.

https://codereview.chromium.org/11550035/diff/71001/cc/pinch_zoom_scrollbar.cc
File cc/pinch_zoom_scrollbar.cc (right):

https://codereview.chromium.org/11550035/diff/71001/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:74: size = owner_->layoutViewportSize();
On 2013/03/07 23:41:17, enne wrote:
> This if statement doesn't make any sense.  You can't mix contentBounds and
> layoutViewportSize.  Perhaps the totalSize should be zero if there's no root
> scroll layer?

Done.

[danakj, 2013-03-08 17:42:06]

On Fri, Mar 8, 2013 at 7:28 AM, <wjmaclean@chromium.org> wrote:

> PTAL
> I've modified to account for device scale factor, and I think this may help
> address (in part) your concerns about mixing contentBounds().
>
> I'm viewing it like this: both contentBounds() and layoutViewportSize()
> are in
> layout pixels, once adjusted for d.s.f.. The fact that contentBounds()
> also has
>

That's not true. contentBounds() can include CSS scale too.


> p.s.f. shouldn't be a problem, since it still indicates how many pixels the
> content has/requires, and layoutViewportSize says how much space to we
> have to
> display them. The latter shouldn't change on a pinch zoom (only on window
> resize), but the former is expected to change, and should affect the
> maximum
> scroll offset.
>
> Does that help?
>
> I've tested both link and desktop builds with different scale factors, and
> it
> works well.
>
>
>
> https://codereview.chromium.**org/11550035/diff/71001/cc/**
>
pinch_zoom_scrollbar.cc<https://codereview.chromium.org/11550035/diff/71001/cc/pinch_zoom_scrollbar.cc>
> File cc/pinch_zoom_scrollbar.cc (right):
>
> https://codereview.chromium.**org/11550035/diff/71001/cc/**
>
pinch_zoom_scrollbar.cc#**newcode74<https://codereview.chromium.org/11550035/diff/71001/cc/pinch_zoom_scrollbar.cc#newcode74>
> cc/pinch_zoom_scrollbar.cc:74: size = owner_->layoutViewportSize();
> On 2013/03/07 23:41:17, enne wrote:
>
>> This if statement doesn't make any sense.  You can't mix contentBounds
>>
> and
>
>> layoutViewportSize.  Perhaps the totalSize should be zero if there's
>>
> no root
>
>> scroll layer?
>>
>
> Done.
>
>
https://codereview.chromium.**org/11550035/<https://codereview.chromium.org/1...
>

[wjmaclean, 2013-03-08 19:06:28]

On 2013/03/08 17:42:06, danakj wrote:
> On Fri, Mar 8, 2013 at 7:28 AM, <mailto:wjmaclean@chromium.org> wrote:
> 
> 
> That's not true. contentBounds() can include CSS scale too.

Is there an example of a real-world page I can test with? It only matters if the
scale is on the root scroll layer.

[danakj, 2013-03-08 19:37:48]

On 2013/03/08 19:06:28, wjmaclean wrote:
> On 2013/03/08 17:42:06, danakj wrote:
> > On Fri, Mar 8, 2013 at 7:28 AM, <mailto:wjmaclean@chromium.org> wrote:
> > 
> > 
> > That's not true. contentBounds() can include CSS scale too.
> 
> Is there an example of a real-world page I can test with? It only matters if
the
> scale is on the root scroll layer.

Enne set me straight, the root scroll layer is the parent of the NCCH, so it's
not something that appears in the DOM and shouldn't be affected by CSS scaling.

[wjmaclean, 2013-03-08 19:57:31]

On 2013/03/08 19:37:48, danakj wrote:
> On 2013/03/08 19:06:28, wjmaclean wrote:
> > On 2013/03/08 17:42:06, danakj wrote:
> > > On Fri, Mar 8, 2013 at 7:28 AM, <mailto:wjmaclean@chromium.org> wrote:
> > > 
> > > 
> > > That's not true. contentBounds() can include CSS scale too.
> > 
> > Is there an example of a real-world page I can test with? It only matters if
> the
> > scale is on the root scroll layer.
> 
> Enne set me straight, the root scroll layer is the parent of the NCCH, so it's
> not something that appears in the DOM and shouldn't be affected by CSS
scaling.

Whew, relief!

[enne (OOO), 2013-03-11 21:08:24]

A bunch of style comments, but I'm still a bit confused about spaces.  I'm happy
to be convinced by confident answers in this department.

I understand PZScrollbar::size() using layout viewport, since you're trying to
size yourself to the clip rect.  aelias wants to remove this value, but I
suspect something can be sorted out here.

I understand PZScrollbar totalSize and maximum using contentBounds, since you
want it to be affected by page scale changes due to pinch.  However, it seems
like you should be using device viewport when you are working with contentBounds
and not trying to back convert.

https://codereview.chromium.org/11550035/diff/76002/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer.h#newcode57
cc/layer.h:57: static const int s_invalidLayerId;
Can you make this an enum to not require storage for this value?

Also, 0 is the invalid layer id.  -1 here means something special for pinch zoom
scrollbars in that they are managed by LayerTreeImpl specially.  Can you make
them different values?

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:334: m_pinchZoomScrollbarHorizontal ?
m_pinchZoomScrollbarHorizontal->id() : -1);
Don't use -1 here.  Use the constant.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:336: m_pinchZoomScrollbarVertical ?
m_pinchZoomScrollbarVertical->id() : -1);
Don't use -1 here.  Use the constant.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:2203: m_settings.usePinchZoomScrollbars = true;
This is inconsistently formatted with the rest of the file.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:128:
target_tree->SetPinchZoomHorizontalLayerId(pinch_zoom_scrollbar_horizontal_layer_id_);
style nit: 80 col.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:129:
target_tree->SetPinchZoomVerticalLayerId(pinch_zoom_scrollbar_vertical_layer_id_);
style nit: 80 col.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:575: float start_opacity = fmax(0.01, layer->opacity());
Can you help me understand how an impl-side animation affects painting?  I still
don't get it.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:598: // TODO() It shouldn't be necessary to manually
remove the old animation.
TODO(wjmaclean)

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.h
File cc/layer_tree_impl.h (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:195: ScrollbarLayerImpl* PinchZoomScrollbarHorizontal();
This should not be public.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:197: ScrollbarLayerImpl* PinchZoomScrollbarVertical();
Neither should this.

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar.cc
File cc/pinch_zoom_scrollbar.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:13: const float PinchZoomScrollbar::kDefaultOpacity =
0.5;
style nit: 0.5f

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:14: const float
PinchZoomScrollbar::kFadeDurationInSeconds = 0.5;
style nit: 0.5f

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:66: return root_scroll_bounds.width() /scale -
viewport_size.width();
Is there a reason not to just use deviceViewportSize directly rather than
scaling values here and in totalSize?

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar_g...
File cc/pinch_zoom_scrollbar_geometry.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.cc:80: return WebRect(track.x + thumb_pos,
track.y + (track.height - thickness) /
style nit: use braces for multiline statements.

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar_p...
File cc/pinch_zoom_scrollbar_painter.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar_p...
cc/pinch_zoom_scrollbar_painter.cc:15: SkCanvas*, const gfx::Rect&) {
style nit: if you can't fit parameters of a function definition on one line,
they all should go on separate lines.

https://codereview.chromium.org/11550035/diff/76002/cc/scrollbar_layer_unitte...
File cc/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/scrollbar_layer_unitte...
cc/scrollbar_layer_unittest.cc:338: ScrollbarLayer::create(scrollbar1.Pass(),
Inconsistent formatting here.

[wjmaclean, 2013-03-12 16:13:07]

I think I've addressed all the concerns in the last round of comments, PTAL?

https://codereview.chromium.org/11550035/diff/76002/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer.h#newcode57
cc/layer.h:57: static const int s_invalidLayerId;
On 2013/03/11 21:08:24, enne wrote:
> Can you make this an enum to not require storage for this value?

Yes, good idea!

> Also, 0 is the invalid layer id.  -1 here means something special for pinch
zoom
> scrollbars in that they are managed by LayerTreeImpl specially.  Can you make
> them different values?

Hmm, if you refer back to comment #20, James R requested that I *don't* use 0,
on the grounds that -1 is the invalid layer id (not the exact same context, but
still we're marking invalid layers). I can live with either, but it would be
good if there could be a common consensus on which value to use.

I have proposed the following solution in the revised patch:

-1 => invalid layer
0 => unused layer (PZ scrollbar layers use this for scrolling layer id)

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:334: m_pinchZoomScrollbarHorizontal ?
m_pinchZoomScrollbarHorizontal->id() : -1);
On 2013/03/11 21:08:24, enne wrote:
> Don't use -1 here.  Use the constant.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:336: m_pinchZoomScrollbarVertical ?
m_pinchZoomScrollbarVertical->id() : -1);
On 2013/03/11 21:08:24, enne wrote:
> Don't use -1 here.  Use the constant.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:2203: m_settings.usePinchZoomScrollbars = true;
On 2013/03/11 21:08:24, enne wrote:
> This is inconsistently formatted with the rest of the file.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:128:
target_tree->SetPinchZoomHorizontalLayerId(pinch_zoom_scrollbar_horizontal_layer_id_);
On 2013/03/11 21:08:24, enne wrote:
> style nit: 80 col.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:129:
target_tree->SetPinchZoomVerticalLayerId(pinch_zoom_scrollbar_vertical_layer_id_);
On 2013/03/11 21:08:24, enne wrote:
> style nit: 80 col.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:575: float start_opacity = fmax(0.01, layer->opacity());
On 2013/03/11 21:08:24, enne wrote:
> Can you help me understand how an impl-side animation affects painting?  I
still don't get it.

The very first time the scrollbars need to be shown, they will not yet have been
painted, so presumably we need to make sure this happens. I suppose ideally the
system would detect if the scrollbar had not yet been painted (no texture id),
and request a repaint. It seems that happens automatically if I set a non-zero
start value, but it is happy to run the entire animation *without* a texture if
I don't.

In the next revision of the CL I have attempted an alternate method to invoke
the painting process.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:598: // TODO() It shouldn't be necessary to manually
remove the old animation.
On 2013/03/11 21:08:24, enne wrote:
> TODO(wjmaclean)

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.h
File cc/layer_tree_impl.h (right):

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:195: ScrollbarLayerImpl* PinchZoomScrollbarHorizontal();
On 2013/03/11 21:08:24, enne wrote:
> This should not be public.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/layer_tree_impl.h#newc...
cc/layer_tree_impl.h:197: ScrollbarLayerImpl* PinchZoomScrollbarVertical();
On 2013/03/11 21:08:24, enne wrote:
> Neither should this.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar.cc
File cc/pinch_zoom_scrollbar.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:13: const float PinchZoomScrollbar::kDefaultOpacity =
0.5;
On 2013/03/11 21:08:24, enne wrote:
> style nit: 0.5f

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:14: const float
PinchZoomScrollbar::kFadeDurationInSeconds = 0.5;
On 2013/03/11 21:08:24, enne wrote:
> style nit: 0.5f

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar.c...
cc/pinch_zoom_scrollbar.cc:66: return root_scroll_bounds.width() /scale -
viewport_size.width();
On 2013/03/11 21:08:24, enne wrote:
> Is there a reason not to just use deviceViewportSize directly rather than
> scaling values here and in totalSize?

No, device viewport should work too.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar_g...
File cc/pinch_zoom_scrollbar_geometry.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar_g...
cc/pinch_zoom_scrollbar_geometry.cc:80: return WebRect(track.x + thumb_pos,
track.y + (track.height - thickness) /
On 2013/03/11 21:08:24, enne wrote:
> style nit: use braces for multiline statements.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar_p...
File cc/pinch_zoom_scrollbar_painter.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/pinch_zoom_scrollbar_p...
cc/pinch_zoom_scrollbar_painter.cc:15: SkCanvas*, const gfx::Rect&) {
On 2013/03/11 21:08:24, enne wrote:
> style nit: if you can't fit parameters of a function definition on one line,
> they all should go on separate lines.

Done.

https://codereview.chromium.org/11550035/diff/76002/cc/scrollbar_layer_unitte...
File cc/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/11550035/diff/76002/cc/scrollbar_layer_unitte...
cc/scrollbar_layer_unittest.cc:338: ScrollbarLayer::create(scrollbar1.Pass(),
On 2013/03/11 21:08:24, enne wrote:
> Inconsistent formatting here.

Done.

[enne (OOO), 2013-03-12 16:27:37]

https://codereview.chromium.org/11550035/diff/88002/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11550035/diff/88002/cc/layer.h#newcode56
cc/layer.h:56: NOTUSED_ID = 0
Can you add a PINCH_ZOOM_ROOT_SCROLL_LAYER_ID = -2 and use that instead
explicitly? The name "NOTUSED" doesn't really correspond to what you're using
this for.

https://codereview.chromium.org/11550035/diff/88002/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/88002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:581: // TODO() It shouldn't be necessary to manually
remove the old animation.
TODO(wjmaclean)

https://codereview.chromium.org/11550035/diff/88002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:587: return layer->HasThumbTexture();
I still think this is the wrong way to go about this.  Impl-side animations
should not be responsible for controlling when a layer paints, especially when
that layer is being painted on the main thread.

You should make pinch zoom scrollbars always painted.

Alternatively, you could write some other patch that lets you paint
ScrollbarLayerImpls from the compositor thread on platforms where that's
supported, and then you don't have to worry about this at all.

[wjmaclean, 2013-03-12 18:25:33]

PTAL - I think I have a (sane) way to force the PZ scrollbars to paint.

https://codereview.chromium.org/11550035/diff/88002/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/88002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:581: // TODO() It shouldn't be necessary to manually
remove the old animation.
On 2013/03/12 16:27:37, enne wrote:
> TODO(wjmaclean)

Done.

(I'm officially going insane ... I clearly remember making this edit, but it
disappeared somehow :-( )

https://codereview.chromium.org/11550035/diff/88002/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:587: return layer->HasThumbTexture();
On 2013/03/12 16:27:37, enne wrote:
> I still think this is the wrong way to go about this.  Impl-side animations
> should not be responsible for controlling when a layer paints, especially when
> that layer is being painted on the main thread.
> 
> You should make pinch zoom scrollbars always painted.
> 
> Alternatively, you could write some other patch that lets you paint
> ScrollbarLayerImpls from the compositor thread on platforms where that's
> supported, and then you don't have to worry about this at all.

OK, I've devised a way for the PZ scrollbars to always paint on the main thread,
and the method could be re-used in other layers that want to impl-side animate.

[danakj, 2013-03-12 19:00:38]

Some style nit gifts and a suggestion.

Instead of adding another method to cc::Layer, can we just make
OpacityIsAnimating() virtual and have it be true for pinch zoom scrollbars? Then
we can use existing mechanisms, and it would basically be saying "the opacity
changes on impl i dont know what it is" which is exactly the situation here
IIUC.

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:336: Layer::INVALID_ID);
nit: this is strange indenting. normally it would line up with the token after
the "?"

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:378: scrollbarLayer->SetIsDrawable(true);
new statement shouldn't be indented like a continuation.

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:379: scrollbarLayer->SetOpacity(0);
0.f

[enne (OOO), 2013-03-12 19:02:29]

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:396:
m_rootLayer->AddChild(m_pinchZoomScrollbarHorizontal);
SetRootLayer will break this hookup.  You need to detach them in the same way
that hud layer is.  Can you add a unit test where the root layer changes but the
scrollbars stay attached to the right tree?

[wjmaclean, 2013-03-12 19:07:14]

Working on a new revision ...

Dana, I think I prefer the DoNotSkip ... method, as it is more general, and
could be used by other layers wanting to impl-side animate.

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:336: Layer::INVALID_ID);
On 2013/03/12 19:00:39, danakj wrote:
> nit: this is strange indenting. normally it would line up with the token after
> the "?"

Done.

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:378: scrollbarLayer->SetIsDrawable(true);
On 2013/03/12 19:00:39, danakj wrote:
> new statement shouldn't be indented like a continuation.

Actually, isn't it the case that this *is* a continuation? And the line below is
missing an indent?

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:379: scrollbarLayer->SetOpacity(0);
On 2013/03/12 19:00:39, danakj wrote:
> 0.f

Done.

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:396:
m_rootLayer->AddChild(m_pinchZoomScrollbarHorizontal);
On 2013/03/12 19:02:29, enne wrote:
> SetRootLayer will break this hookup.  You need to detach them in the same way
> that hud layer is.  Can you add a unit test where the root layer changes but
the
> scrollbars stay attached to the right tree?

OK, working on it for next patch.

[wjmaclean, 2013-03-12 19:11:02]

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11550035/diff/100001/cc/layer_tree_host.cc#ne...
cc/layer_tree_host.cc:378: scrollbarLayer->SetIsDrawable(true);
On 2013/03/12 19:07:15, wjmaclean wrote:
> On 2013/03/12 19:00:39, danakj wrote:
> > new statement shouldn't be indented like a continuation.
> 
> Actually, isn't it the case that this *is* a continuation? And the line below
is
> missing an indent?

Scrub that ... the indenting is wrong on the line above, and further below,
fixing ...

[enne (OOO), 2013-03-12 19:34:21]

On 2013/03/12 19:07:14, wjmaclean wrote:

> Dana, I think I prefer the DoNotSkip ... method, as it is more general, and
> could be used by other layers wanting to impl-side animate.

What other impl-side animation use cases are you thinking of where a layer could
get skipped? I'd prefer to reuse the existing mechanisms like Dana suggests
unless there's some strong reason not to.

[wjmaclean, 2013-03-12 19:35:28]

On 2013/03/12 19:34:21, enne wrote:
> On 2013/03/12 19:07:14, wjmaclean wrote:
> 
> > Dana, I think I prefer the DoNotSkip ... method, as it is more general, and
> > could be used by other layers wanting to impl-side animate.
> 
> What other impl-side animation use cases are you thinking of where a layer
could
> get skipped? I'd prefer to reuse the existing mechanisms like Dana suggests
> unless there's some strong reason not to.

OK, pls ignore the patch I just uploaded then.

[wjmaclean, 2013-03-12 20:30:08]

PTAL

I've had to detect if we're creating a pinch-zoom ScrollbarLayerImpl, since
using OpacityIsAnimating() blocks any opacity updates from ever reaching the
impl-side, and normally LayerImpl's default to opacity = 1.

[enne (OOO), 2013-03-12 21:10:08]

https://codereview.chromium.org/11550035/diff/113001/cc/scrollbar_layer.cc
File cc/scrollbar_layer.cc (right):

https://codereview.chromium.org/11550035/diff/113001/cc/scrollbar_layer.cc#ne...
cc/scrollbar_layer.cc:30: scroll_layer_impl->ToScrollbarLayer()->SetOpacity(0);
Why are opacity updates blocked and this is required?

https://codereview.chromium.org/11550035/diff/113001/cc/scrollbar_layer.cc#ne...
cc/scrollbar_layer.cc:71: return Layer::OpacityIsAnimating() ||
Maybe just return Layer::OpacityIsAnimating || isOverlay() here, since we'll
need this for OSX overlay scrollbars too.

[wjmaclean, 2013-03-12 22:02:22]

https://codereview.chromium.org/11550035/diff/113001/cc/scrollbar_layer.cc
File cc/scrollbar_layer.cc (right):

https://codereview.chromium.org/11550035/diff/113001/cc/scrollbar_layer.cc#ne...
cc/scrollbar_layer.cc:30: scroll_layer_impl->ToScrollbarLayer()->SetOpacity(0);
On 2013/03/12 21:10:08, enne wrote:
> Why are opacity updates blocked and this is required?

The call to "ToScrollbarLayer()" is not necessary, but 

scroll_layer_impl->SetOpacity(0);

is. If you look at Layer::PushPropertiesTo(LayerImpl* layer)

you find

if (!OpacityIsAnimating())
  layer->SetOpacity(opacity_);
  
which is required for animations to work properly. If we piggyback on this then
we need to at least make sure the PZ ScrollbarLayerImpl starts with opacity = 0.

https://codereview.chromium.org/11550035/diff/113001/cc/scrollbar_layer.cc#ne...
cc/scrollbar_layer.cc:71: return Layer::OpacityIsAnimating() ||
On 2013/03/12 21:10:08, enne wrote:
> Maybe just return Layer::OpacityIsAnimating || isOverlay() here, since we'll
> need this for OSX overlay scrollbars too.

Sure, though Clank also uses overlay scrollbars, and I don't know how this might
affect them.

[aelias_OOO_until_Jul13, 2013-03-12 22:18:41]

We also animate the opacity of our overlay scrollbars so it looks fine to me.

[wjmaclean, 2013-03-13 12:21:06]

On 2013/03/12 22:18:41, aelias wrote:
> We also animate the opacity of our overlay scrollbars so it looks fine to me.

Enne, are we good to go with this?

[danakj, 2013-03-13 14:43:36]

Did you see Ian's change? I +'d you on it. You shouldn't need to set
opacity on the impl thread with it, I believe.


On Wed, Mar 13, 2013 at 8:21 AM, <wjmaclean@chromium.org> wrote:

> On 2013/03/12 22:18:41, aelias wrote:
>
>> We also animate the opacity of our overlay scrollbars so it looks fine to
>> me.
>>
>
> Enne, are we good to go with this?
>
>
https://codereview.chromium.**org/11550035/<https://codereview.chromium.org/1...
>

[enne (OOO), 2013-03-13 18:46:31]

I see that you removed the code from CreateLayerImpl, but this feels like you're
back in the same situation again.  This patch is really big though, so I may
have missed something.

Can you walk me through how the pinch zoom scrollbar layer doesn't get skipped
and always paints on the main thread now?

https://codereview.chromium.org/11550035/diff/138001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/138001/cc/layer_tree_impl.cc#ne...
cc/layer_tree_impl.cc:575: // The start opacity value must be > 0 in order that
the scrollbars get
This comment is no longer true.

https://codereview.chromium.org/11550035/diff/138001/cc/layer_tree_impl.cc#ne...
cc/layer_tree_impl.cc:578: float start_opacity = 0;
Shouldn't start_opacity be layer->opacity()?

[wjmaclean, 2013-03-13 20:09:47]

> I see that you removed the code from CreateLayerImpl, but 
> this feels like you're back in the same situation again.

Yes, and no. Ian's modifying the animation framework so that layers animating on
the impl side only will be guaranteed to not be skipped during painting.

> Can you walk me through how the pinch zoom scrollbar layer 
> doesn't get skipped and always paints on the main thread 
> now?

Right now it works (in a flaky sort of way) since Layer::OnOpacityAnimated()
will pick up the new opacity values, but doesn't issue a commit, so it presently
depends on whether a commit wanders by or not. When Ian's patch lands, it will
become reliable.

https://codereview.chromium.org/11550035/diff/138001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11550035/diff/138001/cc/layer_tree_impl.cc#ne...
cc/layer_tree_impl.cc:575: // The start opacity value must be > 0 in order that
the scrollbars get
On 2013/03/13 18:46:32, enne wrote:
> This comment is no longer true.

Removed.

https://codereview.chromium.org/11550035/diff/138001/cc/layer_tree_impl.cc#ne...
cc/layer_tree_impl.cc:578: float start_opacity = 0;
On 2013/03/13 18:46:32, enne wrote:
> Shouldn't start_opacity be layer->opacity()?

Yes, that makes more sense, done.

[wjmaclean, 2013-03-13 20:39:10]

https://codereview.chromium.org/11550035/diff/151001/cc/layer_tree_settings.cc
File cc/layer_tree_settings.cc (right):

https://codereview.chromium.org/11550035/diff/151001/cc/layer_tree_settings.c...
cc/layer_tree_settings.cc:50: , usePinchZoomScrollbars(true)
This will default to false for landing.

[enne (OOO), 2013-03-13 21:46:40]

After discussion with vollick, I don't think updating the animation system here
is really the right solution.  It ends up being more complicated than before
just to make it use the existing system for little benefit.  Now that I
understand what the issues here are, I think we should go back to the original
proposal of adding a new virtual function on Layer.

vollick proposes adding Layer::OpacityCanAnimateOnImplThread, which I quite
like, as it specifies that an animation might not be active right now (but may
be in the future) and it will provide more reason in LayerTreeHostCommon to
imply why a subtree should not be skipped.

[wjmaclean, 2013-03-14 14:28:23]

PTAL

On 2013/03/13 21:46:40, enne wrote:
> After discussion with vollick, I don't think updating the animation system
here
> is really the right solution.  It ends up being more complicated than before
> just to make it use the existing system for little benefit.  Now that I
> understand what the issues here are, I think we should go back to the original
> proposal of adding a new virtual function on Layer.

Sounds good, done.

> vollick proposes adding Layer::OpacityCanAnimateOnImplThread, which I quite
> like, as it specifies that an animation might not be active right now (but may
> be in the future) and it will provide more reason in LayerTreeHostCommon to
> imply why a subtree should not be skipped.

I like Ian's proposed name - my previous patch would have been clearer if I had
thought of a name like this (I think I underestimated the importance of the name
when I submitted that patch).

[enne (OOO), 2013-03-15 16:34:21]

lgtm

Thanks for your patience and all the rounds of review updates.

[wjmaclean, 2013-03-15 16:46:20]

On 2013/03/15 16:34:21, enne wrote:
> lgtm
> 
> Thanks for your patience and all the rounds of review updates.

Thanks! And likewise, thanks for all your patience :-)

[commit-bot: I haz the power, 2013-03-15 17:13:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/11550035/159001

[commit-bot: I haz the power, 2013-03-15 17:47:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/11550035/122002

[commit-bot: I haz the power, 2013-03-15 18:38:31]

Retried try job too often on mac_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-15 19:14:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/11550035/122002

[commit-bot: I haz the power, 2013-03-15 19:39:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/11550035/186001

[commit-bot: I haz the power, 2013-03-15 23:11:45]

Change committed as 188500