Made top controls work with virtual viewport pinch-to-zoom. (Blink)

Made top controls work with virtual viewport pinch-to-zoom. (Blink)

Together with the Chromium-side patch, makes top controls work properly
under the virtual viewport mode of pinch zoom. The key changes on the
Blink side are to adjust both the PinchViewport and FrameView max scroll
bounds to account for the change in the top controls offset on the
compositor. Additionally, I changed these to make the same floating point
and snapping calculations as in the compositor to prevent losing pixel
fractions.

Chromium side: https://codereview.chromium.org/641873003/

Note, this was previously landed in 183639 and reverted in 183653 due
to failing tests on Mac bots.

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

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

[bokan, 2014-10-08 23:03:25]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org

[bokan, 2014-10-08 23:03:38]

ptal.

[aelias_OOO_until_Jul13, 2014-10-09 22:58:53]

lgtm

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

https://codereview.chromium.org/643473002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.cpp:361: if (m_topControlsAdjustment) {
Please remove this if() statement, it's unnecessary complexity since it doesn't
change the behavior.  Maybe replace it with a comment explaining you expect this
will have an effect in the top controls case.

[bokan, 2014-10-09 23:22:49]

bokan@chromium.org changed reviewers:
+ rbyers@chromium.org

[bokan, 2014-10-09 23:22:50]

+rbyers for Source/core OWNER

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

https://codereview.chromium.org/643473002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.cpp:361: if (m_topControlsAdjustment) {
On 2014/10/09 22:58:53, aelias wrote:
> Please remove this if() statement, it's unnecessary complexity since it
doesn't
> change the behavior.  Maybe replace it with a comment explaining you expect
this
> will have an effect in the top controls case.

Done.

[Rick Byers, 2014-10-10 00:06:10]

Definitely a little grotty, but I trust your/aelias's judgement on the best
complexity tradeoff.  If you've got ideas for how this could reasonably be
cleaned up, then please file bugs to track (we can dedicate some time in Q4 to
some post-virtual-viewport refactoring/cleanup if desired).

LGTM with nit.

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

https://codereview.chromium.org/643473002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:359: FloatSize
frameViewSize(contentsSize());
not necessarily for this CL (if it's non-trivial to correct here), but note that
we've elsewhere decided that float doesn't have enough precision to properly
represent the size of a document.  Eg. it could result in off-by-one errors in
long documents (eg. with height+1==height).  Elsewhere (miletus@'s fractional
scroll offset work) we're using doubles, you may want to consider changing
PinchViewport to match (or at least testing on a page with height >1,000,000px -
I just got a real-world bug for a page nearly this long the other day).

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

https://codereview.chromium.org/643473002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.h:120: // FIXME(bokan): This is kind of a hack.
Ideally, we would just resize the
for better or worse, blink style is not to include names/ldaps in FIXMEs:
http://dev.chromium.org/blink/coding-style#TOC-Comments.  If it needs to be
tracked, then file a bug and reference the bug number.

[bokan, 2014-10-10 14:39:56]

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

https://codereview.chromium.org/643473002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.cpp:361: if (m_topControlsAdjustment) {
On 2014/10/09 22:58:53, aelias wrote:
> Please remove this if() statement, it's unnecessary complexity since it
doesn't
> change the behavior.  Maybe replace it with a comment explaining you expect
this
> will have an effect in the top controls case.

Actually, the 'if' is needed, there are cases where the aspect ratios don't
match. The CrOS OSK is the one real world example. That's by design since we
don't want the fixed pos elements to move when the keyboard comes up.

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

https://codereview.chromium.org/643473002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:359: FloatSize
frameViewSize(contentsSize());
On 2014/10/10 00:06:09, Rick Byers wrote:
> not necessarily for this CL (if it's non-trivial to correct here), but note
that
> we've elsewhere decided that float doesn't have enough precision to properly
> represent the size of a document.  Eg. it could result in off-by-one errors in
> long documents (eg. with height+1==height).  Elsewhere (miletus@'s fractional
> scroll offset work) we're using doubles, you may want to consider changing
> PinchViewport to match (or at least testing on a page with height >1,000,000px
-
> I just got a real-world bug for a page nearly this long the other day).

Yes, I've been following the discussion. I'll keep this here for now since it
should match the calculation in LayerImpl::MaxScrollOffset in CC which uses
SizeF. Filed crbug.com/422331 and referenced it with a FIXME.

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

https://codereview.chromium.org/643473002/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.h:120: // FIXME(bokan): This is kind of a hack.
Ideally, we would just resize the
On 2014/10/10 00:06:09, Rick Byers wrote:
> for better or worse, blink style is not to include names/ldaps in FIXMEs:
> http://dev.chromium.org/blink/coding-style#TOC-Comments.  If it needs to be
> tracked, then file a bug and reference the bug number.

Filed crbug.com/422328

[bokan, 2014-10-10 14:42:31]

On 2014/10/10 00:06:10, Rick Byers wrote:
> Definitely a little grotty, but I trust your/aelias's judgement on the best
> complexity tradeoff.  If you've got ideas for how this could reasonably be
> cleaned up, then please file bugs to track (we can dedicate some time in Q4 to
> some post-virtual-viewport refactoring/cleanup if desired).
> 
Yup, I've been littering the codebase with FIXMEs and filed a number of cleanup
bugs. There will definitely be a period of cleanup once feature work is
complete, especially once we can get rid of the old pinch mode, that'll make
lots of cleanup possible.

[Rick Byers, 2014-10-10 16:05:21]

On 2014/10/10 14:42:31, bokan wrote:
> On 2014/10/10 00:06:10, Rick Byers wrote:
> > Definitely a little grotty, but I trust your/aelias's judgement on the best
> > complexity tradeoff.  If you've got ideas for how this could reasonably be
> > cleaned up, then please file bugs to track (we can dedicate some time in Q4
to
> > some post-virtual-viewport refactoring/cleanup if desired).
> > 
> Yup, I've been littering the codebase with FIXMEs and filed a number of
cleanup
> bugs. There will definitely be a period of cleanup once feature work is
> complete, especially once we can get rid of the old pinch mode, that'll make
> lots of cleanup possible.

Sounds good to me, thanks.  Still LGTM

[bokan, 2014-10-14 02:04:11]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-10-14 02:05:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643473002/100001

[commit-bot: I haz the power, 2014-10-14 04:29:34]

Committed patchset #3 (id:100001) as 183639

[apavlov, 2014-10-14 08:19:42]

A revert of this CL (patchset #3 id:100001) has been created in
https://codereview.chromium.org/655763002/ by apavlov@chromium.org.

The reason for reverting is: Broke PinchViewportTest.TestTopControlsAdjustment
on Mac bots (e.g. see
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7/builds...).

[bokan, 2014-10-14 20:18:41]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-10-14 20:19:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643473002/200001

[commit-bot: I haz the power, 2014-10-14 23:11:24]

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

[commit-bot: I haz the power, 2014-10-14 23:11:26]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/31789)

[bokan, 2014-10-14 23:12:09]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-10-14 23:12:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643473002/200001

[commit-bot: I haz the power, 2014-10-15 01:22:53]

Committed patchset #4 (id:200001) as 183713