D6781: Allow konqueror to embed / open in a new window downloaded files using webengine part

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> webenginepage.cpp:91
>              this, &WebEnginePage::slotAuthenticationRequired);
> -    connect(this->profile(), &QWebEngineProfile::downloadRequested, this, &WebEnginePage::downloadRequest);
>      if(!this->profile()->httpUserAgent().contains(QLatin1String("Konqueror")))

Ah! That's why my commit leads to N filedialogs when making one download with N tabs open... Thanks a lot for fixing that!

> webenginepage.cpp:153
> +    //links in new windows
> +    bArgs.setNewTab(newWindow);
> +

So how about if (!newWindow) bArgs.setNewTab(true) ?

> webenginepage.cpp:267
>      settings()->setAttribute(QWebEngineSettings::PluginsEnabled, WebEngineSettings::self()->isPluginsEnabled(reqUrl.host()));
>      // Insert the request into the queue...
> +    emit navigationRequested(this, url);

Can you remove this comment while at it? It doesn't make any sense with or without your added line.

> webenginepage.cpp:722
>      connect(this, SIGNAL(loadFinished(bool)), this, SLOT(slotLoadFinished(bool)));
> +//     disconnect(this->profile(), &QWebEngineProfile::downloadRequested, this, &WebEnginePage::downloadRequest);
>  #if QTWEBENGINE_VERSION >= QT_VERSION_CHECK(5, 7, 0)

remove

> webenginepage.cpp:820
>  
> +        qDebug() << "new part is the same as old part?" << (part() == webenginePart);
> +

remove

> webenginepage.h:74
>  
> +  void navigationRequested(WebEnginePage* page, const QUrl& url);
> +

This looks like 2-spaces indentation, can you use 4 spaces everywhere?
(same problem in .cpp files)

> webenginepartdownloadmanager.cpp:42
> +{
> +  static WebEnginePartDownloadManager *inst = new WebEnginePartDownloadManager();
> +  return inst;

Leaked. Create it without new, i.e.

  static WebEnginePartDownloadManager inst;

> webenginepartdownloadmanager.cpp:48
> +{
> +  if(!m_pages.contains(page)) m_pages.append(page);
> +  connect(page, &WebEnginePage::navigationRequested, this, &WebEnginePartDownloadManager::recordNavigationRequest);

(QT/KF5) coding style: add a space after if, for, etc.

> webenginepartdownloadmanager.cpp:56
> +  QUrl url;
> +  for(QHash<QUrl, WebEnginePage*>::const_iterator it = m_requests.constBegin(); it != m_requests.constEnd(); ++it){
> +    if(it.value() == page) {

This whole loop could be simplified to

  const QUrl url = m_requests.key(static_cast<WebEnginePage *>(page));

> webenginepartdownloadmanager.cpp:63
> +  m_requests.remove(url);
> +  m_pages.removeOne(dynamic_cast<WebEnginePage*>(page));
> +}

dynamic_cast sounds wrong given that the page is already partially destroyed down to the QObject level. But a static_cast should be fine.

> webenginepartdownloadmanager.cpp:70
> +  bool forceNew = false;
> +  if(page) m_requests.remove(it->url());
> +  else if(!page && !m_pages.isEmpty()) {

using take() 2 lines above instead of value() would remove the need for this remove(), no?

REPOSITORY
  R226 Konqueror

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

To: stefanocrocco, dfaure
Cc: #konqueror, #dolphin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20170721/e86155be/attachment.htm>