[Qt] Merge QTouchWebView and QDesktopWebView into one class

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Christian Sejersen]

There seems to be so many similarities between QTouchWebView and QDesktopWebView that they should be merged into one class. It also seems weird you have to determine (and know) up front whether your application runs on a platform that supports touch or not.

[Jocelyn Turcotte]

We had a chat here are that's what came out.

1. From the point of view of the developer using the WebView it makes more sense to have only one API/documentation with platform specific behaviors defined and documented than having two classes, two documentations and only slight differences between them. Whether you have a desktop behavior or a mobile/touch behavior should be defined by the platform running the application or a compile-time flag.

2. From our point of view, we don't want to have to recompile to be able to switch views, we need a global runtime flag to enable desktop or mobile behavior to run tests or do development. It doesn't make sense to have both a desktop view and a mobile view running in an application at the same time anyway.

3. If we have two different QML API behaviors, this needs to be handled by the same C++ class, not two different C++ classes with somewhat the same methods that are fed to qmlRegisterType depending on the "behavior flag". This will help prevent a developer from running his mobile-aimed application on desktop and get a e.g. [Cannot assign to non-existent property "page"].

So the best way would be to have only one top-level WebView C++ class, that would check this either compile or runtime behavior flag, and would instanciate the proper modules and send the proper settings to the WebProcess (e.g. fixed layout and frame flatening).
Alexis had some plan to implement it I think.

[Alexis Menard (darktears)]

Plan :

- Merge QtDesktopWebPageProxy and QtTouchWebPageProxy into QtWebPageProxy
- Have only one view with a way to switch the "mode" and one page.
- AC on for both case (we'll see later how to optimize the desktop case).

[Alexis Menard (darktears)]

Created attachment 113941 [details]
Patch (landed in r99540)

[Alexis Menard (darktears)]

(In reply to comment #3)
> Created an attachment (id=113941) [details]
> Patch

First step. Merge the WebPageProxies into one.

[WebKit Review Bot]

Attachment 113941 [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

Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:205:  The parameter name "painter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 7 files


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

[Alexis Menard (darktears)]

(In reply to comment #5)
> Attachment 113941 [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
> 
> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:205:  The parameter name "painter" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 1 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Was there before and this function is going to die in the next patch :D. Please ignore for now.

[Simon Hausmann]

Comment on attachment 113941 [details]
Patch (landed in r99540)

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

r=me, but qdesktopwebview.cpp doesn't actually compile because it includes QtDesktopWebPageProxy.h, which your patch removes. Replacing the include makes it compile.

I'll fix this when landing it.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:824
> +void QtWebPageProxy::doneWithTouchEvent(const NativeWebTouchEvent& event, bool wasEventHandled)
> +{

This function crashes when running the touch layout tests. I'll add the missing m_interactionEngine null pointer check when landing it.

Please run the layout tests next time (with WebKitTestRunner) :)

[Simon Hausmann]

Committed r99540: <http://trac.webkit.org/changeset/99540>

[Simon Hausmann]

Committed r99540: <http://trac.webkit.org/changeset/99540>

[Simon Hausmann]

Oopos, sorry, re-opening. I guess this will be multi-patch bug?

[Simon Hausmann]

Comment on attachment 113941 [details]
Patch (landed in r99540)

Clearing flags.

[Alexis Menard (darktears)]

Created attachment 114177 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:78:  q_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.h:127:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 28 files


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

[Alexis Menard (darktears)]

(In reply to comment #12)
> Created an attachment (id=114177) [details]
> Patch

First round. I haven't ported the API tests yet. I just want early feedback.

Feel free to review/comment before I finish cleaning the auto-tests/qml tests.

Thanks.

[Alexis Menard (darktears)]

Comment on attachment 114177 [details]
Patch

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

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:-219
> -        onLoadProgressChanged: loadProgress = viewLoader.item.loadProgress

Fixed locally.

[Kenneth Rohde Christiansen]

Comment on attachment 114177 [details]
Patch

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

Just a quick look through

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.h:52
> +    virtual void focusOutEvent(QFocusEvent*);
> +    virtual void mousePressEvent(QMouseEvent *);
> +    virtual void mouseMoveEvent(QMouseEvent *);
> +    virtual void mouseReleaseEvent(QMouseEvent *);

coding style inconsistencies

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:44
> +    , currentViewMode(QQuickWebView::Desktop)

I think our default should be touch, for too many reasons to mention here :-)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:79
> +void QQuickWebViewPrivate::init(QQuickWebView* viewport, WKContextRef contextRef, WKPageGroupRef pageGroupRef)
> +{
> +    q_ptr = viewport;
> +    viewport->setFlags(QQuickItem::ItemClipsChildrenToShape);
> +    QObject::connect(viewport, SIGNAL(visibleChanged()), viewport, SLOT(onVisibleChanged()));
> +    pageView.reset(new QQuickWebPage(viewport));
> +    viewInterface.reset(new WebKit::QtViewInterface(viewport, pageView.data()));
> +    QQuickWebPagePrivate* const pageViewPrivate = pageView.data()->d;
> +    setPageProxy(new QtWebPageProxy(viewInterface.data(), 0, this, contextRef, pageGroupRef));
> +    pageViewPrivate->setPageProxy(pageProxy.data());
> +    QWebPreferencesPrivate::get(pageProxy->preferences())->setAttribute(QWebPreferencesPrivate::AcceleratedCompositingEnabled, true);
> +    pageProxy->init();
> +    initDesktop(viewport);
> +}

a few newlines would make this a lot more readable

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:81
> +void QQuickWebViewPrivate::initDesktop(QQuickWebView* viewport)

We normally use the word initialize in webkit

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:111
> +    if (currentViewMode == QQuickWebView::Desktop)
> +        return;

We usually add a newline after a return

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:187
> +    if (currentViewMode == QQuickWebView::Desktop)
> +        return;

newline

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:200
> +    disableMouseEvents();
> +    QMessageBox::information(0, title, escapeHtml(alertText), QMessageBox::Ok);
> +    enableMouseEvents();

I dont think we want do ever do this on touch

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:242
> +    // Qt does not support multiple files suggestion, so we get just the first suggestion.

same with this

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:345
> +    d->init(this, contextRef, pageGroupRef);

Here we use d, elsewhere d_ptr... We need to decide

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:392
> +        const_cast<QQuickWebViewPrivate*>(d)->navigationController = new QWebNavigationController(d->pageProxy.data());

Is QWebNavigationController a public API? I was wondering since it is Q* and not Qt*

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:402
> +void QQuickWebView::setViewMode(QQuickWebView::ViewMode mode)

We are not going to support more than 2 (I hope) so maybe this should be a boolean setter instead?

setUseTraditionalDesktopBehaviour(...)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:406
> +        return;

newline

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:411
> +    if (d->currentViewMode == QQuickWebView::Desktop)
> +        d->initDesktop(this);
> +    else
> +        d->initTouch(this);

No resetting needed before initializing?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:97
> +     void onVisibleChanged();

is that one supposed to be public? Doesnt seems like usual public Qt API

> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:101
> +    void statusBarMessageChanged(const QString& message);

I am wondering if we should leave our the "Bar" from that method name, it is so desktop like

> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:110
> +    void viewModeChanged();

viewMode is a very bad name, since there is something called exactly that in a W3C Spec! http://www.w3.org/TR/view-mode/

We might even support that at some point

> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:120
> +    WKPageRef pageRef() const;

Don't we want this in the private ? or a

WKPageRef toPageRef(QQuickWebView*); free method?

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:127
>> +    QScopedPointer<QQuickWebViewPrivate> d_ptr;
> 
> d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

why not just call it d?

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:48
> +    void init(QQuickWebView* viewport, WKContextRef contextRef = 0, WKPageGroupRef pageGroupRef = 0);
> +    void initTouch(QQuickWebView* viewport);
> +    void initDesktop(QQuickWebView* viewport);

initialize

[Christian Sejersen]

(In reply to comment #15)
> (From update of attachment 114177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114177&action=review
> 
> > Tools/MiniBrowser/qt/qml/BrowserWindow.qml:-219
> > -        onLoadProgressChanged: loadProgress = viewLoader.item.loadProgress
> 
> Fixed locally.

Why do we need to switch modes? Can't it be dependent on the events we receive (e.g. mousemove, touch)?

[Christian Sejersen]

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 114177 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114177&action=review
> > 
> > > Tools/MiniBrowser/qt/qml/BrowserWindow.qml:-219
> > > -        onLoadProgressChanged: loadProgress = viewLoader.item.loadProgress
> > 
> > Fixed locally.
> 
> Why do we need to switch modes? Can't it be dependent on the events we receive (e.g. mousemove, touch)?

I believe it is preferable to not have any modes at all (if that wasn't clear from the above)

[Jocelyn Turcotte]

Comment on attachment 114177 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:104
> +    if (currentViewMode == QQuickWebView::Touch)

Maybe we could keep the touch/desktop separation for the private classes and have them inherit from a base private class to prevent this kind of statement from growing all over the place. Or some other kind of proper OO mechanism.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:402
>> +void QQuickWebView::setViewMode(QQuickWebView::ViewMode mode)
> 
> We are not going to support more than 2 (I hope) so maybe this should be a boolean setter instead?
> 
> setUseTraditionalDesktopBehaviour(...)

We talked about it last week and I don't think we should support runtime switching between the two modes. We also thought that it would be better to have a hidden static setting and not to support having two web views with different modes running in the same application. This is only necessary for development, in the end the mode should be determined by the platform.

>>> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:127
>>> +    QScopedPointer<QQuickWebViewPrivate> d_ptr;
>> 
>> d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> why not just call it d?

The d_ptr thing was needed to use q_func, d_func, Q_Q and Q_D and have the subclass' private class inherit from the parent's private. Since we don't inherit and need it anymore, we could replace it with a plain d member.

[Alexis Menard (darktears)]

(In reply to comment #16)
> (From update of attachment 114177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114177&action=review
> 
> Just a quick look through
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.h:52
> > +    virtual void focusOutEvent(QFocusEvent*);
> > +    virtual void mousePressEvent(QMouseEvent *);
> > +    virtual void mouseMoveEvent(QMouseEvent *);
> > +    virtual void mouseReleaseEvent(QMouseEvent *);
> 
> coding style inconsistencies

Fixed.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:44
> > +    , currentViewMode(QQuickWebView::Desktop)
> 
> I think our default should be touch, for too many reasons to mention here :-)

Done.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:79
> > +void QQuickWebViewPrivate::init(QQuickWebView* viewport, WKContextRef contextRef, WKPageGroupRef pageGroupRef)
> > +{
> > +    q_ptr = viewport;
> > +    viewport->setFlags(QQuickItem::ItemClipsChildrenToShape);
> > +    QObject::connect(viewport, SIGNAL(visibleChanged()), viewport, SLOT(onVisibleChanged()));
> > +    pageView.reset(new QQuickWebPage(viewport));
> > +    viewInterface.reset(new WebKit::QtViewInterface(viewport, pageView.data()));
> > +    QQuickWebPagePrivate* const pageViewPrivate = pageView.data()->d;
> > +    setPageProxy(new QtWebPageProxy(viewInterface.data(), 0, this, contextRef, pageGroupRef));
> > +    pageViewPrivate->setPageProxy(pageProxy.data());
> > +    QWebPreferencesPrivate::get(pageProxy->preferences())->setAttribute(QWebPreferencesPrivate::AcceleratedCompositingEnabled, true);
> > +    pageProxy->init();
> > +    initDesktop(viewport);
> > +}
> 
> a few newlines would make this a lot more readable

Done

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:81
> > +void QQuickWebViewPrivate::initDesktop(QQuickWebView* viewport)
> 
> We normally use the word initialize in webkit

Fixed

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:111
> > +    if (currentViewMode == QQuickWebView::Desktop)
> > +        return;
> 
> We usually add a newline after a return

Done

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:187
> > +    if (currentViewMode == QQuickWebView::Desktop)
> > +        return;
> 
> newline
> 

Fixed

> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:200
> > +    disableMouseEvents();
> > +    QMessageBox::information(0, title, escapeHtml(alertText), QMessageBox::Ok);
> > +    enableMouseEvents();
> 
> I dont think we want do ever do this on touch

Let's keep it for now, and isolate into two different private objects.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:242
> > +    // Qt does not support multiple files suggestion, so we get just the first suggestion.
> 
> same with this
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:345
> > +    d->init(this, contextRef, pageGroupRef);
> 
> Here we use d, elsewhere d_ptr... We need to decide

Jocelyn answer, the convenience macro, that we use a lot in the view.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:392
> > +        const_cast<QQuickWebViewPrivate*>(d)->navigationController = new QWebNavigationController(d->pageProxy.data());
> 
> Is QWebNavigationController a public API? I was wondering since it is Q* and not Qt*

It is public.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:402
> > +void QQuickWebView::setViewMode(QQuickWebView::ViewMode mode)
> 
> We are not going to support more than 2 (I hope) so maybe this should be a boolean setter instead?

I guess so.

> 
> setUseTraditionalDesktopBehaviour(...)
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:406
> > +        return;
> 
> newline
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:411
> > +    if (d->currentViewMode == QQuickWebView::Desktop)
> > +        d->initDesktop(this);
> > +    else
> > +        d->initTouch(this);
> 
> No resetting needed before initializing?
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.h:97
> > +     void onVisibleChanged();
> 
> is that one supposed to be public? Doesnt seems like usual public Qt API
> 

Fixed.

> > Source/WebKit2/UIProcess/API/qt/qquickwebview.h:101
> > +    void statusBarMessageChanged(const QString& message);
> 
> I am wondering if we should leave our the "Bar" from that method name, it is so desktop like

+1 but for later.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.h:110
> > +    void viewModeChanged();
> 
> viewMode is a very bad name, since there is something called exactly that in a W3C Spec! http://www.w3.org/TR/view-mode/
> 
> We might even support that at some point

I'll make it private.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.h:120
> > +    WKPageRef pageRef() const;
> 
> Don't we want this in the private ? or a
> 
> WKPageRef toPageRef(QQuickWebView*); free method?

WebKitTestRunner use it.

> 
> >> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:127
> >> +    QScopedPointer<QQuickWebViewPrivate> d_ptr;
> > 
> > d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> why not just call it d?
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:48
> > +    void init(QQuickWebView* viewport, WKContextRef contextRef = 0, WKPageGroupRef pageGroupRef = 0);
> > +    void initTouch(QQuickWebView* viewport);
> > +    void initDesktop(QQuickWebView* viewport);
> 
> initialize

Fixed.

[Kenneth Rohde Christiansen]

Comment on attachment 114177 [details]
Patch

This is a step in the right direction! r=me

[Alexis Menard (darktears)]

Created attachment 114376 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #22)
> Created an attachment (id=114376) [details]
> Patch

The big beast.

QML tests are green and C++ API tests are green. The layout tests seems to run fine on my machine both in debug and release (it helped me to catch a deep bug in AC code).

This is of course a big step, perhaps we will find regressions and I will be happy to help to fix them.

The private part of QQuickWebView could be improved to isolate more desktop related stuff like Jocelyn suggested (the JS alert/confirm/input, upload, and so on). It could be done in a following patch. Maybe also the QML tests could be reorganized a little bit to avoid duplication of some tests now that the view is the same. I'll do it in a following patch.

[Kenneth Rohde Christiansen]

Comment on attachment 114376 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:147
> +    if (ev->type() == QEvent::InputMethod)
> +        return false; // This is necessary to avoid an endless loop in connection with QQuickItem::event().

I guess this will need some changes when we will actually start working on IM support

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:156
> +void QQuickWebPage::itemChange(ItemChange change, const ItemChangeData &data)

coding style - & placement

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:172
> +void QQuickWebPagePrivate::initSceneGraphConnections()

initialize?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:191
> +    this->pageProxy = pageProxy;
> +
> +}

unneeded newline

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:196
> +    if (!item)
> +        return 1.0;

I think webkit style tells you to just return 1

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:180
> +    wkPrefs->setDeviceHeight(720);
> +
> +    WebCore::ViewportAttributes attr = WebCore::computeViewportAttributes(viewportArguments, wkPrefs->layoutFallbackWidth(), wkPrefs->deviceWidth(), wkPrefs->deviceHeight(), wkPrefs->deviceDPI(), availableSize);
> +    WebCore::restrictMinimumScaleFactorToViewportSize(attr, availableSize);
> +    WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable(attr);

You missed some new change here from yesterday

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:59
> +    static QQuickWebViewPrivate *get(QQuickWebView *view)

coding style

[Alexis Menard (darktears)]

Created attachment 114472 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:85:  q_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.h:117:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 51 files


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

[Alexis Menard (darktears)]

Created attachment 114475 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:85:  q_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.h:117:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 54 files


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

[Alexis Menard (darktears)]

Committed r99845: <http://trac.webkit.org/changeset/99845>