Bug 63950 – [Qt][WK2] Split Qt API into two different web views (touch and desktop)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Andreas Kling]

As discussed on webkit-qt, we're throwing out QGraphicsWKView/QWKPage and introducing QTouchWebView & QDesktopWebView.
We'll be starting with something fairly minimal, and build them up incrementally.

[Andreas Kling]

Created attachment 99738 [details]
Proposed patch (let's start somewhere)

[WebKit Review Bot]

Attachment 99738 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Last 3072 characters of output:
tespace/braces] [4]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview_p.h:50:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview_p.h:50:  The parameter name "dropAction" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/qt/qwebpageproxy.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/qwebpageproxy.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/qwebpageproxy.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/qwebpageproxy.cpp:435:  More than one command on the same line  [whitespace/newline] [4]
Source/WebKit2/UIProcess/API/qt/tests/qtouchwebview/tst_qtouchwebview.cpp:23:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:43:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:70:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:49:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:57:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:60:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:61:  The parameter name "painter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:61:  The parameter name "option" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:61:  The parameter name "widget" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:40:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 56 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 99738 [details]
Proposed patch (let's start somewhere)

View in context: https://bugs.webkit.org/attachment.cgi?id=99738&action=review

> Tools/MiniBrowser/qt/BrowserWindow.cpp:345
>  void BrowserWindow::toggleDisableJavaScript(bool enable)
>  {
> +#if 0
> +    // FIXME
>      page()->preferences()->setAttribute(QWKPreferences::JavascriptEnabled, !enable);
> +#endif
>  }
>  
>  void BrowserWindow::toggleAutoLoadImages(bool enable)
>  {
> +#if 0
> +    // FIXME
>      page()->preferences()->setAttribute(QWKPreferences::AutoLoadImages, !enable);
> +#endif
>  }

We should probably remove those actions entirely. We will probably not add them back as public API.

[Andreas Kling]

(In reply to comment #3)
> (From update of attachment 99738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99738&action=review
> 
> > Tools/MiniBrowser/qt/BrowserWindow.cpp:345
> >  void BrowserWindow::toggleDisableJavaScript(bool enable)
> >  {
> > +#if 0
> > +    // FIXME
> >      page()->preferences()->setAttribute(QWKPreferences::JavascriptEnabled, !enable);
> > +#endif
> >  }
> >  
> >  void BrowserWindow::toggleAutoLoadImages(bool enable)
> >  {
> > +#if 0
> > +    // FIXME
> >      page()->preferences()->setAttribute(QWKPreferences::AutoLoadImages, !enable);
> > +#endif
> >  }
> 
> We should probably remove those actions entirely. We will probably not add them back as public API.

Fair point. Let's collect more feedback before we reiterate though.

[Kenneth Rohde Christiansen]

Comment on attachment 99738 [details]
Proposed patch (let's start somewhere)

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:26
> +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:27
> +    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

?

[Andreas Kling]

(In reply to comment #5)
> (From update of attachment 99738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99738&action=review
> 
> 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?

Good idea, I'll make it happen!

> This is a huge refac, I am not sure how valuable these method names "(QDesktopWebViewPrivate::startDrag):" are. I suggest just leaving the file names

Fair point.

> > 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.

While I do agree, this logic is carried over from the old WK2 API, and needed for WTR to work properly.

> > 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

I can live with that.

> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:47
> > +{ }
> 
> Is this the right style?

Doubtful. Will tweak.

> > 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.

I recall us discussing that last month.. not a bad idea IMO. Let's do it.

> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:110
> > +void QDesktopWebViewPrivate::loadDidSucceed()
> 
> no loadDidFail?

It's coming in a following patch. We were debating how to represent the error condition, and decided it would be better do do as a separate patch.

> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:193
> > +void QDesktopWebView::paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget)
> 
> Why wrong * style here?

Will fix.

> > 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.

Personally I dislike 'event' because it shadows event(). 'ev' feels like an abbreviation which WebKit doesn't use, but I'm not too fussed about this. :)

> > 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?

I'm not entirely sure. This is needed for WTR right now. We need to find a better solution either way, since we don't want to expose WK2 C types in our public headers.

> >> 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.

Indeed. Follow-up material though.

> > 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?

Maybe. Follow-up material.

> 
> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:59
> > +QUrl QTouchWebPage::url() const
> 
> what about originalUrl? rethink url api? integration with security origin makes sense?

Let's rethink. Follow-up material.

> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:69
> > +class FriendlyWidget : public QWidget
> 
> A small comment for why we need this would be nice

Sure.

> > 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!

Don't be such a Kenneth! :D

> > 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

Sure.

> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:111
> > +bool QTouchWebPage::event(QEvent* e)
> 
> e!

ev!

> 
> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:129
> > +void QTouchWebPage::timerEvent(QTimerEvent *event)
> 
> event!

ev!!!!1111

> > 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?

I imagine this will change a lot with upstreaming of the WK2/mobile branch.

> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:183
> > +        m_scaleCommitTimer.start(0.1, q);
> 
> constants for these ?

Good point, will do.

> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:2
> > +#ifndef qtouchwebpage_h
> > +#define qtouchwebpage_h
> 
> misses copyright header

Oops!

> > 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?

Not sure. Carried over verbatim.

> > 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?

I tend to agree. Benjamin?

> > Source/WebKit2/UIProcess/API/qt/tests/testwindow.h:29
> > +/* TestWindow: Utility class to work ignore QGraphicsView details. */
> 
> I dont understand that english

I'll danishize it for you.

> > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:55
> > +        // TODO
> 
> Use FIXME: I only search for FIXME:'s ... we better all use the same

Yep.

> > 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

Probably. 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?

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

Good idea.

> > Source/WebKit2/UIProcess/qt/qtouchwebpageproxy.cpp:33
> > +    // FIXME: add proper handling of viewport.
> > +    setResizesToContentsUsingLayoutSize(QSize(980, 980));
> 
> :-) I would love that

It's coming!

> > 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?

QtWebPageProxy works for me. I always disliked "QNetworkReplyHandler" too.

> 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

Mmh. Yes. Given that it's a WebKit::PageClient subclass, we can't really avoid using WebCore types. Maybe we should use them wholesale. Follow-up material.

> > 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

Aye. Hopefully we can do this nicely behind the scenes. Definitely follow-up material.

> > Source/WebKit2/UIProcess/qt/qwkcontext.h:38
> > +    // Bridge from the C API
> > +    QWKContext(WKContextRef contextRef, QObject* parent = 0);
> 
> Maybe we dont want that

Probably not.

> > Source/WebKit2/UIProcess/qt/qwkcontext.h:41
> > +    QIcon iconForPageURL(const QUrl&) const;
> 
> URL? Url? inconsistent

True. Carried over from existing QWKContext implementation. And follow-up material. :)

> > 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

Good catch. Will tweak!

> > Source/WebKit2/UIProcess/qt/viewinterface.cpp:37
> > +        qWarning("Cannot support multiple views");
> 
> Multiple views are not supported. qFatal ?

Not sure... Let's discuss it separately.

[Simon Hausmann]

Comment on attachment 99738 [details]
Proposed patch (let's start somewhere)

View in context: https://bugs.webkit.org/attachment.cgi?id=99738&action=review

I think the concept is good. I suggest to fix the style issues (style ews bot reports some valid ones) and add the missing file before landing this...

> Source/WebKit2/UIProcess/qt/touchviewinterface.h:30
> +
> +class TouchViewInterface : public ViewInterface
> +{

Class names that don't have the Q prefix should be in a namespace. I'd pick WebKit :)

> Source/WebKit2/WebKit2.pro:248
> +    UIProcess/qt/viewinterface.h \

I doubt this patch builds, because it seems that viewinterface.h is missing. Shows that the ews bots don't build qt-wk2 :)

[Andreas Kling]

Committed r90458: <http://trac.webkit.org/changeset/90458>

[Yael]

(In reply to comment #8)
> Committed r90458: <http://trac.webkit.org/changeset/90458>

This patch reverted http://trac.webkit.org/changeset/90095 .
Was that intentional?

[Benjamin Poulain]

(In reply to comment #9)
> (In reply to comment #8)
> > Committed r90458: <http://trac.webkit.org/changeset/90458>
> 
> This patch reverted http://trac.webkit.org/changeset/90095 .
> Was that intentional?

Nope. For some reason, 90095 was not on git. Let's fix that.