Port the About page to QtWebEngine
ClosedPublic

Authored by stefanocrocco on Dec 28 2019, 9:20 AM.

Details

Summary

Replace the old About Page based on KHtml with one based on QtWebEngine.

This has been obtained creating a new QWebEngineUrlSchemeHandler for the konq`
scheme which will display the contents of the about page. The old about scheme
can't be used because it's used internally by QtWebEngine.

There are two problems. The first is that if the user's konquerorrc file
contains old default value for the StartURL entry, that value will be used
instead of the new one, which will result in an error message. I don't know how
to force the use of the new default instead of the old one.

The second problem is that clicking on the "Home Folder" link in the launch page
will open the link in the WebEngine part instead of Dolphin part.

Test Plan

after removing the StartURL entry from the user's konquerorrc file,
launch Konqueror and check that the intro page is displayed. Click on one of the
links in the page (Introduction, Tips, Specifications) and check that the
correct page is displayed.

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 requested review of this revision.Dec 28 2019, 9:20 AM
stefanocrocco created this revision.

Thanks, you rock!

CMakeLists.txt
64โ€“65

can be completely removed now

src/konqhistorymanager.cpp
159

Maybe we should extract this scheme into a shared (const char[]) variable or method somewhere...

Especially since this time you could grep for about, but next time it'll be harder to grep for konq :)

src/konqmainwindow.cpp
646

?

webenginepart/src/about/CMakeLists.txt
2

you can kill all this, there's no additional plugin anymore (good!)

The first is that if the user's konquerorrc file contains old default value for the StartURL entry, that value will be used instead of the new one, which will result in an error message.

How about this? http://www.davidfaure.fr/2020/replace_about_with_konq.diff

[rebase your changes to master first, I just pushed some cleanups]

Are you sure about the home folder thing? Notice that it's both a button and a popup. When viewing a webpage (or the old about page) the button goes to your favorite webpage, the popup has the entry for the home directory (dolphinpart). When viewing a folder, it's the other way around.

  • Remove commented out lines
  • Centralize management of konq URLs
stefanocrocco marked 4 inline comments as done.Jan 1 2020, 5:04 PM
stefanocrocco added inline comments.
src/konqhistorymanager.cpp
159

I added a new namespace, KonqUrl which contains functions to acccess all the URLs with konq scheme which are currently in use, both as QLatin1String and as QUrl. I also added several functions to replace checks made in different places in Konqueror (for example, a function to check if an URL is konq:blank)

stefanocrocco marked 2 inline comments as done.Jan 1 2020, 6:09 PM
The first is that if the user's konquerorrc file contains old default value for the StartURL entry, that value will be used instead of the new one, which will result in an error message.

How about this? http://www.davidfaure.fr/2020/replace_about_with_konq.diff

[rebase your changes to master first, I just pushed some cleanups]

It doesn't seem to work. The workaround I found is to write a function which checks the scheme of `KonqSettings::startURL`, changes it to `konq` if it's `about`, then writes back the new setting. This function is then called from `konqmain`.
  • Remove commented out lines
  • Centralize management of konq URLs
  • Fix the value of the StartURL option in the user's config file
dfaure requested changes to this revision.Jan 1 2020, 6:34 PM

Excellent idea, great encapsulation.

src/konqmain.cpp
161

I would have adjusted the URL upon read in the only method that calls KonqSettings::startURL(), i.e. KonqMainWindowFactory::createNewWindow(),
but OK, why not. This way it's a single check per process.

164

latin1(Scheme) reads strange to me here.
url(Scheme) wouldn't make any sense either.

So how about splitting this into a KonqUrl::scheme() method, keeping the enum for things that make sense as a URL?

src/konqmainwindow.cpp
5021

Can I suggest renaming latin1() to string()?

latin1() makes me think of a method that would return a QByteArray, not a QString (or QLatin1String, which acts as a QString).

src/konqmainwindowfactory.cpp
51

should be ported to KonqUrl::url()

src/konqurl.cpp
28

s/[9]/[]/ the compiler can find out by itself

src/konqview.cpp
32 โ†—(On Diff #72556)

unused?

src/konqviewmanager.cpp
915

!KonqUrl::isKonqBlank(forcedUrl)

1192

should be ported to KonqUrl::url()

webenginepart/src/about/konq_aboutpage.cpp
217

missing ':'

This revision now requires changes to proceed.Jan 1 2020, 6:34 PM
  • Add KonqUrl::scheme function and other small fixes
stefanocrocco marked 7 inline comments as done.Jan 2 2020, 12:53 PM

I noticed a problem: at startup, when the konq:blank page is displayed, the focus is given to the view rather than to the location bar as it used to be. This doesn't happen if you create new tabs. It seems that KMainWindowFactory::createNewWindow gives indeed focus to the location bar, but the focus is then moved to WebEngineView. I can't find out why. Do you have any idea?

dfaure accepted this revision.Jan 3 2020, 8:11 AM

Let's get this in, I'll debug the focus issue then.

This revision is now accepted and ready to land.Jan 3 2020, 8:11 AM
This revision was automatically updated to reflect the committed changes.
dfaure added a comment.Jan 3 2020, 9:30 PM

A breakpoint at "ComboBox lost focus..." in konqmainwindow.cpp shows this backtrace:

#8  0x00007ffff7aada31 in QWidget::setFocus (this=0x555556089c10) at /d/qt/5/inst/include/QtWidgets/qwidget.h:420
#9  0x00007ffff7aa7d34 in KonqView::setLoading (this=0x55555601e4c0, loading=true, hasPending=false) at /d/kde/src/5/kde/applications/konqueror/src/konqview.cpp:542
#10 0x00007ffff7aa7ae3 in KonqView::slotStarted (this=0x55555601e4c0, job=0x0) at /d/kde/src/5/kde/applications/konqueror/src/konqview.cpp:511
#11 0x00007ffff7a94c78 in KonqView::qt_static_metacall (_o=0x55555601e4c0, _c=QMetaObject::InvokeMetaMethod, _id=8, _a=0x7fffffffb2e0)
#12 0x00007fffef7d7b4f in doActivate<false> (sender=0x555556002420, signal_index=5, argv=0x7fffffffb2e0) at /d/qt/5/kde/qtbase/src/corelib/kernel/qobject.cpp:3882
#13 0x00007fffef7d1951 in QMetaObject::activate (sender=0x555556002420, m=0x7ffff6ced800 <KParts::ReadOnlyPart::staticMetaObject>, local_signal_index=0, argv=0x7fffffffb2e0) at /d/qt/5/kde/qtbase/src/corelib/kernel/qobject.cpp:3930
#14 0x00007ffff6ab480f in KParts::ReadOnlyPart::started (this=<optimized out>, _t1=<optimized out>)
#15 0x00007fffcc2001cc in WebEnginePart::slotLoadStarted (this=0x555556002420) at /d/kde/src/5/kde/applications/konqueror/webenginepart/src/webenginepart.cpp:488
#16 0x00007fffcc1f8808 in WebEnginePart::qt_static_metacall (_o=0x555556002420, _c=QMetaObject::InvokeMetaMethod, _id=2, _a=0x7fffffffb438)
#17 0x00007fffef7d7b4f in doActivate<false> (sender=0x7fffd000ac60, signal_index=3, argv=0x7fffffffb438) at /d/qt/5/kde/qtbase/src/corelib/kernel/qobject.cpp:3882
#18 0x00007fffef7d1951 in QMetaObject::activate (sender=0x7fffd000ac60, m=0x7fffcc1cc920 <QWebEnginePage::staticMetaObject>, local_signal_index=0, argv=0x0) at /d/qt/5/kde/qtbase/src/corelib/kernel/qobject.cpp:3930
#19 0x00007fffcbf938a9 in QWebEnginePage::loadStarted (this=0x7fffd000ac60) at .moc/moc_qwebenginepage.cpp:852

Makes sense. In *general* when we start loading a webpage, we want to give it focus, so the user can use arrow keys, type a google search, etc.
But the about page is different. I think we want to blacklist konq:* in WebEnginePart::slotLoadStarted?
Right now it has a test for not emitting started in case of "konq:blank", but on startup this is "konq:konqueror".

rjvbb added a subscriber: rjvbb.Jun 11 2020, 4:06 PM

It took me way too long to figure out that this is the source of the regression below, and not a missing "konq:" kioslave ...

For me, the main interest in Konqueror lies in its ability to use WebKit2 (with the rebooted QtWebKit version, evidently), so I can use it in a more or less modern browser interface. (There's just no need for yet another grass-roots wrapper around Chromium IMHO but that's a different issue.)

When I do activate the WebEngine backend I see this on the terminal:

js: Not allowed to load local resource: file:///opt/local/share/kf5/infopage/kde_infopage.css
js: Not allowed to load local resource: file:///opt/local/share/konqueror/about/konq.css
js: Not allowed to load local resource: file:///opt/local/share/icons/Ciment/22x22/places/user-home.png
js: Not allowed to load local resource: file:///opt/local/share/icons/osx/places/48/user-trash-full.png
js: Not allowed to load local resource: file:///opt/local/share/icons/osx/places/22/folder-remote.png
js: Not allowed to load local resource: file:///opt/local/share/icons/oxygen/base/22x22/actions/bookmarks.png
js: Not allowed to load local resource: file:///opt/local/share/icons/Ciment/16x16/actions/go-next.png

but pasting the URL into the konqueror address bar opens the files just fine.

The fix is to implement similar support for the about page (with konq: URLs) in kwebkitpart. Since you're the only person interested in kwebkitpart these days (AFAIK), feel free to go ahead and do that.

rjvbb added a comment.Jun 11 2020, 6:49 PM

David Faure wrote on 20200611::18:16:14 re: "D26253: Port the About page to QtWebEngine"

The fix is to implement similar support for the about page (with konq: URLs) in kwebkitpart.
Since you're the only person interested in kwebkitpart these days (AFAIK), feel free to go ahead and do that.

Not the only one interested in WebKit though...

Implementing something Konqueror-specific in kwebkitpart doesn't really feel right though. I suppose that the konq: URLs are implemented by generating HTML code which is then sent to the engine; is there a reason why this would have to be done in the web kpart? That would also solve the problem for the other view modes (my build still shows KHTML, but also a fancy term for "kate" and okular).

Either way, throwing an error about failure to create an io-slave is inappriopriate, at least in Konqueror which knows that there's no slave for the protocol in question.

And what about the file access errors, maybe I should just file a bug ticket for those?

BTW, my konqueror build on Linux still throws io-slave errors about the http protocols while the identical Mac build running in an as-identical-as-possible environment works fine. The help: protocol does work (though it shows a blank page in the WebKit view mode). This is one reason why I first looked for a missing konq: KIO slave ... but it doesn't help to motivate me to work on a webkit implementation for the konq: urls ... I cannot help but feel that the idea of io slaves is nice but needlessly complicate...

Implementing something Konqueror-specific in kwebkitpart doesn't really feel right though. I suppose that the konq: URLs are implemented by generating HTML code which is then sent to the engine; is there a reason why this would have to be done in the web kpart? That would also solve the problem for the other view modes (my build still shows KHTML, but also a fancy term for "kate" and okular).

The about page has links to execute binaries. Obviously both web engines will refuse to do that with simple generated HTML. This needs special support in the actual kpart.
And no there's nothing wrong with konqueror-specific code in kwebkitpart, it's the only user of kwebkitpart anyway.
(same reasoning for kwebenginepart, which is also why it was moved into the konqueror git repo)

Either way, throwing an error about failure to create an io-slave is inappriopriate, at least in Konqueror which knows that there's no slave for the protocol in question.

Feel free to improve error handling by adding a check in the right place.

And what about the file access errors, maybe I should just file a bug ticket for those?

That's @stefanocrocco 's domain. Unless he reacts here, yes, file a bug ticket and assign it to him.

BTW, my konqueror build on Linux still throws io-slave errors about the http protocols while the identical Mac build running in an as-identical-as-possible environment works fine. The help: protocol does work (though it shows a blank page in the WebKit view mode). This is one reason why I first looked for a missing konq: KIO slave ... but it doesn't help to motivate me to work on a webkit implementation for the konq: urls ... I cannot help but feel that the idea of io slaves is nice but needlessly complicate...

Contradiction. If you implement konq: support in kwebkitpart then there is no kioslave involved.

It took me way too long to figure out that this is the source of the regression below, and not a missing "konq:" kioslave ...

For me, the main interest in Konqueror lies in its ability to use WebKit2 (with the rebooted QtWebKit version, evidently), so I can use it in a more or less modern browser interface. (There's just no need for yet another grass-roots wrapper around Chromium IMHO but that's a different issue.)

When I do activate the WebEngine backend I see this on the terminal:

js: Not allowed to load local resource: file:///opt/local/share/kf5/infopage/kde_infopage.css
js: Not allowed to load local resource: file:///opt/local/share/konqueror/about/konq.css
js: Not allowed to load local resource: file:///opt/local/share/icons/Ciment/22x22/places/user-home.png
js: Not allowed to load local resource: file:///opt/local/share/icons/osx/places/48/user-trash-full.png
js: Not allowed to load local resource: file:///opt/local/share/icons/osx/places/22/folder-remote.png
js: Not allowed to load local resource: file:///opt/local/share/icons/oxygen/base/22x22/actions/bookmarks.png
js: Not allowed to load local resource: file:///opt/local/share/icons/Ciment/16x16/actions/go-next.png

but pasting the URL into the konqueror address bar opens the files just fine.

I'll look into this tomorrow, but I don't understand what you mean by "When I do activate the WebEngine backend". I have the WebEngine backend on by default and I don't get that message. I tried setting the default engine to WebKit, restarting Konqueror, then, from the strarting page, switching to WebEngine using the View Mode menu. But I still don't see it. Could you please give me more details on what you do to see that message? Thanks.

rjvbb added a comment.Jun 12 2020, 8:12 PM

This is exactly what I do:

I tried setting the default engine to WebKit, restarting Konqueror, then, from the strarting page, switching to WebEngine using the View Mode menu.

No need to do anything else. Here's the full output when I launch konqueror (with WebKit as the default engine) and then switch it to WebEngine (after the slave error message):

org.kde.kwindowsystem: Loaded plugin "/opt/local/share/qt5/plugins/kf5/org.kde.kwindowsystem.platforms/KF5WindowSystemX11Plugin.so" for platform "xcb"
kf5.kio.core: couldn't create slave: "klauncher said: Unknown protocol 'konq'.\n"
[12814:12972:0612/220936.807589:ERROR:nss_util.cc(808)] After loading Root Certs, loaded==false: NSS error code: -8018
QFSFileEngine::open: No file name specified
js: Not allowed to load local resource: file:///opt/local/share/kf5/infopage/kde_infopage.css
js: Not allowed to load local resource: file:///opt/local/share/konqueror/about/konq.css
QFSFileEngine::open: No file name specified
js: Not allowed to load local resource: file:///usr/share/icons/Ciment/32x32/places/user-home.png
js: Not allowed to load local resource: file:///usr/share/icons/osx/places/48/user-trash-full.png
js: Not allowed to load local resource: file:///usr/share/icons/osx/places/32/folder-remote.png
js: Not allowed to load local resource: file:///usr/share/icons/oxygen/32x32/places/bookmarks.png
js: Not allowed to load local resource: file:///usr/share/icons/Ciment/16x16/actions/go-next.png
QFSFileEngine::open: No file name specified
rjvbb added a comment.Jun 12 2020, 8:19 PM

Interestingly:

  • right-click in the WebEngine view of the startup/about page, select "View Source"
  • without quitting the source viewer (Kate in my case), open the /tmp/konqueror*.html file in konqueror
  • select the WebEngine view mode
  • document is rendered correctly, without error.

This is exactly what I do:

I tried setting the default engine to WebKit, restarting Konqueror, then, from the strarting page, switching to WebEngine using the View Mode menu.

No need to do anything else. Here's the full output when I launch konqueror (with WebKit as the default engine) and then switch it to WebEngine (after the slave error message):

org.kde.kwindowsystem: Loaded plugin "/opt/local/share/qt5/plugins/kf5/org.kde.kwindowsystem.platforms/KF5WindowSystemX11Plugin.so" for platform "xcb"
kf5.kio.core: couldn't create slave: "klauncher said: Unknown protocol 'konq'.\n"
[12814:12972:0612/220936.807589:ERROR:nss_util.cc(808)] After loading Root Certs, loaded==false: NSS error code: -8018
QFSFileEngine::open: No file name specified
js: Not allowed to load local resource: file:///opt/local/share/kf5/infopage/kde_infopage.css
js: Not allowed to load local resource: file:///opt/local/share/konqueror/about/konq.css
QFSFileEngine::open: No file name specified
js: Not allowed to load local resource: file:///usr/share/icons/Ciment/32x32/places/user-home.png
js: Not allowed to load local resource: file:///usr/share/icons/osx/places/48/user-trash-full.png
js: Not allowed to load local resource: file:///usr/share/icons/osx/places/32/folder-remote.png
js: Not allowed to load local resource: file:///usr/share/icons/oxygen/32x32/places/bookmarks.png
js: Not allowed to load local resource: file:///usr/share/icons/Ciment/16x16/actions/go-next.png
QFSFileEngine::open: No file name specified

I can't really reproduce this issue. Which Qt version are you using? The way Konqueror has QWebEngine handle local resources has changed radically from Qt 5.12 onwards. Do you have the same problems using protocols such as man or info from Konqueror?

I can't really reproduce this issue. Which Qt version are you using?

I don't really expect you did a full build of Qt, the frameworks etc. into a parallel prefix like I do, or did you?

Hum, yes, I did backport the code so it builds against Qt 5.9 but

The way Konqueror has QWebEngine handle local resources has changed radically from Qt 5.12 onwards.

just how would that explain that the generated html code opens just fine when I save it to a .html file first?
I can understand you need to intercept links that require actions not implemented by the engine but why would you need to do that for CSS and image resources?

Does this radical change depend on new QtWebEngine APIs not in 5.9? I do have a Qt 5.12 build installed with a wrapper script causing my KDE builds to use that Qt version. I get the same issue then, which would be expected if my backport shoehorning missed something crucial.

BTW, when I run against Qt 5.12 I see this additional output on the terminal when I switch to the WebEngine mode:

Please register the custom scheme 'error' via QWebEngineUrlScheme::registerScheme() before installing the custom scheme handler.
Please register the custom scheme 'konq' via QWebEngineUrlScheme::registerScheme() before installing the custom scheme handler.
Please register the custom scheme 'help' via QWebEngineUrlScheme::registerScheme() before installing the custom scheme handler.

I'll see if I can get the build to pick up my Qt 5.12 install. Can be tricky with KDE's cmake files but who knows. (This is not crucial enough for me to rebuild all my frameworks against 512, sorry!)

Do you have the same problems using protocols such as man or info from Konqueror?

No (if they work at all, of course).

@stefanocrocco if it makes your life easier I'm fine with konqueror requiring Qt >= 5.12 (currently the CMakeLists.txt says 5.9, but KF5 requires 5.12)

I can't really reproduce this issue. Which Qt version are you using?

I don't really expect you did a full build of Qt, the frameworks etc. into a parallel prefix like I do, or did you?

Hum, yes, I did backport the code so it builds against Qt 5.9 but

The way Konqueror has QWebEngine handle local resources has changed radically from Qt 5.12 onwards.

just how would that explain that the generated html code opens just fine when I save it to a .html file first?
I can understand you need to intercept links that require actions not implemented by the engine but why would you need to do that for CSS and image resources?

Does this radical change depend on new QtWebEngine APIs not in 5.9? I do have a Qt 5.12 build installed with a wrapper script causing my KDE builds to use that Qt version. I get the same issue then, which would be expected if my backport shoehorning missed something crucial.

BTW, when I run against Qt 5.12 I see this additional output on the terminal when I switch to the WebEngine mode:

Please register the custom scheme 'error' via QWebEngineUrlScheme::registerScheme() before installing the custom scheme handler.
Please register the custom scheme 'konq' via QWebEngineUrlScheme::registerScheme() before installing the custom scheme handler.
Please register the custom scheme 'help' via QWebEngineUrlScheme::registerScheme() before installing the custom scheme handler.

The problem is that by default QtWebEngine only allows access to local resources from local files, that is using URLs with a file scheme (this is why it works when you save the HTML code to a local file). In Qt 5.12 onwards QtWebEngine has improved its API in this regard: you need to register custom schemes using QWebEngineUrlScheme::registerScheme() (as the warning says) and this allows you to specify which schemes should be allowed to access local resources. Before Qt 5.12, we worked around this issue for man and info schemes by inspecting the HTML code generated by KIO and embedding css and images inside the HTML code itself. When the About page was ported to QtWebEngine, Qt 5.12 had already been released so I completely forgot about this issue. I'll see if the same workaround can be applied here.

rjvbb added a comment.Jun 13 2020, 2:20 PM
The problem is that by default QtWebEngine only allows access to local resources from local files, that is using URLs with a file scheme (this is why it works when you save the HTML code to a local file).

Okkkk... I was already wondering which of my backport changes could have caused the issue because they all appear to be related to building against KF5 5.60.0, without anything Qt specific. And those KF5 changes are about KRun ... but maybe the replacement functions hook into QtWebEngine?

Anyway, as David says, you could just align Konqueror's Qt requirement with the stricter one from its KF5 dependencies. There is little point not to, after all. Rather spend your time getting rid of the inappropriate errors about a missing konq slave. David already explained why the konq: stuff cannot be implemented upstream from the kparts in Konqueror itself (invoking binaries), but maybe an actual KIO slave could do everything you want too while still serving standard HTML to the kparts doing the rendering?

I'll ping you if I do decide to implement konq: support in kwebkit_part and run into something similar there.

Anyway, as David says, you could just align Konqueror's Qt requirement with the stricter one from its KF5 dependencies. There is little point not to, after all. Rather spend your time getting rid of the inappropriate errors about a missing konq slave. David already explained why the konq: stuff cannot be implemented upstream from the kparts in Konqueror itself (invoking binaries), but maybe an actual KIO slave could do everything you want too while still serving standard HTML to the kparts doing the rendering?

I'll ping you if I do decide to implement konq: support in kwebkit_part and run into something similar there.

I, too, think that increasing the minumum required Qt version is the best option. I was trying to apply the workaround used for man and info schemes before Qt 5.12 to the konq scheme and it made the code much more complex (besides, for some reason it wasn't working). I'll look into this, and in fixing the error message, in the next few days.