D14774: Add QML Extensions API

[David Rosca]

drosca requested changes to this revision.
drosca added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> profilemanager.cpp:309
> +        // Insert default allowed plugins to allowed_plugins table
> +        QStringList defaultAllowedPlugins = Plugins::getDefaultAllowedPlugins();
> +        QSqlQuery query(SqlDatabase::instance()->database());

const

> browsedata.sql:58
>  
> +CREATE TABLE allowed_plugins (
> +    pluginId TEXT NOT NULL UNIQUE,

In previous revision I said that I want this (enabled plugins settings in sqlite database) moved to separate revision. Or even to remove it and make it stored in QSettings, because right I don't really see a reason to have it in database.

> history.cpp:186
> +    QSqlQuery query(SqlDatabase::instance()->database());
> +    query.prepare("SELECT id FROM history WHERE url=?");
> +    query.bindValue(0, url);

`QStringLiteral` (`QSL`)

> history.cpp:310
> +    while (query.next()) {
> +        HistoryEntry *entry = new HistoryEntry;
> +        entry->count = query.value(0).toInt();

It leaks here, nothing will ever delete it. `HistoryEntry` is meant to be passed by value.

> history.cpp:329
> +    HistoryEntry *entry = new HistoryEntry;
> +    if (query.next()) {
> +        entry->count = query.value(0).toInt();

If this is false it will return `HistoryEntry` with not initialized member variables.

> history.h:70
>  
> +    QList<HistoryEntry *> searchHistoryEntry(const QString &text);
> +    HistoryEntry *getHistoryEntry(const QString &text);

`QList<HistoryEntry*>`

> plugins.cpp:100
> +    // For QML plugins, pluginId is qml:<plugin-dir-name>
> +    const QString dirName = plugin->pluginId.remove(0, QLatin1String("qml:").size());
> +    const QString dirPath = DataPaths::locate(DataPaths::Plugins, QSL("qml/") + dirName);

just `remove(0, 4)`, or better `mid(4)`.

> plugins.cpp:423
> +    // DataPaths::currentProfilePath() + QL1S("/extensions") is duplicated in qmlsettings.cpp
> +    // If you change this, please change it there too.
>      plugin->instance->init(state, DataPaths::currentProfilePath() + QL1S("/extensions"));

Why is it duplicated there?

> qmlbrowseraction.h:37
> +     */
> +    Q_PROPERTY(QString identity READ identity WRITE setIdentity NOTIFY identityChanged)
> +

What is identity?

> qmlbrowseraction.h:97
> +    ~QmlBrowserAction();
> +    void classBegin() {}
> +    void componentComplete();

override

> qmlbrowseraction.h:98
> +    void classBegin() {}
> +    void componentComplete();
> +    QmlBrowserActionButton *button() const;

override

> qmlbrowseraction.h:163
> +    QString m_badgeText;
> +    QQmlComponent* m_popup = nullptr;
> +    Locations m_locations = NavigationToolBar;

`QQmlComponent *`

> qmlbrowseraction.h:179
> +    void setBadgeText(const QString &badgeText);
> +    QQmlComponent* popup() const;
> +    void setPopup(QQmlComponent* popup);

`QQmlComponent *`

> qmlextensionscheme.cpp:31
> +        QmlWebEngineUrlRequestJob *request = new QmlWebEngineUrlRequestJob(job);
> +        qmlEngine(this)->newQObject(request);
> +        emit requestStarted(request);

So this was discussed in the previous review and the outcome was that it will not work like that, and it must pass QJSValue in the signal. Am I correct?

> qmli18n.cpp:31
> +{
> +    m_domain = QString("falkon_%1").arg(m_pluginName);
> +    QString localeDir = QStandardPaths::locate(QStandardPaths::GenericDataLocation, "locale", QStandardPaths::LocateDirectory);

QStringLiteral

> qmli18n.cpp:32
> +    m_domain = QString("falkon_%1").arg(m_pluginName);
> +    QString localeDir = QStandardPaths::locate(QStandardPaths::GenericDataLocation, "locale", QStandardPaths::LocateDirectory);
> +    const bool isLanguageSet = qEnvironmentVariableIsSet("LANGUAGE");

const + QStringLiteral

> qmlsettings.cpp:28
> +
> +QmlSettings::~QmlSettings()
> +{

Empty destructor is not needed

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

`QL1S("/")` -> `QL1C('/')`

> qmlsidebar.h:120
> +    QString m_shortcut;
> +    bool m_checkable;
> +    QQmlComponent *m_item = nullptr;

initialization

> qmltab.cpp:421
> +{
> +    for (auto connection : qAsConst(m_lambdaConnections)) {
> +        disconnect(connection);

You should be able to disconnect all with `m_webTab->disconnect(this)` (test first!)

> qmluserscript.h:28
> + */
> +class FALKON_EXPORT QmlUserScript : public QObject
> +{

Some classes are exported and some not, so which way is it? It should not be needed to export any QML support classes, as there is no reason for C++ plugins to use them.

The only issue might be with tests, but it doesn't seem to be used there.

> qmlwindows.cpp:82
> +
> +    qWarning() << "Unable to get window with given id";
> +    return nullptr;

If you want to show this warning then it should have the windowId that wasn't found, otherwise it's just useless.

> qmlwindowstate.h:25
> + */
> +class QmlWindowState : public QObject
> +{

This is only for the WindowState enum? In that case it would be better to create class `Enums` and move all these "standalone" enums there. It will also look better from QML side: instead of "Falkon.WindowState.WindowState.FullScreen" -> "Falkon.Enums.WindowState.FullScreen".

> qmlplugininterface.cpp:121
> +
> +    QWidget *widget = QWidget::createWindowContainer(window);
> +    widget->setFixedSize(window->size());

Use `QQuickWidget` instead.

> qmlpluginloader.cpp:24
> +
> +#ifdef HAVE_LIBINTL
> +#include "qml/api/i18n/qmli18n.h"

`#if HAVE_LIBINTL` because `HAVE_LIBINTL` is defined always.

> qmlpluginloader.cpp:28
> +
> +
> +QmlPluginLoader::QmlPluginLoader(const QString &name, const QString &path, const QString &entryPoint)

extra newline

> qmlpluginloader.cpp:47
> +    m_interface->setName(m_name);
> +    connect(m_interface, &QmlPluginInterface::qmlPluginUnloaded, this, [this]{
> +        delete m_component;

`[this] {` space

> qmlpluginloader.cpp:73
> +    m_engine->globalObject().setProperty("__falkon_i18n", m_engine->newQObject(i18n));
> +    m_engine->globalObject().setProperty("i18n", m_engine->evaluate("function (s) { return __falkon_i18n.i18n(s) }"));
> +    m_engine->globalObject().setProperty("i18np", m_engine->evaluate("function (s1, s2) { return __falkon_i18n.i18np(s1, s2) }"));

QStringLiteral

> qmlplugins.cpp:66
> +    // Bookmarks
> +    qmlRegisterUncreatableType<QmlBookmarkTreeNode>(url, majorVersion, minorVersion, "BookmarkTreeNode", "Unable to register type: BookmarkTreeNode");
> +

reason (last parameter) is QString, so it should use QStringLiteral. Or just leave it empty with `QString()`.

> qmlstaticdata.cpp:81
> +
> +QmlHistoryItem *QmlStaticData::getHistoryItem(HistoryEntry *entry)
> +{

The entire logic here is wrong too, you will never get cache hit because `HistoryEntry` is only ever created from the QML support code. It should use the HistoryEntry contents as key, not address.

> qmlstaticdata.h:53
> +    QmlBookmarkTreeNode *getBookmarkTreeNode(BookmarkItem *item);
> +    QmlCookie *getCookie(QNetworkCookie cookie);
> +    QmlHistoryItem *getHistoryItem(HistoryEntry *entry);

const-ref

REPOSITORY
  R875 Falkon

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

To: anmolgautam, drosca
Cc: falkon, 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/20180902/bd9615af/attachment.html>