Bug 80313 - MiniBrowser --window-size 480x800 www.nytimes.com doesn't paint bottom tiles.
Summary: MiniBrowser --window-size 480x800 www.nytimes.com doesn't paint bottom tiles.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hugo Parente Lima
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-05 12:15 PST by Hugo Parente Lima
Modified: 2012-03-06 13:43 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.96 KB, patch)
2012-03-05 12:24 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (4.32 KB, patch)
2012-03-06 10:42 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hugo Parente Lima 2012-03-05 12:15:36 PST
The problem can be reproduced with the HTML file above and "MiniBrowser --window-size 480x800":

<html>
<head></head>
<style type="text/css">
div {
    height: 500px;
    background-color: grey;
    font-size: 72px;
    text-align: center;
    line-height: 500px;
}

div.odd {
    background-color: silver;
}
</style>
<body>
<div class="odd">Huge square 1/8</div>
<div>Huge square 2/8</div>
<div class="odd">Huge square 3/8</div>
<div>Huge square 4/8</div>
<div class="odd">Huge square 5/8</div>
<div>Huge square 6/8</div>
<div class="odd">Huge square 7/8</div>
<div>Huge square 8/8</div>
</body>
</html>
Comment 1 Hugo Parente Lima 2012-03-05 12:24:03 PST
Created attachment 130181 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-03-05 14:45:58 PST
Comment on attachment 130181 [details]
Patch

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(pageViewRectInCSSCoordinates).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();
}
Comment 3 Hugo Parente Lima 2012-03-05 15:19:11 PST
You are right, I can avoid multiple transformations before doing the intersection.

I'll improve and re-upload the patch (probably tomorrow).

Thanks for the feedback =]
Comment 4 Hugo Parente Lima 2012-03-06 10:42:21 PST
Created attachment 130404 [details]
Patch
Comment 5 WebKit Review Bot 2012-03-06 13:43:26 PST
Comment on attachment 130404 [details]
Patch

Clearing flags on attachment: 130404

Committed r109957: <http://trac.webkit.org/changeset/109957>
Comment 6 WebKit Review Bot 2012-03-06 13:43:31 PST
All reviewed patches have been landed.  Closing bug.