| Summary: | [Qt][WK2] Rewrite MiniBrowser in QML | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zeno Albisser <zeno> | ||||||||||||||||
| Component: | WebKit Qt | Assignee: | Zeno Albisser <zeno> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | gopal.1.raghavan, hausmann, jturcotte, kbalazs, kenneth, laszlo.gombos, menard, s.mathur, vestbo, zoltan | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | 70525, 70529, 70537, 70693 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Zeno Albisser
2011-10-18 02:19:39 PDT
duplicate of https://bugs.webkit.org/show_bug.cgi?id=70368 *** Bug 70368 has been marked as a duplicate of this bug. *** Created attachment 111805 [details]
Patch to remove QAction.
This is the first in a series of patches to rewrite QtWebKit MiniBrowser.
Created attachment 111836 [details]
patch for review.
Created attachment 111837 [details]
patch for review.
fixed ChangeLog diff
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 Created attachment 111980 [details]
patch for review.
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. 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". Created attachment 112110 [details]
patch for review.
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. :) Comment on attachment 112110 [details]
patch for review.
Good stuf! r=me
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. 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 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.
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? (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. (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>(); } *** Bug 69258 has been marked as a duplicate of this bug. *** 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. :)
Committed r98450: <http://trac.webkit.org/changeset/98450> |