[webkit-reviews] review denied: [Bug 56929] [Qt] Tiled painting still causes synchronous layout when accelerated compositing and texture mapper are enabled : [Attachment 87616] Proposed Patch. Fixed style issues

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Carol Szabo
<carol.szabo at nokia.com>'s request for review:
Bug 56929: [Qt] Tiled painting still causes synchronous layout when accelerated
compositing and texture mapper are enabled
https://bugs.webkit.org/show_bug.cgi?id=56929

Attachment 87616: Proposed Patch. Fixed style issues
https://bugs.webkit.org/attachment.cgi?id=87616&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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>