[webkit-reviews] review denied: [Bug 136999] [EFL] Restore previous scroll position using restoreViewState() : [Attachment 239095] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Gyuyoung Kim
<gyuyoung.kim at webkit.org>'s request for review:
Bug 136999: [EFL] Restore previous scroll position using restoreViewState()
https://bugs.webkit.org/show_bug.cgi?id=136999

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

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


Those are really two fixes in one patch.

The WebCore part seems to be a step in the right direction. My only question
has to do with bypassing ScrollView, if there is a good rationale, it should be
explained in the changelog.

The WebKit2 part still don't make any sense as it is implemented. The code
looks correct but the semantic does not fit with anything I know in the
architecture. Again, this may be correct but it would need an improved
changelog.

> Source/WebCore/page/Page.cpp:745
> -		   view->hostWindow()->delegatedScrollRequested(origin);
> +		   view->requestScrollPositionUpdate(origin);

I find it odd that you are calling requestScrollPositionUpdate() directly
instead of going through setScrollPosition(). What's the reason for that?

> Source/WebCore/page/Page.cpp:782
> -	       view->hostWindow()->delegatedScrollRequested(origin);
> +	       view->requestScrollPositionUpdate(origin);

ditto.

> Source/WebKit2/WebProcess/WebPage/efl/WebPageEfl.cpp:237
> +    send(Messages::WebPageProxy::PageScaleFactorDidChange(pageScaleFactor));


This does not really make sense. PageScaleFactorDidChange should only happen
after the Page's pageScaleFactor has been updated.

Where is it updated here?