[webkit-reviews] review denied: [Bug 49184] Flawed rendering design for QtWebKit resulting in artifacts being displayed : [Attachment 75410] Proposed Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> 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 75410: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=75410&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75410&action=review

None of the functions in QWebPageClient need to be virtual.

In general I think the approach is correct, but I see one major problem:

The updates that come in during the layout should not be sent to a QWidget
(ownerWidget). First of all,
the coordinates will be wrong (line 352). Think of a QGraphicsView setup where
that widget will be the
viewport and the coordinates need to be adjusted to the view's transformation
and scrollbars.

Secondly we should call QWidget::update() instead of repaint().

Lastly I don't think QWidget should be used at all and instead in
QWebFramePrivate::renderRelativeCoords you should call
void ChromeClientQt::invalidateContentsAndWindow(const IntRect& windowRect,
bool immediate).

That will take care of coordinate issues (QGraphicsWidget::update() will be
called with item-local coordinates
and they will be translate to the correct viewport ones). It will also support
use-cases where people rely
on the correct emission of the QWebPage::repaintRequested() signal. And lastly
it will allow you to do the change
without introducing a new function in the public QWebFrame API.

> WebCore/platform/qt/QWebPageClient.h:98
> +    virtual QRegion getDirtyRegion() const

Just call it dirtyRegion(), we usually don't use the get suffix.

> WebCore/platform/qt/QWebPageClient.h:113
> +    inline bool skipWidgetUpdates()

This function should be const.

> WebCore/platform/qt/QWebPageClient.h:119
> +    inline void addToDirtyRegion(const QRect &rect)

Coding style, the & placement is incorrect.