[Qt][WK2] Improve visible content rect update

[Andras Becsi]

[Qt][WK2] Improve visible content rect update

[Andras Becsi]

Created attachment 150780 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 150780 [details]
Patch

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())->setVisibleContentRectTrajectoryVector(trajectoryVector);
> +    toWebGraphicsLayer(m_nonCompositedContentLayer.get())->setVisibleContentRectTrajectoryVector(trajectoryVector);

Add comment that it can be zero here?

[Andras Becsi]

Created attachment 151070 [details]
updated patch

[Gyuyoung Kim]

Comment on attachment 151070 [details]
updated patch

Attachment 151070 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13145446

[Andras Becsi]

Created attachment 151073 [details]
updated patch

Fixed Gtk and EFL builds.

[Gyuyoung Kim]

Comment on attachment 151073 [details]
updated patch

Attachment 151073 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13143448

[Build Bot]

Comment on attachment 151073 [details]
updated patch

Attachment 151073 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13145448

[Build Bot]

Comment on attachment 151073 [details]
updated patch

Attachment 151073 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13153308

[Andras Becsi]

Created attachment 151077 [details]
updated patch

Another attempt to fix the build on non-Qt platforms.

[Andras Becsi]

(In reply to comment #2)
> (From update of attachment 150780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150780&action=review
[snip]
> > 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
>

Fixed. At this point during page load the only way I could get the actual viewport size was through WebPage, so I had to remove the TILED_BACKING_STORE guards from the needed functions. I also checked ViewportArguments which is empty here, so is there another way of getting the viewport size?

> > 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

Setting a position here would be wrong, since (except when reloading) the loaded page could be a completely new one, so HistoryController should do the positioning if needed.

> 
> > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:515
> > -    if (trajectoryVector != FloatPoint::zero())
> > -        toWebGraphicsLayer(m_nonCompositedContentLayer.get())->setVisibleContentRectTrajectoryVector(trajectoryVector);
> > +    toWebGraphicsLayer(m_nonCompositedContentLayer.get())->setVisibleContentRectTrajectoryVector(trajectoryVector);
> 
> Add comment that it can be zero here?
Done.

[Andras Becsi]

Comment on attachment 151077 [details]
updated patch

Remove patch from review queue, since part of it already landed in bug 92750 and the patch landed for bug 90550 seems to have fixed my test case. Trying to find a new one.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.