Don't calculate a top controls adjustment if pinch viewport size is uninitialized.

Don't calculate a top controls adjustment if pinch viewport size is uninitialized.

This was causing a NaN assertion further on in FrameView::maximumScrollPosition.

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

[bokan, 2014-10-22 16:52:18]

There's a bit of issue around initialization that this uncovered but I'd like to
unblock pkasting@'s patch (see bug) while I sort it out in the meantime.

[bokan, 2014-10-22 16:52:46]

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

[bokan, 2014-10-22 16:53:01]

There's a bit of issue around initialization that this uncovered but I'd like to
unblock pkasting@'s patch (see bug) while I sort it out in the meantime.

[aelias_OOO_until_Jul13, 2014-10-22 18:21:45]

Could you add another call to didUpdateTopControls() when the pinch viewport
size changes, to make sure we set the adjustment properly as soon as all the
information is there?

[Peter Kasting, 2014-10-22 19:32:20]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2014-10-22 19:32:21]

Drive-by.

Please do go ahead and uncomment the ASSERT in MathExtras.h:clampTo() and remove
the FIXME above it.

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

https://codereview.chromium.org/663993003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1733: if (!pinchViewport.visibleRect().width() ||
!pinchViewport.visibleRect().height())
Can this just be "if (pinchViewport.visibleRect().isEmpty())"?  Or did you
actually want negative-size cases to proceed onwards?

[bokan, 2014-10-22 20:08:36]

On 2014/10/22 19:32:21, Peter Kasting wrote:
> Drive-by.
> 
> Please do go ahead and uncomment the ASSERT in MathExtras.h:clampTo() and
remove
> the FIXME above it.

Done.

[bokan, 2014-10-22 20:08:48]

On 2014/10/22 18:21:45, aelias wrote:
> Could you add another call to didUpdateTopControls() when the pinch viewport
> size changes, to make sure we set the adjustment properly as soon as all the
> information is there?
Done.

[bokan, 2014-10-22 20:08:53]

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

https://codereview.chromium.org/663993003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1733: if (!pinchViewport.visibleRect().width() ||
!pinchViewport.visibleRect().height())
On 2014/10/22 19:32:21, Peter Kasting wrote:
> Can this just be "if (pinchViewport.visibleRect().isEmpty())"?  Or did you
> actually want negative-size cases to proceed onwards?

Done.

[Peter Kasting, 2014-10-22 20:15:51]

LGTM, thanks!

[bokan, 2014-10-22 20:21:36]

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

[bokan, 2014-10-22 20:21:37]

+jchaffraix@ for Source/wtf

[Julien - ping for review, 2014-10-22 22:19:26]

Source/wtf rs-lgtm

[bokan, 2014-10-23 14:58:01]

Alexandre, Source/web look ok?

[aelias_OOO_until_Jul13, 2014-10-23 17:41:29]

lgtm

[bokan, 2014-10-23 18:05:40]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-10-23 18:06:20]

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

[commit-bot: I haz the power, 2014-10-23 20:05:36]

Committed patchset #2 (id:20001) as 184298