[webkit-reviews] review denied: [Bug 126022] [CoordinatedGraphics][WK2] Scale factor and scroll position is not being restored properly in a back/forward load : [Attachment 219798] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Thiago de Barros Lacerda
<thiago.lacerda at openbossa.org>'s request for review:
Bug 126022: [CoordinatedGraphics][WK2] Scale factor and scroll position is not
being restored properly in a back/forward load
https://bugs.webkit.org/show_bug.cgi?id=126022

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

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


r- due to platform #ifdefs in HistoryController.

> Source/WebKit2/ChangeLog:3
> +	   scalebackhistory

?

> Source/WebCore/loader/HistoryController.cpp:162
> +#if PLATFORM(EFL) || PLATFORM(NIX)
> +    double previousScaleFactor = m_previousItem ?
m_previousItem->pageScaleFactor() : m_currentItem->pageScaleFactor();
> +    IntPoint previousScrollPoint = m_previousItem ?
m_previousItem->scrollPoint() : m_currentItem->scrollPoint();
> +    return previousScaleFactor != m_currentItem->pageScaleFactor() ||
previousScrollPoint != m_currentItem->scrollPoint();
> +#else
> +    return !view->wasScrolledByUser();
> +#endif

This #ifdef is not okay. Why can't the code be made common exactly?

The changelog has a little explanation but not enough to determine what is
correct.

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:88
> +    if (isSuspended())
> +	   return;

This is odd. An explanation in the changelog would be welcome.

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:106
> +    m_contentScaleFactor = scaleFactor;

This is a bad idea. The view presents the information of WebPageProxy. The
information about contentScaleFactor already exists on WebPageProxy, I don't
think you should duplicate it.