[webkit-reviews] review denied: [Bug 80313] MiniBrowser --window-size 480x800 www.nytimes.com doesn't paint bottom tiles. : [Attachment 130181] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Hugo Parente Lima
<hugo.lima at openbossa.org>'s request for review:
Bug 80313: MiniBrowser --window-size 480x800 www.nytimes.com doesn't paint
bottom tiles.
https://bugs.webkit.org/show_bug.cgi?id=80313

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

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


> Source/WebKit2/ChangeLog:8
> +	   Map pageView boundingRect to the same coordinate system of webView
boundingRect

I would just write "page view" and "web view", ie. use more natural language

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:662
> -    QRect alignedVisibleContentRect =
visibleRectInCSSCoordinates.toAlignedRect();
> +    const QRectF pageViewRectInCSSCoordinates =
q->mapRectToWebContent(pageView->boundingRect());
> +    const QRectF webViewRectInCSSCoordinates =
q->mapRectToWebContent(q->boundingRect());
> +    const QRect
alignedVisibleContentRect(webViewRectInCSSCoordinates.intersected(pageViewRectI
nCSSCoordinates).toAlignedRect());
> +

Our names are confusing :/

So we have
QScopedPointer<QQuickWebPage> pageView;
which is our item. The view is a bit confusing there.

So isn't q->mapRectToWebContent(pageView->boundingRect()); the same as the
actual web contents? Maybe there is an easier way to get that without
transforming anyway, the name "const QRectF contentsRectInCSSCoordinates" might
be more clear.

Anyway, why not do this differently

Intersect the rect of the pageItem with the view and then map the result?

const QRectF visibleRectInCSSCoordinates =
q->mapRectToWebContent(q->boundingRect().intersected(pageView->boundingRect()))
;

You could also make a methods like QQuickWebViewPrivate::visibleContentsRect()
as we are doing this multiple places (you are only fixing one here).

{
const QRectF viewportRect = q->boundingRect();
const QRectF contentsRect = pageView->boundingRect();

return
q->mapRectToWebContent(viewportRect.intersected(contentsRect)).toAlignedRect();

}