[webkit-reviews] review granted: [Bug 134238] [iOS WK2] Page jumps when rubber-banding on azuremagazine.com : [Attachment 233773] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 134238: [iOS WK2] Page jumps when rubber-banding on azuremagazine.com
https://bugs.webkit.org/show_bug.cgi?id=134238

Attachment 233773: Patch
https://bugs.webkit.org/attachment.cgi?id=233773&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=233773&action=review


I think that is okay for growing content. At the next timer update, the new
BouncingDecelerationOffset will be withint bounds.

> Source/WebKit2/UIProcess/ios/WKScrollView.h:37
> +-
(void)_setContentSizePreservingContentOffsetDuringRubberband:(CGSize)contentSiz
e;
> +

Not sure you need the blank lines below and above, I know nothing about Obj-C
coding style :)

> Source/WebKit2/UIProcess/ios/WKScrollView.mm:234
> +    if (rubberbandAmount.width < 0) // Left
> +	   adjustedOffset.x = -edgeInsets.left + rubberbandAmount.width;

I think you should check this in the case where the page size change when it is
scaled smaller than the viewport minimum size.

It may work as expected because in _currentRubberbandAmount, you check for
top/left before bottom/right, but better check.

> Source/WebKit2/UIProcess/ios/WKScrollView.mm:260
> +    if (CGSizeEqualToSize(currentContentSize, CGSizeZero) ||
CGSizeEqualToSize(currentContentSize, contentSize)) {

You could early return on CGSizeEqualToSize(currentContentSize, contentSize).