Pinch/Zoom Infrastructure & Plumbing CL

Pinch/Zoom Infrastructure & Plumbing CL

This CL supplies the necessary changes to CC to support the
inner/outer viewport model for pinch-zoom and fixed-position
elements. The specification for these changes is contained in
the document "Layer-based Solution for Pinch Zoom / Fixed
Position".

It incorporates a change to how scrollbar parameters are
computed (removes the notion of max_scroll_offset as a
quantity set be the embedder, and instead inferred from the
relative sizes of a clip layer w.r.t. the scroll layer).

Scrollbars are generalized so that a layer may have more than
two scrollbars, and the parameters of the scrollbar are set
w.r.t. the sizes and positions of a clip and a scroll layer.
Further, changes to the scrip/scroll layer automatically
notify any attached scrollbars.

The CL also removes existing references to root_scroll_layer
from LTH, LTI and LTHI and replaces them with either
Inner/OuterViewportScrollLayer (dual-layer operation only
exists at present if the --enable-pinch-virtual-viewport flag
is specified, otherwise behavior is unchanged).

Logic is added to (i) combine scroll offsets for the two
viewports before passing it to the embedder, and (ii)
splitting any offsets received from the embedder
appropriately between the two viewports.

This CL relies on https://codereview.chromium.org/138453004/
for changes in Blink to support it.

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

R=aelias@chromium.org, enne@chromium.org, joi@chromium.org, piman@chromium.org, sky@chromium.org

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

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

[enne (OOO), 2013-10-31 18:45:58]

This is looking pretty reasonable to me.  It's about what I'd expect to see in
terms of making max scroll offsets by implicit instead of explicit.

How are you feeling about this approach having written it all? Do you feel like
this is going in a cleaner direction?

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:642: 
It seems a little weird for this to live on Layer.  Shouldn't this be on
LayerTreeHost/LayerTreeImpl?

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:377: void LayerImpl::SetScrollable(int clip_layer_id) {
SetScrollable => SetClipLayerId, or possibly just get rid of this entirely and
move this code inside of PushPropertiesTo.

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:476: int clip_height() { return clip_layer_ ?
clip_layer_->bounds().height() : 0; }
Why just height?

[wjmaclean, 2013-11-13 21:01:28]

There are multiple owners required for this Cl:

content/public : Jói
content/test : sky
content/renderer/gpu : piman

enne@ and aelias@ for the remainder

Please take a look and let me know what you think.

(The comments below are responses to pre-review comments from enne@)

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:642: 
On 2013/10/31 18:45:58, enne wrote:
> It seems a little weird for this to live on Layer.  Shouldn't this be on
> LayerTreeHost/LayerTreeImpl?

True. I did this primarily so that WebLayerImpl wouldn't have to know about
LayerTreeHost, but we could move it to LTH if that's not an issue.

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:377: void LayerImpl::SetScrollable(int clip_layer_id) {
On 2013/10/31 18:45:58, enne wrote:
> SetScrollable => SetClipLayerId, or possibly just get rid of this entirely and
> move this code inside of PushPropertiesTo.

Sure, we could re-name it to SetClipLayerId(). Not sure I understand what you
intend when you say "get rid of this entirely", can you clarify? Certainly it
can be encapsulated in PushProperties to (though some tests may want access to
this, e.g. the JSON tree tests).

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/23983047/diff/102001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:476: int clip_height() { return clip_layer_ ?
clip_layer_->bounds().height() : 0; }
On 2013/10/31 18:45:58, enne wrote:
> Why just height?

There are currently no consumers for width ... this is called in
LayerTreeHostImpl::ScrollVerticallyByPage() which only cares about the clip
height.

[Jói, 2013-11-13 22:08:09]

//content/public LGTM.

[sky, 2013-11-13 22:28:48]

content/test LGTM

[aelias_OOO_until_Jul13, 2013-11-14 01:25:35]

My main concern with this is around the interaction with URL-bar hiding.  Have
you tried on a full Clank build to show/hide the URL bar, particularly with
top/bottom fixed position elements, with a horizontal scrollbar, or trying to
scroll near the bottom of the document?  It's a rather tricky area because the
viewport size changes on the impl thread ahead of the Blink thread, and I expect
you will see glitches with this current implementation.

(Note I'll be on vacation starting this Friday for 3 weeks so I may not be very
responsive on this code review.)

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:1079: scaled_scroll_bounds.width() -
clip_layer_->bounds().width(),
Using clip_layer_->bounds() is incorrect on Android where the viewport size is
dynamic due to URL-bar hiding.  This only works when the clip layer
masksToBounds.  (I renamed it to containerLayer in most places because it
doesn't necessarily clip.)  For the inner viewport, the calculation in
LayerTreeHostImpl::UnscaledScrollableViewportSize() should be used instead.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:1111: // TODO(wjmaclean): How do I account for vertical
adjust, or do I need to?
"Vertical adjust" is used to keep the horizontal scrollbars at a stable position
at the bottom of the screen while the URL-bar is moving.  (The renderer
compositor's viewport always starts 1px below the URL-bar -- the browser
compositor takes care of the translation -- so the horizontal scrollbar must
move vertically to appear to remain at a fixed spot.)

[piman, 2013-11-14 19:06:15]

On 2013/11/13 21:01:28, wjmaclean wrote:
> content/renderer/gpu : piman

LGTM for that.

[enne (OOO), 2013-11-14 22:59:00]

I took a brief look through the non-test parts of this patch and this is looking
good in general.  I have some implementation and naming nits here and will try
to look through the test code in more detail soon.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:643: gfx::Vector2d Layer::TotalViewportScrollOffset() const {
This seems like a LayerTreeHost function and not a Layer one.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:367: void LayerImpl::SetScrollable(int clip_layer_id) {
As I said on the other patch, I think this is a somewhat awkward name.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:987: last_scroll_offset_ = scroll_offset;
What is last_scroll_offset_ used for?

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:1067: do {
This loop assumes there are no other scale transforms in the tree.  I think that
is reasonable in practice, but in theory this is not general enough.  I think
you either need to be more general or assert your assumptions.  (This is true
for the position function below as well.)

The more general option here is to use screen space transforms here and map one
layer into the space of the other. You could maybe cache this value somewhere
else so you don't have a map every time MaxScrollOffset is called.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:475: void NotifyScrollbars();
Can you name this more clearly? Maybe even just ScrollbarParametersDidChange
would be clear.

Also, can you not make this public?

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:590: //  gfx::Vector2d max_scroll_offset_;
Remove this line?

https://codereview.chromium.org/23983047/diff/192001/cc/layers/scrollbar_laye...
File cc/layers/scrollbar_layer_impl_base.h (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/scrollbar_laye...
cc/layers/scrollbar_layer_impl_base.h:98: 
Needless extra line.

https://codereview.chromium.org/23983047/diff/192001/cc/test/layer_tree_json_...
File cc/test/layer_tree_json_parser.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/test/layer_tree_json_...
cc/test/layer_tree_json_parser.cc:104: // TODO(wjmaclean) At some time in the
future we may wish to test that a
This is pretty unfortunate.  Can you file a bug to fix this up and link to it in
a comment here?  I'm ok with punting on fixing up all of the json and adjusting
the reader to handle this for this patch, but I think this hack shouldn't live.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1109: // If we get to this point, we are assured we
have an outer viewport.
No comment, just DCHECK.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1126: // The DCHECK below is triggering during
zoom-out.
This seems like a good thing to track down.  How far off is this when it's
wrong? Is this an off-by-one/rounding error?

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:400:
active_tree_->SetCurrentlyScrollingLayer(InnerViewportScrollLayer());
Why do you need to set the currently scrolling layer?

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2445: // We manually manage the bubbling
behaviour here as it is different to that
Huh! I had hoped we wouldn't have to special case thsi, but these are all good
reasons.

[wjmaclean, 2013-11-15 17:04:18]

On 2013/11/14 01:25:35, aelias - OOO until Dec 9 wrote:
> My main concern with this is around the interaction with URL-bar hiding.  Have
> you tried on a full Clank build to show/hide the URL bar, particularly with
> top/bottom fixed position elements, with a horizontal scrollbar, or trying to
> scroll near the bottom of the document?  It's a rather tricky area because the
> viewport size changes on the impl thread ahead of the Blink thread, and I
expect
> you will see glitches with this current implementation.
> 
> (Note I'll be on vacation starting this Friday for 3 weeks so I may not be
very
> responsive on this code review.)
> 
> https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc
> File cc/layers/layer_impl.cc (right):
> 
>
https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
> cc/layers/layer_impl.cc:1079: scaled_scroll_bounds.width() -
> clip_layer_->bounds().width(),
> Using clip_layer_->bounds() is incorrect on Android where the viewport size is
> dynamic due to URL-bar hiding.  This only works when the clip layer
> masksToBounds.  (I renamed it to containerLayer in most places because it
> doesn't necessarily clip.)  For the inner viewport, the calculation in
> LayerTreeHostImpl::UnscaledScrollableViewportSize() should be used instead.
> 
>
https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
> cc/layers/layer_impl.cc:1111: // TODO(wjmaclean): How do I account for
vertical
> adjust, or do I need to?
> "Vertical adjust" is used to keep the horizontal scrollbars at a stable
position
> at the bottom of the screen while the URL-bar is moving.  (The renderer
> compositor's viewport always starts 1px below the URL-bar -- the browser
> compositor takes care of the translation -- so the horizontal scrollbar must
> move vertically to appear to remain at a fixed spot.)

Certainly we will be sure this is all working correctly before landing. Have you
left for vacation already?

I was hoping that if the top-controls manager can be made to correctly set the
clip-layer information, then the special-cases can be avoided.

[aelias_OOO_until_Jul13, 2013-11-15 18:08:32]

On 2013/11/15 17:04:18, wjmaclean wrote:
> On 2013/11/14 01:25:35, aelias - OOO until Dec 9 wrote:
> > My main concern with this is around the interaction with URL-bar hiding. 
Have
> > you tried on a full Clank build to show/hide the URL bar, particularly with
> > top/bottom fixed position elements, with a horizontal scrollbar, or trying
to
> > scroll near the bottom of the document?  It's a rather tricky area because
the
> > viewport size changes on the impl thread ahead of the Blink thread, and I
> expect
> > you will see glitches with this current implementation.
> > 
> > (Note I'll be on vacation starting this Friday for 3 weeks so I may not be
> very
> > responsive on this code review.)
> > 
> > https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc
> > File cc/layers/layer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
> > cc/layers/layer_impl.cc:1079: scaled_scroll_bounds.width() -
> > clip_layer_->bounds().width(),
> > Using clip_layer_->bounds() is incorrect on Android where the viewport size
is
> > dynamic due to URL-bar hiding.  This only works when the clip layer
> > masksToBounds.  (I renamed it to containerLayer in most places because it
> > doesn't necessarily clip.)  For the inner viewport, the calculation in
> > LayerTreeHostImpl::UnscaledScrollableViewportSize() should be used instead.
> > 
> >
>
https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
> > cc/layers/layer_impl.cc:1111: // TODO(wjmaclean): How do I account for
> vertical
> > adjust, or do I need to?
> > "Vertical adjust" is used to keep the horizontal scrollbars at a stable
> position
> > at the bottom of the screen while the URL-bar is moving.  (The renderer
> > compositor's viewport always starts 1px below the URL-bar -- the browser
> > compositor takes care of the translation -- so the horizontal scrollbar must
> > move vertically to appear to remain at a fixed spot.)
> 
> Certainly we will be sure this is all working correctly before landing. Have
you
> left for vacation already?
> 
> I was hoping that if the top-controls manager can be made to correctly set the
> clip-layer information, then the special-cases can be avoided.

Yes, though still checking email for now.

If you go that route, the horizontal overlay scrollbar layer would need to be
positioned relative to the bottom of the clip layer to begin with (instead of
from the top), so this would require a Blink change, maybe marking it
fixed-positioned.  The vertical scrollbar size would also require some kind of
special case since it needs to shrink-to-fit (unless we introduced a general
mechanism in CC for scaling one layer to fit within another).

[enne (OOO), 2013-11-19 19:30:21]

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl_uni...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl_uni...
cc/layers/layer_impl_unittest.cc:338:
layer()->SetBounds(gfx::Size(max_scroll_offset.x(), max_scroll_offset.y()));
This is a little confusing.  The max scroll offset of layer() is
max_scroll_offset, but only because layer() has empty bounds (which seems like a
bogus scenario).

All these tests use the same max scroll offset.  Maybe it would be more clear if
you moved that setup into the test framework?

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_position...
File cc/layers/layer_position_constraint_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_position...
cc/layers/layer_position_constraint_unittest.cc:1093: // TODO(wjmaclean) Not
sure why this used to be (-20, 20), is this right?
I would talk to shawnsingh about this, but I think the previous test was right.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2358: // TODO(wjmaclean) Is it ok to choose
distance from more than two scrollbars?
Ask tony@ what's desired here.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2445: // We manually manage the bubbling
behaviour here as it is different to that
On 2013/11/14 22:59:01, enne wrote:
> Huh! I had hoped we wouldn't have to special case thsi, but these are all good
> reasons.

These changes in LayerTreeHostImpl about handling multiple viewports definitely
need a bunch of unit tests.  Unit tests with and without page scale, testing
bubbling while scrolling, and testing scroll offset changes when pinching, unit
tests with a scroll offset delegate.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:213: const int
INNER_VIEWPORT_SCROLL_LAYER_ID = 2;
kInnerViewportScrollLayerId

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:792: // 
overflow->SetMaxScrollOffset(gfx::Vector2d(overflow_size.width(),
Remove this?

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:1079:
scroll_layer->SetScrollOffset(gfx::Vector2d(10, 10));
Why do these numbers need to change?

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:1432: EXPECT_EQ(gfx::SizeF(50.f,
50.f), metadata.viewport_size);
Why change these numbers?

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1685: false, true,
scroll_layer.get());                        // WJM
?

[wjmaclean, 2013-12-24 21:03:49]

Hopefully this is a step closer :-)

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:643: gfx::Vector2d Layer::TotalViewportScrollOffset() const {
On 2013/11/14 22:59:01, enne wrote:
> This seems like a LayerTreeHost function and not a Layer one.

Done.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:367: void LayerImpl::SetScrollable(int clip_layer_id) {
On 2013/11/14 22:59:01, enne wrote:
> As I said on the other patch, I think this is a somewhat awkward name.

Let's try "SetScrollClipLayer", sound ok?

Fixed in both patches.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:987: last_scroll_offset_ = scroll_offset;
On 2013/11/14 22:59:01, enne wrote:
> What is last_scroll_offset_ used for?

Ooops, remnant of diagnostic code :-)

Removed.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:1067: do {
On 2013/11/14 22:59:01, enne wrote:
> This loop assumes there are no other scale transforms in the tree.  I think
that
> is reasonable in practice, but in theory this is not general enough.  I think
> you either need to be more general or assert your assumptions.  (This is true
> for the position function below as well.)
> 
> The more general option here is to use screen space transforms here and map
one
> layer into the space of the other. You could maybe cache this value somewhere
> else so you don't have a map every time MaxScrollOffset is called.

I've made this more general, allow scale (but not translation) on the layers
between the scroll and clip layers. I've used DCHECKs to enforce the
assumptions.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:1111: // TODO(wjmaclean): How do I account for vertical
adjust, or do I need to?
On 2013/11/14 01:25:35, aelias wrote:
> "Vertical adjust" is used to keep the horizontal scrollbars at a stable
position
> at the bottom of the screen while the URL-bar is moving.  (The renderer
> compositor's viewport always starts 1px below the URL-bar -- the browser
> compositor takes care of the translation -- so the horizontal scrollbar must
> move vertically to appear to remain at a fixed spot.)

Done.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:475: void NotifyScrollbars();
On 2013/11/14 22:59:01, enne wrote:
> Can you name this more clearly? Maybe even just ScrollbarParametersDidChange
> would be clear.
> 
> Also, can you not make this public?

Done.

I needs (I think) to remain public until Clank starts using both viewports, so
that layer_tree_host_impl can access it.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:590: //  gfx::Vector2d max_scroll_offset_;
On 2013/11/14 22:59:01, enne wrote:
> Remove this line?

Done.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl_uni...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_impl_uni...
cc/layers/layer_impl_unittest.cc:338:
layer()->SetBounds(gfx::Size(max_scroll_offset.x(), max_scroll_offset.y()));
On 2013/11/19 19:30:22, enne wrote:
> This is a little confusing.  The max scroll offset of layer() is
> max_scroll_offset, but only because layer() has empty bounds (which seems like
a
> bogus scenario).
> 
> All these tests use the same max scroll offset.  Maybe it would be more clear
if
> you moved that setup into the test framework?

Done.

I moved the setup into the test class definition, and made the scenario
less-bogus by giving the root layer non-zero bounds.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_position...
File cc/layers/layer_position_constraint_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/layer_position...
cc/layers/layer_position_constraint_unittest.cc:1093: // TODO(wjmaclean) Not
sure why this used to be (-20, 20), is this right?
On 2013/11/19 19:30:22, enne wrote:
> I would talk to shawnsingh about this, but I think the previous test was
right.

I have contacted Shawn about this, and will resolve this in the next round.

https://codereview.chromium.org/23983047/diff/192001/cc/layers/scrollbar_laye...
File cc/layers/scrollbar_layer_impl_base.h (right):

https://codereview.chromium.org/23983047/diff/192001/cc/layers/scrollbar_laye...
cc/layers/scrollbar_layer_impl_base.h:98: 
On 2013/11/14 22:59:01, enne wrote:
> Needless extra line.

Done.

https://codereview.chromium.org/23983047/diff/192001/cc/test/layer_tree_json_...
File cc/test/layer_tree_json_parser.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/test/layer_tree_json_...
cc/test/layer_tree_json_parser.cc:104: // TODO(wjmaclean) At some time in the
future we may wish to test that a
On 2013/11/14 22:59:01, enne wrote:
> This is pretty unfortunate.  Can you file a bug to fix this up and link to it
in
> a comment here?  I'm ok with punting on fixing up all of the json and
adjusting
> the reader to handle this for this patch, but I think this hack shouldn't
live.

Indeed, this hack was never meant to live ;-)

I'll file the bug and link it here.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1109: // If we get to this point, we are assured we
have an outer viewport.
On 2013/11/14 22:59:01, enne wrote:
> No comment, just DCHECK.

Done.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1126: // The DCHECK below is triggering during
zoom-out.
On 2013/11/14 22:59:01, enne wrote:
> This seems like a good thing to track down.  How far off is this when it's
> wrong? Is this an off-by-one/rounding error?

This seems to be an off-by-one (or two) type of error. I'm working on tracking
it down ... since it only presents itself when the flag is turned on, can we
file a bug for it, and fix it separately from this patch (assuming I don't find
a fix before this CL lands?)

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:400:
active_tree_->SetCurrentlyScrollingLayer(InnerViewportScrollLayer());
On 2013/11/14 22:59:01, enne wrote:
> Why do you need to set the currently scrolling layer?

It might not be needed ... I'll try to verify this.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2358: // TODO(wjmaclean) Is it ok to choose
distance from more than two scrollbars?
On 2013/11/19 19:30:22, enne wrote:
> Ask tony@ what's desired here.

I have asked Tony, will report back when I hear.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2445: // We manually manage the bubbling
behaviour here as it is different to that
On 2013/11/19 19:30:22, enne wrote:
> On 2013/11/14 22:59:01, enne wrote:
> > Huh! I had hoped we wouldn't have to special case thsi, but these are all
good
> > reasons.
> 
> These changes in LayerTreeHostImpl about handling multiple viewports
definitely
> need a bunch of unit tests.  Unit tests with and without page scale, testing
> bubbling while scrolling, and testing scroll offset changes when pinching,
unit
> tests with a scroll offset delegate.

Since we only have multiple viewports when the flag is on, and since this CL
doesn't turn the flag on, I assume it suffices for this CL to pass with existing
tests, and the new tests can come with the follow on CL to turn on the flag?

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2445: // We manually manage the bubbling
behaviour here as it is different to that
On 2013/11/14 22:59:01, enne wrote:
> Huh! I had hoped we wouldn't have to special case thsi, but these are all good
> reasons.

Agreed!

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:213: const int
INNER_VIEWPORT_SCROLL_LAYER_ID = 2;
On 2013/11/19 19:30:22, enne wrote:
> kInnerViewportScrollLayerId

Done.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:792: // 
overflow->SetMaxScrollOffset(gfx::Vector2d(overflow_size.width(),
On 2013/11/19 19:30:22, enne wrote:
> Remove this?

Done.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:1079:
scroll_layer->SetScrollOffset(gfx::Vector2d(10, 10));
On 2013/11/19 19:30:22, enne wrote:
> Why do these numbers need to change?

They don't, I guess. It's just they were set to a value equal to the max scroll
offset (which didn't matter in the old way of setting max scroll offset). The
test does run fine without changing the numbers.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:1432: EXPECT_EQ(gfx::SizeF(50.f,
50.f), metadata.viewport_size);
On 2013/11/19 19:30:22, enne wrote:
> Why change these numbers?

Hmm, I don't think I *should* have changed these ... I suspect I made a mistake
in the condition to apply in LTI::ScrollableViewportSize().

I've reverted these numbers to their original values.

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1685: false, true,
scroll_layer.get());                        // WJM
On 2013/11/19 19:30:22, enne wrote:
> ?

Ooops! Removed spurious comment.

[tony, 2013-12-26 21:06:49]

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2358: // TODO(wjmaclean) Is it ok to choose
distance from more than two scrollbars?
On 2013/12/24 21:03:49, wjmaclean wrote:
> On 2013/11/19 19:30:22, enne wrote:
> > Ask tony@ what's desired here.
> 
> I have asked Tony, will report back when I hear.

This is fine for now.  The old code would animate both scrollbars if the mouse
pointer was close enough to either scrollbar.  This maintains that behavior,
which seems fine for now.

[enne (OOO), 2014-01-02 19:28:16]

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/192001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1126: // The DCHECK below is triggering during
zoom-out.
On 2013/12/24 21:03:49, wjmaclean wrote:
> On 2013/11/14 22:59:01, enne wrote:
> > This seems like a good thing to track down.  How far off is this when it's
> > wrong? Is this an off-by-one/rounding error?
> 
> This seems to be an off-by-one (or two) type of error. I'm working on tracking
> it down ... since it only presents itself when the flag is turned on, can we
> file a bug for it, and fix it separately from this patch (assuming I don't
find
> a fix before this CL lands?)

Ok.  This seems pretty serious, but endless rebaselining can't be fun from your
end.  It seems much better to spend your time getting this landed sooner rather
than later and fix it up after.

https://codereview.chromium.org/23983047/diff/842001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/842001/cc/layers/layer.h#newcod...
cc/layers/layer.h:575: Layer* clip_layer_;
Sorry for all the naming quibbles, but could you call this clip_layer_viewport_
or scroll_clip_layer_ to make it more clear what this variable is used for? 
Layer already has clip parents and clip children, so "clip layer" is a bit
ambiguous.

Could you also leave a comment about what this variable means?

https://codereview.chromium.org/23983047/diff/842001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/842001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1104: #ifndef NDEBUG
#if ENABLE_DCHECK

https://codereview.chromium.org/23983047/diff/842001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/23983047/diff/842001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:242: gfx::Vector2d BlinkScrollOffset(const Layer*
layer) const;
What does BlinkScrollOffset mean? What about TotalViewportScrollOffset?

Also, is this necessary? Scrolls go to Blink in two ways. 
LayerTreeHostClient::ApplyScrollAndScale sets the total scroll for the main
frame (which should include the viewports) and WebLayerScrollClient::didScroll
(the callback called via Layer::SetScrollOffsetFromImplSide) sets offsets per
layer for non-root (I think) layers (which shouldn't include viewports).

I'm trying to say that I think that WebLayerImpl should just ask the layer for
its scroll offset and that the total scroll offset should just get set via
ApplyScrollAndScale, and this function should only get called in that instance,
not in the WebLayerImpl::scrollPosition() function.

[wjmaclean, 2014-01-03 21:57:11]

Addressed comments, fixed some layout tests.

Current status: all unit tests (with the exception of the position constraint
test currently under discussion) and layout tests passing on desktop. Need to
re-test with Clank (over the weekend or Monday).

https://codereview.chromium.org/23983047/diff/842001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/842001/cc/layers/layer.h#newcod...
cc/layers/layer.h:575: Layer* clip_layer_;
On 2014/01/02 19:28:17, enne wrote:
> Sorry for all the naming quibbles, but could you call this
clip_layer_viewport_
> or scroll_clip_layer_ to make it more clear what this variable is used for? 
> Layer already has clip parents and clip children, so "clip layer" is a bit
> ambiguous.
> 
> Could you also leave a comment about what this variable means?

No problem, done. I've renamed the corresponding vars in LayerImpl as well.

https://codereview.chromium.org/23983047/diff/842001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/842001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1104: #ifndef NDEBUG
On 2014/01/02 19:28:17, enne wrote:
> #if ENABLE_DCHECK

Done.

https://codereview.chromium.org/23983047/diff/842001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/23983047/diff/842001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:242: gfx::Vector2d BlinkScrollOffset(const Layer*
layer) const;
On 2014/01/02 19:28:17, enne wrote:
> What does BlinkScrollOffset mean? What about TotalViewportScrollOffset?
> 

This was code I experimented with to fix a failing unit test. It seems that
since I added it the test has either disappeared, or it's behaviour has been
accommodated by some other change I've made, since I can now remove this code
and the tests seem to run fine.

Removed.
> Also, is this necessary? Scrolls go to Blink in two ways. 
> LayerTreeHostClient::ApplyScrollAndScale sets the total scroll for the main
> frame (which should include the viewports) and WebLayerScrollClient::didScroll
> (the callback called via Layer::SetScrollOffsetFromImplSide) sets offsets per
> layer for non-root (I think) layers (which shouldn't include viewports).
> 
> I'm trying to say that I think that WebLayerImpl should just ask the layer for
> its scroll offset and that the total scroll offset should just get set via
> ApplyScrollAndScale, and this function should only get called in that
instance,
> not in the WebLayerImpl::scrollPosition() function.

[aelias_OOO_until_Jul13, 2014-01-07 00:53:41]

I'm interested in trying to apply this patch locally and seeing how it looks on
Clank, but I don't know which revision it's based against and the tooling isn't
great for conflicts.  For your next patchsets, can you please specify the base
revision in your patch descriptions?

[wjmaclean, 2014-01-07 15:19:34]

Sure ... I wouldn't try this *just* yet, as the scroll offsets seem to be
wrong again ... I'm gardening (filling in for fsamuel@) today, but tonight
or tomorrow morning will put up a revised version (with base revision
specified). I can even provide you with Chrome.apk if you like ...


On Mon, Jan 6, 2014 at 7:53 PM, <aelias@chromium.org> wrote:

> I'm interested in trying to apply this patch locally and seeing how it
> looks on
> Clank, but I don't know which revision it's based against and the tooling
> isn't
> great for conflicts.  For your next patchsets, can you please specify the
> base
> revision in your patch descriptions?
>
> https://codereview.chromium.org/23983047/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wjmaclean, 2014-01-09 20:57:24]

New version, this one is worth trying (works against Clank repo at
ee28e4bfe205f456fdd065b7b3135d8c4f9a6cea, (SVN changes up to revision 243907).

Does not yet include adjustment for top-controls w.r.t to scrollbar sizing and
max_scroll_offset, though does have vertical adjust.

Also, fails (temporarily) LayerTreeHostImplTest.ScrollNonAxisAlignedRotatedLayer
since this version *purposefully* disallows non scaling/translation transforms,
and hits a DCHECK for rotations. Should we allow arbitrary rotations?

[enne (OOO), 2014-01-09 21:03:50]

On 2014/01/09 20:57:24, wjmaclean wrote:

> Also, fails (temporarily)
LayerTreeHostImplTest.ScrollNonAxisAlignedRotatedLayer
> since this version *purposefully* disallows non scaling/translation
transforms,
> and hits a DCHECK for rotations. Should we allow arbitrary rotations?

What layer is the rotation on in the tree?

[wjmaclean, 2014-01-09 22:09:00]

On 2014/01/09 21:03:50, enne wrote:
> On 2014/01/09 20:57:24, wjmaclean wrote:
> 
> > Also, fails (temporarily)
> LayerTreeHostImplTest.ScrollNonAxisAlignedRotatedLayer
> > since this version *purposefully* disallows non scaling/translation
> transforms,
> > and hits a DCHECK for rotations. Should we allow arbitrary rotations?
> 
> What layer is the rotation on in the tree?

In the test, it's on a layer that's marked as "scrollable", thus a scroll-layer
in the new clip/scroll/scale-layer parlance. The scroll layer is rotated by 20
degrees, but it's hard to define either (1) MaxScrollOffset() or (2) scrollbar
parameters when the scroll layer is rotated with respect to the clip layer.

We had talked earlier (in the design doc?) about limiting to cases when the
transform between the scroll-layer and clip-layer is, at most, scale and/or
translation (at present I'm enforcing isotropic scaling, though I can see how
that could be relaxed.

An alternative to fix the test would be to apply the rotation to the clip layer
instead, and allow rotation on the clip layer (since this presumably doesn't
alter the transform "between" the two layers, which would then be limited to
pure scale and/or translation. Does that make ok sense?

[wjmaclean, 2014-01-13 16:51:22]

On 2014/01/09 22:09:00, wjmaclean wrote:
> On 2014/01/09 21:03:50, enne wrote:
> > On 2014/01/09 20:57:24, wjmaclean wrote:
> > 
> > > Also, fails (temporarily)
> > LayerTreeHostImplTest.ScrollNonAxisAlignedRotatedLayer
> > > since this version *purposefully* disallows non scaling/translation
> > transforms,
> > > and hits a DCHECK for rotations. Should we allow arbitrary rotations?
> > 
> > What layer is the rotation on in the tree?
> 
> In the test, it's on a layer that's marked as "scrollable", thus a
scroll-layer
> in the new clip/scroll/scale-layer parlance. The scroll layer is rotated by 20
> degrees, but it's hard to define either (1) MaxScrollOffset() or (2) scrollbar
> parameters when the scroll layer is rotated with respect to the clip layer.
> 
> We had talked earlier (in the design doc?) about limiting to cases when the
> transform between the scroll-layer and clip-layer is, at most, scale and/or
> translation (at present I'm enforcing isotropic scaling, though I can see how
> that could be relaxed.
> 
> An alternative to fix the test would be to apply the rotation to the clip
layer
> instead, and allow rotation on the clip layer (since this presumably doesn't
> alter the transform "between" the two layers, which would then be limited to
> pure scale and/or translation. Does that make ok sense?

I decided to downgrade the DCHECK on non-scale transformations to just excluding
them using an if-statement. This seems to make it happier. This solves the issue
of the failing unit test.

This patch-set should work with clank repo at:

de159e0626ad38e87cf300a92dc849e3029ac3d5
SVN changes up to revision 244478

[enne (OOO), 2014-01-15 00:54:50]

Please don't downgrade the DCHECK.  I think it's important to check that
assumption since the code won't handle it correctly.

[wjmaclean, 2014-01-15 14:01:08]

On 2014/01/15 00:54:50, enne wrote:
> Please don't downgrade the DCHECK.  I think it's important to check that
> assumption since the code won't handle it correctly.

OK, I'll roll that back in the next patch set when it is posted.

[aelias_OOO_until_Jul13, 2014-01-16 03:44:03]

OK, I gave this a more detailed look through.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:685: scroll_offset =
layer_tree_host()->DistributeScrollOffsetToViewports(
Instead of doing this, I think we should make Blink be fully aware of the inner
and outer viewports and call SetScrollOffset on both layers.  I don't think it's
generally legitimate to split a combined scroll offset into some arbitrary mix.

One place this distribution might be appropriate is when restoring from a legacy
history item, but in that case that distribution logic should be in Blink, not
CC.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:711: // TODO(wjmaclean) We should template this and put it
into LayerTreeHostCommon
I don't believe main-thread Layers need to know about max scroll offset at all,
so maybe we can just delete this instead?

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:275: bool scrollable() const { return scroll_clip_layer_; }
Could you leave this one checking scrollable_?  It may happen due to things like
"overflow: hidden" that a normally scrollable layer is not, so we need to keep
it in a separate boolean.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:573: Layer* scroll_clip_layer_;
Instead of having this pointer, I think we should walk up the tree to find the
nearest layer with is_container_for_fixed_position_layers_.  You can rename that
variable to is_scroll_container_ to clarify it's now used for more than fixed
position.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1094: gfx::Vector2d LayerImpl::MaxScrollOffset() const {
Just checking: have you verified this logic also works with scrollable
subregions, since you're now ignoring what Blink specifies for those?

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1128: const gfx::Transform& layer_transform =
current_layer->transform();
We don't need to take the other transforms into consideration at all.  They
cannot change the max scroll offset.  Only page scale can, because it resizes
the viewport (CSS scale transforms cannot do that).  So you can delete the code
using layer_transform.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1159: max_offset.Scale(1 / scale_factor);
The old UpdateMaxScrollOffset() logic was simpler than this and didn't do any
explicit scaling at all.  I don't think you need to here either.  It seems to me
that:

inner max scroll offset = OuterViewportSize() - InnerViewportSize()
outer max scroll offset = ScrollableSize() - OuterViewportSize()

and assuming all the methods return values in CSS coordinates, you are done. 
Only the implementation of InnerViewportSize() needs to take page scale into
account.

(InnerViewportSize already exists in the form of the current implementation of
ScrollableViewportSize, and OuterViewportSize is currently called
UnscaledScrollableViewportSize, but I suggest renaming them.)

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1201: gfx::Vector2dF current_offset;
Likewise, I don't think any scaling is needed here.  Just add together inner and
outer total scroll offsets to get an offset in CSS coordinates.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/painted_scrol...
File cc/layers/painted_scrollbar_layer.h (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/painted_scrol...
cc/layers/painted_scrollbar_layer.h:84: scoped_refptr<Layer> scroll_layer_;
Can this be a weak pointer instead?  Refptrs risk the danger of layer trees
inappropriately remaining alive and wasting memory.

Also, can't you use the one from ScrollbarLayerImplBase instead of tracking it
separately?

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_impl_base.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_impl_base.cc:31:
ScrollbarLayerImplBase::~ScrollbarLayerImplBase() {
This should make sure to call RemoveScrollbar() to avoid leaving dangling
pointers.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_impl_base.cc:72: clip_layer_->AddScrollbar(this);
It's weird that the clip layer is getting called AddScrollbar just like the
scroll layer even though the semantic relationship is very different.  What does
the clip layer need this for?  Isn't it enough for the scrollbar to have one-way
knowledge of the clip?

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_unittest.cc:181: #define
UPDATE_AND_EXTRACK_LAYER_POINTERS \
typo: should be EXTRACT

https://codereview.chromium.org/23983047/diff/1112001/cc/test/layer_tree_json...
File cc/test/layer_tree_json_parser.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/test/layer_tree_json...
cc/test/layer_tree_json_parser.cc:104: // TODO(wjmaclean) At some time in the
future we may wish to test that a
I think this problem goes away if you follow my suggestion above not to have a
special pointer for this at all.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (left):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:335: sync_tree->FindRootScrollLayer();
I don't follow why this can be removed (particularly for the case where your new
flag isn't specified), can you put it back or explain why it's no longer needed?

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:335: if (page_scale_layer_ &&
inner_viewport_scroll_layer_) {
I suggest doing DCHECK(inner_viewport_scroll_layer_) instead if it can never
happen to have one without the other.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1078: inner_viewport_scroll_delta +
outer_viewport_scroll_delta,
Blink will ultimately have to be aware of the distinction between inner and
outer viewport scrolls for everything to work properly.  This API should be
updated to provide both values separately.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1083: gfx::Vector2d
LayerTreeHost::DistributeScrollOffsetToViewports(
As mentioned above, I don't think this function should exist here.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:437:
active_tree_->SetCurrentlyScrollingLayer(InnerViewportScrollLayer());
I have a suggested change in AnimatePageScale() that makes this unnecessary,
please delete it.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2159: ((CurrentlyScrollingLayer() ==
InnerViewportScrollLayer() &&
It looks like this should be also checking for OuterViewportScrollLayer in the
new world.  This line is probably why top-controls hiding is not working.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2215: layer_impl == OuterViewportScrollLayer())
This looks incorrect, since unused outer-viewport scrolls can be consumed by the
inner viewport.  So it should only add to unused_root_delta when layer_impl ==
InnerViewportScrollLayer()

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2474: LayerImpl* pinch_scroll_layer =
CurrentlyScrollingLayer();
A pinch can never scroll sublayers, so this variable is unnecessary.  Please
directly scroll InnerViewportScrollLayer()/OuterViewportScrollLayer() below
instead.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2498: if (magnify_delta < 1.f) {
This if statement isn't needed, just always call the Clamp method.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2584: ScrollBy(gfx::Point(), next_scroll -
scroll_total);
This doesn't look right.  I'm not sure the scroll delta is in the right
coordinate space, and it could fall on a scrollable sublayer if there's one at
(0, 0).  How about calling PinchGestureUpdate here instead, since this likewise
needs to change the page scale and scroll offset on the root layers?

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2591: ClearCurrentlyScrollingLayer();
Not needed if you never set it.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:328: gfx::SizeF
LayerTreeImpl::ScrollableViewportSize() const {
The semantics of what this returns are now quite confusing.  This calls for a
rename: please rename UnscaledScrollableViewportSize to OuterViewportSize and
ScrollableViewportSize to InnerViewportSize (and don't change the original
implementation).

[wjmaclean, 2014-01-16 15:07:31]

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:685: scroll_offset =
layer_tree_host()->DistributeScrollOffsetToViewports(
On 2014/01/16 03:44:04, aelias wrote:
> Instead of doing this, I think we should make Blink be fully aware of the
inner
> and outer viewports and call SetScrollOffset on both layers.  I don't think
it's
> generally legitimate to split a combined scroll offset into some arbitrary
mix.
> 
> One place this distribution might be appropriate is when restoring from a
legacy
> history item, but in that case that distribution logic should be in Blink, not
> CC.

Hmm, but the design doc specifies (and I have always been told) that Blink *must
not* know about the two viewport layers (at least, not above the WebView level).

I agree that the splitting algorithm should not be arbitrary, but as long as it
is done in a way that doesn't introduce jitter (inner and/or outer viewport
suddenly changes in a non-smooth way), it should be fine. Given the bubble-up
nature of the scrolls, I think it's possible to do this.

If we suddenly change to letting Blink know about both viewports, that is a
*big* change in direction. This strategy has been in this patch for well over a
month now (probably much longer), so I would have expected this feedback a lot
sooner.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:711: // TODO(wjmaclean) We should template this and put it
into LayerTreeHostCommon
On 2014/01/16 03:44:04, aelias wrote:
> I don't believe main-thread Layers need to know about max scroll offset at
all,
> so maybe we can just delete this instead?

Main thread layers need to know in order to deal with the combining/splitting
issue.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:275: bool scrollable() const { return scroll_clip_layer_; }
On 2014/01/16 03:44:04, aelias wrote:
> Could you leave this one checking scrollable_?  It may happen due to things
like
> "overflow: hidden" that a normally scrollable layer is not, so we need to keep
> it in a separate boolean.

I would suggest that in the case of overflow:hidden we just set the clip layer
to 0.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:573: Layer* scroll_clip_layer_;
On 2014/01/16 03:44:04, aelias wrote:
> Instead of having this pointer, I think we should walk up the tree to find the
> nearest layer with is_container_for_fixed_position_layers_.  You can rename
that
> variable to is_scroll_container_ to clarify it's now used for more than fixed
> position.

What if we need to have a scroll layer container that is not also a fixed
container? For example, I can see this happening with the inner/outer viewports,
where only one will be a fixed position container.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1094: gfx::Vector2d LayerImpl::MaxScrollOffset() const {
On 2014/01/16 03:44:04, aelias wrote:
> Just checking: have you verified this logic also works with scrollable
> subregions, since you're now ignoring what Blink specifies for those?

I'm not sure that I understand what you mean by "ignoring".

Yes, I believe this works with scrollable sub-regions (e.g. an embedded iFrame).

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1128: const gfx::Transform& layer_transform =
current_layer->transform();
On 2014/01/16 03:44:04, aelias wrote:
> We don't need to take the other transforms into consideration at all.  They
> cannot change the max scroll offset.  Only page scale can, because it resizes
> the viewport (CSS scale transforms cannot do that).  So you can delete the
code
> using layer_transform.

You're assuming that this scrolling mechanism is only ever used for viewports,
but it is meant to be more general than that (sub-frames, etc.) In general we
have assumed that scale transformations (other than page scale) can exist
between the clip and the scroll layer.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1159: max_offset.Scale(1 / scale_factor);
On 2014/01/16 03:44:04, aelias wrote:
> The old UpdateMaxScrollOffset() logic was simpler than this and didn't do any
> explicit scaling at all.  I don't think you need to here either.  It seems to
me
> that:
> 
> inner max scroll offset = OuterViewportSize() - InnerViewportSize()
> outer max scroll offset = ScrollableSize() - OuterViewportSize()
> 
> and assuming all the methods return values in CSS coordinates, you are done. 
> Only the implementation of InnerViewportSize() needs to take page scale into
> account.
> 
> (InnerViewportSize already exists in the form of the current implementation of
> ScrollableViewportSize, and OuterViewportSize is currently called
> UnscaledScrollableViewportSize, but I suggest renaming them.)

Again, I think the more general model we want to support allows for scaling
between a clip layer and its scroll layer.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/painted_scrol...
File cc/layers/painted_scrollbar_layer.h (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/painted_scrol...
cc/layers/painted_scrollbar_layer.h:84: scoped_refptr<Layer> scroll_layer_;
On 2014/01/16 03:44:04, aelias wrote:
> Can this be a weak pointer instead?  Refptrs risk the danger of layer trees
> inappropriately remaining alive and wasting memory.
> 
> Also, can't you use the one from ScrollbarLayerImplBase instead of tracking it
> separately?

I will investigate, and re-use the one in ScrollbarLayerImplBase assuming all
else is equal.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_impl_base.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_impl_base.cc:31:
ScrollbarLayerImplBase::~ScrollbarLayerImplBase() {
On 2014/01/16 03:44:04, aelias wrote:
> This should make sure to call RemoveScrollbar() to avoid leaving dangling
> pointers.

Done.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_impl_base.cc:72: clip_layer_->AddScrollbar(this);
On 2014/01/16 03:44:04, aelias wrote:
> It's weird that the clip layer is getting called AddScrollbar just like the
> scroll layer even though the semantic relationship is very different.  What
does
> the clip layer need this for?  Isn't it enough for the scrollbar to have
one-way
> knowledge of the clip?

The scrollbar should not have to rely on polling the clip layer to know when
changes have occurred. Thereforefore both the scroll layer and the scroll clip
layer will push notifications to any registered scrollbars when relevant
geometry changes.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_unittest.cc:181: #define
UPDATE_AND_EXTRACK_LAYER_POINTERS \
On 2014/01/16 03:44:04, aelias wrote:
> typo: should be EXTRACT

Done.

https://codereview.chromium.org/23983047/diff/1112001/cc/test/layer_tree_json...
File cc/test/layer_tree_json_parser.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/test/layer_tree_json...
cc/test/layer_tree_json_parser.cc:104: // TODO(wjmaclean) At some time in the
future we may wish to test that a
On 2014/01/16 03:44:04, aelias wrote:
> I think this problem goes away if you follow my suggestion above not to have a
> special pointer for this at all.

This comment will be addressed based on the outcome of the clip layer pointer.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (left):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:335: sync_tree->FindRootScrollLayer();
On 2014/01/16 03:44:04, aelias wrote:
> I don't follow why this can be removed (particularly for the case where your
new
> flag isn't specified), can you put it back or explain why it's no longer
needed?

The functions that register the inner/outerviewport layers explicitly tell the
trees which layers are which, so there's no longer any need for a "discovery"
mechanism.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:335: if (page_scale_layer_ &&
inner_viewport_scroll_layer_) {
On 2014/01/16 03:44:04, aelias wrote:
> I suggest doing DCHECK(inner_viewport_scroll_layer_) instead if it can never
> happen to have one without the other.

There are tests that do not explicitly specify these layers.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1078: inner_viewport_scroll_delta +
outer_viewport_scroll_delta,
On 2014/01/16 03:44:04, aelias wrote:
> Blink will ultimately have to be aware of the distinction between inner and
> outer viewport scrolls for everything to work properly.  This API should be
> updated to provide both values separately.

Again, it was always my understanding that people were *against* Blink knowing
about the separation of the layers. Either is a valid design decision, but given
that we went the other way, this becomes a big change.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1083: gfx::Vector2d
LayerTreeHost::DistributeScrollOffsetToViewports(
On 2014/01/16 03:44:04, aelias wrote:
> As mentioned above, I don't think this function should exist here.

Understood. Whether it remains or not will need to depend on the resolution of
the Blink-awareness question.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:437:
active_tree_->SetCurrentlyScrollingLayer(InnerViewportScrollLayer());
On 2014/01/16 03:44:04, aelias wrote:
> I have a suggested change in AnimatePageScale() that makes this unnecessary,
> please delete it.

Done.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2159: ((CurrentlyScrollingLayer() ==
InnerViewportScrollLayer() &&
On 2014/01/16 03:44:04, aelias wrote:
> It looks like this should be also checking for OuterViewportScrollLayer in the
> new world.  This line is probably why top-controls hiding is not working.

You mean not working with the flag turned on? Because it does work with the flag
off. I will make this change, though this shouldn't block the current CL (with
the flag off).

Thanks.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2215: layer_impl == OuterViewportScrollLayer())
On 2014/01/16 03:44:04, aelias wrote:
> This looks incorrect, since unused outer-viewport scrolls can be consumed by
the
> inner viewport.  So it should only add to unused_root_delta when layer_impl ==
> InnerViewportScrollLayer()

Thanks for pointing this out. I haven't had much chance to debug with the flag
turned on for Clank.

Done.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2474: LayerImpl* pinch_scroll_layer =
CurrentlyScrollingLayer();
On 2014/01/16 03:44:04, aelias wrote:
> A pinch can never scroll sublayers, so this variable is unnecessary.  Please
> directly scroll InnerViewportScrollLayer()/OuterViewportScrollLayer() below
> instead.

Makes sense ... so default to OuterViewportScrollLayer() if it exists, and if
not use InnerViewportScrollLayer()?

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2498: if (magnify_delta < 1.f) {
On 2014/01/16 03:44:04, aelias wrote:
> This if statement isn't needed, just always call the Clamp method.

Done.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2584: ScrollBy(gfx::Point(), next_scroll -
scroll_total);
On 2014/01/16 03:44:04, aelias wrote:
> This doesn't look right.  I'm not sure the scroll delta is in the right
> coordinate space, and it could fall on a scrollable sublayer if there's one at
> (0, 0).  How about calling PinchGestureUpdate here instead, since this
likewise
> needs to change the page scale and scroll offset on the root layers?

Sure, seems reasonable. I'll investigate and modify accordingly.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2591: ClearCurrentlyScrollingLayer();
On 2014/01/16 03:44:04, aelias wrote:
> Not needed if you never set it.

Done.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:328: gfx::SizeF
LayerTreeImpl::ScrollableViewportSize() const {
On 2014/01/16 03:44:04, aelias wrote:
> The semantics of what this returns are now quite confusing.  This calls for a
> rename: please rename UnscaledScrollableViewportSize to OuterViewportSize and
> ScrollableViewportSize to InnerViewportSize (and don't change the original
> implementation).

I agree with the renaming, though I think we need to first resolve the issue of
scaling transforms (in general) prior to deciding if the implementations need
change.

[enne (OOO), 2014-01-16 19:35:52]

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:685: scroll_offset =
layer_tree_host()->DistributeScrollOffsetToViewports(
On 2014/01/16 03:44:04, aelias wrote:
> Instead of doing this, I think we should make Blink be fully aware of the
inner
> and outer viewports and call SetScrollOffset on both layers.  I don't think
it's
> generally legitimate to split a combined scroll offset into some arbitrary
mix.

I disagree.  I don't think Javascript is ever going to know about the two
viewports, so at some point these are going to need to be combined or
distributed depending on which direction the information is flowing.  Then the
question then is where to do this, and I think this patch has a reasonable
direction.

If you don't think it's legitimate, how do you handle communicating with
Javascript?

[aelias_OOO_until_Jul13, 2014-01-16 19:49:25]

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:685: scroll_offset =
layer_tree_host()->DistributeScrollOffsetToViewports(
On 2014/01/16 15:07:32, wjmaclean wrote:
> On 2014/01/16 03:44:04, aelias wrote:
> > Instead of doing this, I think we should make Blink be fully aware of the
> inner
> > and outer viewports and call SetScrollOffset on both layers.  I don't think
> it's
> > generally legitimate to split a combined scroll offset into some arbitrary
> mix.
> > 
> > One place this distribution might be appropriate is when restoring from a
> legacy
> > history item, but in that case that distribution logic should be in Blink,
not
> > CC.
> 
> Hmm, but the design doc specifies (and I have always been told) that Blink
*must
> not* know about the two viewport layers (at least, not above the WebView
level).

For the purpose of this comment I am including WebViewImpl into the concept of
"Blink".  I don't think that the split should be plumbed down all the way to the
depths of Blink, but WebViewImpl and possibly a few top-level classes like
FrameView need to be aware of it.  Blink will use the outer offset for some
things (like fixed-pos) and the combined offset for other things (like Android
selection handles) so I don't see how it can work for it to only hear about the
combined one.

Sorry if it didn't end up sufficiently emphasized in the design doc, but I
recall emphasizing this (the upper layers wrapping Blink being aware of both
offsets) as a key condition for this split to be feasible.  For what it's worth,
I think the design change is somewhat superficial and your existing work is
mostly compatible with it.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:275: bool scrollable() const { return scroll_clip_layer_; }
On 2014/01/16 15:07:32, wjmaclean wrote:
> On 2014/01/16 03:44:04, aelias wrote:
> > Could you leave this one checking scrollable_?  It may happen due to things
> like
> > "overflow: hidden" that a normally scrollable layer is not, so we need to
keep
> > it in a separate boolean.
> 
> I would suggest that in the case of overflow:hidden we just set the clip layer
> to 0.

But overflow: hidden layers can still have a nonzero scroll offset, even though
they can't be gesture scrolled.  Since you're using the clip layer to compute
max scroll offset I believe this wouldn't be supported?

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:573: Layer* scroll_clip_layer_;
On 2014/01/16 15:07:32, wjmaclean wrote:
> On 2014/01/16 03:44:04, aelias wrote:
> > Instead of having this pointer, I think we should walk up the tree to find
the
> > nearest layer with is_container_for_fixed_position_layers_.  You can rename
> that
> > variable to is_scroll_container_ to clarify it's now used for more than
fixed
> > position.
> 
> What if we need to have a scroll layer container that is not also a fixed
> container? For example, I can see this happening with the inner/outer
viewports,
> where only one will be a fixed position container.

I propose marking both the inner and outer viewports as fixed position
containers.  It's harmless to overmark.  Basically any clip layer would have
this boolean set to true.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1124: current_layer != scroll_clip_layer_->parent();
As one thing that appears like a bug here, you're looking at the clip layer's
transform but that's above the clip layer, not in between the clip layer and
scroll layer.  I think the only thing you may need to consider on the clip layer
is its sublayer_transform (which probably should be looked at for the in-between
layers as well).

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1128: const gfx::Transform& layer_transform =
current_layer->transform();
On 2014/01/16 15:07:32, wjmaclean wrote:
> On 2014/01/16 03:44:04, aelias wrote:
> > We don't need to take the other transforms into consideration at all.  They
> > cannot change the max scroll offset.  Only page scale can, because it
resizes
> > the viewport (CSS scale transforms cannot do that).  So you can delete the
> code
> > using layer_transform.
> 
> You're assuming that this scrolling mechanism is only ever used for viewports,
> but it is meant to be more general than that (sub-frames, etc.) In general we
> have assumed that scale transformations (other than page scale) can exist
> between the clip and the scroll layer.

OK, I read this as trying to compensate for CSS scale transformations coming
from the web author and my point was that no such compensations are necessary. 
It sounds like you're just trying to make things cleaner and more general at a
CC level then?  If so I can see how this makes  sense and I think I'm ok with
it.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1152: if (this ==
layer_tree_impl()->InnerViewportScrollLayer())
If we are going to go down the path of generality then, we should start to think
about how to avoid this kind of if statement, though that can wait for a later
patch.

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2474: LayerImpl* pinch_scroll_layer =
CurrentlyScrollingLayer();
On 2014/01/16 15:07:32, wjmaclean wrote:
> On 2014/01/16 03:44:04, aelias wrote:
> > A pinch can never scroll sublayers, so this variable is unnecessary.  Please
> > directly scroll InnerViewportScrollLayer()/OuterViewportScrollLayer() below
> > instead.
> 
> Makes sense ... so default to OuterViewportScrollLayer() if it exists, and if
> not use InnerViewportScrollLayer()?

Right.

[wjmaclean, 2014-01-16 20:13:30]

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:275: bool scrollable() const { return scroll_clip_layer_; }
On 2014/01/16 19:49:26, aelias wrote:
> On 2014/01/16 15:07:32, wjmaclean wrote:
> > On 2014/01/16 03:44:04, aelias wrote:
> > > Could you leave this one checking scrollable_?  It may happen due to
things
> > like
> > > "overflow: hidden" that a normally scrollable layer is not, so we need to
> keep
> > > it in a separate boolean.
> > 
> > I would suggest that in the case of overflow:hidden we just set the clip
layer
> > to 0.
> 
> But overflow: hidden layers can still have a nonzero scroll offset, even
though
> they can't be gesture scrolled.  Since you're using the clip layer to compute
> max scroll offset I believe this wouldn't be supported?

I don't think so ... why is this behaviour needed? Is it really used? I'm just
not familiar with it.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:573: Layer* scroll_clip_layer_;
On 2014/01/16 19:49:26, aelias wrote:
> On 2014/01/16 15:07:32, wjmaclean wrote:
> > On 2014/01/16 03:44:04, aelias wrote:
> > > Instead of having this pointer, I think we should walk up the tree to find
> the
> > > nearest layer with is_container_for_fixed_position_layers_.  You can
rename
> > that
> > > variable to is_scroll_container_ to clarify it's now used for more than
> fixed
> > > position.
> > 
> > What if we need to have a scroll layer container that is not also a fixed
> > container? For example, I can see this happening with the inner/outer
> viewports,
> > where only one will be a fixed position container.
> 
> I propose marking both the inner and outer viewports as fixed position
> containers.  It's harmless to overmark.  Basically any clip layer would have
> this boolean set to true.

I still don't understand ... what if we want a fixed position element to attach
to the InnerViewport? Don't these elements walk up the tree until they find a
fixed position container, which in this case would end up finding the
OuterViewport first?

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1124: current_layer != scroll_clip_layer_->parent();
On 2014/01/16 19:49:26, aelias wrote:
> As one thing that appears like a bug here, you're looking at the clip layer's
> transform but that's above the clip layer, not in between the clip layer and
> scroll layer.  I think the only thing you may need to consider on the clip
layer
> is its sublayer_transform (which probably should be looked at for the
in-between
> layers as well).


I wondered about that ... we can exclude the clip layer's transform easily
enough in the processing (though some tests combine the clip layer and page
scale layers, just a nuisance case ...)

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1152: if (this ==
layer_tree_impl()->InnerViewportScrollLayer())
On 2014/01/16 19:49:26, aelias wrote:
> If we are going to go down the path of generality then, we should start to
think
> about how to avoid this kind of if statement, though that can wait for a later
> patch.

Yes, we had identified this special case as a weakness, but that's why I'd like
to just do it base on the actual bounds of the layer, plus the transform via the
page scale layer, i.e. not calling ScrollableViewportSize(). In fact, I think we
can avoid making that call at all.

[aelias_OOO_until_Jul13, 2014-01-16 20:21:37]

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:275: bool scrollable() const { return scroll_clip_layer_; }
On 2014/01/16 20:13:36, wjmaclean wrote:
> On 2014/01/16 19:49:26, aelias wrote:
> > On 2014/01/16 15:07:32, wjmaclean wrote:
> > > On 2014/01/16 03:44:04, aelias wrote:
> > > > Could you leave this one checking scrollable_?  It may happen due to
> things
> > > like
> > > > "overflow: hidden" that a normally scrollable layer is not, so we need
to
> > keep
> > > > it in a separate boolean.
> > > 
> > > I would suggest that in the case of overflow:hidden we just set the clip
> layer
> > > to 0.
> > 
> > But overflow: hidden layers can still have a nonzero scroll offset, even
> though
> > they can't be gesture scrolled.  Since you're using the clip layer to
compute
> > max scroll offset I believe this wouldn't be supported?
> 
> I don't think so ... why is this behaviour needed? Is it really used? I'm just
> not familiar with it.

It's a web standard.  There are layout tests for it you can look around for. 
Web developers can disable user scrolling using it, while still scrolling using
Javascript to a certain place, and they do it frequently on sublayers.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:573: Layer* scroll_clip_layer_;
On 2014/01/16 20:13:36, wjmaclean wrote:
> On 2014/01/16 19:49:26, aelias wrote:
> > On 2014/01/16 15:07:32, wjmaclean wrote:
> > > On 2014/01/16 03:44:04, aelias wrote:
> > > > Instead of having this pointer, I think we should walk up the tree to
find
> > the
> > > > nearest layer with is_container_for_fixed_position_layers_.  You can
> rename
> > > that
> > > > variable to is_scroll_container_ to clarify it's now used for more than
> > fixed
> > > > position.
> > > 
> > > What if we need to have a scroll layer container that is not also a fixed
> > > container? For example, I can see this happening with the inner/outer
> > viewports,
> > > where only one will be a fixed position container.
> > 
> > I propose marking both the inner and outer viewports as fixed position
> > containers.  It's harmless to overmark.  Basically any clip layer would have
> > this boolean set to true.
> 
> I still don't understand ... what if we want a fixed position element to
attach
> to the InnerViewport? Don't these elements walk up the tree until they find a
> fixed position container, which in this case would end up finding the
> OuterViewport first?

Web fixed-pos elements are supposed to attach to the outer viewport anyway. 
According to the doc
https://docs.google.com/a/chromium.org/document/d/1KewSqexq4Pd99RUFB-yVoUIJoc...
we already said that both inner and outer clip layers would be fixed containers.

[aelias_OOO_until_Jul13, 2014-01-16 20:36:36]

OK, anyway, we need to address all the concerns before flipping the flag to ship
(particularly the Blink interaction question), but this patch lgtm to land as
it's mostly moving in a good direction and doesn't break non-flag codepaths.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1152: if (this ==
layer_tree_impl()->InnerViewportScrollLayer())
On 2014/01/16 20:13:36, wjmaclean wrote:
> On 2014/01/16 19:49:26, aelias wrote:
> > If we are going to go down the path of generality then, we should start to
> think
> > about how to avoid this kind of if statement, though that can wait for a
later
> > patch.
> 
> Yes, we had identified this special case as a weakness, but that's why I'd
like
> to just do it base on the actual bounds of the layer, plus the transform via
the
> page scale layer, i.e. not calling ScrollableViewportSize(). In fact, I think
we
> can avoid making that call at all.

If you try that, you're going to run into the problem of Android-specific
adjustments for top controls.  It would be nice to bake these into the bounds,
but it's a bit of a can of worms I suggest we do later.

[wjmaclean, 2014-01-16 20:39:40]

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:275: bool scrollable() const { return scroll_clip_layer_; }
On 2014/01/16 20:21:38, aelias wrote:
> On 2014/01/16 20:13:36, wjmaclean wrote:
> > On 2014/01/16 19:49:26, aelias wrote:
> > > On 2014/01/16 15:07:32, wjmaclean wrote:
> > > > On 2014/01/16 03:44:04, aelias wrote:
> > > > > Could you leave this one checking scrollable_?  It may happen due to
> > things
> > > > like
> > > > > "overflow: hidden" that a normally scrollable layer is not, so we need
> to
> > > keep
> > > > > it in a separate boolean.
> > > > 
> > > > I would suggest that in the case of overflow:hidden we just set the clip
> > layer
> > > > to 0.
> > > 
> > > But overflow: hidden layers can still have a nonzero scroll offset, even
> > though
> > > they can't be gesture scrolled.  Since you're using the clip layer to
> compute
> > > max scroll offset I believe this wouldn't be supported?
> > 
> > I don't think so ... why is this behaviour needed? Is it really used? I'm
just
> > not familiar with it.
> 
> It's a web standard.  There are layout tests for it you can look around for. 
> Web developers can disable user scrolling using it, while still scrolling
using
> Javascript to a certain place, and they do it frequently on sublayers.

Ok. That being said, much of the existing code never checks max scroll offset
when doing a scroll, so this may boil down to being careful about where we do or
don't apply clamping to max scroll offset.

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer.h#newco...
cc/layers/layer.h:573: Layer* scroll_clip_layer_;
On 2014/01/16 20:21:38, aelias wrote:
> On 2014/01/16 20:13:36, wjmaclean wrote:
> > On 2014/01/16 19:49:26, aelias wrote:
> > > On 2014/01/16 15:07:32, wjmaclean wrote:
> > > > On 2014/01/16 03:44:04, aelias wrote:
> > > > > Instead of having this pointer, I think we should walk up the tree to
> find
> > > the
> > > > > nearest layer with is_container_for_fixed_position_layers_.  You can
> > rename
> > > > that
> > > > > variable to is_scroll_container_ to clarify it's now used for more
than
> > > fixed
> > > > > position.
> > > > 
> > > > What if we need to have a scroll layer container that is not also a
fixed
> > > > container? For example, I can see this happening with the inner/outer
> > > viewports,
> > > > where only one will be a fixed position container.
> > > 
> > > I propose marking both the inner and outer viewports as fixed position
> > > containers.  It's harmless to overmark.  Basically any clip layer would
have
> > > this boolean set to true.
> > 
> > I still don't understand ... what if we want a fixed position element to
> attach
> > to the InnerViewport? Don't these elements walk up the tree until they find
a
> > fixed position container, which in this case would end up finding the
> > OuterViewport first?
> 
> Web fixed-pos elements are supposed to attach to the outer viewport anyway. 
> According to the doc
>
https://docs.google.com/a/chromium.org/document/d/1KewSqexq4Pd99RUFB-yVoUIJoc...
> we already said that both inner and outer clip layers would be fixed
containers.

Ok, though I still think it's better to keep scroll_clip and fixed
container-ness separate (perhaps for any embedded scroll layers that a page may
have?).

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1112001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1152: if (this ==
layer_tree_impl()->InnerViewportScrollLayer())
On 2014/01/16 20:36:38, aelias wrote:
> On 2014/01/16 20:13:36, wjmaclean wrote:
> > On 2014/01/16 19:49:26, aelias wrote:
> > > If we are going to go down the path of generality then, we should start to
> > think
> > > about how to avoid this kind of if statement, though that can wait for a
> later
> > > patch.
> > 
> > Yes, we had identified this special case as a weakness, but that's why I'd
> like
> > to just do it base on the actual bounds of the layer, plus the transform via
> the
> > page scale layer, i.e. not calling ScrollableViewportSize(). In fact, I
think
> we
> > can avoid making that call at all.
> 
> If you try that, you're going to run into the problem of Android-specific
> adjustments for top controls.  It would be nice to bake these into the bounds,
> but it's a bit of a can of worms I suggest we do later.

Agreed ... I believe that's why it's there for now. I had been hoping we would
bake it into the bonds in a (soon) future patch.

[enne (OOO), 2014-01-21 19:00:05]

Please add a bug number, format your description to 72 characters, and update
the bit about what patch this depends on.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:692: 
nit: unnecessary whitespace.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:739: max_offset.Scale(1 / scale_factor);
1 => 1.f

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:740: return gfx::Vector2d(max_offset.x(), max_offset.y());
Is flooring correct here? Use a named function to convert between Vector2dF and
Vector2d.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.h#newco...
cc/layers/layer.h:570: // This variable indicates which ancestor layer (if any)
defines the maximum
Defines is is not quite right.  It's really the relationship between this
(scroll) layer and its clip layer, right?

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:557: layer->SetScrollClipLayer(scroll_clip_layer_ ?
scroll_clip_layer_->id()
git cl format

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1142: || IsContainerForFixedPositionLayers());
git cl format

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1328: DCHECK(layer);
DCHECK also that the scrollbars list doesn't contain this scrollbar?

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1336: DCHECK(scrollbars_);
DCHECK also that this scrollbar exists in the list?

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_unittest.cc:181: #define
UPDATE_AND_EXTRACT_LAYER_POINTERS \
Can you make this macro more function-like by adding parenthesis and making it
require a semicolon at the end by wrapping it in a do { } while(false)?

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_unittest.cc:187:
scrollbar_layer_impl->ScrollbarParametersDidChange(); \
Trailing \

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1113: /*
Please leave a TODO and a bug number here with a comment.  I would be super
dubious if I came upon this in the code without any context.

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:125: Layer* scroll_layer =
root->children()[0];
What about an accessor function for the scroll layer rather than magic index
into child list?

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:25: class LayerScrollOffsetDelegateProxy : public
LayerScrollOffsetDelegate {
I am not sure that I follow the need for this proxy, but can follow up about it
in later patches.

https://codereview.chromium.org/23983047/diff/1382001/ui/gfx/transform.h
File ui/gfx/transform.h (right):

https://codereview.chromium.org/23983047/diff/1382001/ui/gfx/transform.h#newc...
ui/gfx/transform.h:92: gfx::Vector2dF Scale() const {
Scale2d?

https://codereview.chromium.org/23983047/diff/1382001/ui/gfx/transform.h#newc...
ui/gfx/transform.h:143: bool IsScale() const {
Scale2d?

[wjmaclean, 2014-01-21 22:37:26]

PTAL?

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:692: 
On 2014/01/21 19:00:06, enne wrote:
> nit: unnecessary whitespace.

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:739: max_offset.Scale(1 / scale_factor);
On 2014/01/21 19:00:06, enne wrote:
> 1 => 1.f

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:740: return gfx::Vector2d(max_offset.x(), max_offset.y());
On 2014/01/21 19:00:06, enne wrote:
> Is flooring correct here? Use a named function to convert between Vector2dF
and
> Vector2d.

I think flooring makes sense (since we'd rather stop a bit short rather than go
over?), but I'm willing to round too if there's a good reason for it.

Named conversion function used.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer.h#newco...
cc/layers/layer.h:570: // This variable indicates which ancestor layer (if any)
defines the maximum
On 2014/01/21 19:00:06, enne wrote:
> Defines is is not quite right.  It's really the relationship between this
> (scroll) layer and its clip layer, right?

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1142: || IsContainerForFixedPositionLayers());
On 2014/01/21 19:00:06, enne wrote:
> git cl format

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1328: DCHECK(layer);
On 2014/01/21 19:00:06, enne wrote:
> DCHECK also that the scrollbars list doesn't contain this scrollbar?

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1336: DCHECK(scrollbars_);
On 2014/01/21 19:00:06, enne wrote:
> DCHECK also that this scrollbar exists in the list?

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_unittest.cc:181: #define
UPDATE_AND_EXTRACT_LAYER_POINTERS \
On 2014/01/21 19:00:06, enne wrote:
> Can you make this macro more function-like by adding parenthesis and making it
> require a semicolon at the end by wrapping it in a do { } while(false)?

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_unittest.cc:187:
scrollbar_layer_impl->ScrollbarParametersDidChange(); \
On 2014/01/21 19:00:06, enne wrote:
> Trailing \

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1113: /*
On 2014/01/21 19:00:06, enne wrote:
> Please leave a TODO and a bug number here with a comment.  I would be super
> dubious if I came upon this in the code without any context.

Done.

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:125: Layer* scroll_layer =
root->children()[0];
On 2014/01/21 19:00:06, enne wrote:
> What about an accessor function for the scroll layer rather than magic index
> into child list?

Done.

I made the Layer scroll_layer var a member ... hopefully this is better?

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/23983047/diff/1382001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:25: class LayerScrollOffsetDelegateProxy : public
LayerScrollOffsetDelegate {
On 2014/01/21 19:00:06, enne wrote:
> I am not sure that I follow the need for this proxy, but can follow up about
it
> in later patches.

OK.

https://codereview.chromium.org/23983047/diff/1382001/ui/gfx/transform.h
File ui/gfx/transform.h (right):

https://codereview.chromium.org/23983047/diff/1382001/ui/gfx/transform.h#newc...
ui/gfx/transform.h:92: gfx::Vector2dF Scale() const {
On 2014/01/21 19:00:06, enne wrote:
> Scale2d?

Done.

https://codereview.chromium.org/23983047/diff/1382001/ui/gfx/transform.h#newc...
ui/gfx/transform.h:143: bool IsScale() const {
On 2014/01/21 19:00:06, enne wrote:
> Scale2d?

Done.

[enne (OOO), 2014-01-21 22:41:32]

lgtm

[commit-bot: I haz the power, 2014-01-21 22:54:05]

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

[commit-bot: I haz the power, 2014-01-21 23:14:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-21 23:19:38]

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

[commit-bot: I haz the power, 2014-01-22 00:39:16]

Failed to trigger a try job on android_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-01-22 00:39:38]

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

[commit-bot: I haz the power, 2014-01-22 01:28:42]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-22 18:00:24]

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

[commit-bot: I haz the power, 2014-01-23 01:40:16]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-23 19:10:25]

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

[commit-bot: I haz the power, 2014-01-24 00:56:17]

Failed to trigger a try job on android_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-01-24 00:56:37]

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

[aelias_OOO_until_Jul13, 2014-01-24 01:12:28]

https://codereview.chromium.org/23983047/diff/2412002/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23983047/diff/2412002/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1145: gfx::Size scaled_scroll_bounds(bounds());
Why are you using the scroll layer's bounds instead of the first child?  Did you
fix the Blink bug?  This might explain why max scroll offset is negative?  This
would also cause pages to be sometimes nonscrollable and might get your patch
reverted.

[wjmaclean, 2014-01-24 01:23:16]

On 2014/01/24 01:12:28, aelias wrote:
> https://codereview.chromium.org/23983047/diff/2412002/cc/layers/layer_impl.cc
> File cc/layers/layer_impl.cc (right):
> 
>
https://codereview.chromium.org/23983047/diff/2412002/cc/layers/layer_impl.cc...
> cc/layers/layer_impl.cc:1145: gfx::Size scaled_scroll_bounds(bounds());
> Why are you using the scroll layer's bounds instead of the first child?  Did
you
> fix the Blink bug?  This might explain why max scroll offset is negative? 
This
> would also cause pages to be sometimes nonscrollable and might get your patch
> reverted.

We fixed the Blink bug (we think).

[aelias_OOO_until_Jul13, 2014-01-24 02:09:53]

OK great, lgtm.

[commit-bot: I haz the power, 2014-01-24 07:43:53]

Change committed as 246812

[Mike West, 2014-01-24 12:08:59]

A revert of this CL has been created in
https://codereview.chromium.org/146713002/ by mkwst@chromium.org.

The reason for reverting is: This patch broke several blink-side compositing/rtl
tests[1][2]. It's
unclear whether these tests are valuable, given some comments
in TestExpectations, but since I know nothing about compositing
I'll leave the decision about what to do with these tests up to
someone who does.

[1]:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
[2]:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7/builds/....

[commit-bot: I haz the power, 2014-01-29 02:52:46]

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

[wjmaclean, 2014-01-29 03:14:11]

On 2014/01/29 02:52:46, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/wjmaclean%40chromium.org/23983047/2902001

Have filed https://codereview.chromium.org/148243010/ regarding the tests that
caused the revert, attempting to re-land.

[commit-bot: I haz the power, 2014-01-29 04:29:42]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-29 12:36:09]

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

[commit-bot: I haz the power, 2014-01-29 13:32:37]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[wjmaclean, 2014-01-29 13:54:38]

Committed patchset #25 manually as r247684 (presubmit successful).

[Adam Rice, 2014-01-29 14:30:42]

A revert of this CL has been created in
https://codereview.chromium.org/135183016/ by ricea@chromium.org.

The reason for reverting is: Broke Linux ASAN Tests (3) bot..

[enne (OOO), 2014-01-29 21:25:28]

https://codereview.chromium.org/23983047/diff/2932001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23983047/diff/2932001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:718: layer_tree_host()->LayerById(scroll_clip_layer_id_);
Inside of MaxScrollOffset seems like an unfortunate place to add a recursive
tree walk.

Can you do any better?  How often does this function get called?

[wjmaclean, 2014-01-29 21:46:13]

You're right, the recursive walk *is* overkill ... removed.

[enne (OOO), 2014-01-29 21:55:56]

lgtm.  Thanks for the cleanup.

[commit-bot: I haz the power, 2014-01-29 22:04:49]

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

[commit-bot: I haz the power, 2014-01-30 00:08:41]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps,
check_deps2git, chromedriver_unittests, components_unittests,
content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests,
jingle_unittests, media_unittests, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests,
sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_perf_unittests, telemetry_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-30 03:18:50]

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

[commit-bot: I haz the power, 2014-01-30 13:55:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-30 23:13:55]

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

[commit-bot: I haz the power, 2014-01-31 00:49:52]

Change committed as 248092

[commit-bot: I haz the power, 2014-01-31 00:49:56]

CQ bit was unchecked on CL. Ignoring.