Bug 70315 – [Qt][WK2] Rewrite MiniBrowser in QML

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

[Zeno Albisser]

The current MiniBrowser is not working properly on most platforms.
On Linux the address bar is usually not responsive and on Mac no content is being displayed at all.
The reason for that are mostly the refactorings in Qt5.
We should therefore rewrite the MiniBrowser and use the latest technology provided in Qt5.

[Gopal Raghavan]

duplicate of https://bugs.webkit.org/show_bug.cgi?id=70368

[Zeno Albisser]

*** Bug 70368 has been marked as a duplicate of this bug. ***

[Zeno Albisser]

Created attachment 111805 [details]
Patch to remove QAction.

This is the first in a series of patches to rewrite QtWebKit MiniBrowser.

[Zeno Albisser]

Created attachment 111836 [details]
patch for review.

[Zeno Albisser]

Created attachment 111837 [details]
patch for review.

fixed ChangeLog diff

[Tor Arne Vestbø]

Comment on attachment 111837 [details]
patch for review.

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

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:334
> +    forceActiveFocus();

This change should probably be landed by its own, in both the touch and desktop webview, with an explaination of the reasoning.

> Tools/MiniBrowser/qt/BrowserWindow.cpp:47
> +    rootObject()->setProperty("printLoadedUrls", options->printLoadedUrls);

We probably want to expose one single "options" QObject so we can easily add more options.

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:152
> +        qmlRegisterType<QTouchWebView>("QtQuick", 2, 0, "BrowserView");
> +    } else
> +        qmlRegisterType<QDesktopWebView>("QtQuick", 2, 0, "BrowserView");

We should use the public QML apis, not the C++ APIs.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:28
> +        anchors.top: parent.top
> +        anchors.left: parent.left
> +        anchors.right: parent.right

Style: whenever there's more than one property set, used grouped syntax anchors { }

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:37
> +                width:  height

Style, no aligning of variables (remove extra space)

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:63
> +                width:  height

style

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:88
> +                height: navigationBar.height-2

Style, spacing, same rules as c++

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:94
> +                    source: navigation.canStop ? "../icons/stop.png" : "../icons/refresh.png"

You don't need ../ here, the icon should be available through qrc as "icons/stop.png"

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:116
> +            anchors.left: controlsRow.right
> +            anchors.right: parent.right
> +            anchors.margins: 2
> +            anchors.verticalCenter: parent.verticalCenter

Style: whenever there's more than one property set, used grouped syntax anchors { }

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:117
> +            height: navigationBar.height-4

Spacing

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:123
> +                width: parent.width/100*browserView.loadProgress

Spacing

[Zeno Albisser]

Created attachment 111980 [details]
patch for review.

[Kenneth Rohde Christiansen]

Comment on attachment 111980 [details]
patch for review.

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

> Source/WebKit2/ChangeLog:6
> +        Add a first QML based implementation of MiniBrowser.

Bit weird English. Add the first QML based implementation of MiniBrowser

> Tools/ChangeLog:6
> +
> +

remove double newline

> Tools/MiniBrowser/qt/BrowserWindow.cpp:64
> +{
> +    QObject* webView = rootObject()->property("webView").value<QDesktopWebView*>();
> +    if (!webView)
> +        webView = rootObject()->property("webView").value<QTouchWebView*>();
> +    return webView;
> +}

I guess a comment would be good here. Why would there not always be a desktop view?

> Tools/MiniBrowser/qt/BrowserWindow.cpp:87
> +        QMetaObject::invokeMethod(rootObject(), "setFocusToAddressBar", Qt::DirectConnection);

focusAddressBar?

> Tools/MiniBrowser/qt/MiniBrowserApplication.h:48
> +    WindowOptions(QObject * parent = 0)

Style

> Tools/MiniBrowser/qt/MiniBrowserApplication.h:50
> +        , m_printLoadedUrls(false)

I would call it printUrlsLoaded.... but maybe that is just me

> Tools/MiniBrowser/qt/MiniBrowserApplication.h:56
> +    void setPrintLoadedUrls(bool p) { m_printLoadedUrls = p; }

I think something like 'enabled' would serve better than p. It is not obvious what p means.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:5
> +    //do not define anchors or a initialisize here! This would messup with QSGViewSizeRootObjetToView.

// Do ...

I am not faminilar with the word initialisize, but I guess you mean 'an initializer' . There is also missing a C in Object (QSGView....)

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:167
> +        source: options.useTouchWebView ? "TouchView.qml" : "DesktopView.qml"

Ah that is where it comes from.

[Tor Arne Vestbø]

Comment on attachment 111980 [details]
patch for review.

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

> Tools/MiniBrowser/qt/BrowserWindow.cpp:37
> +#include <QtDeclarative/QDeclarativeEngine>

#include <QDeclarativeEngine> should be enough

> Tools/MiniBrowser/qt/BrowserWindow.cpp:40
> +Q_DECLARE_METATYPE(QDesktopWebView*)
> +Q_DECLARE_METATYPE(QTouchWebView*)

Do we still need these?

> Tools/MiniBrowser/qt/BrowserWindow.cpp:90
> +    if ((event->modifiers() == Qt::ControlModifier && event->key() == Qt::Key_L)
> +        || event->key() == Qt::Key_F6) {
> +        QMetaObject::invokeMethod(rootObject(), "setFocusToAddressBar", Qt::DirectConnection);
> +        event->accept();
> +    } else
> +        QSGView::keyPressEvent(event);

Can we do this using http://doc.qt.nokia.com/4.7-snapshot/qml-keys.html ?

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:76
> +    qmlRegisterType<QWebNavigationController>();

We don't register this in the qml plugin?

> Tools/MiniBrowser/qt/MiniBrowserApplication.h:48
> +    WindowOptions(QObject * parent = 0)

style: star

> Tools/MiniBrowser/qt/MiniBrowserApplication.h:61
> +    void setPrintLoadedUrls(bool p) { m_printLoadedUrls = p; }
> +    bool printLoadedUrls() const { return m_printLoadedUrls; }
> +    void setUseTouchWebView(bool t) { m_useTouchWebView = t; }
> +    bool useTouchWebView() const { return m_useTouchWebView; }
> +    void setStartMaximized(bool m) { m_startMaximized = m; }
> +    bool startMaximized() const { return m_startMaximized; }

use words, not single characters for the arguments

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:5
> +    //do not define anchors or a initialisize here! This would messup with QSGViewSizeRootObjetToView.

initial size, "mess up".

[Zeno Albisser]

Created attachment 112110 [details]
patch for review.

[Zeno Albisser]

Comment on attachment 111980 [details]
patch for review.

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

>> Source/WebKit2/ChangeLog:6
>> +        Add a first QML based implementation of MiniBrowser.
> 
> Bit weird English. Add the first QML based implementation of MiniBrowser

fixed.

>> Tools/ChangeLog:6
>> +
> 
> remove double newline

done.

>> Tools/MiniBrowser/qt/BrowserWindow.cpp:37
>> +#include <QtDeclarative/QDeclarativeEngine>
> 
> #include <QDeclarativeEngine> should be enough

yes, right.

>> Tools/MiniBrowser/qt/BrowserWindow.cpp:40
>> +Q_DECLARE_METATYPE(QTouchWebView*)
> 
> Do we still need these?

Yes we still need those to be able to access the webView, that was created in QML, in C++ code. But i put these lines into the respective headers and made it a separate commit. https://bugs.webkit.org/show_bug.cgi?id=70693

>> Tools/MiniBrowser/qt/BrowserWindow.cpp:90
>> +        QSGView::keyPressEvent(event);
> 
> Can we do this using http://doc.qt.nokia.com/4.7-snapshot/qml-keys.html ?

Yes we can. But we need a separate Keys handler in the TextInput element to allow for a consistent behavior. - done.

>> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:76
>> +    qmlRegisterType<QWebNavigationController>();
> 
> We don't register this in the qml plugin?

Yes we do. - removed.

>>> Tools/MiniBrowser/qt/MiniBrowserApplication.h:48
>>> +    WindowOptions(QObject * parent = 0)
>> 
>> Style
> 
> style: star

done.

>> Tools/MiniBrowser/qt/MiniBrowserApplication.h:50
>> +        , m_printLoadedUrls(false)
> 
> I would call it printUrlsLoaded.... but maybe that is just me

sounds a bit synthetic to me... i would prefer to leave it the way it is, unless there is a rule in coding style that applies or something similar.

>> Tools/MiniBrowser/qt/MiniBrowserApplication.h:56
>> +    void setPrintLoadedUrls(bool p) { m_printLoadedUrls = p; }
> 
> I think something like 'enabled' would serve better than p. It is not obvious what p means.

done

>> Tools/MiniBrowser/qt/MiniBrowserApplication.h:61
>> +    bool startMaximized() const { return m_startMaximized; }
> 
> use words, not single characters for the arguments

yes - done. ;-)

>>> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:5
>>> +    //do not define anchors or a initialisize here! This would messup with QSGViewSizeRootObjetToView.
>> 
>> // Do ...
>> 
>> I am not faminilar with the word initialisize, but I guess you mean 'an initializer' . There is also missing a C in Object (QSGView....)
> 
> initial size, "mess up".

Okay... that was really messed up. I agree. - fixed.

>> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:167
>> +        source: options.useTouchWebView ? "TouchView.qml" : "DesktopView.qml"
> 
> Ah that is where it comes from.

Yes it is. :)

[Tor Arne Vestbø]

Comment on attachment 112110 [details]
patch for review.

Good stuf! r=me

[Alexis Menard (darktears)]

Comment on attachment 111805 [details]
Patch to remove QAction.

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

> Source/WebKit2/UIProcess/API/qt/qwebnavigationcontroller.h:41
> +    Q_PROPERTY(bool canReload READ canReload NOTIFY navigationStateChanged)

FINAL is still valid here. This class will never be subclassed.

> Source/WebKit2/UIProcess/API/qt/qwebnavigationcontroller.h:-51
> -    QAction* navigationAction(QtWebKit::NavigationAction which) const;

If you remove this C++ API then you also want to move the QtWebKit namespace private as it is the only place we use it in a public header.

> Source/WebKit2/UIProcess/API/qt/qwebnavigationcontroller.h:52
> +    Q_INVOKABLE void goBack();

There is no need for Q_INVOKABLE. slots works out of the box.

[Zeno Albisser]

Comment on attachment 111805 [details]
Patch to remove QAction.

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

>> Source/WebKit2/UIProcess/API/qt/qwebnavigationcontroller.h:41
>> +    Q_PROPERTY(bool canReload READ canReload NOTIFY navigationStateChanged)
> 
> FINAL is still valid here. This class will never be subclassed.

Changed that.

>> Source/WebKit2/UIProcess/API/qt/qwebnavigationcontroller.h:-51
>> -    QAction* navigationAction(QtWebKit::NavigationAction which) const;
> 
> If you remove this C++ API then you also want to move the QtWebKit namespace private as it is the only place we use it in a public header.

good point. :)

>> Source/WebKit2/UIProcess/API/qt/qwebnavigationcontroller.h:52
>> +    Q_INVOKABLE void goBack();
> 
> There is no need for Q_INVOKABLE. slots works out of the box.

fixed.

There is a separate bug for this refactoring now. -> https://bugs.webkit.org/show_bug.cgi?id=70525

[Zeno Albisser]

Created attachment 112172 [details]
patch for landing. - subordinated patches (tasks that are blocking this task) should be merged first!

This patch also includes the binary diff of the icons used for MiniBrowser.

[Balazs Kelemen]

Comment on attachment 112172 [details]
patch for landing. - subordinated patches (tasks that are blocking this task) should be merged first!

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

> Tools/MiniBrowser/qt/BrowserWindow.cpp:67
> +QObject* BrowserWindow::webView() const
> +{
> +    QObject* webView = rootObject()->property("webView").value<QDesktopWebView*>();
> +    // The webView is created in QML, therefore it might not exist yet.
> +    if (!webView)
> +        webView = rootObject()->property("webView").value<QTouchWebView*>();
> +    return webView;
> +}

I don't get this. If the view doesn't exist when this is called than you don't know whether it is the desktop or the touch view, do you?

[Zeno Albisser]

(In reply to comment #16)
> (From update of attachment 112172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112172&action=review
> 
> > Tools/MiniBrowser/qt/BrowserWindow.cpp:67
> > +QObject* BrowserWindow::webView() const
> > +{
> > +    QObject* webView = rootObject()->property("webView").value<QDesktopWebView*>();
> > +    // The webView is created in QML, therefore it might not exist yet.
> > +    if (!webView)
> > +        webView = rootObject()->property("webView").value<QTouchWebView*>();
> > +    return webView;
> > +}
> 
> I don't get this. If the view doesn't exist when this is called than you don't know whether it is the desktop or the touch view, do you?

Well, you can't really know of which type an inexistent object is. So... no.
But let's go it through: In case there has no view been created so far. The porperty("webView) will return an invalid QVariant. Therfore the value function will not be able to convert to the required type and return a default constructed pointer. - So the return value of webView() will be a nullptr.

[Kenneth Rohde Christiansen]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 112172 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=112172&action=review
> > 
> > > Tools/MiniBrowser/qt/BrowserWindow.cpp:67
> > > +QObject* BrowserWindow::webView() const
> > > +{
> > > +    QObject* webView = rootObject()->property("webView").value<QDesktopWebView*>();
> > > +    // The webView is created in QML, therefore it might not exist yet.
> > > +    if (!webView)
> > > +        webView = rootObject()->property("webView").value<QTouchWebView*>();
> > > +    return webView;
> > > +}
> > 
> > I don't get this. If the view doesn't exist when this is called than you don't know whether it is the desktop or the touch view, do you?
> 
> Well, you can't really know of which type an inexistent object is. So... no.
> But let's go it through: In case there has no view been created so far. The porperty("webView) will return an invalid QVariant. Therfore the value function will not be able to convert to the required type and return a default constructed pointer. - So the return value of webView() will be a nullptr.

We could make the code more clear:

QObject* BrowserWindow::webView() const {
{
    QVariant view = rootObject()->property("webView");

    // The view is either a QDesktopWebView or a QTouchWebView, depending on how MiniBrowser was started.
    if (view.canConvert<QDesktopWebView>())
         return view.value<QDesktopWebView>();
    else
         return view.value<QTouchWebView>();
}

[Zeno Albisser]

*** Bug 69258 has been marked as a duplicate of this bug. ***

[Zeno Albisser]

Created attachment 112341 [details]
patch for landing. - Adding a temporary solution for TouchWebView.

Simon: I fixed the issue as discussed and amended the existing patch accordingly. Could you please commit all the changes in one go? - Thanks. :)

[Simon Hausmann]

Committed r98450: <http://trac.webkit.org/changeset/98450>