Bug 43168 - [Qt] QtTestBrowser not setting preferredContentsSize for resizesToContents
Summary: [Qt] QtTestBrowser not setting preferredContentsSize for resizesToContents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-07-28 21:19 PDT by Antonio Gomes
Modified: 2010-08-03 08:20 PDT (History)
3 users (show)

See Also:


Attachments
patch v1 (5.67 KB, patch)
2010-08-02 12:42 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v2 (6.42 KB, patch)
2010-08-02 13:53 PDT, Antonio Gomes
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
(committed r64556, r=kenneth) patch v2.1 - right patch (6.49 KB, patch)
2010-08-02 23:41 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-07-28 21:19:23 PDT
Using resizesToContents is working bad in our reference/demo browser, QtTestBrowser:

- we are not using this property together with setPreferredContentsSize (from QWebPage), which is explicitly adviced in our docs: http://doc.trolltech.com/4.7-snapshot/qgraphicswebview.html#resizesToContents-prop

- also the default value hardcoded when no preferred contents size is set is not good.

void QGraphicsWebViewPrivate::updateResizesToContentsForPage()
{
..
if (resizesToContents) {
  // resizes to contents mode requires preferred contents size to be set
  if (!page->preferredContentsSize().isValid())
    page->setPreferredContentsSize(QSize(960, 800));


** Also, scrolling gets broken on simple websites when we toggle it on, but it can be for another bug.
Comment 1 Antonio Gomes 2010-08-02 12:42:25 PDT
Created attachment 63247 [details]
patch v1

Patch addresses the problem for QtTestBrowser by properly handle scene, webview and viewport sizes when toggle resizesToContents on/off.
Comment 2 Kenneth Rohde Christiansen 2010-08-02 12:48:32 PDT
Comment on attachment 63247 [details]
patch v1

WebKitTools/QtTestBrowser/webview.cpp:91
 +          setVerticalScrollBarPolicy(Qt::ScrollBarAsNeeded);
Strange... if it resizes to contents, no scrollbars should be shown... why are they set  to as needed?
Comment 3 Kenneth Rohde Christiansen 2010-08-02 12:50:24 PDT
Comment on attachment 63247 [details]
patch v1

WebKitTools/QtTestBrowser/webview.cpp:91
 +          setVerticalScrollBarPolicy(Qt::ScrollBarAsNeeded);
Strange... if it resizes to contents, no scrollbars should be shown... why are they set  to as needed?

WebKitTools/QtTestBrowser/webview.cpp:100
 +          scene()->setSceneRect(viewportRect);
shouldnt these be set automatically elsewhere so they are always consistent.

WebKitTools/QtTestBrowser/webview.cpp:93
 +          scene()->setSceneRect(QRectF());
why this?

WebKitTools/QtTestBrowser/launcherwindow.cpp:409
 +              toggleResizesToContents(true);
why not toggleResizesToContents(gResizesToContents) ?
Comment 4 Antonio Gomes 2010-08-02 12:56:44 PDT
(In reply to comment #2)
> (From update of attachment 63247 [details])
> WebKitTools/QtTestBrowser/webview.cpp:91
>  +          setVerticalScrollBarPolicy(Qt::ScrollBarAsNeeded);
> Strange... if it resizes to contents, no scrollbars should be shown... why are they set  to as needed?

QGraphicsWebView *wont* have scrollbars as you said, but in order to make the content completely viewable (including offscreen content), the QGraphicsView still perform the scrolling, so it is set back to 'auto'.

QGraphicsView's viewport is what is going to be getting scrolled instead then.

> WebKitTools/QtTestBrowser/webview.cpp:100
>  +          scene()->setSceneRect(viewportRect);
> shouldnt these be set automatically elsewhere so they are always consistent.

In fact once setSceneRect(xxx) is called (see existing code in WebViewGraphicsBased::resizeEvent), scene will be sizing xxx until we call it with a invalid QRectF. 
 
> WebKitTools/QtTestBrowser/webview.cpp:93
>  +          scene()->setSceneRect(QRectF());
> why this?

See from the docs:

"The scene rectangle defines the extent of the scene. It is primarily used by QGraphicsView to determine the view's default scrollable area, and by QGraphicsScene to manage item indexing.
If unset, or if set to a null QRectF, sceneRect() will return the largest bounding rect of all items on the scene since the scene was created (i.e., a rectangle that grows when items are added to or moved in the scene, but never shrinks)."
Comment 5 Antonio Gomes 2010-08-02 13:53:18 PDT
Created attachment 63256 [details]
patch v2

Same patch as attachment 63247 [details], but with more explainations in the code, as per discussion with Kenneth on IRC.
Comment 6 Antonio Gomes 2010-08-02 23:41:30 PDT
Created attachment 63304 [details]
(committed r64556, r=kenneth) patch v2.1 - right patch

[Right patch this time].
Comment 7 Antonio Gomes 2010-08-03 08:20:07 PDT
Comment on attachment 63304 [details]
(committed r64556, r=kenneth) patch v2.1 - right patch

Clearing flags on attachment: 63304

Committed r64556: <http://trac.webkit.org/changeset/64556>