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

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam 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 219697: Patch
https://bugs.webkit.org/attachment.cgi?id=219697&action=review

------- Additional Comments from Noam Rosenthal <noam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=219697&action=review


> Source/WebCore/ChangeLog:17
> +	   * loader/HistoryController.cpp:
> +	   (WebCore::HistoryController::restoreScrollPositionAndViewState):

Explain what you're actually doing to rectify this.

> Source/WebKit2/ChangeLog:13
> +	   When user is navigating back/forward to a page that has been
scrolled and/or scaled, that page must be shown
> +	   with its last scroll position and scale factor.
> +	   Also, when a user was using the WKView API to set the scale factor
(WKViewSetContentScaleFactor) this scale was
> +	   not being forwarded to WebCore. Then, when going back/forward to a
page HistoryController was not knowing about
> +	   its real scale factor.
> +	   A WebKitAPI test is being added to validate this behaviour.

This is a description of the bug. Also describe the solution otherwise the code
is hard to follow.

> Source/WebCore/loader/HistoryController.cpp:159
> +	   double currentScaleFactor = m_currentItem->pageScaleFactor();
> +	   IntPoint currentScrollPoint = m_currentItem->scrollPoint();
> +#if PLATFORM(EFL) || PLATFORM(NIX)
> +	   double previousScaleFactor = m_previousItem ?
m_previousItem->pageScaleFactor() : currentScaleFactor;
> +	   IntPoint previousScrollPoint = m_previousItem ?
m_previousItem->scrollPoint() : currentScrollPoint;
> +
> +	   if (previousScaleFactor != currentScaleFactor || previousScrollPoint
!= currentScrollPoint) {
> +#else
>	   if (!view->wasScrolledByUser()) {
> -	       if (page && m_frame.isMainFrame() &&
m_currentItem->pageScaleFactor())
> -		   page->setPageScaleFactor(m_currentItem->pageScaleFactor(),
m_currentItem->scrollPoint());
> +#endif // PLATFORM(EFL) || PLATFORM(NIX)
> +	       if (page && m_frame.isMainFrame() && currentScaleFactor)
> +		   page->setPageScaleFactor(currentScaleFactor,
currentScrollPoint);
>	       else
> -		   view->setScrollPosition(m_currentItem->scrollPoint());
> +		   view->setScrollPosition(currentScrollPoint);
>	   }
>      }

This snippet is a bit spaghettified... I would rather try to figure out how
ports where this already works (Mac?) do this, and try to consolidate rather
than have port #ifdefs in WebCore.

> Source/WebKit2/UIProcess/PageClient.h:127
> +#if PLATFORM(EFL) || PLATFORM(NIX)
> +    virtual void didChangePageScaleFactor(double scaleFactor) = 0;
> +#endif

I think this can be an empty implementation rather than some #ifdefed thing.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1147
> +#if PLATFORM(EFL) || PLATFORM(NIX)
> +    if (m_frame == m_frame->page()->mainWebFrame())
> +	  
m_frame->page()->send(Messages::WebPageProxy::PageDidRequestScroll(m_frame->cor
eFrame()->loader().history().currentItem()->scrollPoint()));
> +#endif

How is this done in Mac?
If it's not, maybe it's better to make this into a Settings flag rather than an
#ifdef block.