D14513: Add QML Extensions API

[David Rosca]

drosca added inline comments.

INLINE COMMENTS

> qmlbookmarks.cpp:233
> +    int itemsLength = items.length();
> +    for (int i = 0; i < itemsLength; i ++) {
> +        BookmarkItem *item = items.at(i);

Why did you change the for loop? Unless there is a reason, don't use for(int i = 0, ....)

> qmlbrowseraction.h:164
> +    QQmlComponent* m_popup = nullptr;
> +    Locations m_locations;
> +    QmlBrowserActionButton *m_button = nullptr;

Missing initialization

> qmlcookie.h:23
> +#include <QNetworkCookie>
> +#include <QDebug>
> +

This should not be needed.

> drosca wrote in qmlextensionscheme.cpp:30
> Where is this deleted?

The question still stands? If it doesn't get to the QML engine, it will leak.

Example where it may happen (I did not test it myself, so plase check) is if you instantiate `Falkon.ExtensionScheme { }` item in QML but don't create the `requestStarted` handler (eg. no `onRequestStarted` code).

> drosca wrote in qmlfileutils.cpp:27
> You already know this from QmlEngine.

?

> qmlaction.h:33
>  public:
> -    explicit PluginsManager(QWidget* parent = 0);
> -    ~PluginsManager();
> -
> -    void load();
> -    void save();
> -
> -private Q_SLOTS:
> -    void settingsClicked();
> -    void currentChanged(QListWidgetItem* item);
> -    void itemChanged(QListWidgetItem* item);
> -
> -    void refresh();
> +    explicit QmlAction(QAction *action, QmlEngine *engine, QObject *parent = nullptr);
> +    void setProperties(const QVariantMap &map);

Why? You can get the QmlEngine pointer with `qmlEngine(QObject*)` function that is called on any QObject that was created by the engine.

> qmlsettings.cpp:31
> +{
> +    m_settings->deleteLater();
> +}

Why not just create QSettings with correct parent?

> qmlsidebar.cpp:136
> +    action->setCheckable(m_checkable);
> +    if (QIcon::hasThemeIcon(m_iconUrl)) {
> +        action->setIcon(QIcon::fromTheme(m_iconUrl));

This seems like the same code for handling icon loading is pasted on multiple places, should be extracted to function.

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

init

> qmltabs.cpp:323
> +
> +    for (BrowserWindow *window : staticData->windowIdHash().keys()) {
> +        if (staticData->windowIdHash().value(window) == windowId) {

Again, use stl iterators. This creates temporary list only to iterate over it, that's not needed at all.

> qmluserscripts.h:87
> +
> +    QList<QObject *> toQObjectList(const QList<QWebEngineScript> &list) const;
> +};

`QList<QObject*>`

> qmlplugin.h:29
>  private:
> -    void sortItems();
> -
> -    Ui::PluginsList* ui;
> -    bool m_loaded;
> +    static bool m_qmlSupportLoaded;
>  };

Just make it static variable in `loadPlugin` function as it is not used anywhere else.

> qmlplugininterface.cpp:42
> +QmlPluginInterface::QmlPluginInterface()
> +    : m_qmlReusableTab(new QmlTab())
> +{

Why this at all? It should just use staticData->getTab!

> qmlstaticdata.cpp:36
> +    connect(mApp->plugins(), &PluginProxy::mainWindowCreated, this, [this](BrowserWindow *window) {
> +        m_windowIdHash.insert(window, m_newWindowId ++);
> +    });

There should be no whitespace before/after increment, so it should be `foo++`.

> qmlstaticdata.h:62
> +inline uint qHash(QNetworkCookie cookie) {
> +    qDebug() << QString::fromUtf8(cookie.toRawForm());
> +    return qHash(QString::fromUtf8(cookie.toRawForm()));

remove

> qmlstaticdata.h:66
> +
> +Q_GLOBAL_STATIC(QmlStaticData, staticData)

It should be something like `static QmlStaticData* instance()` member function and not adding `staticData` to global namespace.

REPOSITORY
  R875 Falkon

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

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/20180810/f5a6442c/attachment.html>