Bug 43852 - [Qt] resizeToContent seems to trigger infinite resize on some pages
Summary: [Qt] resizeToContent seems to trigger infinite resize on some pages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Adenilson Cavalcanti Silva
URL: http://ie.microsoft.com/testdrive/Per...
Keywords: Qt, QtTriaged
: 42754 (view as bug list)
Depends on: 49371 49373 49375
Blocks: 32653 55056
  Show dependency treegraph
 
Reported: 2010-08-11 07:27 PDT by Benjamin Poulain
Modified: 2011-09-28 14:02 PDT (History)
20 users (show)

See Also:


Attachments
Test case (632 bytes, text/html)
2010-11-10 03:15 PST, Benjamin Poulain
no flags Details
Consolidation of various patches from trunk & webkit2 branch (12.03 KB, text/plain)
2010-11-19 17:18 PST, Nancy Piedra
no flags Details
Webkit 2.1 changes needed to fix resizesToContents issue (22.43 KB, text/plain)
2010-11-23 05:26 PST, Nancy Piedra
no flags Details
Consolidation of patches needed to fix resizesToContents for Webkit 2.1 branch (19.53 KB, text/plain)
2010-11-30 15:37 PST, Nancy Piedra
no flags Details
v2 of the consolidation of patches for qtwebkit-2.1 (24.11 KB, patch)
2010-12-01 07:20 PST, Ademar Reis
no flags Details | Formatted Diff | Diff
v3 of the consolidation of patches for qtwebkit-2.1 (24.96 KB, patch)
2010-12-01 07:38 PST, Ademar Reis
no flags Details | Formatted Diff | Diff
v4 of the consolidation of patches for qtwebkit-2.1 (25.09 KB, patch)
2010-12-02 13:43 PST, Ademar Reis
no flags Details | Formatted Diff | Diff
Patch. (5.83 KB, patch)
2011-06-13 11:10 PDT, Yael
no flags Details | Formatted Diff | Diff
patch (16.50 KB, patch)
2011-07-02 17:40 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch (13.03 KB, patch)
2011-07-12 15:37 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch (13.08 KB, patch)
2011-07-13 08:13 PDT, Luiz Agostini
kenneth: review-
Details | Formatted Diff | Diff
patch (11.54 KB, patch)
2011-07-19 14:20 PDT, Luiz Agostini
benjamin: review-
Details | Formatted Diff | Diff
Layout test results on the Mac port after the patch (218.89 KB, application/x-gzip)
2011-08-31 06:25 PDT, Ademar Reis
no flags Details
Output folder of tests (3 cases: vanilla, innerHeight/Width() patch, previous + visibleWidth/Height (1.92 MB, application/x-bzip2)
2011-09-12 09:53 PDT, Adenilson Cavalcanti Silva
no flags Details
Vanilla + new patch (using visibleContentRect directly in DOMWindow) (411.66 KB, application/x-bzip2)
2011-09-13 08:12 PDT, Adenilson Cavalcanti Silva
no flags Details
Patch (1.64 KB, patch)
2011-09-13 18:57 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Patch (5.38 KB, patch)
2011-09-16 07:17 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Patch with more descriptive Changelog and cleaned up test (5.42 KB, patch)
2011-09-16 08:13 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Test with more descriptive ChangeLog and cleaner test (5.42 KB, patch)
2011-09-16 08:17 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Adding external references to W3C spec in changelog comments + minor fixes (8.25 KB, patch)
2011-09-19 08:47 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Adding external references to W3C spec in changelog comments + minor fixes (6.16 KB, patch)
2011-09-19 08:59 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Adding external references to W3C spec in changelog comments + minor fixes (6.25 KB, patch)
2011-09-19 09:05 PDT, Adenilson Cavalcanti Silva
kenneth: review+
kenneth: commit-queue-
Details | Formatted Diff | Diff
Updated changelog comments + minor fixes (5.94 KB, patch)
2011-09-19 17:23 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2010-08-11 07:27:53 PDT
On some pages, like the microsoft test drive of canvas: http://ie.microsoft.com/testdrive/Performance/FishIE%20tank/Default.html , the page with resizeToContent keeps growing.

To reproduce:
-open QtTestBrowser
-set graphics view and resize to content to true
-open http://ie.microsoft.com/testdrive/Performance/FishIE%20tank/Default.html
-enjoy :)
Comment 1 Antonio Gomes 2010-08-11 07:31:26 PDT
It might be my fault. Looking ...
Comment 2 Antonio Gomes 2010-08-11 11:45:55 PDT
(In reply to comment #1)
> It might be my fault. Looking ...

No my fault, but QtWRT had the *exactly* same probably with resizesToContents ON (on the ovi store widget). 

In both cases there was something like:

(...)
window.addEventListener("resize", OnWindowResize, false);

(...)
function OnWindowResize(e) {
  var bodyWidth = window.innerWidth;
  var bodyHeight = window.innerHeight;

  WIDTH = bodyWidth;
  HEIGHT = bodyHeight;
  document.getElementById('canvas1').width = WIDTH;
  document.getElementById('canvas1').height = HEIGHT;
  document.getElementById('background').width = WIDTH;
  document.getElementById('background').height = HEIGHT;
(...)
}
Comment 3 Nancy Piedra 2010-11-09 05:20:56 PST
I have seen this bug on other sites including www.nationalgeographic.com and map.google.com when iphone 4 user agent is used with Qt Webkit.
Comment 4 Suresh Voruganti 2010-11-09 05:51:41 PST
Fix is required for Qtwebkit 2.1, as this is blocking browser release. Pls prioritize this bug.
Comment 5 Nancy Piedra 2010-11-09 07:41:57 PST
Also, yahoo.com has this problem when using the iphone 4 user agent.
Comment 6 Benjamin Poulain 2010-11-10 03:15:45 PST
Created attachment 73479 [details]
Test case
Comment 7 Benjamin Poulain 2010-11-10 05:05:37 PST
@Jacob
The solution to this is to report the right window size from Javascript. Zalan is working on a patch for this, please wait for his input before working on this bug.
Comment 8 Nancy Piedra 2010-11-10 05:09:39 PST
We will test the patch on Qt Webkit 2.1 as soon as it is available.
Comment 9 zalan 2010-11-10 07:15:40 PST
Since the logical window grows with the content, when resize-to-content is on, window.innerHeight/innerWidth reports false view(window) size. the fix is to decouple the logical window size from window.innerHeight/innerWidth and report the actual visible content rect. 
Reporting growing window size makes the web page think, that the user resizes the window, so the page keeps requesting for relayout with repositioned(growing) content -> infinite loop.

Kenneth is working on a patch right now, which needs to be landed first. my patch(using his code) will come right after his gets landed (expected today/tomorrow)
Comment 10 Nancy Piedra 2010-11-10 07:36:32 PST
Is Kenneth patch under a different bugzilla id?  If so, please add the id here.
Comment 11 zalan 2010-11-10 07:38:46 PST
no bugzilla id yet. I'll add it here, when it is created.
Comment 12 zalan 2010-11-11 02:25:07 PST
depends on 49371.

https://bugs.webkit.org/show_bug.cgi?id=49371
Comment 13 Kenneth Rohde Christiansen 2010-11-11 04:37:42 PST
This bugs depends on two other patches which will need to be cherry-picked
Comment 14 Ademar Reis 2010-11-12 12:44:22 PST
Will we have a patch for this bug or just the other fixes (bug 49371 and bug 49375) are enough?
Comment 15 Ademar Reis 2010-11-12 13:02:34 PST
(In reply to comment #14)
> Will we have a patch for this bug or just the other fixes (bug 49371 and bug 49375) are enough?

Answering myself: on both trunk and qtwebkit-2.1 the problem remains after applying the other patches.
Comment 16 Kenneth Rohde Christiansen 2010-11-15 02:39:09 PST
You will need to modify the QtTestBrowser to actually call the new api.
Comment 17 Nancy Piedra 2010-11-15 05:08:32 PST
I don't see any new Qt API in 49371 and 49375.  Is there another dependency?
Comment 18 Nancy Piedra 2010-11-15 05:53:09 PST
I think I found the other dependency. It is 49373.  Will add it as a dependency to this bug.
Comment 19 Dinu Jacob 2010-11-15 06:56:56 PST
(In reply to comment #9)
> Since the logical window grows with the content, when resize-to-content is on, window.innerHeight/innerWidth reports false view(window) size. the fix is to decouple the logical window size from window.innerHeight/innerWidth and report the actual visible content rect. 
> Reporting growing window size makes the web page think, that the user resizes the window, so the page keeps requesting for relayout with repositioned(growing) content -> infinite loop.
> 
> Kenneth is working on a patch right now, which needs to be landed first. my patch(using his code) will come right after his gets landed (expected today/tomorrow)

Hi Zalan,
Are you working on a patch for this as the dependencies have been landed?
Comment 20 Ademar Reis 2010-11-17 07:13:18 PST
Please make an effort to provide the patch until tomorrow so that we have time to cherry-pick/backport it to 2.1 on time for the next weekly tag. Does it sound realistic?
Comment 21 zalan 2010-11-17 07:24:40 PST
Yes, i am working on the patch atm. Will probably upload it early tomorrow.
Comment 22 Dinu Jacob 2010-11-17 08:31:05 PST
Setting m_actualVisibleContentRect through ScrollView::setActualVisibleContentRect didn't seem to have any impact. The innerWidth and innerHeight is the width and height of the frameRect of the Widget. When resizesToContent is set, the frameRect of ScrollView (and Widget) is being updated to the new rect value. When resizesToContent is set, qgraphicswebview listens to QWebFrame's contentsSizeChanged signal. In the slot it updates its geometry and also sets QWebPage viewport. In QWebPage::setViewport, the frameRect of ScrollView is set to the new rect value. In the resize handler, this new value is being read. 

In this scenario do we need to update the QWebPage viewport size?
Comment 23 Dinu Jacob 2010-11-17 08:33:23 PST
(In reply to comment #22)
> Setting m_actualVisibleContentRect through ScrollView::setActualVisibleContentRect didn't seem to have any impact. The innerWidth and innerHeight is the width and height of the frameRect of the Widget. When resizesToContent is set, the frameRect of ScrollView (and Widget) is being updated to the new rect value. When resizesToContent is set, qgraphicswebview listens to QWebFrame's contentsSizeChanged signal. In the slot it updates its geometry and also sets QWebPage viewport. In QWebPage::setViewport, the frameRect of ScrollView is set to the new rect value. In the resize handler, this new value is being read. 
> 
> In this scenario do we need to update the QWebPage viewport size?

Also, am I missing something else here?
Comment 24 Dinu Jacob 2010-11-17 08:37:33 PST
(In reply to comment #23)
> (In reply to comment #22)
> > Setting m_actualVisibleContentRect through ScrollView::setActualVisibleContentRect didn't seem to have any impact. The innerWidth and innerHeight is the width and height of the frameRect of the Widget. When resizesToContent is set, the frameRect of ScrollView (and Widget) is being updated to the new rect value. When resizesToContent is set, qgraphicswebview listens to QWebFrame's contentsSizeChanged signal. In the slot it updates its geometry and also sets QWebPage viewport. In QWebPage::setViewport, the frameRect of ScrollView is set to the new rect value. In the resize handler, this new value is being read. 
> > 
> > In this scenario do we need to update the QWebPage viewport size?
> 
> Also, am I missing something else here?

If the viewport size is not set (in which case the frameRect is not updated), it works correctly.
Comment 25 zalan 2010-11-17 08:59:42 PST
as i mentioned in one of my previous comments, the fix is to decouple the logical window size from window.innerHeight/innerWidth and report the actual visible content rect. iphone does exactly this.

int DOMWindow::innerWidth() const
...
    return static_cast<int>(view->actualVisibleContentRect().width() / m_frame->pageZoomFactor());
...
Comment 26 zalan 2010-11-17 12:32:51 PST
Kenneth is refactoring (fixing) 49371 (one of the bug reports that this bug depends on) and the innerHeight/innerWidth fix will be incorporated into that patch. Easier that way.
Comment 27 Kenneth Rohde Christiansen 2010-11-17 13:18:04 PST
(In reply to comment #26)
> Kenneth is refactoring (fixing) 49371 (one of the bug reports that this bug depends on) and the innerHeight/innerWidth fix will be incorporated into that patch. Easier that way.

Yes, me and Kling will land a bunch of minor patches today and tomorrow.

We will need a patch similar to http://trac.webkit.org/changeset/72221 for webkit1 to not break painting with tiling due to these changes.
Comment 28 Dinu Jacob 2010-11-17 14:15:19 PST
I understand that using actualVisibleContentRect will provide the actual visible rect to innerHeight/innerWidth. But, the viewport size of qwebpage will still be updated to the content size. Should the qwebpage viewport size be updated as only the content size is changing here?
Comment 29 Kenneth Rohde Christiansen 2010-11-17 14:18:10 PST
(In reply to comment #28)
> I understand that using actualVisibleContentRect will provide the actual visible rect to innerHeight/innerWidth. But, the viewport size of qwebpage will still be updated to the content size. Should the qwebpage viewport size be updated as only the content size is changing here?

Wait for everything to be upstreamed please.

Basically you need to call setAcualVisibleContentRect when your browser view changes size (for instance on rotation) and when a pan even ends. With that it just basically just work.

At least this works for me with my test browser and webkit2.

But as I said before we need to enabled paintsEntireContents in FrameView when setResizesToContents is called. Someone needs to make a patch for that.
Comment 30 Dinu Jacob 2010-11-17 15:29:53 PST
(In reply to comment #29)
> (In reply to comment #28)
> > I understand that using actualVisibleContentRect will provide the actual visible rect to innerHeight/innerWidth. But, the viewport size of qwebpage will still be updated to the content size. Should the qwebpage viewport size be updated as only the content size is changing here?
> 
> Wait for everything to be upstreamed please.
> 
> Basically you need to call setAcualVisibleContentRect when your browser view changes size (for instance on rotation) and when a pan even ends. With that it just basically just work.
> 
> At least this works for me with my test browser and webkit2.
> 
> But as I said before we need to enabled paintsEntireContents in FrameView when setResizesToContents is called. Someone needs to make a patch for that.

Changes to enabled paintsEntireContents are already in trunk (landed through bug https://bugs.webkit.org/show_bug.cgi?id=49375)
Comment 31 Kenneth Rohde Christiansen 2010-11-17 15:31:02 PST
> Changes to enabled paintsEntireContents are already in trunk (landed through bug https://bugs.webkit.org/show_bug.cgi?id=49375)

That will still need cherry-picking though!
Comment 32 Andreas Kling 2010-11-17 15:53:03 PST
Today's stuff that needs cherry-picking into 2.1:

fbb71f0c9a7797935898c505320ca88c39814256 (not from today, but a prereq)

f10a2e0a45afa77f1646576b9bb1082c05c154d5
f49b9c38ddf842cbfecd589ee3bc63be5ed6b3aa
0aaed612d81fc83bdab8beeb1f2945652b0dd3a1
ec0feeba426f51c3580e0caf91fe6634d1766aac
9a083a2065a107372f60545a5014c58622a6ff8c
Comment 33 Kenneth Rohde Christiansen 2010-11-17 15:55:24 PST
(In reply to comment #32)
> Today's stuff that needs cherry-picking into 2.1:
> 
> fbb71f0c9a7797935898c505320ca88c39814256 (not from today, but a prereq)
> 
> f10a2e0a45afa77f1646576b9bb1082c05c154d5
> f49b9c38ddf842cbfecd589ee3bc63be5ed6b3aa
> 0aaed612d81fc83bdab8beeb1f2945652b0dd3a1
> ec0feeba426f51c3580e0caf91fe6634d1766aac
> 9a083a2065a107372f60545a5014c58622a6ff8c

We also needs this not-yet-upstreamed patch:

http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/commit/5d3a7345562a5f5c6950895ca243c462e157ff36
Comment 34 Andreas Kling 2010-11-18 05:14:39 PST
Moar!

From trunk:

ebf88d993a75c0d04203a13f05474c42c99dd964

From qtwebkit-webkit2-dev:

b53c3975c2461cf29babcae72b55fcf43bdc4f9d
f7cefd7bf6233e686098b9380fda61bf684f7e14 (this is the one Kenneth was referring to)
Comment 35 Nancy Piedra 2010-11-18 06:13:38 PST
Are all these patches already in trunk and webkit1? I'm not sure we want to cherry-pick from qt webkit2 branch to qt webkit 2.1.

Also, has anyone tested the use case described in this bug with all these patches on webkit1?
Comment 36 Ademar Reis 2010-11-19 11:06:29 PST
(In reply to comment #35)
> 
> Also, has anyone tested the use case described in this bug with all these patches on webkit1?

We need this bug fixed on trunk (and therefore tested) before we spend time cherry-picking this (and solving several conflicts along the way) to 2.1.

@Jacob: are you still working on this? Could you make sure the patches are properly integrated to trunk (or at least the respective bugs open) and maybe run a test to make sure these commits together really fix the problem?
Comment 37 Dinu Jacob 2010-11-19 15:19:29 PST
(In reply to comment #36)
> (In reply to comment #35)
> > 
> > Also, has anyone tested the use case described in this bug with all these patches on webkit1?
> 
> We need this bug fixed on trunk (and therefore tested) before we spend time cherry-picking this (and solving several conflicts along the way) to 2.1.
> 
> @Jacob: are you still working on this? Could you make sure the patches are properly integrated to trunk (or at least the respective bugs open) and maybe run a test to make sure these commits together really fix the problem?

I tested the changes on trunk. I included two of the patches that were not trunk and the provided test case works correctly. Also, picked up the relevant changes into 2.1 branch and continuing testing with that (attached test case itself works).
Comment 38 Nancy Piedra 2010-11-19 17:18:03 PST
Created attachment 74448 [details]
Consolidation of various patches from trunk & webkit2 branch

I attached a consolidation of all the changes which seem to fix the problem we were having.  Some of the changes are already on trunk but others were pulled directly from the webkit2 branch.  After discussion with others here in Boston we're not sure this is the right way to go about fixing the problem so please don't cherry-pick to webkit 2.1 yet.
Comment 39 Nancy Piedra 2010-11-23 05:26:37 PST
Created attachment 74650 [details]
Webkit 2.1 changes needed to fix resizesToContents issue

I've attached a file with all the changes needed on Webkit 2.1 to fix the resizesToContents problem.

This patch mostly contains changes already on trunk, accept the following two which are from the webkit2 branch

http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/commit/f7cefd7bf6233e686098b9380fda61bf684f7e14
http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/commit/b53c3975c2461cf29babcae72b55fcf43bdc4f9d

These changes will need to go to trunk before we can cherry-pick to 2.1.
Comment 40 Nancy Piedra 2010-11-23 06:37:36 PST
These are the changes I integrated in addition to the two webkit2 patches I've identified above:

71239
71241
71352
71635
71733
71736
71803
71804
72238
72242
72248
72252
72253
72283
72465
Comment 41 Nancy Piedra 2010-11-30 15:37:36 PST
Created attachment 75219 [details]
Consolidation of patches needed to fix resizesToContents for Webkit 2.1 branch

I've attached a patch for Qt Webkit 2.1 that contains all the changes needed to fix this issue.

This patch is meant to be applied to Qt Webkit 2.1 branch.
Comment 42 Ademar Reis 2010-12-01 07:20:26 PST
Created attachment 75270 [details]
v2 of the consolidation of patches for qtwebkit-2.1

Original patch from Nancy with the following changes:
  - Coding style fixes
  - Removed a debug statement
  - Added changelog

Since this is a relatively complex patch (includes an API change, touches a lot of files and includes changes not on trunk), I'm asking for a review before I push it to 2.1.

We also need to update the symbian .def files, I'm trying to find someone do help me on that.
Comment 43 Kenneth Rohde Christiansen 2010-12-01 07:24:35 PST
Comment on attachment 75270 [details]
v2 of the consolidation of patches for qtwebkit-2.1

View in context: https://bugs.webkit.org/attachment.cgi?id=75270&action=review

Looks fine. maybe you want some scrolling tests as well. Or at least make sure that works.

> WebCore/page/Chrome.cpp:93
> +m_client->delegatedScrollRequested(scrollDelta);

missing indentation

> WebCore/page/FrameView.cpp:353
> +bool FrameView::delegatesScrolling() const
> +{
> +    ASSERT(m_frame);
> +
> +    if (parent())
> +         return false;
> +
> +    return m_frame->settings() && m_frame->settings()->shouldDelegateScrolling();
> +}
> +
> +bool FrameView::avoidScrollbarCreation() const

This code was changed in another patch on trunk... it is not WebCore setting anymore. you can pick that or leave it

> WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:412
> +emit m_webPage->scrollRequested(delta.width(), delta.height(), QRect(QPoint(0, 0), m_webPage->viewportSize()));

misses indentation
Comment 44 Ademar Reis 2010-12-01 07:38:02 PST
Created attachment 75273 [details]
v3 of the consolidation of patches for qtwebkit-2.1

More coding-style fixes + update on the symbian .def file
Comment 45 Nancy Piedra 2010-12-02 06:29:24 PST
I found an issue with this change when resizesToContents is not enabled.

(Not because of Ademar's port but some problem with the original change).

I want to debug further before submitting that patch to Webkit 2.1.
Comment 46 Yael 2010-12-02 13:21:03 PST
(In reply to comment #45)
> I found an issue with this change when resizesToContents is not enabled.
> 
> (Not because of Ademar's port but some problem with the original change).
> 
> I want to debug further before submitting that patch to Webkit 2.1.

The bug is in FrameLoaderClientQt.cpp.
In FrameLoaderClientQt::transitionToCommittedForNewPage(), we should change 

m_frame->view()->setActualVisibleContentRect(IntRect(IntPoint::zero(), currentVisibleContentSize));
to 
    if (m_frame->view()->paintsEntireContents())
        m_frame->view()->setActualVisibleContentRect(IntRect(IntPoint::zero(), currentVisibleContentSize));
    else
        m_frame->view()->setActualVisibleContentRect(IntRect());

And the bug goes away. 

I don't have the complete WebKit 2.1 environment to generate a new patch, but I hope you got the idea :-)
Comment 47 Ademar Reis 2010-12-02 13:27:09 PST
(In reply to comment #46)
> (In reply to comment #45)
> > I found an issue with this change when resizesToContents is not enabled.
> > 
> > (Not because of Ademar's port but some problem with the original change).
> > 
> > I want to debug further before submitting that patch to Webkit 2.1.
> 
> The bug is in FrameLoaderClientQt.cpp.
> In FrameLoaderClientQt::transitionToCommittedForNewPage(), we should change 
> 
> m_frame->view()->setActualVisibleContentRect(IntRect(IntPoint::zero(), currentVisibleContentSize));
> to 
>     if (m_frame->view()->paintsEntireContents())
>         m_frame->view()->setActualVisibleContentRect(IntRect(IntPoint::zero(), currentVisibleContentSize));
>     else
>         m_frame->view()->setActualVisibleContentRect(IntRect());
> 
> And the bug goes away. 
> 
> I don't have the complete WebKit 2.1 environment to generate a new patch, but I hope you got the idea :-)

OK, I'm prepare a new patch and push it to a test branch that already is set up. If nothing wrong is found, this fix will be part of the weekly release made on mondays. Thanks.
Comment 48 Ademar Reis 2010-12-02 13:43:17 PST
Created attachment 75411 [details]
v4 of the consolidation of patches for qtwebkit-2.1

New patch with the latest update from Yael (see previous bug comments)

The diff from the previous patch is:

diff --git a/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp b/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
index 275acaa..9426bdc 100644
--- a/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
+++ b/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
@@ -279,7 +279,10 @@ void FrameLoaderClientQt::transitionToCommittedForNewPage()
         m_frame->view()->setPaintsEntireContents(page->d->client->viewResizesToContentsEnabled());
 
     // The HistoryController will update the scroll position later if needed.
-    m_frame->view()->setActualVisibleContentRect(IntRect(IntPoint::zero(), currentVisibleContentSize));
+    if (m_frame->view()->paintsEntireContents())
+        m_frame->view()->setActualVisibleContentRect(IntRect(IntPoint::zero(), currentVisibleContentSize));
+    else
+        m_frame->view()->setActualVisibleContentRect(IntRect());
 }
Comment 49 Ademar Reis 2010-12-07 11:18:56 PST
patch v4 has been pushed to qtwebkit-2.1 as http://gitorious.org/webkit/qtwebkit/commit/e0e3111
Comment 50 Eric Seidel 2010-12-14 01:24:29 PST
Is this patch to be landed?
Comment 51 Eric Seidel 2010-12-14 01:24:50 PST
Comment on attachment 75270 [details]
v2 of the consolidation of patches for qtwebkit-2.1

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 75270 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 52 Ademar Reis 2010-12-16 10:49:20 PST
(In reply to comment #50)
> Is this patch to be landed?

It should (some form of it anyway). Blocking bug 32653 (that keeps track of bugs fixed on qtwebkit releases but not on trunk).
Comment 53 Yael 2011-01-07 13:49:07 PST
This infinite resize can still be seen in latest Symbian build.
Load maps.yahoo.com, and observe the infinite loop.
Loading the same site without resize to content loads fine.
I will attach a callstack of the infinite recursion in https://bsetpl02.americas.nokia.com/browse/BR-5861
Comment 54 Benjamin Poulain 2011-01-07 14:04:23 PST
(In reply to comment #53)
> I will attach a callstack of the infinite recursion in https://bsetpl02.americas.nokia.com/browse/BR-5861

Any chance you make an autotest from your testcase?

The solution only works if setActualVisibleContentRect(const QRect& rect) is used correctly. A few auto test are necessary here in my opinion.
Comment 55 Yael 2011-01-07 14:18:50 PST
(In reply to comment #54)
> (In reply to comment #53)
> > I will attach a callstack of the infinite recursion in https://bsetpl02.americas.nokia.com/browse/BR-5861
> 
> Any chance you make an autotest from your testcase?
> 
> The solution only works if setActualVisibleContentRect(const QRect& rect) is used correctly. A few auto test are necessary here in my opinion.

My test case is maps.yahoo.com. Sorry, but I don't have a reduction for it ATM.
All you need to do is configure QtTestBrowser to resizesToContent and load that page.
Comment 56 Yael 2011-01-07 14:19:34 PST
(In reply to comment #55)

> All you need to do is configure QtTestBrowser to resizesToContent and load that page.

On Symbian.
Comment 57 Benjamin Poulain 2011-01-07 15:28:03 PST
(In reply to comment #55)
> > Any chance you make an autotest from your testcase?
> > 
> > The solution only works if setActualVisibleContentRect(const QRect& rect) is used correctly. A few auto test are necessary here in my opinion.
> 
> My test case is maps.yahoo.com. Sorry, but I don't have a reduction for it ATM.
> All you need to do is configure QtTestBrowser to resizesToContent and load that page.

I can have missunderstood but I think opening a page with resize to content will always fail, even with the patch.

Kenneth, correct me if I am wrong, but shouldn't the test case set actualVisibleContentRect() to be valid?
Comment 58 Kenneth Rohde Christiansen 2011-01-07 15:37:49 PST
(In reply to comment #57)
> (In reply to comment #55)
> > > Any chance you make an autotest from your testcase?
> > > 
> > > The solution only works if setActualVisibleContentRect(const QRect& rect) is used correctly. A few auto test are necessary here in my opinion.
> > 
> > My test case is maps.yahoo.com. Sorry, but I don't have a reduction for it ATM.
> > All you need to do is configure QtTestBrowser to resizesToContent and load that page.
> 
> I can have missunderstood but I think opening a page with resize to content will always fail, even with the patch.
> 
> Kenneth, correct me if I am wrong, but shouldn't the test case set actualVisibleContentRect() to be valid?

It should, indeed.
Comment 59 Yael 2011-01-07 16:30:19 PST
(In reply to comment #58)
> (In reply to comment #57)
> > (In reply to comment #55)
> > > > Any chance you make an autotest from your testcase?
> > > > 
> > > > The solution only works if setActualVisibleContentRect(const QRect& rect) is used correctly. A few auto test are necessary here in my opinion.
> > > 
> > > My test case is maps.yahoo.com. Sorry, but I don't have a reduction for it ATM.
> > > All you need to do is configure QtTestBrowser to resizesToContent and load that page.
> > 
> > I can have missunderstood but I think opening a page with resize to content will always fail, even with the patch.
> > 
> > Kenneth, correct me if I am wrong, but shouldn't the test case set actualVisibleContentRect() to be valid?
> 
> It should, indeed.

I am not super familliar with resizeToContent, by I assume that QtTestBrowser is configured correctly, no?
Comment 60 Kenneth Rohde Christiansen 2011-01-08 02:17:58 PST
(In reply to comment #59)
> (In reply to comment #58)
> > (In reply to comment #57)
> > > (In reply to comment #55)
> > > > > Any chance you make an autotest from your testcase?
> > > > > 
> > > > > The solution only works if setActualVisibleContentRect(const QRect& rect) is used correctly. A few auto test are necessary here in my opinion.
> > > > 
> > > > My test case is maps.yahoo.com. Sorry, but I don't have a reduction for it ATM.
> > > > All you need to do is configure QtTestBrowser to resizesToContent and load that page.
> > > 
> > > I can have missunderstood but I think opening a page with resize to content will always fail, even with the patch.
> > > 
> > > Kenneth, correct me if I am wrong, but shouldn't the test case set actualVisibleContentRect() to be valid?
> > 
> > It should, indeed.
> 
> I am not super familliar with resizeToContent, by I assume that QtTestBrowser is configured correctly, no?

It is not. Every time the view is (re)sized or a pinch or panning animation has ended, we need to call setActualVisibleContentsRect to the area we are looking at. We are not doing that at all.

That is the only way for WebCore to know where we are on the page as we are delegating scrolling etc to the application.
Comment 61 Kenneth Rohde Christiansen 2011-01-08 02:18:49 PST
> > I am not super familliar with resizeToContent, by I assume that QtTestBrowser is configured correctly, no?
> 
> It is not. Every time the view is (re)sized or a pinch or panning animation has ended, we need to call setActualVisibleContentsRect to the area we are looking at. We are not doing that at all.
> 
> That is the only way for WebCore to know where we are on the page as we are delegating scrolling etc to the application.

Btw, if you go fix this, please update the documentation as well.
Comment 62 Yael 2011-01-10 13:05:45 PST
Thanks for your advice. I was asked to work on a high priority crash and will get back to this as soon as I can.
Comment 63 Yael 2011-01-19 17:32:49 PST
On the trunk of webkit.org, I can load maps.yahoo.com with no problems, even with resizedToContent turned on. On webkit2.1 branch, it either crashes or takes a very long time.
One reason could be that Nancy's patch is only in WebKit 2.1.
I did notice that the page asks repeatedly for the size of the HTML element, and resizes itself.
Comment 64 Yael 2011-01-22 14:28:15 PST
The patch in https://bugs.webkit.org/show_bug.cgi?id=52449 fixed the crash on Symbian, so I guess it was not a resizeToContent issue after all. Sorry :)
Comment 65 Viatcheslav Ostapenko 2011-02-06 14:46:47 PST
*** Bug 42754 has been marked as a duplicate of this bug. ***
Comment 66 Alexis Menard (darktears) 2011-04-07 12:54:07 PDT
(In reply to comment #64)
> The patch in https://bugs.webkit.org/show_bug.cgi?id=52449 fixed the crash on Symbian, so I guess it was not a resizeToContent issue after all. Sorry :)


So what's the conclusion? Should we close that bug? I'm not sure to get all what you said.
Comment 67 Nancy Piedra 2011-04-08 05:22:25 PDT
I will check to make sure the original use case is no longer reproducible and then we can probably close this bug.
Comment 68 Nancy Piedra 2011-04-12 05:45:56 PDT
On trunk, the original use case seems to be no longer reproducible.  I set the bug to RESOLVED-WORKSFORME.
Comment 69 Benjamin Poulain 2011-04-12 05:56:19 PDT
I reopen.
This bug is not closed until there is an Qt autotest for the use case.
Comment 70 Nancy Piedra 2011-04-12 12:45:44 PDT
I will create a test.
Comment 71 Nancy Piedra 2011-04-14 05:02:10 PDT
Actually, I'm able to reproduce this now.  Will look into it.
Comment 72 Nancy Piedra 2011-04-14 06:59:36 PDT
This bug is still reproducible with the test case that Benjamin had attached on 2010-11-10.

I reproduced it by launching QtTestBrowser like this:
QtTestBrowser -resizes-to-contents -graphicsbased <name of test file>

After speaking with Yael & looking at the changes on qtwebkit2 branch, I don't feel I am equipped to try and fix this issue.

Yael said she is going to be looking at this issue, so I am assigning to her.

Also, I tried to create a Qt API test case to reproduce this but for some reason I could only reproduce the problem in the QtTestBrowser and not my small test case.
Comment 73 Alexis Menard (darktears) 2011-05-10 14:47:48 PDT
(In reply to comment #72)
> This bug is still reproducible with the test case that Benjamin had attached on 2010-11-10.
> 
> I reproduced it by launching QtTestBrowser like this:
> QtTestBrowser -resizes-to-contents -graphicsbased <name of test file>
> 
> After speaking with Yael & looking at the changes on qtwebkit2 branch, I don't feel I am equipped to try and fix this issue.
> 
> Yael said she is going to be looking at this issue, so I am assigning to her.
> 
> Also, I tried to create a Qt API test case to reproduce this but for some reason I could only reproduce the problem in the QtTestBrowser and not my small test case.

Any news on that?
Comment 74 Yael 2011-05-11 15:01:27 PDT
(In reply to comment #73)
> (In reply to comment #72)
> > This bug is still reproducible with the test case that Benjamin had attached on 2010-11-10.
> > 
> > I reproduced it by launching QtTestBrowser like this:
> > QtTestBrowser -resizes-to-contents -graphicsbased <name of test file>
> > 
> > After speaking with Yael & looking at the changes on qtwebkit2 branch, I don't feel I am equipped to try and fix this issue.
> > 
> > Yael said she is going to be looking at this issue, so I am assigning to her.
> > 
> > Also, I tried to create a Qt API test case to reproduce this but for some reason I could only reproduce the problem in the QtTestBrowser and not my small test case.
> 
> Any news on that?

This should be fixed once we upstream the patches from the WebKit2 development branch.
Comment 75 Alexis Menard (darktears) 2011-05-11 15:20:50 PDT
(In reply to comment #74)
> (In reply to comment #73)
> > (In reply to comment #72)
> > > This bug is still reproducible with the test case that Benjamin had attached on 2010-11-10.
> > > 
> > > I reproduced it by launching QtTestBrowser like this:
> > > QtTestBrowser -resizes-to-contents -graphicsbased <name of test file>
> > > 
> > > After speaking with Yael & looking at the changes on qtwebkit2 branch, I don't feel I am equipped to try and fix this issue.
> > > 
> > > Yael said she is going to be looking at this issue, so I am assigning to her.
> > > 
> > > Also, I tried to create a Qt API test case to reproduce this but for some reason I could only reproduce the problem in the QtTestBrowser and not my small test case.
> > 
> > Any news on that?
> 
> This should be fixed once we upstream the patches from the WebKit2 development branch.

It's marked as blocker for 2.2 so perhaps we could upstream that particular patch no? Otherwise we just remove it from 2.2 blocker but I think it's not nice.
Comment 76 Laszlo Gombos 2011-05-21 15:50:59 PDT
Comment on attachment 75411 [details]
v4 of the consolidation of patches for qtwebkit-2.1

Cleared Kenneth Rohde Christiansen's review+ from attachment 75411 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 77 Yael 2011-06-13 11:10:16 PDT
Created attachment 96977 [details]
Patch.

Rebase the latest patch so that it can be applied to QtWebKit 2.2.
I noticed a few small issues, all of them exist also in QtWebKit 2.1.
Comment 78 Ademar Reis 2011-06-14 06:36:48 PDT
(In reply to comment #77)
> Created an attachment (id=96977) [details]
> Patch.
> 
> Rebase the latest patch so that it can be applied to QtWebKit 2.2.
> I noticed a few small issues, all of them exist also in QtWebKit 2.1.

Please tell us what are these small issues so that we can consider them expected behavior when testing this patch.
Comment 79 Yael 2011-06-14 09:42:28 PDT
(In reply to comment #78)
> (In reply to comment #77)
> > Created an attachment (id=96977) [details] [details]
> > Patch.
> > 
> > Rebase the latest patch so that it can be applied to QtWebKit 2.2.
> > I noticed a few small issues, all of them exist also in QtWebKit 2.1.
> Please tell us what are these small issues so that we can consider them expected behavior when testing this patch.

Issue #1:
1. Load www.cnn.com
2. Switch to QGraphicsWebView
3. Enable resizeToContent and tiling.
4. Scroll halfway down.
5. Scroll to the right. 
6. You will see a gray area at the right side, instead of the content of the page.

Issue #2:
1. Switch to QGraphicsWebView
2. Enable resizeToContent and tiling.
3. Load www.cnn.com.
4. Scroll down.
5. Load another page.
6. The scroll position does not reset to 0.

Issue #3:
1. Switch to QGraphicsWebView
2. Enable resizeToContent and tiling.
3. Load www.aldaily.com
4. Load the test page from Bengamin (attached here)
5. You will observe that the size reported is the same as the size of www.aldaily.com. 

If I remember more issues, I will report them here.
Comment 80 Luiz Agostini 2011-06-29 17:20:09 PDT
The problem with the test case is that it does not consider that the <body> element has 8px margins by default. The full content size is then composed by the body content size plus body margins.

Here is how the infinite loop works:

1 - content is resized
2 - window is resized to the new content size due to resizeToContent
3 - window resize event is called and fullScreenDiv is resized to the new window size
4 - the content size changes to fullScreenDiv size plus body margins and we get back to 1

Every time we resize the fullScreenDiv to the window size, the window size is increased by the body margins and we get the infinite loop! It seems that the problem is in the html and not in QtWebKit. Infinite loop is the correct behavior for that html.

If we add the following lines to the test case then the infinite loop goes away:

<style>
body { margin: 0px; }
</style>

I have not been able to reproduce the problem with any of the other urls that were mentioned in the comments.

I think we could close this bug.
Comment 81 Yael 2011-06-30 05:38:54 PDT
(In reply to comment #80)
> I have not been able to reproduce the problem with any of the other urls that were mentioned in the comments.
> 
I am able to reproduce the error with http://ie.microsoft.com/testdrive/Performance/FishIE%20tank/Default.html that is mentioned above. (I tested trunk, not the QtWebKit 2.2 branch).
> I think we could close this bug.
I don't think it is valid to change the test case so that it will pass, instead of fixing the issue.
Comment 82 Luiz Agostini 2011-06-30 06:01:42 PDT
(In reply to comment #81)
> (In reply to comment #80)
> > I have not been able to reproduce the problem with any of the other urls that were mentioned in the comments.
> > 
> I am able to reproduce the error with http://ie.microsoft.com/testdrive/Performance/FishIE%20tank/Default.html that is mentioned above. (I tested trunk, not the QtWebKit 2.2 branch).

I will take a look.

> > I think we could close this bug.
> I don't think it is valid to change the test case so that it will pass, instead of fixing the issue.

I am not talking about changing the test case. I am talking about dropping it completely because with resizeToContents the test case contents size is supposed to increase with no limit. The content size increase seen in the test case is not caused by a bug in QtWebKit. Please consider the explanation that I gave in my previous comment.
Comment 83 Yael 2011-06-30 06:35:19 PDT
(In reply to comment #82)
> (In reply to comment #81)
> > (In reply to comment #80)
> > > I have not been able to reproduce the problem with any of the other urls that were mentioned in the comments.
> > > 
> > I am able to reproduce the error with http://ie.microsoft.com/testdrive/Performance/FishIE%20tank/Default.html that is mentioned above. (I tested trunk, not the QtWebKit 2.2 branch).
> 
> I will take a look.
> 
> > > I think we could close this bug.
> > I don't think it is valid to change the test case so that it will pass, instead of fixing the issue.
> 
> I am not talking about changing the test case. I am talking about dropping it completely because with resizeToContents the test case contents size is supposed to increase with no limit. The content size increase seen in the test case is not caused by a bug in QtWebKit. Please consider the explanation that I gave in my previous comment.

I understand the technical details of your explanation, but I think that from a user's point of view, we should not be resizing QGraphicsWebView endlessly.
Comment 84 Luiz Agostini 2011-06-30 07:56:15 PDT
(In reply to comment #83)
> (In reply to comment #82)
> > (In reply to comment #81)
> > > (In reply to comment #80)
> > > > I have not been able to reproduce the problem with any of the other urls that were mentioned in the comments.
> > > > 
> > > I am able to reproduce the error with http://ie.microsoft.com/testdrive/Performance/FishIE%20tank/Default.html that is mentioned above. (I tested trunk, not the QtWebKit 2.2 branch).
> > 
> > I will take a look.
> > 
> > > > I think we could close this bug.
> > > I don't think it is valid to change the test case so that it will pass, instead of fixing the issue.
> > 
> > I am not talking about changing the test case. I am talking about dropping it completely because with resizeToContents the test case contents size is supposed to increase with no limit. The content size increase seen in the test case is not caused by a bug in QtWebKit. Please consider the explanation that I gave in my previous comment.
> 
> I understand the technical details of your explanation, but I think that from a user's point of view, we should not be resizing QGraphicsWebView endlessly.

resizeToContents means resize the QGraphicsWebView every time the contents of the page is resized, right?
What should we do when the javascript in the page is resizing its contents endlessly?
Comment 85 Alexis Menard (darktears) 2011-06-30 08:02:25 PDT
(In reply to comment #84)
> (In reply to comment #83)
> > (In reply to comment #82)
> > > (In reply to comment #81)
> > > > (In reply to comment #80)
> > > > > I have not been able to reproduce the problem with any of the other urls that were mentioned in the comments.
> > > > > 
> > > > I am able to reproduce the error with http://ie.microsoft.com/testdrive/Performance/FishIE%20tank/Default.html that is mentioned above. (I tested trunk, not the QtWebKit 2.2 branch).
> > > 
> > > I will take a look.
> > > 
> > > > > I think we could close this bug.
> > > > I don't think it is valid to change the test case so that it will pass, instead of fixing the issue.
> > > 
> > > I am not talking about changing the test case. I am talking about dropping it completely because with resizeToContents the test case contents size is supposed to increase with no limit. The content size increase seen in the test case is not caused by a bug in QtWebKit. Please consider the explanation that I gave in my previous comment.
> > 
> > I understand the technical details of your explanation, but I think that from a user's point of view, we should not be resizing QGraphicsWebView endlessly.
> 
> resizeToContents means resize the QGraphicsWebView every time the contents of the page is resized, right?
> What should we do when the javascript in the page is resizing its contents endlessly?

I tend to agree a bit with Luiz. You can never prevent bad coding even though what the end user see is bad. Is this way of hacking website common? If not we should probably contact the author.
Comment 86 Yael 2011-06-30 12:12:51 PDT
(In reply to comment #85)
> I tend to agree a bit with Luiz. You can never prevent bad coding even though what the end user see is bad. Is this way of hacking website common? If not we should probably contact the author.

But what about comment #69 ?
Comment 87 Benjamin Poulain 2011-06-30 13:16:14 PDT
This: https://bugs.webkit.org/attachment.cgi?id=73479 should work. If only because it works fine with regular layout on Desktop. We should not have infinite resize on mobile, that would break the expectation that mobile works mostly the same way.

And yep, if there is no test case, one should be made in API tests in my opinion :-D
Comment 88 Luiz Agostini 2011-07-02 17:40:47 PDT
Created attachment 99566 [details]
patch
Comment 89 Kenneth Rohde Christiansen 2011-07-02 17:49:59 PDT
Comment on attachment 99566 [details]
patch

Me and Zalan spent considerable amounts of time getting this right for the N9 browser. Please have a look at that code to see if you are diverting. This needs to be right :-)
Comment 90 Kenneth Rohde Christiansen 2011-07-02 18:04:44 PDT
Comment on attachment 99566 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99566&action=review

Zalan definately needs to look at this as well

> Source/WebCore/ChangeLog:29
> +        The method ScrollView::updateScrollbars were changed to add/remove the scrollbars based
> +        on the value returned by delegatesScrolling().

Isn't this a separate thing?

> Source/WebCore/page/FrameView.cpp:-2075
> -        IntSize currentSize = IntSize(width(), height());

What is the difference between IntSize and LayoutSize? LayoutSize is new?

> Source/WebCore/page/FrameView.cpp:2091
> +void FrameView::setActualVisibleContentRect(const IntRect& actualVisibleContentRect)

On our branch we have this in ScrollView... I wonder where makes the most sense.

> Source/WebCore/page/FrameView.cpp:2094
> +    sendResizeEventIfNeeded();

hmm, why do you need to send resize events here? We are updating the actualvisible contents rect quite a lot and this shouldnt change the size. Also I guess you are only interested in this for the mainframe right?

> Source/WebCore/platform/ScrollView.cpp:478
> -    if (m_scrollbarsSuppressed || (hScroll != ScrollbarAuto && vScroll != ScrollbarAuto)) {
> +    if (delegatesScrolling()) {
> +        newHasHorizontalScrollbar = false;
> +        newHasVerticalScrollbar = false;
> +    }

couldnt this be done earlier in the method? Like if you delegate scrolliong you are likely to never have scrollbars and we would like to avoid computing scrollbars at all

> Tools/QtTestBrowser/webview.cpp:233
> +void WebViewGraphicsBased::scrollContentsBy(int dx, int dy)
> +{
> +    QGraphicsView::scrollContentsBy(dx, dy);
> +    if (m_resizesToContents)
> +        m_item->updateActualVisibleContentRect();
> +}

you dont support zooming? If we do then we would need to update the visible content rect when that changes as well
Comment 91 Kenneth Rohde Christiansen 2011-07-02 18:20:50 PDT
Comment on attachment 99566 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99566&action=review

> Source/WebCore/platform/ScrollView.h:147
> -    IntRect actualVisibleContentRect() const { return m_actualVisibleContentRect.isEmpty() ? visibleContentRect() : m_actualVisibleContentRect; }
> -    void setActualVisibleContentRect(const IntRect& actualVisibleContentRect) { m_actualVisibleContentRect = actualVisibleContentRect; }
> +    IntRect actualVisibleContentRect() const { return m_actualVisibleContentRect; }
> +    virtual void setActualVisibleContentRect(const IntRect& actualVisibleContentRect) { m_actualVisibleContentRect = actualVisibleContentRect; }

Im a bit afraid of this change. We are using actualVisibleContentRect so many places on our branch... you will need to check that you are not breaking any of these cases.
Comment 92 Luiz Agostini 2011-07-02 18:48:14 PDT
(In reply to comment #90)
> (From update of attachment 99566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99566&action=review
> 
> Zalan definately needs to look at this as well
> 
> > Source/WebCore/ChangeLog:29
> > +        The method ScrollView::updateScrollbars were changed to add/remove the scrollbars based
> > +        on the value returned by delegatesScrolling().
> 
> Isn't this a separate thing?

The API allows you to turn resizeToContents on/off. If you do you will see that this issue becomes very annoying. 

> 
> > Source/WebCore/page/FrameView.cpp:-2075
> > -        IntSize currentSize = IntSize(width(), height());
> 
> What is the difference between IntSize and LayoutSize? LayoutSize is new?

typedef IntSize LayoutSize;
It came after a rebase today.

> 
> > Source/WebCore/page/FrameView.cpp:2091
> > +void FrameView::setActualVisibleContentRect(const IntRect& actualVisibleContentRect)
> 
> On our branch we have this in ScrollView... I wonder where makes the most sense.

It is in ScrollView. I made it virtual and overrode it in FrameView to handle resize events.

> 
> > Source/WebCore/page/FrameView.cpp:2094
> > +    sendResizeEventIfNeeded();
> 
> hmm, why do you need to send resize events here? We are updating the actualvisible contents rect quite a lot and this shouldnt change the size. Also I guess you are only interested in this for the mainframe right?

The resize event will be sent only if the dimensions have changed. It is needed because we need to send the event when the view (QGraphicsView) dimensions have changed. 

> 
> > Source/WebCore/platform/ScrollView.cpp:478
> > -    if (m_scrollbarsSuppressed || (hScroll != ScrollbarAuto && vScroll != ScrollbarAuto)) {
> > +    if (delegatesScrolling()) {
> > +        newHasHorizontalScrollbar = false;
> > +        newHasVerticalScrollbar = false;
> > +    }
> 
> couldnt this be done earlier in the method? Like if you delegate scrolliong you are likely to never have scrollbars and we would like to avoid computing scrollbars at all

The problem again is that it is possible, by the api, to turn scroll delegation (or resizeToContents) on/off. Then the rendering gets really weird.

> 
> > Tools/QtTestBrowser/webview.cpp:233
> > +void WebViewGraphicsBased::scrollContentsBy(int dx, int dy)
> > +{
> > +    QGraphicsView::scrollContentsBy(dx, dy);
> > +    if (m_resizesToContents)
> > +        m_item->updateActualVisibleContentRect();
> > +}
> 
> you dont support zooming? If we do then we would need to update the visible content rect when that changes as well

I see.
Comment 93 Luiz Agostini 2011-07-02 18:53:40 PDT
(In reply to comment #91)
> (From update of attachment 99566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99566&action=review
> 
> > Source/WebCore/platform/ScrollView.h:147
> > -    IntRect actualVisibleContentRect() const { return m_actualVisibleContentRect.isEmpty() ? visibleContentRect() : m_actualVisibleContentRect; }
> > -    void setActualVisibleContentRect(const IntRect& actualVisibleContentRect) { m_actualVisibleContentRect = actualVisibleContentRect; }
> > +    IntRect actualVisibleContentRect() const { return m_actualVisibleContentRect; }
> > +    virtual void setActualVisibleContentRect(const IntRect& actualVisibleContentRect) { m_actualVisibleContentRect = actualVisibleContentRect; }
> 
> Im a bit afraid of this change. We are using actualVisibleContentRect so many places on our branch... you will need to check that you are not breaking any of these cases.

I was supposing that the use of actualVisibleContentRect was mainly in the branch. In the trunk I just found one use of it and it was broken. To solve it I made this change.

Without this change the users of ScrollView cannot get back the value supplied to setActualVisibleContentRect. It was causing problems to FrameLoaderClientQt::transitionToCommittedForNewPage().
Comment 94 Kenneth Rohde Christiansen 2011-07-03 03:45:51 PDT
> > > Source/WebCore/ChangeLog:29
> > > +        The method ScrollView::updateScrollbars were changed to add/remove the scrollbars based
> > > +        on the value returned by delegatesScrolling().
> > 
> > Isn't this a separate thing?
> 
> The API allows you to turn resizeToContents on/off. If you do you will see that this issue becomes very annoying.

For WebKit1 yes, but we will not support this in Qt WebKit going forward, and if is makes the mobile case slightly slower that is a bad thing. Thus lets try to avoid as much scrollbar overhead as possible. If there is no overhead (we had that before) then I am ok with this change.

> 
> > 
> > > Source/WebCore/page/FrameView.cpp:-2075
> > > -        IntSize currentSize = IntSize(width(), height());
> > 
> > What is the difference between IntSize and LayoutSize? LayoutSize is new?
> 
> typedef IntSize LayoutSize;
> It came after a rebase today.

Aha, interesting


> The resize event will be sent only if the dimensions have changed. It is needed because we need to send the event when the view (QGraphicsView) dimensions have changed. 

But that has nothing to do with the visibleContentRect as such, and just like above unlikely on mobile platforms where we cannot affort overhead in often called methods. On our branch we have a setActualViewportSize - that is what you want to use for this!

> > > Source/WebCore/platform/ScrollView.cpp:478
> > > -    if (m_scrollbarsSuppressed || (hScroll != ScrollbarAuto && vScroll != ScrollbarAuto)) {
> > > +    if (delegatesScrolling()) {
> > > +        newHasHorizontalScrollbar = false;
> > > +        newHasVerticalScrollbar = false;
> > > +    }
> > 
> > couldnt this be done earlier in the method? Like if you delegate scrolliong you are likely to never have scrollbars and we would like to avoid computing scrollbars at all
> 
> The problem again is that it is possible, by the api, to turn scroll delegation (or resizeToContents) on/off. Then the rendering gets really weird.

Won't be possible with our future API, but ok.
Comment 95 Ademar Reis 2011-07-04 10:44:59 PDT
(In reply to comment #91)

[snip]

> Im a bit afraid of this change. We are using actualVisibleContentRect so many places on our branch... you will need to check that you are not breaking any of these cases.

I think we should not limit ourselves because of some potential incompatibility with "the branch". The reality at this point is trunk and 2.2 which is about to be released. The burden of testing/rebasing should be on whoever is working on the branch, not on trunk.

Just my two cents.
Comment 96 Luiz Agostini 2011-07-04 11:24:29 PDT
(In reply to comment #94)
> > > > Source/WebCore/ChangeLog:29
> > > > +        The method ScrollView::updateScrollbars were changed to add/remove the scrollbars based
> > > > +        on the value returned by delegatesScrolling().
> > > 
> > > Isn't this a separate thing?
> > 
> > The API allows you to turn resizeToContents on/off. If you do you will see that this issue becomes very annoying.
> 
> For WebKit1 yes, but we will not support this in Qt WebKit going forward, and if is makes the mobile case slightly slower that is a bad thing. Thus lets try to avoid as much scrollbar overhead as possible. If there is no overhead (we had that before) then I am ok with this change.
> 
> > 
> > > 
> > > > Source/WebCore/page/FrameView.cpp:-2075
> > > > -        IntSize currentSize = IntSize(width(), height());
> > > 
> > > What is the difference between IntSize and LayoutSize? LayoutSize is new?
> > 
> > typedef IntSize LayoutSize;
> > It came after a rebase today.
> 
> Aha, interesting
> 
> 
> > The resize event will be sent only if the dimensions have changed. It is needed because we need to send the event when the view (QGraphicsView) dimensions have changed. 
> 
> But that has nothing to do with the visibleContentRect as such, and just like above unlikely on mobile platforms where we cannot affort overhead in often called methods. On our branch we have a setActualViewportSize - that is what you want to use for this!
> 
> > > > Source/WebCore/platform/ScrollView.cpp:478
> > > > -    if (m_scrollbarsSuppressed || (hScroll != ScrollbarAuto && vScroll != ScrollbarAuto)) {
> > > > +    if (delegatesScrolling()) {
> > > > +        newHasHorizontalScrollbar = false;
> > > > +        newHasVerticalScrollbar = false;
> > > > +    }
> > > 
> > > couldnt this be done earlier in the method? Like if you delegate scrolliong you are likely to never have scrollbars and we would like to avoid computing scrollbars at all
> > 
> > The problem again is that it is possible, by the api, to turn scroll delegation (or resizeToContents) on/off. Then the rendering gets really weird.
> 
> Won't be possible with our future API, but ok.

delegatesScrolling, setDelegatesScrolling, actualVisibleContentRect and setActualVisibleContentRect for example are all public methods of ScrollView. Any other port may freely use them or even modify them if they consider that it is needed.

IMHO to avoid performance overhead on a certain branch/product by not fully implementing a feature in trunk will not work.
Comment 97 Kenneth Rohde Christiansen 2011-07-04 12:37:16 PDT
> delegatesScrolling, setDelegatesScrolling, actualVisibleContentRect and setActualVisibleContentRect for example are all public methods of ScrollView. Any other port may freely use them or even modify them if they consider that it is needed.
> 
> IMHO to avoid performance overhead on a certain branch/product by not fully implementing a feature in trunk will not work.

True, but you know as well as I do, that the branch was not created as a result of not wanting to work on trunk, it was done due to time constraints and a rapidly changing webkit2. That said, the future of QtWebKit is based on WebKit2 and there are plans to upstream everything on the branch as soon as we are ready with the final software for the N9 (and the well deserved vacation period ends). That means that if we do it in a way that makes it too complicated for us to use trunk as a replacement for the branch, then we will just end up with yet another branch for the successive releases which is bad.

So I am not reluctant to getting this fixed on trunk, all I am doing is asking to put a bit more work into it, so that it actually serves our needs for the N9 and similar future products. We've spent considerable time fixing many, many issues related to viewport handling, and we simple cannot afford doing this once again.
Comment 98 Kenneth Rohde Christiansen 2011-07-04 12:47:07 PDT
> I think we should not limit ourselves because of some potential incompatibility with "the branch". The reality at this point is trunk and 2.2 which is about to be released. The burden of testing/rebasing should be on whoever is working on the branch, not on trunk.
> 
> Just my two cents.

Then you are not seeing the big picture.

The branch is a proof of what we as a company need trunk to be. Not working towards that goal is IMHO catastrophic, if you consider how few resources we are having.

If you don't have the time to do it properly, fix it on your QtWebKit2.2 branch. But remember, QtWebKit2.2 is a one time thing only, WebKit2/Mobile is where our future lies.
Comment 99 Kenneth Rohde Christiansen 2011-07-04 12:53:39 PDT
> delegatesScrolling, setDelegatesScrolling, actualVisibleContentRect and setActualVisibleContentRect for example are all public methods of ScrollView. Any other port may freely use them or even modify them if they consider that it is needed.
> 
> IMHO to avoid performance overhead on a certain branch/product by not fully implementing a feature in trunk will not work.

I didn't ask that, I just said that it has to be kept in mind.

Everyone who are using these API's are using them for mobile version of WebKit and they have the exact same performance requirements as we do - and everyone keeps using branches as trunk is simple not yet good enough in these respects. We want to change that as based our products on trunk, and I know we are not alone with those wishes.

Today I do not think there exists any ports commercially in use that share the same code for mobile/desktop, but it has been a long time goal for Qt - so let's keep trying to achieve that goal.
Comment 100 Ademar Reis 2011-07-04 13:04:32 PDT
(In reply to comment #98)
> 
> If you don't have the time to do it properly, fix it on your QtWebKit2.2 branch. But remember, QtWebKit2.2 is a one time thing only, WebKit2/Mobile is where our future lies.

Quite the oposite: I think we need to do it properly, I just think that properly doesn't mean "test all use-cases that we have on a private branch". That burden should be on whoever is working on the branch (nothing against working to conciliate our use-cases, but that should be a "best effort", not a requirement).

Instead of yet again adding this to 2.2 only, we should fix this on trunk and if necessary change the code later with what you have on your branch.
Comment 101 Kenneth Rohde Christiansen 2011-07-04 13:21:30 PDT
> Quite the oposite: I think we need to do it properly, I just think that properly doesn't mean "test all use-cases that we have on a private branch". That burden should be on whoever is working on the branch (nothing against working to conciliate our use-cases, but that should be a "best effort", not a requirement).
> 
> Instead of yet again adding this to 2.2 only, we should fix this on trunk and if necessary change the code later with what you have on your branch.

Great. Unfortunately, both Zalan and me are extremely busy due to product releases to actually help much ourselves. That is why I asked Luiz to look at the branch himself. Both of us are available to help if anyone have questions to why things was done in a specific way - I am already trying to help by joining these discussions and giving feedback on the patch.

But I cannot (currently) spend the time needed to actually properly compare this implementation and what we have on the branch, and I am afraid that event if it is slightly different, it might cause us a major headache in the near future. All of our fixes so far have been minor code changes but they have been very tricky and taken up a lot of time.

Test coverage is also very important as we have had many regressions, but that might require some infrastructure, as it depends on device-sizes, rotation, suspension, etc.
Comment 102 Luiz Agostini 2011-07-04 15:37:34 PDT
(In reply to comment #97)
> > delegatesScrolling, setDelegatesScrolling, actualVisibleContentRect and setActualVisibleContentRect for example are all public methods of ScrollView. Any other port may freely use them or even modify them if they consider that it is needed.
> > 
> > IMHO to avoid performance overhead on a certain branch/product by not fully implementing a feature in trunk will not work.
> 
> True, but you know as well as I do, that the branch was not created as a result of not wanting to work on trunk, it was done due to time constraints and a rapidly changing webkit2. That said, the future of QtWebKit is based on WebKit2 and there are plans to upstream everything on the branch as soon as we are ready with the final software for the N9 (and the well deserved vacation period ends). That means that if we do it in a way that makes it too complicated for us to use trunk as a replacement for the branch, then we will just end up with yet another branch for the successive releases which is bad.
> 
> So I am not reluctant to getting this fixed on trunk, all I am doing is asking to put a bit more work into it, so that it actually serves our needs for the N9 and similar future products. We've spent considerable time fixing many, many issues related to viewport handling, and we simple cannot afford doing this once again.

Man, I was talking about a simple technical issue: Someone, in future, may change updateScrollbars to handle delegatesScrolling and our performance optimization is gone. It is just too fragile.

(In reply to comment #91)
> (From update of attachment 99566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99566&action=review
> 
> > Source/WebCore/platform/ScrollView.h:147
> > -    IntRect actualVisibleContentRect() const { return m_actualVisibleContentRect.isEmpty() ? visibleContentRect() : m_actualVisibleContentRect; }
> > -    void setActualVisibleContentRect(const IntRect& actualVisibleContentRect) { m_actualVisibleContentRect = actualVisibleContentRect; }
> > +    IntRect actualVisibleContentRect() const { return m_actualVisibleContentRect; }
> > +    virtual void setActualVisibleContentRect(const IntRect& actualVisibleContentRect) { m_actualVisibleContentRect = actualVisibleContentRect; }
> 
> Im a bit afraid of this change. We are using actualVisibleContentRect so many places on our branch... you will need to check that you are not breaking any of these cases.

Another example:

If we keep actualVisibleContentRect() as it is then the caller will never know if he is receiving m_actualVisibleContentRect or visibleContentRect(). It happens that 
FrameLoaderClientQt::transitionToCommittedForNewPage() needs to pass m_actualVisibleContentRect from the old view to the new one, and it makes sense to actualVisibleContentRect() to return what setActualVisibleContentRect() has received . So, IMHO we will need to change something, it may be the implementation of actualVisibleContentRect(), its name or both. Any of the options will affect the implementation in branch.

This bug has been around for a long time and nothing has happened for a while. That is why I volunteered to work on it. I have no private interest in this issue.

I know that you have done a great work on webkit2. I know that you still very busy and that you plan to upstream everything in future, after your vacations (deserved indeed), but this bug is here now, blocking 2.2. Although we all want to pretend that the old API does not exist, it does.

I will be happy to remove the changes that I proposed to delegatesScrolling and updateScrollbars, if we are aware that just by using resizeToContents the user of the API is able to put the view in a inconsistent situation that leads to a completely crap rendering.

The change that I proposed to actualVisibleContentRect would solve what seems to be a bug in trunk today, as explained above. But I am ok about removing it as well.

I will look at the branch to see how you solved the resize event issue and do the same here.
Comment 103 Kenneth Rohde Christiansen 2011-07-04 15:47:55 PDT
> Man, I was talking about a simple technical issue: Someone, in future, may change updateScrollbars to handle delegatesScrolling and our performance optimization is gone. It is just too fragile.

Great, that is all you had to say. Having looked at it to access if something like this would be easily fixed in another patch is all what was needed.

> Another example:
> 
> If we keep actualVisibleContentRect() as it is then the caller will never know if he is receiving m_actualVisibleContentRect or visibleContentRect(). It happens that 

Why would he need to? The reason we have actualVisibleContentsRect is that visibleContentRect is virtual and overridden elsewhere. So when a ActualVisibleContentsRect is set, that one should always be used.

> FrameLoaderClientQt::transitionToCommittedForNewPage() needs to pass m_actualVisibleContentRect from the old view to the new one, and it makes sense to actualVisibleContentRect() to return what setActualVisibleContentRect() has received . So, IMHO we will need to change something, it may be the implementation of actualVisibleContentRect(), its name or both. Any of the options will affect the implementation in branch.

Yes, I would like you to talk to Zalan about this. He introduced setActualViewportSize on the branch which I think serves your needs. Zalan should be able to enlighten you here or you can have a look at our code.


> This bug has been around for a long time and nothing has happened for a while. That is why I volunteered to work on it. I have no private interest in this issue.

I know, and great that you are doing that :-)

> I will be happy to remove the changes that I proposed to delegatesScrolling and updateScrollbars, if we are aware that just by using resizeToContents the user of the API is able to put the view in a inconsistent situation that leads to a completely crap rendering.

Let's do it as a separate patch maybe? Then we can see if we can do it in a way that has no performance impact, or if it is already neglectable. 

> The change that I proposed to actualVisibleContentRect would solve what seems to be a bug in trunk today, as explained above. But I am ok about removing it as well.
> 
> I will look at the branch to see how you solved the resize event issue and do the same here.

Great Luiz!
Comment 104 Ademar Reis 2011-07-06 12:39:48 PDT
So what are the chances of seeing this patch being applied to trunk? Luiz: do you plan to work a bit more on it?
Comment 105 Luiz Agostini 2011-07-12 15:37:53 PDT
Created attachment 100576 [details]
patch
Comment 106 Kenneth Rohde Christiansen 2011-07-13 01:25:45 PDT
Comment on attachment 100576 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100576&action=review

> Source/WebCore/platform/ScrollView.h:151
>      LayoutUnit visibleWidth() const { return visibleContentRect().width(); }
>      LayoutUnit visibleHeight() const { return visibleContentRect().height(); }
>  
> +    int logicalWidth() const { return m_actualVisibleContentRect.isEmpty() ? width() : m_actualVisibleContentRect.width(); }
> +    int logicalHeight() const { return m_actualVisibleContentRect.isEmpty() ? height() : m_actualVisibleContentRect.height(); }

Wouldn't it be more correct using the above visibleWidth methods? and modifying those? It also seems that there is a LayoutUnit typedef now.
Comment 107 Luiz Agostini 2011-07-13 08:12:46 PDT
(In reply to comment #106)
> (From update of attachment 100576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100576&action=review
> 
> > Source/WebCore/platform/ScrollView.h:151
> >      LayoutUnit visibleWidth() const { return visibleContentRect().width(); }
> >      LayoutUnit visibleHeight() const { return visibleContentRect().height(); }
> >  
> > +    int logicalWidth() const { return m_actualVisibleContentRect.isEmpty() ? width() : m_actualVisibleContentRect.width(); }
> > +    int logicalHeight() const { return m_actualVisibleContentRect.isEmpty() ? height() : m_actualVisibleContentRect.height(); }
> 
> Wouldn't it be more correct using the above visibleWidth methods? and modifying those? It also seems that there is a LayoutUnit typedef now.

Without this patch DOMWindow::innerWidth() returns ScrollView::width() and DOMWindow::innerHeight() returns ScrollView::height().

I introduced those methods to avoid any behavior change in DOMWindow and ScrollView.
Comment 108 Luiz Agostini 2011-07-13 08:13:15 PDT
Created attachment 100675 [details]
patch
Comment 109 Kenneth Rohde Christiansen 2011-07-13 08:59:55 PDT
(In reply to comment #107)
> (In reply to comment #106)
> > (From update of attachment 100576 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=100576&action=review
> > 
> > > Source/WebCore/platform/ScrollView.h:151
> > >      LayoutUnit visibleWidth() const { return visibleContentRect().width(); }
> > >      LayoutUnit visibleHeight() const { return visibleContentRect().height(); }
> > >  
> > > +    int logicalWidth() const { return m_actualVisibleContentRect.isEmpty() ? width() : m_actualVisibleContentRect.width(); }
> > > +    int logicalHeight() const { return m_actualVisibleContentRect.isEmpty() ? height() : m_actualVisibleContentRect.height(); }
> > 
> > Wouldn't it be more correct using the above visibleWidth methods? and modifying those? It also seems that there is a LayoutUnit typedef now.
> 
> Without this patch DOMWindow::innerWidth() returns ScrollView::width() and DOMWindow::innerHeight() returns ScrollView::height().
> 
> I introduced those methods to avoid any behavior change in DOMWindow and ScrollView.

Not trying to be annoying here, but there are some facts.

1) mobile browsers making use of actualVisibleContentRect needs it to always override visibleContentRect.
2) It is really the *visible* width / height, so /logical/ sounds wrong to me. 

Anyway, I think we need Hyatt's input.
Comment 110 Kenneth Rohde Christiansen 2011-07-15 05:17:03 PDT
Luiz, I landed http://trac.webkit.org/changeset/91064 which is related.
Comment 111 Kenneth Rohde Christiansen 2011-07-15 05:23:19 PDT
Comment on attachment 100675 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100675&action=review

r- and this won't apply anymore.

> Source/WebCore/page/DOMWindow.cpp:1095
> -    return static_cast<int>(view->height() / m_frame->pageZoomFactor());
> +    return static_cast<int>(view->logicalHeight() / m_frame->pageZoomFactor());

I would suggest using view->visibleHeight() here. This will give the same result for the non-tiled backing store case. As visibleContentRect() sets the visibleHeight to view->height(). Or at least used to until the mac port introduced a scaling workaround, using a new m_bounds or similar.

If this breaks for mac due to their workaround, we can ifdef it so that they will continue using the current code. I don't understand their workaround well enough to say whether this will be the case or not. But it might be as it seems that they are lying about the actual size due to clipping.
Comment 112 Luiz Agostini 2011-07-19 14:20:04 PDT
Created attachment 101385 [details]
patch
Comment 113 Luiz Agostini 2011-07-25 11:55:28 PDT
Benjamin, Kling, would you mind taking a look?
Comment 114 Luiz Agostini 2011-08-05 11:47:53 PDT
ping review
Comment 115 Benjamin Poulain 2011-08-06 04:58:42 PDT
Comment on attachment 101385 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101385&action=review

Eh Kenneth, what's up? Mind having a look and guiding this toward landing? :)

Luiz, could you also integrate the original test (and the alternate tests people proposed)?

> Source/WebCore/page/DOMWindow.cpp:1094
> -    
> +

Unrelated change, should be in a separate patch .... just kidding ;)

> Source/WebCore/page/DOMWindow.cpp:1099
> +#if PLATFORM(QT)
> +    return static_cast<int>(view->visibleHeight() / m_frame->pageZoomFactor());
> +#else
>      return static_cast<int>(view->height() / m_frame->pageZoomFactor());
> +#endif

This seems very wrong in my opinion, it means we have a problem of architecture in WebKit.
A class like DOMWindow should not have platform specific changes like this.

> Source/WebCore/page/DOMWindow.cpp:1115
> +#if PLATFORM(QT)
> +    return static_cast<int>(view->visibleWidth() / m_frame->pageZoomFactor());
> +#else
>      return static_cast<int>(view->width() / m_frame->pageZoomFactor());
> +#endif

Same problem.

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:632
> +    QGraphicsWebView* webView = new QGraphicsWebView();

Why do you allocate the view on the heap? It looks like it is leaked in this function.
Comment 116 Kenneth Rohde Christiansen 2011-08-06 10:40:41 PDT
(In reply to comment #115)
> (From update of attachment 101385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101385&action=review
> 
> Eh Kenneth, what's up? Mind having a look and guiding this toward landing? :)

Yes, I can do that. I am back on Monday.

> > Source/WebCore/page/DOMWindow.cpp:1099
> > +#if PLATFORM(QT)
> > +    return static_cast<int>(view->visibleHeight() / m_frame->pageZoomFactor());
> > +#else
> >      return static_cast<int>(view->height() / m_frame->pageZoomFactor());
> > +#endif
> 
> This seems very wrong in my opinion, it means we have a problem of architecture in WebKit.
> A class like DOMWindow should not have platform specific changes like this.

the code should actually work for all platforms. The only worry is that Mac is doing some work arounds using some "bounds" in FrameView, which might (or might not) break this for the mac. Luiz, can't you test this? 

Otherwise we can at least change the ifdef to TILED_BACKING_STORE - but that is only slightly better.
Comment 117 Ademar Reis 2011-08-18 08:08:12 PDT
(In reply to comment #116)
> (In reply to comment #115)
> > (From update of attachment 101385 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=101385&action=review
> > 
> > Eh Kenneth, what's up? Mind having a look and guiding this toward landing? :)
> 
> Yes, I can do that. I am back on Monday.
> 
> > > Source/WebCore/page/DOMWindow.cpp:1099
> > > +#if PLATFORM(QT)
> > > +    return static_cast<int>(view->visibleHeight() / m_frame->pageZoomFactor());
> > > +#else
> > >      return static_cast<int>(view->height() / m_frame->pageZoomFactor());
> > > +#endif
> > 
> > This seems very wrong in my opinion, it means we have a problem of architecture in WebKit.
> > A class like DOMWindow should not have platform specific changes like this.
> 
> the code should actually work for all platforms. The only worry is that Mac is doing some work arounds using some "bounds" in FrameView, which might (or might not) break this for the mac. Luiz, can't you test this? 
> 
> Otherwise we can at least change the ifdef to TILED_BACKING_STORE - but that is only slightly better.

I'm not familiar with the details of this patch, but now that Luiz left INdT/Nokia, I don't have much hope of seeing this landed in trunk anytime soon.

Does anyone plan to work on this patch (Kenneth?) Would you guys be comfortable with the latest patch landing in QtWebKit-2.2?
Comment 118 Kenneth Rohde Christiansen 2011-08-18 09:02:54 PDT
> > > > Source/WebCore/page/DOMWindow.cpp:1099
> > > > +#if PLATFORM(QT)
> > > > +    return static_cast<int>(view->visibleHeight() / m_frame->pageZoomFactor());
> > > > +#else
> > > >      return static_cast<int>(view->height() / m_frame->pageZoomFactor());
> > > > +#endif

> I'm not familiar with the details of this patch, but now that Luiz left INdT/Nokia, I don't have much hope of seeing this landed in trunk anytime soon.

I am currently away from working on trunk due to some high priority bugs for the N9. But basically this patch should just use

return static_cast<int>(view->visibleHeight() / m_frame->pageZoomFactor());

We just need someone to confirm that that does not break any tests on any platforms. The only platform where I think this might break is the Mac due to some workaround they are having. Would it be possible for someone at INDT to test whether this patch breaks any tests on the mac and on at least one other platform?

> Does anyone plan to work on this patch (Kenneth?) Would you guys be comfortable with the latest patch landing in QtWebKit-2.2?

Having it in trunk is better and we do need it there as well.
Comment 119 Ademar Reis 2011-08-30 10:41:51 PDT
(In reply to comment #118)
> > I'm not familiar with the details of this patch, but now that Luiz left INdT/Nokia, I don't have much hope of seeing this landed in trunk anytime soon.
> 
> I am currently away from working on trunk due to some high priority bugs for the N9. But basically this patch should just use
> 
> return static_cast<int>(view->visibleHeight() / m_frame->pageZoomFactor());
> 
> We just need someone to confirm that that does not break any tests on any platforms. The only platform where I think this might break is the Mac due to some workaround they are having. Would it be possible for someone at INDT to test whether this patch breaks any tests on the mac and on at least one other platform?

The patch breaks the following tests: (thanks to Jarred Nicholls for the tests on Mac!)

On Linux and Mac:
client-width-height-quirks.html
client-width-height.html
inner-width-height.html

On Mac only:
frame-loading-via-document-write.html

I also tested the patch in the qtwebkit-2.2 branch and there it has no effect (testcase still reproduceable), so I guess we'll need something different there.

Realistically speaking, I don't see this bug being closed in QtWebKit-2.2 anytime soon. I'm moving it to the "nice-to-have" list and will keep it there until an affected party (probably a team developing a mobile browser) allocates resources to fix it. I've notified the QtWebKit Program Managers of this action.
Comment 120 Kenneth Rohde Christiansen 2011-08-30 14:39:07 PDT
(In reply to comment #119)
> (In reply to comment #118)
> > > I'm not familiar with the details of this patch, but now that Luiz left INdT/Nokia, I don't have much hope of seeing this landed in trunk anytime soon.
> > 
> > I am currently away from working on trunk due to some high priority bugs for the N9. But basically this patch should just use
> > 
> > return static_cast<int>(view->visibleHeight() / m_frame->pageZoomFactor());
> > 
> > We just need someone to confirm that that does not break any tests on any platforms. The only platform where I think this might break is the Mac due to some workaround they are having. Would it be possible for someone at INDT to test whether this patch breaks any tests on the mac and on at least one other platform?
> 
> The patch breaks the following tests: (thanks to Jarred Nicholls for the tests on Mac!)
> 
> On Linux and Mac:
> client-width-height-quirks.html
> client-width-height.html
> inner-width-height.html
> 
> On Mac only:
> frame-loading-via-document-write.html
> 
> I also tested the patch in the qtwebkit-2.2 branch and there it has no effect (testcase still reproduceable), so I guess we'll need something different there.
> 
> Realistically speaking, I don't see this bug being closed in QtWebKit-2.2 anytime soon. I'm moving it to the "nice-to-have" list and will keep it there until an affected party (probably a team developing a mobile browser) allocates resources to fix it. I've notified the QtWebKit Program Managers of this action.

Great, thanks for testing this. Was this with the patch from Luiz or with the above suggested changes? Also, when you say Mac is that then the mac port or Qt on mac? I was looking for results testing with the actual apple mac port.

Also the quirk test probably fails as I believe our DRT doesnt have quirks enabled. Do you have the diffs for these failing tests?
Comment 121 Ademar Reis 2011-08-31 05:04:02 PDT
(In reply to comment #120)
> (In reply to comment #119)

<snip>

> > The patch breaks the following tests: (thanks to Jarred Nicholls for the tests on Mac!)
> > 
> > On Linux and Mac:
> > client-width-height-quirks.html
> > client-width-height.html
> > inner-width-height.html
> > 
> > On Mac only:
> > frame-loading-via-document-write.html
> > 
> > I also tested the patch in the qtwebkit-2.2 branch and there it has no effect (testcase still reproduceable), so I guess we'll need something different there.
> > 
> > Realistically speaking, I don't see this bug being closed in QtWebKit-2.2 anytime soon. I'm moving it to the "nice-to-have" list and will keep it there until an affected party (probably a team developing a mobile browser) allocates resources to fix it. I've notified the QtWebKit Program Managers of this action.
> 
> Great, thanks for testing this. Was this with the patch from Luiz or with the above suggested changes? Also, when you say Mac is that then the mac port or Qt on mac? I was looking for results testing with the actual apple mac port.
> 

Patch with the suggested changes, tested on Mac port (not Qt on Mac).

> Also the quirk test probably fails as I believe our DRT doesnt have quirks enabled. Do you have the diffs for these failing tests?

I'll submit them later today (in a meeting right now)
Comment 122 Ademar Reis 2011-08-31 06:25:20 PDT
Created attachment 105772 [details]
Layout test results on the Mac port after the patch

Attaching layout-test results on Mac (port). To keep the file smaller, I removed some .pngs from the tarball (of tests failing before the patch anyway).

Again, these are the tests that failed after the patch:

client-width-height-quirks.html
client-width-height.html
frame-loading-via-document-write.html
inner-width-height.html
horizontal-scroll-after-back.html
Comment 123 Kenneth Rohde Christiansen 2011-08-31 06:49:06 PDT
(In reply to comment #122)
> Created an attachment (id=105772) [details]
> Layout test results on the Mac port after the patch
> 
> Attaching layout-test results on Mac (port). To keep the file smaller, I removed some .pngs from the tarball (of tests failing before the patch anyway).
> 
> Again, these are the tests that failed after the patch:
> 
> client-width-height-quirks.html
> client-width-height.html
> frame-loading-via-document-write.html
> inner-width-height.html
> horizontal-scroll-after-back.html

Ah, I digged in a bit and it seems that innerWidth should include scrollbars. Meaning that the following methods in ScrollView.cpp

149	    LayoutUnit visibleWidth() const { return visibleContentRect().width(); }
150	    LayoutUnit visibleHeight() const { return visibleContentRect().height(); }

should become

149	    LayoutUnit visibleWidth() const { return visibleContentRect(/* includeScrollbars */ true).width(); }
150	    LayoutUnit visibleHeight() const { return visibleContentRect(/* includeScrollbars */ true).height(); }

Jarred, up for testing with that?

You can try modifying LayoutTests/fast/dom/client-width-height-quirks.html to print out the innerWidth and clientWidth before and after the change.
Comment 124 Jarred Nicholls 2011-08-31 06:59:55 PDT
(In reply to comment #123)
> (In reply to comment #122)

<snip>

> > 
> > client-width-height-quirks.html
> > client-width-height.html
> > frame-loading-via-document-write.html
> > inner-width-height.html
> > horizontal-scroll-after-back.html
> 
> Ah, I digged in a bit and it seems that innerWidth should include scrollbars. Meaning that the following methods in ScrollView.cpp
> 
> 149        LayoutUnit visibleWidth() const { return visibleContentRect().width(); }
> 150        LayoutUnit visibleHeight() const { return visibleContentRect().height(); }
> 
> should become
> 
> 149        LayoutUnit visibleWidth() const { return visibleContentRect(/* includeScrollbars */ true).width(); }
> 150        LayoutUnit visibleHeight() const { return visibleContentRect(/* includeScrollbars */ true).height(); }
> 
> Jarred, up for testing with that?
> 
> You can try modifying LayoutTests/fast/dom/client-width-height-quirks.html to print out the innerWidth and clientWidth before and after the change.

Yeah I can test it; it's on my list of doom now ;)
Comment 125 Kenneth Rohde Christiansen 2011-09-12 01:36:39 PDT
> Yeah I can test it; it's on my list of doom now ;)

Any update on this? :-) New updated patch for instance?
Comment 126 Adenilson Cavalcanti Silva 2011-09-12 09:53:38 PDT
Created attachment 107057 [details]
Output folder of tests (3 cases: vanilla, innerHeight/Width() patch, previous + visibleWidth/Height

Compiled in OSX Lion using mac port of webkit.
Comment 127 Kenneth Rohde Christiansen 2011-09-12 12:38:09 PDT
(In reply to comment #126)
> Created an attachment (id=107057) [details]
> Output folder of tests (3 cases: vanilla, innerHeight/Width() patch, previous + visibleWidth/Height
> 
> Compiled in OSX Lion using mac port of webkit.

Oh, seems that I was wrong. The last tests shows hundreds of failures.

Could you try to debug by testing

> client-width-height-quirks.html
> client-width-height.html
> frame-loading-via-document-write.html
> inner-width-height.html
> horizontal-scroll-after-back.html

manually?

I guess visibleWidth is used elsewhere. Maybe you should just call visibleContentRect(true) directly instead.
Comment 128 Kenneth Rohde Christiansen 2011-09-12 14:37:48 PDT
> I guess visibleWidth is used elsewhere. Maybe you should just call visibleContentRect(true) directly instead

I took a deeper look at the code and it is obvious that changing visibleWidth to include scrollbars will  break a lot of logic in ScrollView.cpp, so instead we should call visibleContents(/* include scrollbars */ true).width() inside innerWidth(), which fits with the spec: 

The innerWidth attribute must return the viewport width including the size of a rendered scroll bar (if any). (http://www.w3.org/TR/cssom-view/#dom-window-innerwidth)

I hope this works on Mac.

What currently is happening is that it is calling Widget::width() which returns the frameRect().width(). frameRect() on Qt just returns m_frame which is the size of the currently FrameView viewport plus scrollbars - ie. the same as visibleContentRect(true).

The Mac seems to call getOuterRect() on their widget, but I believe this should still result in the same size.
Comment 129 Adenilson Cavalcanti Silva 2011-09-13 08:12:47 PDT
Created attachment 107178 [details]
Vanilla + new patch (using visibleContentRect directly in DOMWindow)

Tests executed in OSX Lion using webkit mac port.
Comment 130 Kenneth Rohde Christiansen 2011-09-13 08:22:46 PDT
(In reply to comment #129)
> Created an attachment (id=107178) [details]
> Vanilla + new patch (using visibleContentRect directly in DOMWindow)
> 
> Tests executed in OSX Lion using webkit mac port.

That looks pretty promising!

Only four differences which might be flaky as they all seems unrelated. Maybe you can do a few additional test runs with the patch to identify whether these are flaky or not.

media/video-loop.html
svg/custom/svg-fonts-in-html.html
svg/custom/svg-fonts-with-no-element-reference.html
http/tests/navigation/anchor-frames.html
Comment 131 Adenilson Cavalcanti Silva 2011-09-13 08:25:56 PDT
Maybe a note while failing new test is http/tests/navigation/anchor-frames.html (Could it be impacted by the patch?).
Comment 132 Kenneth Rohde Christiansen 2011-09-13 08:27:02 PDT
(In reply to comment #131)
> Maybe a note while failing new test is http/tests/navigation/anchor-frames.html (Could it be impacted by the patch?).

It is an http test, so I doubt it
Comment 133 Adenilson Cavalcanti Silva 2011-09-13 18:57:56 PDT
Created attachment 107273 [details]
Patch
Comment 134 Antonio Gomes 2011-09-13 20:08:07 PDT
Comment on attachment 107273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107273&action=review

> Source/WebCore/page/DOMWindow.cpp:1096
> +    return static_cast<int>(view->visibleContentRect(true).height() / m_frame->pageZoomFactor());

(true /*trueMeansXXXHere*/)

> Source/WebCore/page/DOMWindow.cpp:1108
> +    return static_cast<int>(view->visibleContentRect(true).width() / m_frame->pageZoomFactor());

Ditto.
Comment 135 Antonio Gomes 2011-09-13 20:09:05 PDT
Comment on attachment 107273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107273&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This line should be replaced by a reasonable explaination for us to not have tests to this.

Also, explain the change in the changelog :)
Comment 136 Kenneth Rohde Christiansen 2011-09-14 01:29:09 PDT
Comment on attachment 107273 [details]
Patch

Adding something like "exercised by existing tests"  should be suffcient
Comment 137 Adenilson Cavalcanti Silva 2011-09-15 00:52:20 PDT
Today I executed the pixel tests (182MB the tarball), but the resume is:

a) Vanilla
24549 test cases (96%) succeeded
865 test cases (3%) had incorrect layout
1 test case (<1%) timed out
947 test cases (3%) had stderr output

b) Patched
24549 test cases (96%) succeeded
865 test cases (3%) had incorrect layout
1 test case (<1%) timed out
944 test cases (3%) had stderr output
Comment 138 Kenneth Rohde Christiansen 2011-09-15 02:06:22 PDT
Comment on attachment 107273 [details]
Patch

The patch is almost there, but I will r- it due to the comments given by me and Antonio. If you could fix those and upload another one that would be great.
Comment 139 Adenilson Cavalcanti Silva 2011-09-15 07:34:48 PDT
Kenneth and Antonio

Thanks a lot for the remarks in the initial patch, I'm planning to upload a revised one pretty soon.


Adenilson
Comment 140 Adenilson Cavalcanti Silva 2011-09-16 07:17:41 PDT
Created attachment 107647 [details]
Patch
Comment 141 Adenilson Cavalcanti Silva 2011-09-16 08:13:31 PDT
Created attachment 107654 [details]
Patch with more descriptive Changelog and cleaned up test
Comment 142 Adenilson Cavalcanti Silva 2011-09-16 08:17:27 PDT
Created attachment 107655 [details]
Test with more descriptive ChangeLog and cleaner test
Comment 143 Kenneth Rohde Christiansen 2011-09-17 04:01:37 PDT
Comment on attachment 107655 [details]
Test with more descriptive ChangeLog and cleaner test

View in context: https://bugs.webkit.org/attachment.cgi?id=107655&action=review

> Source/WebCore/ChangeLog:9
> +        Including the scrollbar dimensions (if there are any) in the inner height/width to
> +        to prevent infinite resize when using resizeToContents feature.

Better write this differently:

Modify how innerWidth/Height are being computed so that they return correct values when the tiling backing store is being used. We how use the visibleContentsRect including scrollbars (if any) which is correct according to the spec : link here. This prevents...

> Source/WebCore/ChangeLog:11
> +        Test: test_qgraphicswebview::windowResizeEvent()

The innerWidth/Height correctness is covered by existing tests. The non-infinite resizing is tested by a Qt autotest ...

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:612
> +// The whole test came Straight from Agostini's patch (issue #43852)

Just write in the ChangeLog that this test is done by Luiz Agostini.

Test by Luiz Agostini. or so.
Comment 144 Adenilson Cavalcanti Silva 2011-09-19 08:47:24 PDT
Created attachment 107863 [details]
Adding external references to W3C spec in changelog comments + minor fixes
Comment 145 WebKit Review Bot 2011-09-19 08:50:03 PDT
Attachment 107863 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/qt/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 146 Adenilson Cavalcanti Silva 2011-09-19 08:59:43 PDT
Created attachment 107866 [details]
Adding external references to W3C spec in changelog comments + minor fixes
Comment 147 WebKit Review Bot 2011-09-19 09:01:49 PDT
Attachment 107866 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/qt/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 148 Adenilson Cavalcanti Silva 2011-09-19 09:05:22 PDT
Created attachment 107867 [details]
Adding external references to W3C spec in changelog comments + minor fixes
Comment 149 Kenneth Rohde Christiansen 2011-09-19 14:09:55 PDT
Comment on attachment 107867 [details]
Adding external references to W3C spec in changelog comments + minor fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=107867&action=review

r=me as the code is fine and it passes tests. I am cq-ing though as it is an important change and I want the changelog to be as clear as possible and to the point. Good work though!

> Source/WebCore/ChangeLog:19
> +        Inner height and width are now calculated using WebCore::ScrollView::visibleContentRect
> +        including the scrollbars (if there are any).
> +
> +        Referring the W3C spec "CSSOM View Module", section "Extensions to
> +        the Window Interface":
> +        - "... innerWidth attribute must return the viewport width
> +        including the size of a rendered scroll bar (if any)."
> +        - "The innerHeight attribute must return the viewport height
> +        including the size of a rendered scroll bar (if any)."
> +
> +        It will now return correct values when using tiling backing store
> +        thus preventing infinite resize events.

I would make this more to the point, so that the change is easy to understand for most people.:


InnerHeight and -Width are now calculated using ScrollView::visibleContentRect() including scrollbars (if any), instead of using ScrollView::frameRect() as before.

This makes no behavior change when not using the tiled backing store and is compliant with the definition stated in the CSSOM View Module. The change was made so that we would be returning the correct values when using the tiled backing store and thus fix the original bug report, which was to avoid infinite resize events caused by wrong innerHeight and -Width values.

> Source/WebCore/ChangeLog:25
> +
> +

Please remove double newlines before committing

> Source/WebCore/page/DOMWindow.cpp:1096
> +    return static_cast<int>(view->visibleContentRect(/* include the scrollbars */ true).height() / m_frame->pageZoomFactor());

This is fine, but it is more common to just write the original method argument. ie. includeScrollbars or whatever it is called.
Comment 150 Adenilson Cavalcanti Silva 2011-09-19 17:23:21 PDT
Created attachment 107946 [details]
Updated changelog comments + minor fixes
Comment 151 WebKit Review Bot 2011-09-20 03:40:16 PDT
Comment on attachment 107946 [details]
Updated changelog comments + minor fixes

Clearing flags on attachment: 107946

Committed r95525: <http://trac.webkit.org/changeset/95525>
Comment 152 WebKit Review Bot 2011-09-20 03:40:35 PDT
All reviewed patches have been landed.  Closing bug.