[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