[webkit-reviews] review denied: [Bug 76275] [Qt] [WK2] WebView should use Flickable instead of QScroller to handle positioning : [Attachment 123940] proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jan 26 03:56:12 PST 2012


Simon Hausmann <hausmann at webkit.org> has denied Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 76275: [Qt] [WK2] WebView should use Flickable instead of QScroller to
handle positioning
https://bugs.webkit.org/show_bug.cgi?id=76275

Attachment 123940: proposed patch
https://bugs.webkit.org/attachment.cgi?id=123940&action=review

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


Let's do another round with comments from me and Kenneth :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:148
> +template<typename T> T QQuickWebViewPrivate::readFlickableProperty(const
char* name) const
> +{
> +    ASSERT(flickProvider);
> +    return flickProvider->property(name).value<T>();
> +}
> +
> +void QQuickWebViewPrivate::writeFlickableProperty(const char* name, const
QVariant& value)
> +{
> +    ASSERT(flickProvider);
> +    flickProvider->setProperty(name, value);
> +}

I think the Qt naming conventions here would suggest

    T flickableProperty(const char* name) const

and

    void setFlickableProperty(const char* name, const QVariant& value)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:150
> +void QQuickWebView::handleInput(QInputEvent *event)

I suggest to rename this method. The name is too generic for what it does, i.e.
a keyboard event is input (it's a QInputEvent), but this function doesn't
handle it but just forwards it to the flickable. 

I don't like that this is a "public" method in QQuickWebView, but I guess
that's easier than storing the private in the touch interaction engine for the
moment.

I suggest to rename it to something more specific, such as
handleTouchFlickEvent(QTouchEvent*); (yes, take a touch event, very
specifically)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:154
> +    qApp->notify(d->flickProvider, event);

I think the correct way of writing this would be

    QCoreApplication::sendEvent(d->flickProvider, event);

It is functionally equivalent, but it doesn't require the instance itself and
it's the pattern that is more common instead of calling notify directly.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:161
> +    QMetaObject::invokeMethod(d->flickProvider, "returnToBounds",
Qt::DirectConnection);

Perhaps it would be worth it to resolve the QMetaMethod after flick provider
creation and therefore avoid a string method lookup for every invocation.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:294
> +    QDeclarativeEngine engine(webView);
> +    QDeclarativeComponent component(&engine, webView);

Ugh, creating a separate engine and then reparenting the object out of it is
ugly.

Could you try this instead?

QDeclarativeEngine* engine = qmlEngine(webView);

I _think_ the engine will be available already at that point.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:295
>> +	component.setData("import QtQuick 2.0\nFlickable {}", QUrl());
> 
> comment?

As well as QByteArrayLiteral("import QtQuick...") please :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1083
> +qreal QQuickWebViewExperimental::contentY() const
> +{
> +    Q_Q(const QQuickWebView);
> +    return q->contentY();
> +}

Instead of QQuickWebViewExperimental::contentY() calling
QQuickWebView::contentY() which then calls
QQuickWebViewPrivate::readFlickableProperty() I think it would be better
to eliminate the QQuickWebView::contentY() indirection.

The QQuickWebView::contentY() function - and the other methods like that - are
only called from one place, QQuickWebViewExperimental. The
QQuickWebView::contentY() function also
does _not_ represent public API of any sort, so I think it's easier to get rid
of it and just have

qreal QQuickWebViewExperimental::contentY() const
{
    return d->flickableProperty<qreal>("contentY");
}

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:237
> +    Q_PROPERTY(qreal contentWidth READ contentWidth WRITE setContentWidth
NOTIFY contentWidthChanged)
> +    Q_PROPERTY(qreal contentHeight READ contentHeight WRITE setContentHeight
NOTIFY contentHeightChanged)
> +    Q_PROPERTY(qreal contentX READ contentX WRITE setContentX NOTIFY
contentXChanged)
> +    Q_PROPERTY(qreal contentY READ contentY WRITE setContentY NOTIFY
contentYChanged)
> +    Q_PROPERTY(QQuickItem* contentItem READ contentItem CONSTANT)
> +    Q_PROPERTY(QDeclarativeListProperty<QObject> flickableContent READ
flickableContent)

I would be nice to have a comment here saying that the set of these properties
are essentially "QML Flickable" API.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:88
>> +	void writeFlickableProperty(const char* name, const QVariant& value);
> 
> I guess you could declare those two as inline free functions (maybe in an
anonymous namespace) and avoid cluttering the private with them. Donno if that
is a good idea or not :-)

I would also prefer the template methods to be inline. Out-of-line template
methods do not really work, but in this case they "do" because it's only used
in one compilation unit.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:111
> -    void scrollStateChanged(QScroller::State);
> +    void contentMovingChanged();

I admit that I liked the term scrollStateChanged over contentMovingChanged. But
that's just me :)


More information about the webkit-reviews mailing list