Delete zoomed_viewport_offset_ and its users.

Delete zoomed_viewport_offset_ and its users.

After http://webk.it/107424 lands, there is no longer any need to conceal
the true scroll offset from WebKit. We can use the normal root
layer scroll offset for almost everything and avoid needing to apply this
extra transformation.

NOTRY=true
BUG=171183
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179467

[aelias_OOO_until_Jul13, 2013-01-22 08:57:05]

Hi, PTAL.  This patch deletes the inner-viewport offset stuff, simplifying lots
of things.  wjmaclean@, I think your scrollbar work will be easier to implement
with this model, and the navigation and slow-scroll problems will vanish. 
What's broken by this change as it stands is the position: fixed behavior; let's
chat tomorrow about the right way to bring that back.

[wjmaclean, 2013-01-22 16:56:38]

LGTM, with some minor comments. Most importantly, if we're handling scroll
offset by exposing it to WebKit (which is good) then we don't need to keep a
full scale matrix for the implTransform, and should get rid of it.

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h
File cc/pinch_zoom_viewport.h (right):

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h#newc...
cc/pinch_zoom_viewport.h:59: gfx::Transform ImplTransform(bool
page_scale_pinch_zoom_enabled) const;
Do we really need to keep an entire matrix around for this if it's just a scale?
I would prefer not to ...

https://codereview.chromium.org/12045002/diff/1/content/browser/android/conte...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/12045002/diff/1/content/browser/android/conte...
content/browser/android/content_startup_flags.cc:61: //  if
(!parsed_command_line->HasSwitch(cc::switches::kEnableImplSidePainting))
Delete commented lines.

[danakj, 2013-01-22 23:42:14]

https://codereview.chromium.org/12045002/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/12045002/diff/1/cc/layer_tree_host_impl.cc#ol...
cc/layer_tree_host_impl.cc:301: gfx::PointF deviceViewportPoint =
gfx::ScalePoint(viewportPoint, m_deviceScaleFactor);
This looks like it will break hidpi. The findLayerThatIsHitByPoint() function is
expecting a point in screen space.

I thought we have a test for this.

https://codereview.chromium.org/12045002/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12045002/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1233: gfx::PointF actualLocalEndPoint =
localStartPoint + layerImpl.scrollDelta() - previousDelta;
We definitely have some tests for this code, does this not break them? Some of
them probably need to be removed, others updated.

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.cc
File cc/pinch_zoom_viewport.cc (right):

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.cc#new...
cc/pinch_zoom_viewport.cc:53: gfx::SizeF
PinchZoomViewport::LayoutSpaceViewportSize() const {
This is just some math. Can you inline in the header please?

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h
File cc/pinch_zoom_viewport.h (right):

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h#newc...
cc/pinch_zoom_viewport.h:59: gfx::Transform ImplTransform(bool
page_scale_pinch_zoom_enabled) const;
On 2013/01/22 16:56:39, wjmaclean wrote:
> Do we really need to keep an entire matrix around for this if it's just a
scale?
> I would prefer not to ...

+1. Can we just use total_page_scale_factor?

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h#newc...
cc/pinch_zoom_viewport.h:61: gfx::SizeF LayoutSpaceViewportSize() const;
This is basically the same name as layout_viewport_size but it's a different
thing.

How about ZoomedViewportSizeInLayoutSpace()?

[wjmaclean, 2013-01-23 16:27:02]

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h
File cc/pinch_zoom_viewport.h (right):

https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h#newc...
cc/pinch_zoom_viewport.h:59: gfx::Transform ImplTransform(bool
page_scale_pinch_zoom_enabled) const;
On 2013/01/22 16:56:39, wjmaclean wrote:
> Do we really need to keep an entire matrix around for this if it's just a
scale?
> I would prefer not to ...

Sorry to do this to you, but I now think this should (temporarily) remain a
matrix, at least until we convert main-frame scrollbars to overlay (or get rid
of them altogether). My reasoning is that we'll still need to be able to
translate the existing scrollbars when we zoom in, as they won't be
automatically moved according to the position of the root scroll layer. That
means we'll need LayerImpl to still deal with implTransform as a matrix.

When we revamp desktop main-frame scrollbars, we can convert this to a scalar.

[danakj, 2013-01-23 16:50:06]

On Wed, Jan 23, 2013 at 11:27 AM, <wjmaclean@chromium.org> wrote:

>
> https://codereview.chromium.**org/12045002/diff/1/cc/pinch_**
>
zoom_viewport.h<https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h>
> File cc/pinch_zoom_viewport.h (right):
>
> https://codereview.chromium.**org/12045002/diff/1/cc/pinch_**
>
zoom_viewport.h#newcode59<https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h#newcode59>
> cc/pinch_zoom_viewport.h:59: gfx::Transform ImplTransform(bool
> page_scale_pinch_zoom_enabled) const;
> On 2013/01/22 16:56:39, wjmaclean wrote:
>
>> Do we really need to keep an entire matrix around for this if it's
>>
> just a scale?
>
>> I would prefer not to ...
>>
>
> Sorry to do this to you, but I now think this should (temporarily)
> remain a matrix, at least until we convert main-frame scrollbars to
> overlay (or get rid of them altogether). My reasoning is that we'll
> still need to be able to translate the existing scrollbars when we zoom
> in, as they won't be automatically moved according to the position of
> the root scroll layer. That means we'll need LayerImpl to still deal
> with implTransform as a matrix.
>
> When we revamp desktop main-frame scrollbars, we can convert this to a
> scalar.
>

What about adding a Vector2d offset to this class for the scrollbars to
make use of? The implTransform has always been somewhat opaque I think.

[wjmaclean, 2013-01-23 16:57:40]

On 2013/01/23 16:50:06, danakj wrote:
> On Wed, Jan 23, 2013 at 11:27 AM, <mailto:wjmaclean@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/12045002/diff/1/cc/pinch_**
> >
>
zoom_viewport.h<https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h>
> > File cc/pinch_zoom_viewport.h (right):
> >
> > https://codereview.chromium.**org/12045002/diff/1/cc/pinch_**
> >
>
zoom_viewport.h#newcode59<https://codereview.chromium.org/12045002/diff/1/cc/pinch_zoom_viewport.h#newcode59>
> > cc/pinch_zoom_viewport.h:59: gfx::Transform ImplTransform(bool
> > page_scale_pinch_zoom_enabled) const;
> > On 2013/01/22 16:56:39, wjmaclean wrote:
> >
> >> Do we really need to keep an entire matrix around for this if it's
> >>
> > just a scale?
> >
> >> I would prefer not to ...
> >>
> >
> > Sorry to do this to you, but I now think this should (temporarily)
> > remain a matrix, at least until we convert main-frame scrollbars to
> > overlay (or get rid of them altogether). My reasoning is that we'll
> > still need to be able to translate the existing scrollbars when we zoom
> > in, as they won't be automatically moved according to the position of
> > the root scroll layer. That means we'll need LayerImpl to still deal
> > with implTransform as a matrix.
> >
> > When we revamp desktop main-frame scrollbars, we can convert this to a
> > scalar.
> >
> 
> What about adding a Vector2d offset to this class for the scrollbars to
> make use of? The implTransform has always been somewhat opaque I think.

That would work too ... I was thinking mostly in terms of minimising the
interface churn between now and when we're ready to revamp the scrollbars.

[aelias_OOO_until_Jul13, 2013-01-24 00:47:02]

OK, I deleted the unit tests affected by this patch as they all tested logic
that I deleted.  I applied the nit comments.

For implTransform, I don't want to change it now given that we are doing M25
mass cherry-picks and renames are particularly painful with that kind of thing. 
But I definitely agree it should be removed in the future, I'll do that in a
cleanup patch after M26 branch point.

https://codereview.chromium.org/12045002/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/12045002/diff/1/cc/layer_tree_host_impl.cc#ol...
cc/layer_tree_host_impl.cc:301: gfx::PointF deviceViewportPoint =
gfx::ScalePoint(viewportPoint, m_deviceScaleFactor);
On 2013/01/22 23:42:15, danakj wrote:
> This looks like it will break hidpi. The findLayerThatIsHitByPoint() function
is
> expecting a point in screen space.
> 
> I thought we have a test for this.

Removed this change, it was speculative.

[aelias_OOO_until_Jul13, 2013-01-24 00:55:27]

Filed bug to track implTransform removal:
https://code.google.com/p/chromium/issues/detail?id=171851

[danakj, 2013-01-24 00:58:22]

On Wed, Jan 23, 2013 at 7:47 PM, <aelias@chromium.org> wrote:

> OK, I deleted the unit tests affected by this patch as they all tested
> logic
> that I deleted.  I applied the nit comments.
>
> For implTransform, I don't want to change it now given that we are doing
> M25
> mass cherry-picks and renames are particularly painful with that kind of
> thing.
> But I definitely agree it should be removed in the future, I'll do that in
> a
> cleanup patch after M26 branch point.


OK please add appropriate TODOs


>
>
>
> https://codereview.chromium.**org/12045002/diff/1/cc/layer_**
>
tree_host_impl.cc<https://codereview.chromium.org/12045002/diff/1/cc/layer_tree_host_impl.cc>
> File cc/layer_tree_host_impl.cc (left):
>
> https://codereview.chromium.**org/12045002/diff/1/cc/layer_**
>
tree_host_impl.cc#oldcode301<https://codereview.chromium.org/12045002/diff/1/cc/layer_tree_host_impl.cc#oldcode301>
> cc/layer_tree_host_impl.cc:**301: gfx::PointF deviceViewportPoint =
> gfx::ScalePoint(viewportPoint, m_deviceScaleFactor);
> On 2013/01/22 23:42:15, danakj wrote:
>
>> This looks like it will break hidpi. The findLayerThatIsHitByPoint()
>>
> function is
>
>> expecting a point in screen space.
>>
>
>  I thought we have a test for this.
>>
>
> Removed this change, it was speculative.
>
>
https://codereview.chromium.**org/12045002/<https://codereview.chromium.org/1...
>

[danakj, 2013-01-24 16:00:38]

Some compile errors:

./../webkit/compositor_bindings/web_layer_unittest.cc: In member function
'virtual void<unnamed>::WebLayerTest::SetUp()':
../../webkit/compositor_bindings/web_layer_unittest.cc:50:error: cannot allocate
an object of abstract type 'WebKit::WebLayerTreeViewImpl'
../../webkit/compositor_bindings/web_layer_tree_view_impl.h:24: note:   because
the following virtual functions are pure within 'WebKit::WebLayerTreeViewImpl':
../../third_party/WebKit/Source/Platform/chromium/public/WebLayerTreeView.h:107:
note: 	virtual WebKit::WebFloatPoint
WebKit::WebLayerTreeView::adjustEventPointForPinchZoom(const
WebKit::WebFloatPoint&) const

You're going to need to make that function non-pure virtual in webkit first.

[danakj, 2013-01-24 16:02:17]

Oh, nvm I see it in https://bugs.webkit.org/show_bug.cgi?id=107424

But you're going to have to remove the OVERRIDE from the chromium side in
WebLayerTreeViewIMpl before the webkit side lands.

[danakj, 2013-01-24 16:11:09]

LGTM but I think you should adjust instead of deleting one of the tests thunks.

I'm kinda sad that we'll lose all this test coverage, but moving this out of cc
is defintely awesome. Can we have layout tests for this behaviour covered by the
unit tests being deleted now, instead?

Bug filed here: https://code.google.com/p/chromium/issues/detail?id=171952

https://codereview.chromium.org/12045002/diff/5003/cc/layer_tree_host_impl_un...
File cc/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/12045002/diff/5003/cc/layer_tree_host_impl_un...
cc/layer_tree_host_impl_unittest.cc:591:
EXPECT_EQ(m_hostImpl->rootLayer()->maxScrollOffset(), gfx::Vector2d(50, 50));
Can you keep this test and compare to 100, 100 here instead?

[trchen, 2013-01-24 23:05:31]

https://codereview.chromium.org/12045002/diff/5003/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/12045002/diff/5003/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1259: viewportAppliedPan = unscrolled -
viewport->ApplyScroll(unscrolled);
This eliminated the use of viewport. Do we still need the PinchZoomViewport
class?

[aelias_OOO_until_Jul13, 2013-01-28 10:00:42]

I'm going to remove PinchZoomViewport in the followup patch
https://codereview.chromium.org/12093015/.

Brought back the implPinch test hunk with modification, and also fixed the
viewport used for MaxScrollOffset to use the clip layer (which is more correct
in the presence of non-overlay scrollbars).

[danakj, 2013-01-28 17:58:46]

https://codereview.chromium.org/12045002/diff/19001/cc/layer_tree_host_impl_u...
File cc/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/12045002/diff/19001/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:591:
EXPECT_EQ(m_hostImpl->rootLayer()->maxScrollOffset(), gfx::Vector2d(50, 50));
Is this the test hunk you mention bringing back with adjustments? Did it miss
out on the upload?

[aelias_OOO_until_Jul13, 2013-01-28 18:27:16]

Sorry, it ended up in the next patch in the sequence because there were
conflicts.  ( https://codereview.chromium.org/12093015/ )  Anyway, these will
all land in short succession.

[danakj, 2013-01-28 19:25:27]

I see, thanks. LGTM

On Mon, Jan 28, 2013 at 1:27 PM, <aelias@chromium.org> wrote:

> Sorry, it ended up in the next patch in the sequence because there were
> conflicts.  (
https://codereview.chromium.**org/12093015/<https://codereview.chromium.org/1...)
 Anyway, these will
> all land in short succession.
>
>
https://codereview.chromium.**org/12045002/<https://codereview.chromium.org/1...
>
> --
>
>
>

[aelias_OOO_until_Jul13, 2013-01-29 01:30:56]

Need LGTM from enne@ on webkit/compositor_bindings/ and sievers@ on
content/browser/android/content_startup_flags.cc (flipping flag to new scaling
mode by default).

[no sievers, 2013-01-29 01:33:08]

On 2013/01/29 01:30:56, aelias wrote:
> Need LGTM from enne@ on webkit/compositor_bindings/ and sievers@ on
> content/browser/android/content_startup_flags.cc (flipping flag to new scaling

lgtm

[enne (OOO), 2013-01-29 01:34:47]

lgtm

[aelias_OOO_until_Jul13, 2013-01-29 22:58:41]

Adding Yaron for OWNERS for the 3 disabled tests.  Bug for them filed at
http://crbug.com/172967

[Yaron, 2013-01-29 23:08:39]

On 2013/01/29 22:58:41, aelias wrote:
> Adding Yaron for OWNERS for the 3 disabled tests.  Bug for them filed at
> http://crbug.com/172967

lgtm

[Ted C, 2013-01-29 23:09:26]

lgtm

[aelias_OOO_until_Jul13, 2013-01-29 23:14:57]

Past try failures addressed, landing with NOTRY.

[commit-bot: I haz the power, 2013-01-29 23:30:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/12045002/18029

[commit-bot: I haz the power, 2013-01-29 23:47:33]

Change committed as 179467