[webkit-reviews] review denied: [Bug 56929] [Qt] Tiled painting still causes synchronous layout when accelerated compositing and texture mapper are enabled : [Attachment 86696] Proposed patch

[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 86696: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=86696&action=review

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