D14513: Created QML Extensions API & Moved plugins info to SQL

[David Rosca]

drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  Also documentation should be only in headers, so move the function docs to headers from source files.

INLINE COMMENTS

> CMakeLists.txt:337
> +if (LibIntl_FOUND)
> +    target_compile_definitions(FalkonPrivate PRIVATE LibIntl_FOUND=1)
> +endif()

This should be define in `config.h` `HAVE_LIBNTL`.

> browserwindow.h:249
>  
> +Q_DECLARE_METATYPE(BrowserWindow*)
> +

Is it needed here? Can you move inside qml support sources instead?

> mainapplication.h:79
>      QList<BrowserWindow*> windows() const;
> +    QHash<BrowserWindow*, int> windowIdHash() const;
>  

This shouldn't be here, move it inside qml plugin sources.

> bookmarkitem.h:106
>  
> +Q_DECLARE_METATYPE(BookmarkItem*)
> +

Same as above.

> plugins.cpp:294
> +{
> +    QmlPlugins::registerQmlTypes();
> +}

This should be done before creating first qml engine, it doesn't need to be done on startup.

> plugins.cpp:495
> +
> +void Plugins::initQmlPlugin(Plugin *plugin)
> +{

Complete QML plugins loading logic should be moved to separate class, instead of doing it here. Same as Python support (but without actually being in separate library).

> plugins.h:42
>      QPixmap icon;
> +    QString entryPoint;
>      bool hasSettings = false;

This is QML plugin specific, so should not be here.

> plugins.h:78
> +        // QmlPlugin
> +        QmlPluginLoader *qmlPluginLoader = nullptr;
> +

You should use `data` variable instead.

> qmlbookmarks.cpp:217
> +    mApp->bookmarks()->removeBookmark(item);
> +    return true;
> +}

Should return result of `removeBookmark`

> qmlbookmarks.cpp:246
> +    QList<QObject*> ret;
> +    for (auto item : items) {
> +        ret.append(bookmarkTreeNodeData->get(item));

`qAsConst(items)`, otherwise items will detach.
This applies to all other instances of this issue (using C++11 for on non-const Qt container).

> qmlbookmarks.cpp:345
> +
> +bool QmlBookmarks::isTreeNodeEqualsItem(QmlBookmarkTreeNode *treeNode, BookmarkItem *item) const
> +{

I don't really like this function, why don't just keep `BookmarkItem` pointer in `QmlBookmarkTreeNode` and then comparing is trivial.

> qmlbookmarks.h:35
> +
> +    Q_INVOKABLE bool isBookmarked(const QString &url) const;
> +    Q_INVOKABLE QmlBookmarkTreeNode *rootItem() const;

Functions are not documented.

> qmlbookmarktreenode.h:86
> +    };
> +    Q_ENUMS(Type)
> +

`Q_ENUM` (without s)

> qmlbookmarktreenode.h:112
> +private:
> +    QMap<BookmarkItem*, QmlBookmarkTreeNode*> m_nodes;
> +};

Should be `QHash` instead.

> qmlbrowseraction.h:208
> +    QString m_iconUrl;
> +    QQmlComponent *m_popup;
> +};

`QQmlComponent *m_popup = nullptr;`

Always prefer C++11 style initialization.

> qmlcookie.cpp:94
> +
> +QmlCookie *QmlCookieData::get(QNetworkCookie *cookie)
> +{

The entire logic in Cookies is wrong.
You should not create QNetworkCookie on heap (with new), because it is not needed and also it just makes this "caching" have no effect - because you cache pointers but you will always create new pointer, so it will never hit cache.

> qmlcookies.cpp:30
> +        // FIXME: improve this
> +        QmlCookie *cookie = cookieData->get(new QNetworkCookie(network_cookie));
> +        QVariantMap map;

This leaks

> qmlcookies.cpp:114
> +                && (!map.contains(QSL("session")) || cookie.isSessionCookie() == session)) {
> +            QNetworkCookie *netCookie = new QNetworkCookie(cookie);
> +            QmlCookie *qmlCookie = cookieData->get(netCookie);

Leak

> qmlkeyevent.cpp:23
> +    : QObject(parent)
> +    , m_keyEvent(keyEvent)
> +{

What if the QKeyEvent is deleted while QmlKeyEvent is still around?

> qmlkeyevent.cpp:30
> +{
> +    if (!m_keyEvent) {
> +        return -1;

These checks are kind of redundant, because it could only happen if you pass `nullptr` as keyEvent to constructor, and you will never get null event from Qt.

> qmlmouseevent.cpp:23
> +    : QObject(parent)
> +    , m_mouseEvent(mouseEvent)
> +{

Same issue as with QKeyEvent

> qmlextensionscheme.cpp:30
> +    connect(m_schemeHandler, &QmlExtensionSchemeHandler::_requestStarted, this, [this](QWebEngineUrlRequestJob *job) {
> +        QmlWebEngineUrlRequestJob *request = new QmlWebEngineUrlRequestJob(job);
> +        emit requestStarted(request);

Where is this deleted?

> qmlfileutils.cpp:27
> +{
> +    // Get the plugin root directory - of the form
> +    // some-path-in-plugin-paths/qml/plugin-directory

You already know this from QmlEngine.

> qmlfileutils.h:43
> +     */
> +    Q_INVOKABLE QString readAllFileContents(const QString &fileName);
> +    /**

Should be QByteArray

> qmlhistory.cpp:49
> +
> +    foreach(auto entry, result) {
> +        auto item = historyItemData->get(entry);

Don't use foreach, use for instead.

> qmli18n.cpp:25
> +{
> +    m_pluginName = QzTools::filterCharsFromFilename(pluginName);
> +    // QzTools::filterCharsFromFilename doesn't replaces spaces

pluginName should already come from existing directory name, so you don't need to filter characters that cannot be in filename.

> qmli18n.cpp:36
> +    QString localeDir = QStandardPaths::locate(QStandardPaths::GenericDataLocation, "locale", QStandardPaths::LocateDirectory);
> +    bindtextdomain(domain.toUtf8(), localeDir.toUtf8());
> +    textdomain(domain.toUtf8());

How does this work for multiple loaded Qml extensions?

> qmli18n.h:25
> +// fatal error C1189: #error :  The C++ Standard Library forbids macroizing keywords
> +#undef inline
>  

Use `extern "C"`

> qmlmenu.cpp:67
> +    QMenu *newMenu = new QMenu();
> +    for (const QString &key : map.keys()) {
> +        if (key == QSL("icon")) {

Use stl iterators for iterating over map, instead of creating temporary list with calling keys().

> qmlwebhittestresult.cpp:21
> +
> +#define NOT !
> +

?

> qmlwebhittestresult.cpp:156
> +{
> +    QUrl media = m_webHitTestResult.mediaUrl();
> +    return QString::fromUtf8(media.toEncoded());

const

> qmlwebhittestresult.h:39
> +    Q_INVOKABLE bool mediaMuted() const;
> +    Q_INVOKABLE QString tagName() const;
> +    Q_INVOKABLE QString baseUrl() const;

Should be properties.

> qmlnotifications.cpp:49
> +    QPixmap icon;
> +    if (iconUrl.startsWith(QSL(":"))) {
> +        // Icon is loaded from falkon resource

Qml extensions can't create qrc resources, so why allow to load icons from it?

> qmlsettings.cpp:132
> +    m_settingsPath += QL1S("/") + m_name + QL1S("/settings.ini");
> +    m_settings = new QSettings(m_settingsPath, QSettings::IniFormat);
> +}

leaks

> qmlsidebar.cpp:138
> +        action->setIcon(QIcon::fromTheme(m_iconUrl));
> +    } else if (m_iconUrl.startsWith(QSL(":"))) {
> +        // Icon is loaded from falkon resource

It shouldn't do this, as mentioned above.
Also instead of 1-char string you should use QChar/QLatin1Char wherever possible.

> qmltab.cpp:442
> +
> +void QmlTab::createConnections()
> +{

If this is called multiple times, where does it disconnects from old webtab?

> qmltab.h:23
> +#include "../windows/qmlwindow.h"
> +#include <QJSValue>
> +#include "qml/api/menus/qmlwebhittestresult.h"

In header file: first are system includes and then local includes. Also separate with newline.

> qmltab.h:48
> +     * Zoom level will have the following values
> +     * <table>
> +     * <caption>Zoom Levels</caption>

Should be enough to just mention zoom levels are 0-18

> qmltabs.cpp:447
> +    LoadRequest req;
> +    req.setUrl(QUrl::fromEncoded(urlString.toUtf8()));
> +    const int ret = window->tabWidget()->addView(req);

LoadRequest have constructo that takes QUrl.

> qmltopsites.cpp:37
> +{
> +    // FIXME: this slows the startup of browser
> +

?

> qmluserscript.h:54
> +     */
> +    Q_PROPERTY(int injectionPoint READ injectionPoint WRITE setInjectionPoint NOTIFY injectionPointChanged)
> +public:

Should be `InjectionPoint` type.

> qmlplugininterface.cpp:120
> +
> +    QWidget *widget = QWidget::createWindowContainer(window);
> +    QDialog *dialog = new QDialog(parent);

It should use QQuickWidget or even just simply show the QQuickWindow, why does it adds buttons?

> qmlplugininterface.cpp:135
> +    if (!m_mouseDoubleClick.isCallable()) {
> +        qWarning() << "Unable to call" << __FUNCTION__;
> +        return false;

This is not an error, if extension doesn't assign handler then it won't be callable.

> qmlplugininterface.cpp:234
> +    }
> +    QmlTab *qmlTab = new QmlTab();
> +    qmlTab->setWebPage(page);

It should use exisitng QmlTab, instead of allocating it every time this function is called.

> qmlpluginloader.cpp:56
> +    m_interface->setName(name);
> +    m_engine->rootContext()->setContextProperty("__name__", name);
> +}

Again, no.

> qmlpluginloader.cpp:63
> +    m_component = new QQmlComponent(m_engine, m_path);
> +    m_engine->rootContext()->setContextProperty("__path__", m_path);
> +}

There is no reason to expose this path to extension code.
If you need to save custom data to engine you can eg. subclass it.

> qmlplugins.cpp:200
> +
> +        auto *object = new QmlUserScripts();
> +        return object;

Just `return new QmlUser...;`

REPOSITORY
  R875 Falkon

REVISION DETAIL
  https://phabricator.kde.org/D14513

To: anmolgautam, drosca
Cc: falkon, tuomisto, herrold, anmolgautam, SGOrava, iodelay, spoorun, ptabis, navarromorales, cochise, clivej, ach
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/falkon/attachments/20180802/ab5a6aa3/attachment.html>