43852 – [Qt] resizeToContent seems to trigger infinite resize on some pages

Benjamin Poulain

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

Antonio Gomes

Comment 1 2010-08-11 07:31:26 PDT

It might be my fault. Looking ...

Antonio Gomes

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

Nancy Piedra

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

Suresh Voruganti

Comment 4 2010-11-09 05:51:41 PST

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

Nancy Piedra

Comment 5 2010-11-09 07:41:57 PST

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

Benjamin Poulain

Comment 6 2010-11-10 03:15:45 PST

Created attachment 73479 [details] Test case

Benjamin Poulain

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

Nancy Piedra

Comment 8 2010-11-10 05:09:39 PST

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

zalan

Comment 9 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)

Nancy Piedra

Comment 10 2010-11-10 07:36:32 PST

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

zalan

Comment 11 2010-11-10 07:38:46 PST

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

zalan

Comment 12 2010-11-11 02:25:07 PST

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

Kenneth Rohde Christiansen

Comment 13 2010-11-11 04:37:42 PST

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

Ademar Reis

Comment 14 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?

Ademar Reis

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

Kenneth Rohde Christiansen

Comment 16 2010-11-15 02:39:09 PST

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

Nancy Piedra

Comment 17 2010-11-15 05:08:32 PST

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

Nancy Piedra

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

Dinu Jacob

Comment 19 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?

Ademar Reis

Comment 20 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?

zalan

Comment 21 2010-11-17 07:24:40 PST

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

Dinu Jacob

Comment 22 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?

Dinu Jacob

Comment 23 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?

Dinu Jacob

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

zalan

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

zalan

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

Kenneth Rohde Christiansen

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

Dinu Jacob

Comment 28 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?

Kenneth Rohde Christiansen

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

Dinu Jacob

Comment 30 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)

Kenneth Rohde Christiansen

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

Andreas Kling

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

Kenneth Rohde Christiansen

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

Andreas Kling

Comment 34 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)

Nancy Piedra

Comment 35 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?

Ademar Reis

Comment 36 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?

Dinu Jacob

Comment 37 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).

Nancy Piedra

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

Nancy Piedra

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

Nancy Piedra

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

Nancy Piedra

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

Ademar Reis

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

Kenneth Rohde Christiansen

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

Ademar Reis

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

Nancy Piedra

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

Yael

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

Ademar Reis

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

Ademar Reis

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

Ademar Reis

Comment 49 2010-12-07 11:18:56 PST

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

Eric Seidel (no email)

Comment 50 2010-12-14 01:24:29 PST

Is this patch to be landed?

Eric Seidel (no email)

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

Ademar Reis

Comment 52 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).

Yael

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

Benjamin Poulain

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

Yael

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

Yael

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

Benjamin Poulain

Comment 57 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?

Kenneth Rohde Christiansen

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

Yael

Comment 59 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?

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Yael

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

Yael

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

Yael

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

Viatcheslav Ostapenko

Comment 65 2011-02-06 14:46:47 PST

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

Alexis Menard (darktears)

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

Nancy Piedra

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

Nancy Piedra

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

Benjamin Poulain

Comment 69 2011-04-12 05:56:19 PDT

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

Nancy Piedra

Comment 70 2011-04-12 12:45:44 PDT

I will create a test.

Nancy Piedra

Comment 71 2011-04-14 05:02:10 PDT

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

Nancy Piedra

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

Alexis Menard (darktears)

Comment 73 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?

Yael

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

Alexis Menard (darktears)

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

Laszlo Gombos

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

Yael

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

Ademar Reis

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

Yael

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

Luiz Agostini

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

Yael

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

Luiz Agostini

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

Yael

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

Luiz Agostini

Comment 84 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?

Alexis Menard (darktears)

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

Yael

Comment 86 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 ?

Benjamin Poulain

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

Luiz Agostini

Comment 88 2011-07-02 17:40:47 PDT

Created attachment 99566 [details] patch

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Luiz Agostini

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

Luiz Agostini

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

Kenneth Rohde Christiansen

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

Ademar Reis

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

Luiz Agostini

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Ademar Reis

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

Kenneth Rohde Christiansen

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

Luiz Agostini

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

Kenneth Rohde Christiansen

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

Ademar Reis

Comment 104 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?

Luiz Agostini

Comment 105 2011-07-12 15:37:53 PDT

Created attachment 100576 [details] patch

Kenneth Rohde Christiansen

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

Luiz Agostini

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

Luiz Agostini

Comment 108 2011-07-13 08:13:15 PDT

Created attachment 100675 [details] patch

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

Comment 110 2011-07-15 05:17:03 PDT

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

Kenneth Rohde Christiansen

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

Luiz Agostini

Comment 112 2011-07-19 14:20:04 PDT

Created attachment 101385 [details] patch

Luiz Agostini

Comment 113 2011-07-25 11:55:28 PDT

Benjamin, Kling, would you mind taking a look?

Luiz Agostini

Comment 114 2011-08-05 11:47:53 PDT

ping review

Benjamin Poulain

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

Kenneth Rohde Christiansen

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

Ademar Reis

Comment 117 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?

Kenneth Rohde Christiansen

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

Ademar Reis

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

Kenneth Rohde Christiansen

Comment 120 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?

Ademar Reis

Comment 121 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)

Ademar Reis

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

Kenneth Rohde Christiansen

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

Jarred Nicholls

Comment 124 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 ;)

Kenneth Rohde Christiansen

Comment 125 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?

Adenilson Cavalcanti Silva

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Adenilson Cavalcanti Silva

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

Kenneth Rohde Christiansen

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

Adenilson Cavalcanti Silva

Comment 131 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?).

Kenneth Rohde Christiansen

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

Adenilson Cavalcanti Silva

Comment 133 2011-09-13 18:57:56 PDT

Created attachment 107273 [details] Patch

Antonio Gomes

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

Antonio Gomes

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

Kenneth Rohde Christiansen

Comment 136 2011-09-14 01:29:09 PDT

Comment on attachment 107273 [details] Patch Adding something like "exercised by existing tests" should be suffcient

Adenilson Cavalcanti Silva

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

Kenneth Rohde Christiansen

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

Adenilson Cavalcanti Silva

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

Adenilson Cavalcanti Silva

Comment 140 2011-09-16 07:17:41 PDT

Created attachment 107647 [details] Patch

Adenilson Cavalcanti Silva

Comment 141 2011-09-16 08:13:31 PDT

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

Adenilson Cavalcanti Silva

Comment 142 2011-09-16 08:17:27 PDT

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

Kenneth Rohde Christiansen

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

Adenilson Cavalcanti Silva

Comment 144 2011-09-19 08:47:24 PDT

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

WebKit Review Bot

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

Adenilson Cavalcanti Silva

Comment 146 2011-09-19 08:59:43 PDT

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

WebKit Review Bot

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

Adenilson Cavalcanti Silva

Comment 148 2011-09-19 09:05:22 PDT

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

Kenneth Rohde Christiansen

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

Adenilson Cavalcanti Silva

Comment 150 2011-09-19 17:23:21 PDT

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

WebKit Review Bot

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

WebKit Review Bot

Comment 152 2011-09-20 03:40:35 PDT

All reviewed patches have been landed. Closing bug.

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
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.

ademar
christian.webkit
edisson.braga
eric
hyatt
jarred
kenneth
kling
laszlo.gombos
luiz
menard
mtutunik
nancy.piedra
savagobr
suresh.voruganti
tnoleto
tonikitoo
webkit.review.bot
yael
zalan