[webkit-reviews] review granted: [Bug 63950] [Qt][WK2] Split Qt API into two different web views (touch and desktop) : [Attachment 99738] Proposed patch (let's start somewhere)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 5 15:52:13 PDT 2011
Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Andreas Kling
<kling at webkit.org>'s request for review:
Bug 63950: [Qt][WK2] Split Qt API into two different web views (touch and
desktop)
https://bugs.webkit.org/show_bug.cgi?id=63950
Attachment 99738: Proposed patch (let's start somewhere)
https://bugs.webkit.org/attachment.cgi?id=99738&action=review
------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99738&action=review
That is one heck of a patch :-)
Some mostly minor nitpicks that you can take into consideration
Did you guys make a meta bug for all issues? things missing to me added that we
currently have on trunk? hook up AC etc?
It looks good, so let's get it in and work on it from there.
> Source/WebKit2/ChangeLog:37
> + (QDesktopWebViewPrivate::contentSizeChanged):
> + (QDesktopWebViewPrivate::isActive):
> + (QDesktopWebViewPrivate::hasFocus):
> + (QDesktopWebViewPrivate::isVisible):
> + (QDesktopWebViewPrivate::startDrag):
This is a huge refac, I am not sure how valuable these method names
"(QDesktopWebViewPrivate::startDrag):" are. I suggest just leaving the file
names
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:31
> + , page(this, contextRef ? new QWKContext(contextRef) :
defaultWKContext(), pageGroupRef)
I think we need to find out how to handle page groups properly at some point.
He had issue with regard to that in webkit1 for ages.
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:36
> +void QDesktopWebViewPrivate::setViewNeedsDisplay(const QRect&
invalidatedRect)
I like these to be called Area... like invalidatedArea. I can better understand
that visually
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:47
> +{ }
Is this the right style?
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:98
> +#ifndef QT_NO_CURSOR
Didn't we want to get rid of these compile flags in Qt5? Maybe now is the
time... the more ways to compile this, the more slightly incompatible varieties
we will have and more to maintain as well.
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:110
> +void QDesktopWebViewPrivate::loadDidSucceed()
no loadDidFail?
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:193
> +void QDesktopWebView::paint(QPainter *painter, const
QStyleOptionGraphicsItem *option, QWidget *widget)
Why wrong * style here?
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:198
> +bool QDesktopWebView::event(QEvent* e)
resizeEvent(QGraphicsSceneResizeEvent* event) uses event. Here we are using e,
and from our branch I remember ev :-) maybe it is time to settle. Personally I
opt for ev - it is short, and event might be used locally.
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:205
> +WKPageRef QDesktopWebView::pageRef() const
Maybe we want to get rid of these? Isnt the C api already declared failure?
>> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:57
>> + void urlChanged(const QUrl &url);
>
> The parameter name "url" adds no information, so it should be removed.
[readability/parameter_name] [5]
URL api would probably be nice to rethink - it seems there were some issues in
webkit1.
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:67
> + WKPageRef pageRef() const;
OK it is private... It might be possible to create a free function instead?
Does it make sense?
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:59
> +QUrl QTouchWebPage::url() const
what about originalUrl? rethink url api? integration with security origin makes
sense?
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:69
> +class FriendlyWidget : public QWidget
A small comment for why we need this would be nice
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:78
> + // find the view which has the focus:
:-P that is not a proper sentence :-) now please don't shoot me!
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:87
> + QList<QGraphicsView*> views = scene()->views();
> + const int viewCount = views.count();
> + QGraphicsView* focusedView = 0;
> + for (int i = 0; i < viewCount; ++i) {
> + if (views[i]->hasFocus()) {
> + focusedView = views[i];
> + break;
> + }
> + }
This looks like code that I have seem in many places before (not necessarily in
this patch)... refactor out?
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:91
> + FriendlyWidget* friendlyWindow =
static_cast<FriendlyWidget*>(window);
Lets create a toFriendlyWidget and add an ASSERT in it. Better to do that now
that later
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:111
> +bool QTouchWebPage::event(QEvent* e)
e!
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:129
> +void QTouchWebPage::timerEvent(QTimerEvent *event)
event!
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:146
> + , m_isChangingScale(false)
I guess we will end up with a m_isChangingPosition as well... maybe
isSuspended?
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:183
> + m_scaleCommitTimer.start(0.1, q);
constants for these ?
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:2
> +#ifndef qtouchwebpage_h
> +#define qtouchwebpage_h
misses copyright header
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:32
> + // FIXME: should not be public
> + virtual QRectF visibleRect() const;
Can this be fixed already? or what is blocking it?
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:42
> + virtual void timerEvent(QTimerEvent *);
style
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:45
> + Q_SLOT void focusNextPrevChildCallback(bool next);
Any way to make this use Q_SLOTS?
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:50
> + Q_PRIVATE_SLOT(d, void onScaleChanged())
void is supposed to be there?
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:36
> + static QTouchWebPagePrivate* getPageViewPrivate(QTouchWebPage* view) {
return view->d; }
That is very verbose. QTouchWebPagePrivate::obtainFrom(QTouchWebPage*) would
suffice
> Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:37
> + QScopedPointer<QTouchWebPage> pageView;
What is the advantage of QScopedPointer instead of OwnPtr? I find it confusing
to know when to use one of the other. Cant we stick with OwnPtr?
>
Source/WebKit2/UIProcess/API/qt/tests/commonviewtests/tst_commonviewtests.cpp:2
6
> +class tst_CommonViewTests : public QObject {
Great! I love tests!
>
Source/WebKit2/UIProcess/API/qt/tests/commonviewtests/webviewabstraction.cpp:24
> +WebViewAbstraction::WebViewAbstraction()
interesting test
>
Source/WebKit2/UIProcess/API/qt/tests/commonviewtests/webviewabstraction.cpp:48
> + m_touchWebViewWindow.show();
> + m_desktopWebViewWindow.show();
> + QTest::qWaitForWindowShown(&m_desktopWebViewWindow);
At some point it would be nice to test how we are sending events such as
window.onblur/onfocus. Things that require a view implementation and might not
be so easy to test with the layout tests
>
Source/WebKit2/UIProcess/API/qt/tests/qdesktopwebview/tst_qdesktopwebview.cpp:2
7
> + Q_OBJECT
> +private slots:
I would add a newline between these
> Source/WebKit2/UIProcess/API/qt/tests/testwindow.h:29
> +/* TestWindow: Utility class to work ignore QGraphicsView details. */
I dont understand that english
> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:55
> + // TODO
Use FIXME: I only search for FIXME:'s ... we better all use the same
> Source/WebKit2/UIProcess/qt/ClientImpl.h:48
> +void qt_wk_didSameDocumentNavigationForFrame(WKPageRef, WKFrameRef,
WKSameDocumentNavigationType, WKTypeRef, const void* clientInfo);
> +
> +// ui client
I wonder if we should split these in separate files... anyway, not now
> Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.cpp:71
> +{
> + // We ignore the viewport definition on the Desktop.
> +}
I wonder how long :-)
> Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.cpp:73
> +bool QDesktopWebPageProxy::handleEvent(QEvent* e)
ev?
> Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.cpp:102
> + // For some reason mouse press results in mouse hover (which is
FIXME? or NOTE?
> Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.cpp:124
> + m_webPageProxy->handleMouseEvent(NativeWebMouseEvent(e, 1));
maybe add a /* mouse clicks */ infront of the 1
> Source/WebKit2/UIProcess/qt/qtouchwebpageproxy.cpp:33
> + // FIXME: add proper handling of viewport.
> + setResizesToContentsUsingLayoutSize(QSize(980, 980));
:-) I would love that
> Source/WebKit2/UIProcess/qt/qwebpageproxy.h:21
> +#ifndef qwebpageproxy_h
I still believe should should be called QtWebPageProxy. Why? SomethingQt.cpp is
the Qt specific implementation of a Something.h. QSomething is a public Qt
class. QtSomething is a Qt only specific class. As this is already in /qt/
maybe it should have no prefix at all?
Apart from this I dislike the mixture of Qt and WebCore types in this class...
like when I add a method, should I return QRect or IntRect... this is going to
confuse people
> Source/WebKit2/UIProcess/qt/qwkcontext.cpp:87
> +void QWKContext::setIconDatabasePath(const QString& path)
> +{
> + // FIXME: There is currently no way to disable the icon database once
it's enabled.
Maybe such an api is too specialized anyway? I guess we need to do some
handling of this by default and only let browsers change things
> Source/WebKit2/UIProcess/qt/qwkcontext.h:38
> + // Bridge from the C API
> + QWKContext(WKContextRef contextRef, QObject* parent = 0);
Maybe we dont want that
> Source/WebKit2/UIProcess/qt/qwkcontext.h:41
> + QIcon iconForPageURL(const QUrl&) const;
URL? Url? inconsistent
> Source/WebKit2/UIProcess/qt/touchviewinterface.cpp:21
> +#include "touchviewinterface.h"
This is not a public header... why lowercase it - it is very unlike the rest of
webkit
> Source/WebKit2/UIProcess/qt/viewinterface.cpp:37
> + qWarning("Cannot support multiple views");
Multiple views are not supported. qFatal ?
> Tools/MiniBrowser/qt/BrowserWindow.cpp:41
> +// FIXME
?
More information about the webkit-reviews
mailing list