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

Authored by stefanocrocco on Jul 19 2017, 9:51 AM.

Details

Summary

These changes make the webengine part emit the openUrlRequest signal in response to the QWebProfile::downloadRequested. This way konqueror reacts to the download request in the same way it does for khtml or kwebengine parts.

To achieve this, a new class, WebEnginePartDownloadManager, is introduced. This class connects to QWebProfile::downloadRequested signal and to a new signal, navigationRequested, from each existing WebEnginePage. The navigationRequested signal (emitted by a web page's acceptNavigationRequest method) allows the download manager to associate each download request with a navigation request, and, consequently, with the WebEnginePage which requested the download (this is necessary because, as far as I know, there's no way to associate the download request with a QWebEnginePage automatically). At this point, the openUrlRequest is emitted from the part corresponding to the page. Note that the download is not performed using QWebEngineDownloadItem (that is, QWebEngineDownloadItem::accept is not called).

Sometimes, a download is requested without a corresponding call to acceptNavigationRequest: in this case, the signal is emitted from an arbitrary part, specifying to open the file in a new window.

There are still some issues:

  • sometimes, expecially for some downloads started from javascript, the above algorithm fails: the file isn't downloaded at all or it's displayed in the current tab instead of a new one or, in rare cases, produces an endless loop (which, however, doesn't freeze the UI)
  • if acceptNavigationRequest isn't called, the file isn't opened in a new window but in the same window and tab. This, however, also happens when using KHTML, so I think the issue is somewhere else.
Test Plan

Go to a web page providing a link to download a file, then click on the link. The file should be embedded, saved or openend in a new window according to the settings.

Diff Detail

Repository
R226 Konqueror
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
stefanocrocco created this revision.Jul 19 2017, 9:51 AM
Restricted Application added subscribers: Dolphin, Konqueror. · View Herald TranscriptJul 19 2017, 9:51 AM
dfaure added inline comments.Jul 21 2017, 3:20 PM
webenginepart/src/webenginepage.cpp
91

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

153

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

267

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

722

remove

820

remove

webenginepart/src/webenginepage.h
74

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

webenginepart/src/webenginepartdownloadmanager.cpp
43

Leaked. Create it without new, i.e.

static WebEnginePartDownloadManager inst;
49

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

57

This whole loop could be simplified to

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

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

71

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

dfaure requested changes to this revision.Jul 21 2017, 3:21 PM
This revision now requires changes to proceed.Jul 21 2017, 3:21 PM
stefanocrocco edited edge metadata.

Made the requested changes. Regarding KBrowserArguments::setNewTab, I removed that line completely, as that workaround doesn't work anymore. Clicking on a link with attributes target="_blank" download` doesn't display the file anywhere, even if it seems a view is created for it (at least, I see a qDebug output from KonqView::openUrl at line 150 with the correct URL. I'll have to investigate this issue.

dfaure accepted this revision.Jul 22 2017, 10:45 AM

Let's get this in even if there are a few issues left, it's certainly much better than the current behaviour ;)

I talked to Allan about getting the page pointer (if any) when WebEngine signals a download, he says there were other requests for that, so maybe one day we'll get that. Meanwhile this is necessary indeed.

webenginepart/src/webenginepartdownloadmanager.h
43

From this line onwards it's all intended with 2 spaces instead of 4

This revision is now accepted and ready to land.Jul 22 2017, 10:45 AM
cfeck added a subscriber: cfeck.Jul 22 2017, 10:58 AM

Please use categorized qCDebug, especially those logging urls.

Fixed indentation of webenginepartdownloadmanager.h.

dfaure accepted this revision.Jul 22 2017, 1:44 PM

Please push, then I'll port all of webenginepart to qCDebug in a separate commit.

Many thanks.

This revision was automatically updated to reflect the committed changes.