Added pinch viewport offset to history item.

Added pinch viewport offset to history item.

Added field to HistoryItem to save pinch viewport's offset within the main
frame and patched up the history system to be aware of the pinch viewport.

Also, removed "savedScrollAndScale" dead code from WebView

Follow-up Chromium patch: https://codereview.chromium.org/266673014

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

[bokan, 2014-05-02 01:27:13]

PTAL at this and associated patch.

[aelias_OOO_until_Jul13, 2014-05-07 08:06:03]

lgtm modulo comment below

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

https://codereview.chromium.org/263853008/diff/20001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:148: // TODO: We should probably be calling
scaleDidChange type functions here.
Separate from this patch, but the only thing in you might need to call is
m_pageScaleConstraintsSet.setNeedsReset(false);  (which might come up in Android
scale initialization cases later).

https://codereview.chromium.org/263853008/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/263853008/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1007:
m_frame->page()->setPageScaleFactor(m_currentItem->pageScaleFactor(),
frameScrollOffset);
In the legacy historyitem case, I think you also need to subtract the computed
pinchViewportOffset from the scrollPoint.  Your unit test fails to catch this
bug because the outer viewport is at the bottom right of the document in that
test case.

[aelias_OOO_until_Jul13, 2014-05-07 08:41:34]

https://codereview.chromium.org/263853008/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/263853008/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1007:
m_frame->page()->setPageScaleFactor(m_currentItem->pageScaleFactor(),
frameScrollOffset);
On 2014/05/07 08:06:03, aelias wrote:
> In the legacy historyitem case, I think you also need to subtract the computed
> pinchViewportOffset from the scrollPoint.  Your unit test fails to catch this
> bug because the outer viewport is at the bottom right of the document in that
> test case.

Ignore that comment, since I realized it has to always be zero in the
non-bottom-right case anyway.  lgtm as is.

[bokan, 2014-05-07 12:49:04]

abarth@, could you please review Source/core and public/web? Thanks.

[eseidel, 2014-05-08 18:58:02]

I don't feel like I particularly well understand this change, but the code looks
safe/reasonable.

https://codereview.chromium.org/263853008/diff/20001/Source/core/loader/Histo...
File Source/core/loader/HistoryItem.cpp (left):

https://codereview.chromium.org/263853008/diff/20001/Source/core/loader/Histo...
Source/core/loader/HistoryItem.cpp:119: m_scrollPoint.setX(0);
m_scrollPoint = IntPoint();
m_pinchViewportScrollPoint = IntPoint();

[bokan, 2014-05-08 19:27:46]

https://codereview.chromium.org/263853008/diff/20001/Source/core/loader/Histo...
File Source/core/loader/HistoryItem.cpp (left):

https://codereview.chromium.org/263853008/diff/20001/Source/core/loader/Histo...
Source/core/loader/HistoryItem.cpp:119: m_scrollPoint.setX(0);
On 2014/05/08 18:58:02, eseidel wrote:
> m_scrollPoint = IntPoint();
> m_pinchViewportScrollPoint = IntPoint();

Done. (Though the latter is a FloatPoint)

[abarth-chromium, 2014-05-09 14:11:23]

Looks pretty good.  Some minor comments below.

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

https://codereview.chromium.org/263853008/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:69: }
Maybe call reset() in the constructor to keep the two in sync?

https://codereview.chromium.org/263853008/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:96: setLocation(FloatPoint(0, 0));
setLocation(FloatPoint());

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:190: if
(m_frame->document()->settings()->pinchVirtualViewportEnabled())
m_frame->settings()

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:193:
m_currentItem->setPinchViewportScrollPoint(FloatPoint(-1, -1));
Can we use something other than (-1, -1) here?  That could be a valid value for
the scroll point.  If the value isn't relevant, please use FloatPoint().  If you
need to signal "no value", please do that explicitly instead of using in-band
signaling.

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1003: float mainFrameScale =
m_frame->document()->settings()->pinchVirtualViewportEnabled() ? 1 :
m_currentItem->pageScaleFactor();
m_frame->settings()

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1019: if (pinchViewportOffset.x() == -1 &&
pinchViewportOffset.y() == -1)
Again, please don't use in-band signaling.

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1020: pinchViewportOffset =
IntPoint(frameScrollOffset - view->scrollPosition());
Why an IntPoint?

[bokan, 2014-05-09 14:32:56]

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

https://codereview.chromium.org/263853008/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:69: }
On 2014/05/09 14:11:23, abarth wrote:
> Maybe call reset() in the constructor to keep the two in sync?

Done.

https://codereview.chromium.org/263853008/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:96: setLocation(FloatPoint(0, 0));
On 2014/05/09 14:11:23, abarth wrote:
> setLocation(FloatPoint());

Done.

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:190: if
(m_frame->document()->settings()->pinchVirtualViewportEnabled())
On 2014/05/09 14:11:23, abarth wrote:
> m_frame->settings()

Done.

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:193:
m_currentItem->setPinchViewportScrollPoint(FloatPoint(-1, -1));
On 2014/05/09 14:11:23, abarth wrote:
> Can we use something other than (-1, -1) here?  That could be a valid value
for
> the scroll point.  If the value isn't relevant, please use FloatPoint().  If
you
> need to signal "no value", please do that explicitly instead of using in-band
> signaling.

It's not a valid value for the pinch viewport's scroll. It's strictly contained
within the main frame. Also, this should also go away after a few releases, it's
just so HistoryItems with scroll offsets from the old pinch path can still be
restored reasonably.

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1003: float mainFrameScale =
m_frame->document()->settings()->pinchVirtualViewportEnabled() ? 1 :
m_currentItem->pageScaleFactor();
On 2014/05/09 14:11:23, abarth wrote:
> m_frame->settings()

Done.

https://codereview.chromium.org/263853008/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1020: pinchViewportOffset =
IntPoint(frameScrollOffset - view->scrollPosition());
On 2014/05/09 14:11:23, abarth wrote:
> Why an IntPoint?

Done. Changed to FloatPoint.

[abarth-chromium, 2014-05-09 14:54:07]

ok, LGTM

Can you add a comment explain when we can remove the -1, -1 ?

[bokan, 2014-05-09 14:59:31]

On 2014/05/09 14:54:07, abarth wrote:
> ok, LGTM
> 
> Can you add a comment explain when we can remove the -1, -1 ?

Done. Thanks for prompt response!

[bokan, 2014-05-09 14:59:36]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-05-09 15:00:19]

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

[commit-bot: I haz the power, 2014-05-09 16:21:00]

Change committed as 173757