Bug 56929

Created attachment 86634 [details] Proposed patch.

Comment on attachment 86634 [details] Proposed patch. R- this one as discussed on IRC.

Created attachment 86696 [details] Proposed patch

Comment on attachment 86696 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=86696&action=review > Source/WebKit/qt/Api/qwebframe.cpp:316 > - renderRelativeCoords(context, (QWebFrame::RenderLayer)(QWebFrame::ScrollBarLayer | QWebFrame::PanIconLayer), clip); > + renderFrameWidgets(context, (QWebFrame::RenderLayer)(QWebFrame::ScrollBarLayer | QWebFrame::PanIconLayer), clip); Maybe change the signature to use QFlags and avoid the ugly cast? > Source/WebKit/qt/Api/qwebframe.cpp:391 > +void QWebFramePrivate::renderFrameWidgets(GraphicsContext* context, QWebFrame::RenderLayer layer, QRegion clip) I think the name Widgets is missleading here because of the PanIconLayer (and someone could think page's widgets are rendered here). What about: ::renderFrameOverlayLayers() or simply ::renderFrameScollbarAndPanIconLayer() :) > Source/WebKit/qt/Api/qwebframe.cpp:394 > + if (!layer & (QWebFrame::PanIconLayer | QWebFrame::ScrollBarLayer)) > + return; This could be an assertion instead of a runtime check. If someone call the method with layer == Content, he clearly made a mistake and should be notified. In QWebFramePrivate::renderRelativeCoords(), you would mask the layer to get the right flags and make the call explicit.

I just had an idea for testing this, you tell me if that works :) 1) have a white page containing a big iframe without border but with scrollbar 2) render the top left corner (let say (0, 0, 100x100), it should only contain white since there is no content inside the iframe 3) resize the iframe programmatically so the scrollbars enter the top left corner 4) change the background color of the mainframe to green 4) re-render the top left rect. Check if all pixels are still white Without your patch, the scrollbar of iframe will be in the way, over a white background. With your patch, we should get only the white background.

(In reply to comment #4) > (From update of attachment 86696 [details]) > > + renderFrameWidgets(context, (QWebFrame::RenderLayer)(QWebFrame::ScrollBarLayer | QWebFrame::PanIconLayer), clip); > Maybe change the signature to use QFlags and avoid the ugly cast? I would gladly do that, but I would like to do that on renderRelativeCoords as well in this case. Shouldn't I? > > Source/WebKit/qt/Api/qwebframe.cpp:391 > > +void QWebFramePrivate::renderFrameWidgets(GraphicsContext* context, QWebFrame::RenderLayer layer, QRegion clip) > I think the name Widgets is missleading here because of the PanIconLayer (and someone could think page's widgets are rendered here). > What about: > ::renderFrameOverlayLayers() I thought about OverlayLayers, but I found it more confusing since it might refer to content overlay layers. > or simply ::renderFrameScollbarAndPanIconLayer() :) I considered this name as well, but I thought it is too long and too specific, other things such as title bar, title bar buttons, or who knows what might be included. I also considered renderFrameDecorations and renderFrameControls but that may be confusing as well. What do you think about renderFrameExtras / renderFrameVisuals ? > > Source/WebKit/qt/Api/qwebframe.cpp:394 > > + if (!layer & (QWebFrame::PanIconLayer | QWebFrame::ScrollBarLayer)) > > + return; > This could be an assertion instead of a runtime check. This is a runtime check copied from renderRelativeCoords. It cannot simply be turned into an ASSERT as renderRelativeCoords can cause false assertions. I moved it here since I considered that the robustness added by it overweighs its low cost. I figured that most callers would have to do the test anyway so I save some memory by putting it here, rather then to do the check before every call. If you want I can totally remove the check in which case I uselessly walk through the rects of the region painting nothing if the function is called with neither of the interesting layers specified, or I can move the test to renderRelativeCoords and put an ASSERT in the function. > If someone call the method with layer == Content, he clearly made a mistake and should be notified. In QWebFramePrivate::renderRelativeCoords(), you would mask the layer to get the right flags and make the call explicit. I do not understand the last sentence above.

(In reply to comment #4) > > + renderFrameWidgets(context, (QWebFrame::RenderLayer)(QWebFrame::ScrollBarLayer | QWebFrame::PanIconLayer), clip); > Maybe change the signature to use QFlags and avoid the ugly cast? In my previous comment I meant to use QFlags wherever RenderLayer is used and can be potentially orred including the interface methods QFrame::render.

(In reply to comment #5) > I just had an idea for testing this, you tell me if that works :) > 1) have a white page containing a big iframe without border but with scrollbar > 2) render the top left corner (let say (0, 0, 100x100), it should only contain white since there is no content inside the iframe > 3) resize the iframe programmatically so the scrollbars enter the top left corner > 4) change the background color of the mainframe to green > 4) re-render the top left rect. Check if all pixels are still white > Without your patch, the scrollbar of iframe will be in the way, over a white background. > With your patch, we should get only the white background. I assume that you want me to do the second render call on the IFRAME before passing through the message loop after the code that changed the size was run so that backing store refresh and layout do not have a chance to happen. In this case I think that the scrollbars will show in the rendered area (if at all) only if they existed before the size change, as in the absence of layout there is noone to create them, so the patch would make a difference only if scrollbars did not exist before the resize. I do not see why an IFRAME would be useful, furthermore I see it problematic, because the IFRAMEs may not get backingStores, thus the patch would not make a difference. If the IFRAME get's a backing store, the image would be ugly with and without my patch as the backing store of the parent would not be repainted, thus the scrollbars would show both with and without my patch in the middle of previous IFRAME content. The test case I would imagine is a webpage with a Q(Graphics)WebView sized to 100x100 pix showing a webPage with white background containing a single empty div with: width 50; height 50; background-color: green. Resize this div to 150x150 pix and call Q(Graphics)WebView::paint right away. The same picture as before the resize should show (no scroll bars and white in the bottom and right halves), stay in the event loop a few seconds, call Q(Graphics)WebWebView::paint again, and the image should show the scrollbars and green all over. Before the patch in the first call scrollbars would show with white in the bottom and right areas which is an invalid state, the second call would be the same in both cases. Does this test look goog to you?

(In reply to comment #6) > > Maybe change the signature to use QFlags and avoid the ugly cast? > > I would gladly do that, but I would like to do that on renderRelativeCoords as well in this case. Shouldn't I? Yep, sounds reasonable. > What do you think about renderFrameExtras / renderFrameVisuals ? Yep, I like both, I let you pick the best one. > > > Source/WebKit/qt/Api/qwebframe.cpp:394 > > > + if (!layer & (QWebFrame::PanIconLayer | QWebFrame::ScrollBarLayer)) > > > + return; > > This could be an assertion instead of a runtime check. > > This is a runtime check copied from renderRelativeCoords. It cannot simply be turned into an ASSERT as renderRelativeCoords can cause false assertions. Yep, I was obviously thinking about changing also the way it is called, like masking the layers. > I moved it here since I considered that the robustness added by it overweighs its low cost. I figured that most callers would have to do the test anyway so I save some memory by putting it here, rather then to do the check before every call. If you want I can totally remove the check in which case I uselessly walk through the rects of the region painting nothing if the function is called with neither of the interesting layers specified, or I can move the test to renderRelativeCoords and put an ASSERT in the function. I was not talking about performance but code robustness. Your call on that, I don't mind the if() if you think it is more robust..

Created attachment 87612 [details] Proposed Patch. With Test. Addressed benjamin's concerns.

Attachment 87612 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebframe.cpp', u'Sou..." exit_code: 1 Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:221: Missing spaces around / [whitespace/operators] [3] Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:223: Missing spaces around / [whitespace/operators] [3] Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:226: Missing spaces around / [whitespace/operators] [3] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 87616 [details] Proposed Patch. Fixed style issues

(In reply to comment #12) > Created an attachment (id=87616) [details] > Proposed Patch. Fixed style issues Have you tried WebKit-patch to upload your patches? It runs the style checker automatically. It is a neat tool.

Comment on attachment 87616 [details] Proposed Patch. Fixed style issues View in context: https://bugs.webkit.org/attachment.cgi?id=87616&action=review > Source/WebKit/qt/tests/qgraphicswebview/resources/56929.html:7 > +// resizeDiv(); ?? > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:25 > +#if defined(ENABLE_TILED_BACKING_STORE) && ENABLE_TILED_BACKING_STORE You can remove the #ifdefine here. This will still build without ENABLE_TILED_BACKING_STORE so we can avoid adding complexity here.

Comment on attachment 87616 [details] Proposed Patch. Fixed style issues View in context: https://bugs.webkit.org/attachment.cgi?id=87616&action=review I like that you made the test. > Source/WebKit/qt/Api/qwebframe.cpp:393 > + if (!layers & (QWebFrame::PanIconLayer | QWebFrame::ScrollBarLayer)) I am pretty sure ! has precedence over &. If not, parenthesis would just make thing clearer and avoided me from saying false statement on bugs :) > Source/WebKit/qt/Api/qwebframe_p.h:107 > + void renderFrameExtras(WebCore::GraphicsContext*, QFlags<QWebFrame::RenderLayer>, QRegion clip); const QRegion& by convention >> Source/WebKit/qt/tests/qgraphicswebview/resources/56929.html:7 >> +// resizeDiv(); > > ?? ?? >> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:25 >> +#if defined(ENABLE_TILED_BACKING_STORE) && ENABLE_TILED_BACKING_STORE > > You can remove the #ifdefine here. This will still build without ENABLE_TILED_BACKING_STORE so we can avoid adding complexity here. You can remove the #ifdefine here. This will still build without ENABLE_TILED_BACKING_STORE so we can avoid adding complexity here. > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:205 > + QGraphicsView view(new QGraphicsScene(QRectF(0.0, 0.0, 100.0, 100.0))); Aren't you leaking the QGraphicsScene? I don't think the scene is reparented to the view since you can have multiple views for a single scene. > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:208 > + webView.setGeometry(view.sceneRect()); This looks strange, isn't the sceneRect() depending on what is in the scene() (so webView in this case)? > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:217 > + // This will not paint anything as the tiles are not ready <Kenneth>. at the end</Kenneth>

(In reply to comment #15) > > + if (!layers & (QWebFrame::PanIconLayer | QWebFrame::ScrollBarLayer)) > I am pretty sure ! has precedence over &. Sorry, I was mostly just copying code here and I missed the issue. > >> Source/WebKit/qt/tests/qgraphicswebview/resources/56929.html:7 > >> +// resizeDiv(); This was here for easy debugging, I'll take it out, this is also why I put the page as a resource, so that it can be easily loaded in a browser to see what I am after. > >> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:25 > >> +#if defined(ENABLE_TILED_BACKING_STORE) && ENABLE_TILED_BACKING_STORE > > > > You can remove the #ifdefine here. This will still build without ENABLE_TILED_BACKING_STORE so we can avoid adding complexity here. Yes, it will build without TILED_BACKING_STORE enabled, but it will fail at runtime, because the assertions do not hold if TILED_BACKING_STORE is not enabled. Regular rendering requires layout in paint, which will show the scrollbars and an inconsistent image (remember bug 49184, still did not get around to it, and probably I never will as I am getting ready to leave the WebKit arena). > > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:205 > > + QGraphicsView view(new QGraphicsScene(QRectF(0.0, 0.0, 100.0, 100.0))); > Aren't you leaking the QGraphicsScene? I don't think the scene is reparented to the view since you can have multiple views for a single scene. Yes, I am. Originally I constructed the scene passing view as its parent, but that caused a crash since the view was not yet initialized. I later removed the parent and have not accounted for that. > > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:208 > > + webView.setGeometry(view.sceneRect()); > > This looks strange, isn't the sceneRect() depending on what is in the scene() (so webView in this case)? Actually not according to the 4.7 doc. What you are describing only applies if the said rect has not been explicitly set which I did in this case. (http://doc.qt.nokia.com/latest/qgraphicsscene.html#sceneRect-prop). > > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:217 > > + // This will not paint anything as the tiles are not ready > > <Kenneth>. at the end</Kenneth> I like this it shows a really careful review (send my best to Kenneth, but this kind of review item is Darin Adler's trademark, I believe ;-)).

Created attachment 87780 [details] Patch

Comment on attachment 87780 [details] Patch Looks good.

Comment on attachment 87780 [details] Patch Clearing flags on attachment: 87780 Committed r82676: <http://trac.webkit.org/changeset/82676>

All reviewed patches have been landed. Closing bug.