[webkit-reviews] review denied: [Bug 28862] [Qt][API] Add a new QGraphicsWidget based version of the "QWebView" : [Attachment 39165] patch 1 - v0.2.1 - add QWebGraphicsItem to the API and QGVLauncher sample application

[bugzilla-daemon at webkit.org]

Ariya Hidayat <ariya.hidayat at trolltech.com> has denied Antonio Gomes
(tonikitoo) <tonikitoo at gmail.com>'s request for review:
Bug 28862: [Qt][API] Add a new QGraphicsWidget based version of the "QWebView"
https://bugs.webkit.org/show_bug.cgi?id=28862

Attachment 39165: patch 1 - v0.2.1 - add QWebGraphicsItem to the API and
QGVLauncher sample application 
https://bugs.webkit.org/attachment.cgi?id=39165&action=review

------- Additional Comments from Ariya Hidayat <ariya.hidayat at trolltech.com>

> +#include "qwebpage_p.h"

Is this really needed?

> +#include <QGraphicsScene>
> +#include <QGraphicsView>

Can we use the <QtGui/Foo> pattern also here?

> +    setFlag(QGraphicsItem::ItemUsesExtendedStyleOption, true);

This is Qt 4.6 only. Please guard it with QT_VERSION properly for <= 4.5.

> +    if (this->progress == progress / 100.0)

Please use qFuzzyCompare.

> +void QWebGraphicsItemPrivate::_q_doScroll(int dx, int dy, const QRect&
rectToScroll)
> +{
> +    q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> +}

I found out that this does not really work if I subclass QWebGraphicsItem.
An example: I have my customized QWebGraphicsItem that clips the painting so
that the corners are rounded. When this function is invoked, say scrolling
vertically, the whole painted item is scrolled, meaning the bottom rounded
corners are also scrolled. The correct thing of course that the _contents_ of
the web page is scrolled, not merely the item.
The real fix would be to call update, instead of scroll. I understand that this
is optimization issue, but I'd rather have a correctly painted item than a fast
but garbage one.

> +bool QWebGraphicsItem::interactive() const

Following Qt-ish naming, this should be isInteractive instead.

> +    emit interactiveChanged();

The name could be misleading. What about interactivityChanged?

> +++ b/WebKit/qt/Api/qwebgraphicsitem_p.h

Is there any reason we need separate file for this? Unline other qweb* classes,
QWebGraphicsItemPrivate is not need by anyone else, right?

Overall, looks really good!

r- until the issues above are addressed.