It might be my fault. Looking ...

(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;
(...)
}

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.

Fix is required for Qtwebkit 2.1, as this is blocking browser release. Pls prioritize this bug.

Also, yahoo.com has this problem when using the iphone 4 user agent.

Created attachment 73479 [details]
Test case

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

We will test the patch on Qt Webkit 2.1 as soon as it is available.

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)

Is Kenneth patch under a different bugzilla id?  If so, please add the id here.

no bugzilla id yet. I'll add it here, when it is created.

depends on 49371.

https://bugs.webkit.org/show_bug.cgi?id=49371

This bugs depends on two other patches which will need to be cherry-picked

Will we have a patch for this bug or just the other fixes (bug 49371 and bug 49375) are enough?

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

You will need to modify the QtTestBrowser to actually call the new api.

I don't see any new Qt API in 49371 and 49375.  Is there another dependency?

I think I found the other dependency. It is 49373.  Will add it as a dependency to this bug.

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

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?

Yes, i am working on the patch atm. Will probably upload it early tomorrow.

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?

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

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

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());
...

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.

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

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?

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

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

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

Today's stuff that needs cherry-picking into 2.1:

fbb71f0c9a7797935898c505320ca88c39814256 (not from today, but a prereq)

f10a2e0a45afa77f1646576b9bb1082c05c154d5
f49b9c38ddf842cbfecd589ee3bc63be5ed6b3aa
0aaed612d81fc83bdab8beeb1f2945652b0dd3a1
ec0feeba426f51c3580e0caf91fe6634d1766aac
9a083a2065a107372f60545a5014c58622a6ff8c

(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

Moar!

From trunk:

ebf88d993a75c0d04203a13f05474c42c99dd964

From qtwebkit-webkit2-dev:

b53c3975c2461cf29babcae72b55fcf43bdc4f9d
f7cefd7bf6233e686098b9380fda61bf684f7e14 (this is the one Kenneth was referring to)

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?

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

(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).

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.

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.

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

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.

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

Created attachment 75273 [details]
v3 of the consolidation of patches for qtwebkit-2.1

More coding-style fixes + update on the symbian .def file

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.

(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 :-)

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

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());
 }

patch v4 has been pushed to qtwebkit-2.1 as http://gitorious.org/webkit/qtwebkit/commit/e0e3111

Is this patch to be landed?

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.

(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).

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

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

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

(In reply to comment #55)

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

On Symbian.

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

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

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

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

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

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.

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.

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

*** Bug 42754 has been marked as a duplicate of this bug. ***

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

I will check to make sure the original use case is no longer reproducible and then we can probably close this bug.

On trunk, the original use case seems to be no longer reproducible.  I set the bug to RESOLVED-WORKSFORME.

I reopen.
This bug is not closed until there is an Qt autotest for the use case.

I will create a test.

Actually, I'm able to reproduce this now.  Will look into it.

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.

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

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

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

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.

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

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

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.

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

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

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

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

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

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

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

Created attachment 99566 [details]
patch

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

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

(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().

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

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

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

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

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

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

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

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

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

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

So what are the chances of seeing this patch being applied to trunk? Luiz: do you plan to work a bit more on it?

Created attachment 100576 [details]
patch

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.

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

Created attachment 100675 [details]
patch

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

Luiz, I landed http://trac.webkit.org/changeset/91064 which is related.

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.

Created attachment 101385 [details]
patch

Benjamin, Kling, would you mind taking a look?

ping review

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.

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

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

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

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

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

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

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

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

(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 ;)

> Yeah I can test it; it's on my list of doom now ;)

Any update on this? :-) New updated patch for instance?

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.

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

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

Created attachment 107178 [details]
Vanilla + new patch (using visibleContentRect directly in DOMWindow)

Tests executed in OSX Lion using webkit mac port.

(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

Maybe a note while failing new test is http/tests/navigation/anchor-frames.html (Could it be impacted by the patch?).

(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

Created attachment 107273 [details]
Patch

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 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 on attachment 107273 [details]
Patch

Adding something like "exercised by existing tests"  should be suffcient

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

Kenneth and Antonio

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


Adenilson

Created attachment 107647 [details]
Patch

Created attachment 107654 [details]
Patch with more descriptive Changelog and cleaned up test

Created attachment 107655 [details]
Test with more descriptive ChangeLog and cleaner test

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.

Created attachment 107863 [details]
Adding external references to W3C spec in changelog comments + minor fixes

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.

Created attachment 107866 [details]
Adding external references to W3C spec in changelog comments + minor fixes

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.

Created attachment 107867 [details]
Adding external references to W3C spec in changelog comments + minor fixes

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.

Created attachment 107946 [details]
Updated changelog comments + minor fixes

Comment on attachment 107946 [details]
Updated changelog comments + minor fixes

Clearing flags on attachment: 107946

Committed r95525: <http://trac.webkit.org/changeset/95525>

All reviewed patches have been landed.  Closing bug.