Bug 60893

Qt Linux Platform's builder is not complaining about this test. As it is using Qt 4.7.3, I believe it might be a regression on Qt, so I'm waiting to know the exact commit that it is using so I can try that (as they don't have a tag for 4.7.3), and I'm still trying to figure out why I can't build Qt-Mobility's master branch with the people on #qt-mobility. So in the meantime, I was checking why this bug is happening. The problem with this test case is that at the end of the rendering stage, it seems that nothing was actually rendered inside QPicture, so the resulting boundingRect remains (0,0 0x0). After doing some few tests, I've noticed that whenever I try to paint anything inside the QPicture, using the proper pointers to it inside the inner functions, it updates its boundingRect just as expected in the end (I was doing basic stuff like painting a rect, or an ellipse). But as I get deeper on it, the code started to get really confusing, so I still couldn't figure out why, in the end, "nothing" was really rendered. So let's say it is really rendering, what could be happening here is an access to an invalid pointer deep inside, which might be related somehow to a possible regression on Qt. I didn't discard the possibility that this test was not working before and by coincidence the results matched, but I feel this is very unlikely. I'll keep this reported as I advance on it.

Created attachment 94208 [details] Now it waits the content to be loaded before testing and make sure QPicture will expand if its painter is used to render. As this test uses a QPicture's painter to render the frame, its bounding rect is supposed to grow as bigger the painting gets inside it. What was happening here was a very strange behavior on its expanding policy. If you render stuff without translating anything, things will work as expected. The issue works as follows: 1. Set a clipping rect on the painter. 2. Fill it with a bigger rect and check the QPicture's bounding rect (it should match the clipping rect). 3. Now set a clipping rect to be somewhere's else, so it would have to expand when filled. [ignore to make it work] 4. Translate to the top left point of this recently determined clipping rect. Your clipping rect would be changed to the follow (0,0 WxH) [/ignore] 5. Fill again with a bigger rect, and check if the QPicture's bounding rect is increased as you would expect. I don't know any other better way to explain this, so I've looked into ScrollBarThemeQt::paint and realized it would paint the place where the scrollbar should be with a background color before the actual scrollbar. So if I paint this background color before the translation, the QPicture's bounding rect would expand correctly, and the resulting painting would not change.

I have tested this expanding policy of QPicture on Qt 4.7.2, 4.7.3 and 4.7.4, they all work the same way. So I believe that it has passed on the buildbot because it wasn't making use of such themes, which would probably don't have any translation issue.

Comment on attachment 94208 [details] Now it waits the content to be loaded before testing and make sure QPicture will expand if its painter is used to render. View in context: https://bugs.webkit.org/attachment.cgi?id=94208&action=review > Source/WebCore/ChangeLog:12 > + If you use a QPicture's painter to render a themed scrollbar, > + as it does a translation to a certain point before painting, > + QPicture's bounding rect may not be expanded because the new > + clipping region may be inside its bounding rect in the original > + coordinate system (before translation). This change log is very confusing, specially after reading your comments in the bug. Is it a problem with QPicture, with the QStyle in use in your env or with QtWebKit? As the problem does not show up in all machines I would say that it is in your env. Did you try using QWindowsStyle in the test? I think that this together with the waitForSignal may solve the problem and we would be sure that the test will always use the same style in all platforms. If you see problems in QPicture or in the QStyle that you use, you should file bugs for them. Anyway, this change log needs to be improved a lot. :)

In this case it could be better to use the same style to every platform only for this test. The problem about this idea is that I should use a QWebView instead of a QWebPage and change its style. Then I've realized that QWebView's setStyle doesn't affect the page scrollbars (reported here: https://bugs.webkit.org/show_bug.cgi?id=61459). So for now I would have to change QApplication style, but it would affect the style theme for all tests. Is it a good idea to make this test to use the same style theme for every platform? Or maybe the current setting is also good as we could detect errors in different platform themes? The current problem of this test failing here (and not in buildbot) is actually a problem on QPicture (I've filed a bug here: http://bugreports.qt.nokia.com/browse/QTBUG-19496) as I've explained previously. A workaround for it is just to avoid to translate before painting in every theme specific way, and this would make it expand correctly to all themes. That's why I've changed the order to make it draw the background first and then translate to draw the scrollbar itself. In addition, this test is called "render" but it is not validating much of rendering. It's only checking its final geometry which is certainly not enough to say whether rendering was done correctly or not, and it is also using a non-reliable source (until now) of geometry to decide (QPicture). So there are many things wrong about it. I want to know if it is really a great idea to have the same theme style for all platforms to verify its geometry, or if I should just wait for QPicture's fix. Anyway, in the meantime, I will be looking for a fix for the bug #61459.

(In reply to comment #5) > In this case it could be better to use the same style to every platform only for this test. The problem about this idea is that I should use a QWebView instead of a QWebPage and change its style. Then I've realized that QWebView's setStyle doesn't affect the page scrollbars (reported here: https://bugs.webkit.org/show_bug.cgi?id=61459). So for now I would have to change QApplication style, but it would affect the style theme for all tests. > > Is it a good idea to make this test to use the same style theme for every platform? Or maybe the current setting is also good as we could detect errors in different platform themes? > > The current problem of this test failing here (and not in buildbot) is actually a problem on QPicture (I've filed a bug here: http://bugreports.qt.nokia.com/browse/QTBUG-19496) as I've explained previously. A workaround for it is just to avoid to translate before painting in every theme specific way, and this would make it expand correctly to all themes. That's why I've changed the order to make it draw the background first and then translate to draw the scrollbar itself. > > In addition, this test is called "render" but it is not validating much of rendering. It's only checking its final geometry which is certainly not enough to say whether rendering was done correctly or not, and it is also using a non-reliable source (until now) of geometry to decide (QPicture). So there are many things wrong about it. > > I want to know if it is really a great idea to have the same theme style for all platforms to verify its geometry, or if I should just wait for QPicture's fix. Anyway, in the meantime, I will be looking for a fix for the bug #61459. I suggested using a fixed style because this problem seems to be in QPicture and in a QStyle. As we are testing QtWebKit and not QPicture or a certain QStyle I think that it makes sense. But the changes that you proposed in ScrollbarThemeQt.cpp are ok. If it is a problem to use a fixed style then just provide a better change log and submit it again for review.

Created attachment 95229 [details] Modified Changelog to summarize changes more efficiently If it's a real consensus that this test should use the same style for every platform, then I think we could create a more efficient test in this format that could compare every pixel to check if the rendering was really correct in all plataforms. For this original test purposes, these changes are sufficient to avoid the bug on QPicture. So I've also changed the name of the test to "renderGeometry", because the previously name really didn't fit what was really tested there.

Comment on attachment 95229 [details] Modified Changelog to summarize changes more efficiently This looks suspicious, you're moving the fillRect before the translate, and this doesn't affect anything but QPicture? o_O

Suppose you have a rect (X,Y) WxH. If you paint it at this moment, you'll fill points from (X,Y) to (X+W,Y+H), right? This is what I'm currently doing. So instead of painting it in the beginning, take the top left point of this rectangle - that would be (X,Y) - and then translate the painter to that point. Now your origin (0,0) is actually (X,Y). If you now get this rect and move it to point (0,0), you'll have the rect: (0,0) WxH. Now let's paint this rect inside the painter and see if we get any difference. As the points inside the painter were translated, for every point (a,b) in the rect, you'll be actually painting (a+X,b+Y) in the original coordinates, and we know the range of a is [0,W] and b is [0,H] because of our translated rect defined previously. Thus, the points painted in the original coordinate system would be (0+X,0+Y) to (W+X,H+Y), that is exactly the same as painting it before this translation. Hopes this helps to clarify.

(In reply to comment #8) > (From update of attachment 95229 [details]) > This looks suspicious, you're moving the fillRect before the translate, and this doesn't affect anything but QPicture? o_O Never mind this, I had a few too many sheets in the wind at the time and didn't notice the QRect::moveTo() call. :)

Created attachment 95453 [details] Same patch as before (for additional comments, see last patch's comments)

Comment on attachment 95453 [details] Same patch as before Curious workaround. r=me

Created attachment 95474 [details] Applied my changes on top of the master branch head (comments already made in the previous patch)

Comment on attachment 95474 [details] Applied my changes on top of the master branch head Clearing flags on attachment: 95474 Committed r87767: <http://trac.webkit.org/changeset/87767>

Revision r87767 cherry-picked into qtwebkit-2.2 with commit f3934e4 <http://gitorious.org/webkit/qtwebkit/commit/f3934e4>

Comment on attachment 95453 [details] Same patch as before Clearing flags to take it our from the pending-commit queue as it appears to me that the correct version of the patch has been landed. Please reopen if this is not the case.