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

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> 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 39117: patch 1 - v0.1 - add QWebGraphicsItem to the API and
QGVLauncher sample application.
https://bugs.webkit.org/attachment.cgi?id=39117&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
Looks good to me in principle, just a few minor things that should be fixed
IMHO before landing.

>     * Api/headers.pri:
>     * Api/qwebgraphicsitem.cpp: Added.
>     (QWebGraphicsItem::QWebGraphicsItem):
>     (QWebGraphicsItem::~QWebGraphicsItem):
[...lots of functions here...]

I think you can leave out the long list of functions from the changelog, it's
just bloat :)


> +/*!
> +    \brief The WebPage item allows you to add web content to a canvas.
> +    \inherits QGraphicsWidget.

Please add a \since 4.6 tag here


> +void QWebGraphicsItem::paint(QPainter* painter, const
QStyleOptionGraphicsItem* option, QWidget*)
> +{
> +    page()->mainFrame()->render(painter, option->exposedRect.toRect());
> +}

I think the paint() function should get a /*! \reimp */ comment, to make it
clear in the API docs that this
is just a re-implementation.

> +/*!
> +    Makes \a page the new web page of the web view.
> +
> +    The parent QObject of the provided page remains the owner
> +    of the object. If the current document is a child of the web
> +    view, it will be deleted.
> +
> +    \sa page()

The docs here say "web view" but should say "web graphicsitem".

> +void QWebGraphicsItem::setUrl(const QUrl &url)
> +{
> +    if (url == page()->mainFrame()->url())

I think it's better to avoid the call to page() here and instead
simply write:

    if (d->page && d->page->mainFrame()->url() == url)

> +/*!
> +  \property QWebGraphicsItem::textSizeMultiplier
> +  \brief the scaling factor for all text in the frame
> +  \obsolete
> +
> +  Use setZoomFactor instead, in combination with the
> +  ZoomTextOnly attribute in QWebSettings.
> +
> +  \note Setting this property also enables the
> +  ZoomTextOnly attribute in QWebSettings.
> +
> +  By default, this property contains a value of 1.0.
> +*/

Please don't add this property. It's marked with \obsolete, there's no reason
to add
new stuff that is already obsolete :-)

> +void QWebGraphicsItem::updateGeometry()
> +{
> +    QGraphicsWidget::updateGeometry();
> +    QSize size = geometry().size().toSize();
> +    page()->setViewportSize(size);
> +}
> +
> +void QWebGraphicsItem::setGeometry(const QRectF& rect)
> +{
> +    QGraphicsWidget::setGeometry(rect);
> +    // NOTE: call geometry() as setGeometry ensures that
> +    // the geometry is within legal bounds (minimumSize, maximumSize)
> +    QSize size = geometry().size().toSize();
> +    page()->setViewportSize(size);
> +}

If those two functions are re-implementations from QGraphicsLayoutItem, then
they should be marked in the docs with /*! \reimp */

> +QString QWebGraphicsItem::status() const
> +{
> +    return d->statusBarMessage;
> +}

This function is missing documentation.


> +/*!
> +    Sets the content of the web view to the specified \a html.
> +
> +    External objects such as stylesheets or images referenced in the HTML
> +    document are located relative to \a baseUrl.
> +
> +    The \a html is loaded immediately; external objects are loaded
asynchronously.
> +
> +    When using this method, WebKit assumes that external resources such as
> +    JavaScript programs or style sheets are encoded in UTF-8 unless
otherwise
> +    specified. For example, the encoding of an external script can be
specified
> +    through the charset attribute of the HTML script tag. Alternatively, the

> +    encoding can also be specified by the web server.
> +
> +    \sa load(), setContent(), QWebFrame::toHtml()
> +*/

The docs here mentioned "web view" again, where it should be "web
graphicsitem". I see
this is also the case in some of the other functions.

> +void QWebGraphicsItem::hoverMoveEvent(QGraphicsSceneHoverEvent* ev)
> +{
> +    if (d->interactive && d->page) {
> +	   const bool accepted = ev->isAccepted();
> +	   QMouseEvent me = QMouseEvent(QEvent::MouseMove,
> +		   ev->pos().toPoint(), Qt::NoButton,
> +		   Qt::NoButton, Qt::NoModifier);
> +	   d->page->setView(ev->widget());
> +	   d->page->event(&me);
> +	   ev->setAccepted(accepted);
> +    }
> +
> +    if (!ev->isAccepted())
> +	   QGraphicsItem::hoverMoveEvent(ev);
> +}
> +
> +void QWebGraphicsItem::hoverLeaveEvent(QGraphicsSceneHoverEvent* ev)
> +{
> +    Q_UNUSED(ev);
> +}

These two re-implementations are missing the \reimp tag in the docs.

> +void QWebGraphicsItem::keyPressEvent(QKeyEvent* ev)

Ditto :-)




> +    virtual void setGeometry(const QRectF& rect);
> +    virtual void updateGeometry();
> +    void paint(QPainter*, const QStyleOptionGraphicsItem* options, QWidget*
widget = 0);

I like the use of the virtual keyword in header files for re-implemented
functions, to
indicate that it's a re-implementation. I think you forgot it for paint()
though :-)


> +    QGraphicsScene* scene() { return m_scene; }
> +    QWebGraphicsItem* webItem() { return m_item; }

const missing? :)


It is a bit unfortunate that the QGVLauncher duplicates code from QtLauncher.
It
might be a good thing to clean up in the future.

> diff --git a/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
b/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
> index d659833..e79ae55 100644
> --- a/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
> +++ b/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
> @@ -45,6 +45,7 @@
>  #include "qwebframe_p.h"
>  #include "qwebsecurityorigin.h"
>  #include "qwebsecurityorigin_p.h"
> +#include "qwebview.h"
>  
>  #include <qtooltip.h>
>  #include <qtextdocument.h>
> @@ -307,8 +308,8 @@ void ChromeClientQt::repaint(const IntRect& windowRect,
bool contentChanged, boo
>  {
>      // No double buffer, so only update the QWidget if content changed.
>      if (contentChanged) {
> -	   QWidget* view = m_webPage->view();
> -	   if (view) {
> +	   // Only do implicit paints for QWebView's
> +	   if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view())) {
>	       QRect rect(windowRect);
>	       rect = rect.intersected(QRect(QPoint(0, 0),
m_webPage->viewportSize()));
>	       if (!rect.isEmpty())
> @@ -323,8 +324,8 @@ void ChromeClientQt::repaint(const IntRect& windowRect,
bool contentChanged, boo
>  
>  void ChromeClientQt::scroll(const IntSize& delta, const IntRect&
scrollViewRect, const IntRect&)
>  {
> -    QWidget* view = m_webPage->view();
> -    if (view)
> +    // Only do implicit paints for QWebView's
> +    if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view()))
>	   view->scroll(delta.width(), delta.height(), scrollViewRect);
>      emit m_webPage->scrollRequested(delta.width(), delta.height(),
scrollViewRect);
>  }

Hmm, I'm not so sure about these two changes. The WebGraphicsItem doesn't call
setView(), right? So why is this needed?


Oh, I just also forgot one more thing: The QWebGraphicsItem destructor calls
setView(0) - should it do that? :)