[webkit-reviews] review denied: [Bug 49184] Flawed rendering design for QtWebKit resulting in artifacts being displayed : [Attachment 75154] Fixed some more style issues

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin.poulain at nokia.com> has denied Carol Szabo
<carol.szabo at nokia.com>'s request for review:
Bug 49184: Flawed rendering design for QtWebKit resulting in artifacts being
displayed
https://bugs.webkit.org/show_bug.cgi?id=49184

Attachment 75154: Fixed some more style issues
https://bugs.webkit.org/attachment.cgi?id=75154&action=review

------- Additional Comments from Benjamin Poulain <benjamin.poulain at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=75154&action=review

> WebCore/ChangeLog:12
> +	   No behavior changes, hence no new tests.
> +	   This patch fixes flickers and artifacts that are hard to reproduce
> +	   on desktop computers.
> +	   The goal of this patch is that every paint call on the WebView
> +	   produces a correct image of the page with no artifacts.

Changelog does not clearly explain the problem, just the consequences. The
following sentence comes out of nowhere without more explanation.

> WebCore/platform/qt/QWebPageClient.h:98
> +    virtual QRect getDirtyRect() const { return dirtyRect;}

Missing space after the semicolon.

> WebCore/platform/qt/QWebPageClient.h:100
> +    virtual void clearDirtyRect() { dirtyRect.setWidth(0); }

This should be dirtyRect = QRect();

> WebCore/platform/qt/QWebPageClient.h:107
> +    QRect dirtyRect;

Why this variable misses the prefix? "m_".

Personally, I would also prefer the variable to be private, and have a
protected method addToDirtyRect(const QRect&). That one could be inline, but at
least we would have some encapsulation.

> WebKit/qt/Api/qgraphicswebview.cpp:309
>  #if USE(ACCELERATED_COMPOSITING) && !USE(TEXTURE_MAPPER)
> -    page()->mainFrame()->render(painter, d->overlay() ?
QWebFrame::ContentsLayer : QWebFrame::AllLayers,
option->exposedRect.toAlignedRect());
> +    GraphicsContext context(painter);
> +    page()->mainFrame()->d->renderRelativeCoords(&context, d->overlay() ?
QWebFrame::ContentsLayer : QWebFrame::AllLayers,
> +						   
QRegion(option->exposedRect.toRect()), QWebFramePrivate::UseDirtyRect);
>  #else
> -    page()->mainFrame()->render(painter, QWebFrame::AllLayers,
option->exposedRect.toRect());
> +    page()->mainFrame()->d->renderRelativeCoords(&context,
QWebFrame::AllLayers,
> +						   
QRegion(option->exposedRect.toRect()), QWebFramePrivate::UseDirtyRect);
>  #endif

I think this would crash if the painter is null, while it would not with the
call to QWebFrame::render().

> WebKit/qt/Api/qwebframe.cpp:364
> +	       return;

Why would you stop rendering if there is no page client?

> WebKit/qt/Api/qwebframe.h:228
> +    friend class QWebView;

I don't get it, why do you add QWebView as a friend of the class here?



I stop here, the patch seems to have been a bit rushed. Please polish it so one
doesn't have to stop at each file :)