Made PinchViewport a ScrollableArea

Made PinchViewport a ScrollableArea.

Implemented ScrollableArea on PinchViewport and hooked it up
to all the layer scrolling mechanisms. This removes the need
for DistributeScrollOffsetToViewports on the layer tree.

Also did a little cleanup with member names and namespacing.

This is a 2-sided patch, the other CL:
https://codereview.chromium.org/210543002

BUG=349941
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170110

[bokan, 2014-03-13 16:11:04]

PTAL.

[aelias_OOO_until_Jul13, 2014-03-14 06:44:11]

Hmm, I'm not sure I like this.  There's some impedance mismatches between
ScrollableArea and what we want with the inner viewport: 1) inner viewport's
scroll offset is in float, 2) inner viewport can resize, 3) ScrollableArea has
lots of unnecessary junk for non-overlay scrollbars, rubberbanding etc.

Does having ScrollableArea as a base class unlock any code sharing
opportunities?  If none, then I say let's just not do this and keep the code
simpler for ourselves without having to think at every turn how to make things
fit within existing ScrollableArea concepts.  (Or, if you do have a vision for
where the design should go that requires us to derive from ScrollableArea, then
we should discuss how to restructure ScrollableArea itself so that it fits our
needs better.)

[bokan, 2014-03-14 15:02:42]

On 2014/03/14 06:44:11, aelias wrote:
> Hmm, I'm not sure I like this.  There's some impedance mismatches between
> ScrollableArea and what we want with the inner viewport: 1) inner viewport's
> scroll offset is in float, 2) inner viewport can resize, 3) ScrollableArea has
> lots of unnecessary junk for non-overlay scrollbars, rubberbanding etc.
> 
> Does having ScrollableArea as a base class unlock any code sharing
> opportunities?  If none, then I say let's just not do this and keep the code
> simpler for ourselves without having to think at every turn how to make things
> fit within existing ScrollableArea concepts.  (Or, if you do have a vision for
> where the design should go that requires us to derive from ScrollableArea,
then
> we should discuss how to restructure ScrollableArea itself so that it fits our
> needs better.)

Making it a ScrollableArea has a number of advantages. I have a follow up patch
that removes all the special-casing of the main frame scrolling; essentially,
using all the existing GraphicsLayer <-> cc::Layer synchronization. It will also
help in making the pinch viewport naturally allow slow-path scrolls and
bubbling.
I'm happy to restructure ScrollableArea where you think it makes sense.

As far as the points you brought up:
1), the inner_viewport_layer stores its offset in ints. I assume you mean it's
a float because of the scale. You said before the correct way to think about
this
is as the outer viewport increasing in size. In this way, we can just mirror the
layer tree implementation. Keep the scroll offset unchanged but calculate things
like MaxScrollOffset based on the content*scaleFactor.

2) FrameView could also resize, both via window resizes and as the pinch
viewport
previously. There's nothing in ScrollableArea that prevents this.

3) I agree there's lots of cruft in ScrollableArea that could be moved elsewhere
or split up into a more specialized descendant of ScrollableArea.

[aelias_OOO_until_Jul13, 2014-03-14 19:56:41]

OK, I can buy that.  Instead of reviewing/landing this first, could you bake
your followup patch into this one and I can review the larger whole?  This patch
is somewhat meaningless on its own since it looks like the implementation of the
methods is not really meant to work yet.  I would rather review a larger patch
intended to make things functional.

https://codereview.chromium.org/199253002/diff/1/Source/core/frame/PinchViewp...
File Source/core/frame/PinchViewport.h (right):

https://codereview.chromium.org/199253002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.h:75: // TODO: Ensure pinch works properly with
non-TB-RL writing directions. I.e.
I think this actually doesn't need any changes?  The outer viewport may start at
the top-right but the inner starts at the same size as the inner viewport so it
doesn't matter what corner of the outer viewport it starts from.  (Since
PinchViewport will only be used for the inner viewport as I understand it?)

https://codereview.chromium.org/199253002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.h:114: IntRect m_visibleRect;
I think this should be an IntSize because the base class already stores an
IntPoint scroll offset.

[bokan, 2014-03-14 20:26:01]

On 2014/03/14 19:56:41, aelias wrote:
> OK, I can buy that.  Instead of reviewing/landing this first, could you bake
> your followup patch into this one and I can review the larger whole?  This
patch
> is somewhat meaningless on its own since it looks like the implementation of
the
> methods is not really meant to work yet.  I would rather review a larger patch
> intended to make things functional.

Sure thing, I'd just like to stress test it a little more and make sure
everything's
working as intended. I'll put up a new CL for Monday.

[bokan, 2014-03-14 20:26:13]

https://codereview.chromium.org/199253002/diff/1/Source/core/frame/PinchViewp...
File Source/core/frame/PinchViewport.h (right):

https://codereview.chromium.org/199253002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.h:75: // TODO: Ensure pinch works properly with
non-TB-RL writing directions. I.e.
On 2014/03/14 19:56:41, aelias wrote:
> I think this actually doesn't need any changes?  The outer viewport may start
at
> the top-right but the inner starts at the same size as the inner viewport so
it
> doesn't matter what corner of the outer viewport it starts from.  (Since
> PinchViewport will only be used for the inner viewport as I understand it?)

Yup, you're right, I'll remove the TODO.

https://codereview.chromium.org/199253002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.h:114: IntRect m_visibleRect;
On 2014/03/14 19:56:41, aelias wrote:
> I think this should be an IntSize because the base class already stores an
> IntPoint scroll offset.

You're referring to m_scrollOrigin? That's not the offset, it's related to
writing direction. I think ScrollableArea leaves tracking the offset to
descendants (e.g. ScrollView has m_scrollOffset)

[bokan, 2014-03-24 17:19:33]

Alexandre, PTAL. This patch hooks up the GraphicsLayer<->ScrollableArea hooks
for the PinchViewport so that scroll offsets get get pushed between Blink and
the LayerTree. I have a patch that moves the scale factor into PinchViewport but
it's non trivial so I thought I'd keep them separate. Let me know if you'd
rather see them combined. Thanks.

[aelias_OOO_until_Jul13, 2014-03-24 21:47:23]

OK, this lgtm modulo minor comments below.  Adding leviw@ for Source/core and
Source/platform OWNERS.  (However, he is on vacation until Thursday so I
recommend uploading your page scale patch as a dependent patch so I can review
it before this lands.)

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:307: // TODO: This should be
ScrollableArea's default implementation.
Could you do that now?  I think it shouldn't touch other subclasses much.

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:1185: // TODO(bokan): With pinch
virtual viewport the special case will no
Could you make this conditional on the pinchVirtualViewport flag being false
instead?

[bokan, 2014-03-24 23:36:30]

Can I add the followup patch(es) so that Reitveld knows they're dependent on
this one or did you just want me to put up a CL that'll include this one as
well?

I've also put up the Chromium side patch at
https://codereview.chromium.org/210543002/

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:307: // TODO: This should be
ScrollableArea's default implementation.
On 2014/03/24 21:47:24, aelias wrote:
> Could you do that now?  I think it shouldn't touch other subclasses much.

Done. I removed the override on RenderLayerScrollableArea, though it used
pixelSnappedClientWidth/Height instead of visibleWidth/Height, the only
difference being that visibleWidth/Height includes scrollbars, is that ok or
should I leave the override? Should RLSA's visibleWidth/Height use the
pixelSnappedClientWidth instead (since ScrollView's visibleWidth excludes
scrollbars)?

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:1185: // TODO(bokan): With pinch
virtual viewport the special case will no
On 2014/03/24 21:47:24, aelias wrote:
> Could you make this conditional on the pinchVirtualViewport flag being false
> instead?
The isMainFrame argument is conditional on that flag when this method is
called in ScrollingCoordinator. Did you want me to move that condition
into here?

[aelias_OOO_until_Jul13, 2014-03-25 00:05:48]

OK, looks like Rietveld doesn't have proper support for dependent patchsets, so
feel free to upload the big patch for now.

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:307: // TODO: This should be
ScrollableArea's default implementation.
On 2014/03/24 23:36:30, bokan wrote:
> On 2014/03/24 21:47:24, aelias wrote:
> > Could you do that now?  I think it shouldn't touch other subclasses much.
> 
> Done. I removed the override on RenderLayerScrollableArea, though it used
> pixelSnappedClientWidth/Height instead of visibleWidth/Height, the only
> difference being that visibleWidth/Height includes scrollbars, is that ok or
> should I leave the override? Should RLSA's visibleWidth/Height use the
> pixelSnappedClientWidth instead (since ScrollView's visibleWidth excludes
> scrollbars)?

It seems more correct not to include scrollbars, that way this will advance
exactly one page.  So we can take this as an accidental bugfix :)

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:1185: // TODO(bokan): With pinch
virtual viewport the special case will no
On 2014/03/24 23:36:30, bokan wrote:
> On 2014/03/24 21:47:24, aelias wrote:
> > Could you make this conditional on the pinchVirtualViewport flag being false
> > instead?
> The isMainFrame argument is conditional on that flag when this method is
> called in ScrollingCoordinator. Did you want me to move that condition
> into here?

Yes, I think that would make more sense.  Since it's still the main frame, the
current implementation is lying to this method.

[aelias_OOO_until_Jul13, 2014-03-25 00:07:01]

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:307: // TODO: This should be
ScrollableArea's default implementation.
On 2014/03/25 00:05:48, aelias wrote:
> On 2014/03/24 23:36:30, bokan wrote:
> > On 2014/03/24 21:47:24, aelias wrote:
> > > Could you do that now?  I think it shouldn't touch other subclasses much.
> > 
> > Done. I removed the override on RenderLayerScrollableArea, though it used
> > pixelSnappedClientWidth/Height instead of visibleWidth/Height, the only
> > difference being that visibleWidth/Height includes scrollbars, is that ok or
> > should I leave the override? Should RLSA's visibleWidth/Height use the
> > pixelSnappedClientWidth instead (since ScrollView's visibleWidth excludes
> > scrollbars)?
> 
> It seems more correct not to include scrollbars, that way this will advance
> exactly one page.  So we can take this as an accidental bugfix :)

Err, sorry, I misunderstood the direction of the change.  It should be  changed
back to use pixelSnappedClientWidth then, or some other method that excludes
scrollbars.

[bokan, 2014-03-25 00:54:42]

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:307: // TODO: This should be
ScrollableArea's default implementation.
On 2014/03/25 00:07:02, aelias wrote:
> On 2014/03/25 00:05:48, aelias wrote:
> > On 2014/03/24 23:36:30, bokan wrote:
> > > On 2014/03/24 21:47:24, aelias wrote:
> > > > Could you do that now?  I think it shouldn't touch other subclasses
much.
> > > 
> > > Done. I removed the override on RenderLayerScrollableArea, though it used
> > > pixelSnappedClientWidth/Height instead of visibleWidth/Height, the only
> > > difference being that visibleWidth/Height includes scrollbars, is that ok
or
> > > should I leave the override? Should RLSA's visibleWidth/Height use the
> > > pixelSnappedClientWidth instead (since ScrollView's visibleWidth excludes
> > > scrollbars)?
> > 
> > It seems more correct not to include scrollbars, that way this will advance
> > exactly one page.  So we can take this as an accidental bugfix :)
> 
> Err, sorry, I misunderstood the direction of the change.  It should be 
changed
> back to use pixelSnappedClientWidth then, or some other method that excludes
> scrollbars.

Done.

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:1185: // TODO(bokan): With pinch
virtual viewport the special case will no
On 2014/03/25 00:05:48, aelias wrote:
> On 2014/03/24 23:36:30, bokan wrote:
> > On 2014/03/24 21:47:24, aelias wrote:
> > > Could you make this conditional on the pinchVirtualViewport flag being
false
> > > instead?
> > The isMainFrame argument is conditional on that flag when this method is
> > called in ScrollingCoordinator. Did you want me to move that condition
> > into here?
> 
> Yes, I think that would make more sense.  Since it's still the main frame, the
> current implementation is lying to this method.

It doesn't look like there's any hooks in GraphicsLayer into anything that has
Settings/WebSettings so I don't think I can do this without introducing some
layering violations. I could make this method take the setting's value in
another argument but that feels awkward -- I could change isMainFrame to
scrollsCallGraphicsLayerDidScroll or similar. What's your preference?

[aelias_OOO_until_Jul13, 2014-03-25 01:08:59]

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/199253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:1185: // TODO(bokan): With pinch
virtual viewport the special case will no
On 2014/03/25 00:54:42, bokan wrote:
> On 2014/03/25 00:05:48, aelias wrote:
> > On 2014/03/24 23:36:30, bokan wrote:
> > > On 2014/03/24 21:47:24, aelias wrote:
> > > > Could you make this conditional on the pinchVirtualViewport flag being
> false
> > > > instead?
> > > The isMainFrame argument is conditional on that flag when this method is
> > > called in ScrollingCoordinator. Did you want me to move that condition
> > > into here?
> > 
> > Yes, I think that would make more sense.  Since it's still the main frame,
the
> > current implementation is lying to this method.
> 
> It doesn't look like there's any hooks in GraphicsLayer into anything that has
> Settings/WebSettings so I don't think I can do this without introducing some
> layering violations. I could make this method take the setting's value in
> another argument but that feels awkward -- I could change isMainFrame to
> scrollsCallGraphicsLayerDidScroll or similar. What's your preference?

OK, whatever, the TODO is fine then.  No big deal as long as we delete it later.

[leviw_travelin_and_unemployed, 2014-03-26 16:44:38]

It's odd to me that pinch viewport is becoming a ScrollableArea, but I
appreciate the cleanup you can get that way.

LGTM.

https://codereview.chromium.org/199253002/diff/60001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/199253002/diff/60001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:117: // TODO(bokan): Meaningless name,
rename to isActiveFocus
Generally we use "FIXME" in Blink instead of TODO.

[bokan, 2014-03-26 19:56:44]

IMO it makes sense conceptually too, if you think about ScrollableAreas as
objects that host content larger than their bounds and the content can be moved
around. In that sense, pinch viewport fits quite nicely. Granted, ScrollableArea
has some concepts that don't apply, but I think those could/should be split out
at some point.

https://codereview.chromium.org/199253002/diff/60001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/199253002/diff/60001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.h:117: // TODO(bokan): Meaningless name,
rename to isActiveFocus
On 2014/03/26 16:44:38, Levi wrote:
> Generally we use "FIXME" in Blink instead of TODO.

Fixed.

[bokan, 2014-03-26 22:10:56]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-03-26 22:10:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/199253002/100001

[commit-bot: I haz the power, 2014-03-26 22:46:10]

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

[commit-bot: I haz the power, 2014-03-26 22:46:10]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[bokan, 2014-03-26 23:26:24]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-03-26 23:26:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/199253002/100001

[commit-bot: I haz the power, 2014-03-26 23:57:38]

Change committed as 170110