[webkit-reviews] review denied: [Bug 90547] [Qt][WK2] Improve visible content rect update : [Attachment 150780] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 90547: [Qt][WK2] Improve visible content rect update
https://bugs.webkit.org/show_bug.cgi?id=90547

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=150780&action=review


> Source/WebKit2/UIProcess/qt/QtViewportHandler.cpp:715
> +    // A zero trajectoryVector indicates that tiles all around the viewport
are requested.
> +    if (!trajectoryVector.isNull() && m_lastVisibleContentsRect ==
currentVisibleContentsRect)

Good catch

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1230
>  
> +    IntSize currentVisibleContentSize;
> +    if (const FrameView* view = m_frame->coreFrame()->view())
> +	   currentVisibleContentSize = view->visibleContentRect().size();
> +

So what if the current page is zoomed in, then this wont be correct for the new
loaded one.

If you know about the viewport meta at this point you can calculate the first
visible contents rect given the real viewport rect

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1239
> +	   // The HistoryController will update the scroll position later if
needed.
> +	  
m_frame->coreFrame()->view()->setFixedVisibleContentRect(IntRect(IntPoint::zero
(), currentVisibleContentSize));

Sure, but doesnt that mean that we can be smarter here? probably doesnt matter

>
Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp
:515
> -    if (trajectoryVector != FloatPoint::zero())
> -	  
toWebGraphicsLayer(m_nonCompositedContentLayer.get())->setVisibleContentRectTra
jectoryVector(trajectoryVector);
> +   
toWebGraphicsLayer(m_nonCompositedContentLayer.get())->setVisibleContentRectTra
jectoryVector(trajectoryVector);

Add comment that it can be zero here?