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.


  If I noted something that occurs in multiple places, my note applies to all other places too, so you should change it everywhere.
  
  Class members should be initialized in class definition (in header files) with default values.
  
  Also please make sure that wrapper objects are correctly deleted and doesn't leak.

INLINE COMMENTS

> config.h.cmake:13
> +
> +#define HAVE_LIBINTL LibIntl_FOUND

Should be `#cmakedefine HAVE_LIBNTL`

> drosca wrote in mainapplication.h:79
> This shouldn't be here, move it inside qml plugin sources.

^
I mean, the whole windowIdHash should not be in BrowserWindow.

> profilemanager.cpp:308
> +
> +        // Insert default allowed plugins to allowed_plugins table
> +        QStringList defaultAllowedPlugins = Plugins::getDefaultAllowedPlugins();

Please move this (change to saving plugin state to database) to separate revision, as I already noted.

> history.h:72
> +    HistoryEntry *getHistoryEntry(const QString &text);
> +    void deleteRange(double startTime, double endTime);
> +

There is already `indexesFromTimeRange` that you can use instead of adding new method. Also, timestamp is integer and not double.

> history.h:90
>  Q_DECLARE_TYPEINFO(HistoryEntry, Q_MOVABLE_TYPE);
> +Q_DECLARE_METATYPE(HistoryEntry)
>  

Why?

> plugins.h:33
>  class SpeedDial;
> +class Extensions;
>  

?

> plugins.h:145
>      QLibrary *m_pythonPlugin = nullptr;
> +    bool m_qmlSupportLoaded;
>  };

Again, please move the *entire* logic of loading/initing/registering QML extensions to separate class.

> drosca wrote in qmlbookmarks.cpp:345
> I don't really like this function, why don't just keep `BookmarkItem` pointer in `QmlBookmarkTreeNode` and then comparing is trivial.

Then you don't need this function at all, and always directly compare QmlBookmarkTreeNode::item()

> qmlbookmarktreenode.cpp:38
> +{
> +    if (!m_item) {
> +        return Invalid;

Again, why these nullptr checks? When will you ever construct QmlBookmarkTreeNode with null BookmarkItem?

> qmlbrowseraction.cpp:30
> +    : QObject(parent)
> +    , m_popup(nullptr)
> +{

Again, use C++11 initialization everywhere!

> qmlbrowseraction.cpp:56
> +{
> +    for (BrowserWindow *window : mApp->windows()) {
> +        removeButton(window);

Again, this detaches temporary list. You *should never* use C++11 `for` on non-const Qt container.

> qmlbrowseraction.cpp:218
> +    } else {
> +        auto qmlEngine = dynamic_cast<QmlEngine *>(m_popup->creationContext()->engine());
> +        if (!qmlEngine) {

If it is QObject-derived class, always use `qobject_cast`.

> drosca wrote in qmlcookie.cpp:94
> 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.

You didn't change anything, this still stands!

> qmlcookies.cpp:29
> +    connect(mApp->cookieJar(), &CookieJar::cookieAdded, this, [this](QNetworkCookie network_cookie){
> +        QmlCookie *cookie = cookieData->get(&network_cookie);
> +        QVariantMap map;

This is completely wrong, it can actually lead to invalid results now due to possibility of this reference always having the same address (because it is on stack).
Do you know what this code does now?

> qmlkeyevent.h:72
> +
> +    void makeNull();
> +

Rename to `clear()`

> qmlmouseevent.cpp:33
> +    }
> +    return (int)m_mouseEvent->button();
> +}

Don't use C-style casts, use `static_cast<int>` or `int(value)`

> qmlqzobjects.h:20
>  
> -#include <QWidget>
> +#include <qzcommon.h>
> +#include <QObject>

This is not system include

> qmlhistory.cpp:43
> +
> +    for (const auto entry : qAsConst(result)) {
> +        auto item = historyItemData->get(entry);

You can just declare result as const instead of using qAsConst.

> qmlaction.cpp:64
> +{
> +    m_pluginPath = path;
> +}

You should be able to get engine with `qmlEngine(this)` instead of needing to pass the plugin path manually.

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/20180807/102fb67d/attachment.html>