D14513: Add QML Extensions API

[David Rosca]

drosca added inline comments.

INLINE COMMENTS

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

Well, now it doesn't leak but it will only be released on exiting application, so it's effectively the same problem and you didn't fix anything.

> qmlfileutils.cpp:29
> +    m_path = filePath;
> +    if (!m_path.endsWith(QL1C('/'))) {
> +        m_path.append(QL1C('/'));

Why? This shouldn't be needed.
Always keep paths without trailing slash.

> qmli18n.cpp:26
> +    m_pluginName = pluginName;
> +    setlocale(LC_MESSAGES, "");
> +    initTranslations();

What does this do?

> qmltabs.cpp:274
> +
> +    QList<QObject*> list;
> +    for (WebTab *tab : tabList) {

If you know beforehand how big the resulting list will be, you should call `reserve`.

> qmltabs.cpp:324
> +
> +    QHashIterator<BrowserWindow*, int> it(QmlStaticData::instance().windowIdHash());
> +    while (it.hasNext()) {

Please use STL iterators.

  auto h = QmlStaticData::instance().windowIdHash();
  for (auto it = h.constBegin(); it != h.constEnd(); ++it) {
  }
  /// or cbegin()/cend()

> qmlwindow.cpp:38
> +
> +    return QmlStaticData::instance().windowIdHash().value(m_window);
> +}

This is equivalent to

  return QmlStaticData::instance().windowIdHash().value(m_window, -1);

> qmlplugin.cpp:32
> +{
> +    static bool m_qmlSupportLoaded = false;
> +    if (!m_qmlSupportLoaded) {

This is not member variable so it shouldn't have `m_` prefix. It shouldn't have any prefix.

> qmlplugin.cpp:65
> +
> +    auto qmlPluginLoader = static_cast<QmlPluginLoader*>(plugin->data.value<QmlPluginLoader*>());
> +    if (!qmlPluginLoader) {

You are casting twice here, `data.value<QmlPluginLoader*>()` already gives you `QmlPluginLoader*`

> qmlplugins.cpp:68
> +    const char *url = "org.kde.falkon";
> +    int majorVersion = 1;
> +    int minorVersion = 0;

`const`

> qmlstaticdata.cpp:140
> +{
> +    static QmlBookmarks bookmarks;
> +    return &bookmarks;

Why it isn't created on heap and parented to `QmlStaticData`? That way you don't need to manually specify CppOwnership. Ideally you should only specify ownership explicitly only when there is no better way.

> qmlstaticdata.h:51
> +
> +    static QmlStaticData& instance();
> +    QmlBookmarkTreeNode *getBookmarkTreeNode(BookmarkItem *item);

`static QmlStaticData &instance();`

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/20180811/2fcff01e/attachment.html>