[Pinch-to-zoom] Moved scale factor into PinchViewport

[Pinch-to-zoom] Moved scale factor into PinchViewport

Page scale now owned and synchronized in PinchViewport class
when using --enable-pinch-virtual-viewport.

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

[bokan, 2014-04-08 14:46:04]

PTAL, the virtual viewport path no longer sets FrameView's scale factor.

[aelias_OOO_until_Jul13, 2014-04-09 08:41:21]

This all looks sensible.  Adding leviw@ for Source/core and Source/platform
OWNERS.

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebInputEvent...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebInputEvent...
Source/web/WebInputEventConversion.cpp:79: static IntPoint
pinchViewportOffset(const Widget* widget)
Could you add a a test case for this to WebInputEventConversionTest.cpp ?

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebInputEvent...
Source/web/WebInputEventConversion.cpp:83: FrameView* rootView = 
toFrameView(widget->root());
nit: extra space after =

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2689: IntSize remaining = IntSize(origin.x, origin.y)
- view->scrollOffset();
Hmm, this is doing distribution which we would like to avoid.  This method
doesn't have a lot of callers -- probably the only important one is for
historyitem restore.  We know that (and only that) needs distribution, so let's
more this logic there.

I think setPageScaleFactor with a scroll offset is fundamentally ambiguous in
the new world, so we should probably plan to delete this method.

[bokan, 2014-04-09 15:30:59]

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebInputEvent...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebInputEvent...
Source/web/WebInputEventConversion.cpp:83: FrameView* rootView = 
toFrameView(widget->root());
On 2014/04/09 08:41:22, aelias wrote:
> nit: extra space after =

Done.

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2689: IntSize remaining = IntSize(origin.x, origin.y)
- view->scrollOffset();
On 2014/04/09 08:41:22, aelias wrote:
> Hmm, this is doing distribution which we would like to avoid.  This method
> doesn't have a lot of callers -- probably the only important one is for
> historyitem restore.  We know that (and only that) needs distribution, so
let's
> more this logic there.
> 
> I think setPageScaleFactor with a scroll offset is fundamentally ambiguous in
> the new world, so we should probably plan to delete this method.

There's actually a few other uses (Fullscreen elements, page scale animations,
and DevTools pinch emulation) and HistoryItem uses Page::setPageScaleFactor. I
changed the call sites to set scale and offset in separate calls and will remove
this method. This will keep old-style pinch working. I'll address all the above
use cases in virtual viewport pinch later.

[aelias_OOO_until_Jul13, 2014-04-09 18:35:31]

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2689: IntSize remaining = IntSize(origin.x, origin.y)
- view->scrollOffset();
On 2014/04/09 15:30:59, bokan wrote:
> On 2014/04/09 08:41:22, aelias wrote:
> > Hmm, this is doing distribution which we would like to avoid.  This method
> > doesn't have a lot of callers -- probably the only important one is for
> > historyitem restore.  We know that (and only that) needs distribution, so
> let's
> > more this logic there.
> > 
> > I think setPageScaleFactor with a scroll offset is fundamentally ambiguous
in
> > the new world, so we should probably plan to delete this method.
> 
> There's actually a few other uses (Fullscreen elements, page scale animations,
> and DevTools pinch emulation) and HistoryItem uses Page::setPageScaleFactor. I
> changed the call sites to set scale and offset in separate calls and will
remove
> this method. This will keep old-style pinch working. I'll address all the
above
> use cases in virtual viewport pinch later.

Well, keep in mind that clamping can cause issue when updating scroll and scale
separately.  It's *probably* fine if you set the scroll position afterwards,
although it's still weird to update scroll position twice and I've sometimes
been surprised by bugs that can happen from this kind of thing.  So I'd suggest
not changing the behavior of the old pinch path for now.

[bokan, 2014-04-09 19:57:18]

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebInputEvent...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebInputEvent...
Source/web/WebInputEventConversion.cpp:79: static IntPoint
pinchViewportOffset(const Widget* widget)
On 2014/04/09 08:41:22, aelias wrote:
> Could you add a a test case for this to WebInputEventConversionTest.cpp ?

Done.

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2689: IntSize remaining = IntSize(origin.x, origin.y)
- view->scrollOffset();
On 2014/04/09 18:35:32, aelias wrote:
> On 2014/04/09 15:30:59, bokan wrote:
> > On 2014/04/09 08:41:22, aelias wrote:
> > > Hmm, this is doing distribution which we would like to avoid.  This method
> > > doesn't have a lot of callers -- probably the only important one is for
> > > historyitem restore.  We know that (and only that) needs distribution, so
> > let's
> > > more this logic there.
> > > 
> > > I think setPageScaleFactor with a scroll offset is fundamentally ambiguous
> in
> > > the new world, so we should probably plan to delete this method.
> > 
> > There's actually a few other uses (Fullscreen elements, page scale
animations,
> > and DevTools pinch emulation) and HistoryItem uses Page::setPageScaleFactor.
I
> > changed the call sites to set scale and offset in separate calls and will
> remove
> > this method. This will keep old-style pinch working. I'll address all the
> above
> > use cases in virtual viewport pinch later.
> 
> Well, keep in mind that clamping can cause issue when updating scroll and
scale
> separately.  It's *probably* fine if you set the scroll position afterwards,
> although it's still weird to update scroll position twice and I've sometimes
> been surprised by bugs that can happen from this kind of thing.  So I'd
suggest
> not changing the behavior of the old pinch path for now.

Ok, I replaced all uses in unit tests since they all still pass (most cases were
just WebPoint(0,0)) and the simple cases in FullscreenController and
InspectorController. I've left the WebViewImpl-internal uses that are
higher-risk. This let me remove it from the WebView interface.

I replaced the other "pageScale" methods with "pinchViewportScale" named
methods. I think the naming is more consistent and less confusing (I've talked
to numerous people who confused page-scale with browser-zoom).

[bokan, 2014-04-09 20:21:52]

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/225303014/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2689: IntSize remaining = IntSize(origin.x, origin.y)
- view->scrollOffset();
On 2014/04/09 19:57:19, bokan wrote:
> On 2014/04/09 18:35:32, aelias wrote:
> > On 2014/04/09 15:30:59, bokan wrote:
> > > On 2014/04/09 08:41:22, aelias wrote:
> > > > Hmm, this is doing distribution which we would like to avoid.  This
method
> > > > doesn't have a lot of callers -- probably the only important one is for
> > > > historyitem restore.  We know that (and only that) needs distribution,
so
> > > let's
> > > > more this logic there.
> > > > 
> > > > I think setPageScaleFactor with a scroll offset is fundamentally
ambiguous
> > in
> > > > the new world, so we should probably plan to delete this method.
> > > 
> > > There's actually a few other uses (Fullscreen elements, page scale
> animations,
> > > and DevTools pinch emulation) and HistoryItem uses
Page::setPageScaleFactor.
> I
> > > changed the call sites to set scale and offset in separate calls and will
> > remove
> > > this method. This will keep old-style pinch working. I'll address all the
> > above
> > > use cases in virtual viewport pinch later.
> > 
> > Well, keep in mind that clamping can cause issue when updating scroll and
> scale
> > separately.  It's *probably* fine if you set the scroll position afterwards,
> > although it's still weird to update scroll position twice and I've sometimes
> > been surprised by bugs that can happen from this kind of thing.  So I'd
> suggest
> > not changing the behavior of the old pinch path for now.
> 
> Ok, I replaced all uses in unit tests since they all still pass (most cases
were
> just WebPoint(0,0)) and the simple cases in FullscreenController and
> InspectorController. I've left the WebViewImpl-internal uses that are
> higher-risk. This let me remove it from the WebView interface.
> 
> I replaced the other "pageScale" methods with "pinchViewportScale" named
> methods. I think the naming is more consistent and less confusing (I've talked
> to numerous people who confused page-scale with browser-zoom).

Erm, I had to leave setPageScale in WebView since it'll need a 3-sided patch due
to uses outside blink (webkit_test_runner).

[aelias_OOO_until_Jul13, 2014-04-09 21:22:51]

lgtm for Source/web.  Needs review from leviw@ for Source/core and
Source/platform, and abarth@ for public/.

https://codereview.chromium.org/225303014/diff/120001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/225303014/diff/120001/public/web/WebView.h#ne...
public/web/WebView.h:268: virtual void setPageScaleFactorLimits(float
minPageScale, float maxPageScale) = 0;
Here are three more "PageScaleFactor" methods if you want to rename things.

[aelias_OOO_until_Jul13, 2014-04-10 16:23:48]

https://codereview.chromium.org/225303014/diff/120001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/225303014/diff/120001/public/web/WebView.h#ne...
public/web/WebView.h:251: virtual void setPinchViewportScaleFactor(float) = 0;
On second thought, I would suggest that you not change the name of
"PageScaleFactor" here.  The most important thing is to keep the name of the
value consistent across the codebase which means you're going down the path of
renaming a lot more stuff like the entire PageScaleAnimation and
PageScaleConstraintsSet classes.  But those things don't even have to do with
pinch so it's not clear that's an improvement.

Ultimately I think readers of this code do need a basic understanding of the
difference between page scale and full-page zoom.  If they don't, they'll still
be confused regardless of what we name things.  And once they do, the code is
unambiguous because we consistently use "scale" for the first one and "zoom" for
the other.  So I don't think this is urgent or obviously an improvement given
the other concern about not everything being pinch.

Moreover I think you should focus on shipping the behavior change and spend your
time on a big mechanical rename right now.  This would be easier when Blink is
merged with Chromium, anyway.

[bokan, 2014-04-10 22:24:07]

On 2014/04/10 16:23:48, aelias wrote:
> https://codereview.chromium.org/225303014/diff/120001/public/web/WebView.h
> File public/web/WebView.h (right):
> 
>
https://codereview.chromium.org/225303014/diff/120001/public/web/WebView.h#ne...
> public/web/WebView.h:251: virtual void setPinchViewportScaleFactor(float) = 0;
> On second thought, I would suggest that you not change the name of
> "PageScaleFactor" here.  The most important thing is to keep the name of the
> value consistent across the codebase which means you're going down the path of
> renaming a lot more stuff like the entire PageScaleAnimation and
> PageScaleConstraintsSet classes.  But those things don't even have to do with
> pinch so it's not clear that's an improvement.
> 
> Ultimately I think readers of this code do need a basic understanding of the
> difference between page scale and full-page zoom.  If they don't, they'll
still
> be confused regardless of what we name things.  And once they do, the code is
> unambiguous because we consistently use "scale" for the first one and "zoom"
for
> the other.  So I don't think this is urgent or obviously an improvement given
> the other concern about not everything being pinch.
> 
> Moreover I think you should focus on shipping the behavior change and spend
your
> time on a big mechanical rename right now.  This would be easier when Blink is
> merged with Chromium, anyway.

Yah, I think you're right. I've changed back to pageScale___ for names.

[leviw_travelin_and_unemployed, 2014-04-14 21:37:21]

lgtm.

[abarth-chromium, 2014-04-14 21:57:01]

public/ LGTM

This stuff is super complicated, and I'm largely relying on aelias here to keep
us sane.

[bokan, 2014-04-15 11:16:47]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-04-15 11:17:07]

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

[commit-bot: I haz the power, 2014-04-15 11:49:15]

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

[commit-bot: I haz the power, 2014-04-15 11:49:16]

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

[bokan, 2014-04-15 11:55:25]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-04-15 11:55:39]

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

[commit-bot: I haz the power, 2014-04-15 13:11:07]

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

[commit-bot: I haz the power, 2014-04-15 13:11:07]

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

[bokan, 2014-04-15 15:14:30]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-04-15 15:14:45]

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

[commit-bot: I haz the power, 2014-04-15 16:04:28]

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

[commit-bot: I haz the power, 2014-04-15 16:04:29]

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

[bokan, 2014-04-15 16:23:58]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-04-15 16:24:21]

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

[commit-bot: I haz the power, 2014-04-15 17:06:39]

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

[commit-bot: I haz the power, 2014-04-15 17:06:40]

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

[bokan, 2014-04-15 17:33:38]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-04-15 17:33:57]

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

[commit-bot: I haz the power, 2014-04-15 18:07:29]

Change committed as 171596