Differential D29066

Ensure that WebEnginePart uses proxy settings from KCM
ClosedPublic
Actions

Authored by stefanocrocco on Apr 21 2020, 5:26 PM.

Details

Reviewers
dfaure
Summary

Force WebEnginePart to follow the global KDE proxy settings.

QtWebEngine uses the proxy set with QNetworkProxy::setApplicationProxy, which
means that it ignores the KDE proxy settings chosen by the user.

To force QtWebEngine to respect the user's preferences, read the proxy settings
using KProtocolManager and call QNetworkProxy::setApplicationProxy
appropriately. QtWebEngine only supports a global proxy settings, so you can't
have different proxies for different URLs or different proxies for HTTP and
HTTPS URLs (except, perhaps, by calling QNetworkProxy::setApplicationProxy for
every call to Qt::WebEngine::acceptNavigationRequest). Because of this
limitation, the following approach is used:

  • call KProtocolManager::proxyForUrl twice: once with a fake HTTP URL and once with a fake HTTPS protocol
  • if the two proxies are different, ask which one should be used.

Diff Detail

Repository
R226 Konqueror
Branch
respect-proxy-settings
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27135
Build 27153: arc lint + arc unit
stefanocrocco requested review of this revision.Apr 21 2020, 5:26 PM
stefanocrocco created this revision.
dfaure accepted this revision.Apr 22 2020, 1:27 PM

Note that KProtocolManager also supports per-host proxies (via KPAC which runs javascript). This obviously won't work here.

This revision is now accepted and ready to land.Apr 22 2020, 1:27 PM
stefanocrocco added a comment.Apr 22 2020, 5:11 PM
In D29066#654708, @dfaure wrote:

Note that KProtocolManager also supports per-host proxies (via KPAC which runs javascript). This obviously won't work here.

Given that currently there's no way to make that work with QtWebEngine, do you think that displaying a warning would be enough?

dfaure added a comment.Apr 22 2020, 7:54 PM

Yes, you could check KProtocolManager::proxyType() and warn if it's PACProxy or WPADProxy.

stefanocrocco updated this revision to Diff 80987.Apr 23 2020, 11:43 AM
  • Don't attempt to use WPAD or PAC proxies
  • Offer the user the ability to immediately display the proxy settins dialog
stefanocrocco requested review of this revision.Apr 23 2020, 11:43 AM
stefanocrocco added a comment.Jun 13 2020, 9:23 AM

David, have you had time to check the changes I made to this?

dfaure accepted this revision.Jun 13 2020, 11:52 AM

Sorry, slipped my mind.

Just minor things. Feel free to push with or without those fixes.

src/konqmainwindow.cpp
1732

nullptr

1766

Assuming fmModules[i] is never empty, the check !startingModule.isEmpty() is redundant.

5549

This question could remind the user what the settings for both actually are.
One might not remember what was set long ago...

(OK you also offer opening the settings, but that means going there, reading, coming back here, triggering this method again, to finally make the choice...)

src/konqmainwindow.h
369

const QString &

This revision is now accepted and ready to land.Jun 13 2020, 11:52 AM
stefanocrocco updated this revision to Diff 83266.Jun 13 2020, 4:30 PM
  • Improve the message text
  • Only ask about invalid proxy settings for QtWebEngine if it's the default engine
stefanocrocco requested review of this revision.Jun 13 2020, 4:32 PM
dfaure accepted this revision.Jun 13 2020, 6:01 PM

Good idea to check the default webengine. Just in case :)

src/konqmainwindow.cpp
1795

the redundant check happens here too ;)

This revision is now accepted and ready to land.Jun 13 2020, 6:01 PM
stefanocrocco added a comment.Jun 14 2020, 8:46 AM

David, sorry to disturb you but I'm having trouble while pushing this. I think it's related to the GitLab migration. I had to run git kclone in my local clone to pull the latest changes. Now, I can't use arc land because it complains that

This command requires arc to connect to a Phabricator install, but no
Phabricator installation is configured. To configure a Phabricator URI:

I tried using git push origin master and I get this error:

git@invent.kde.org: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I tried looking at the mails about the switch to GitLab and at the on-line documentation, but I couldn't find anything. Do you know what I'm doing wrong? Thanks

dfaure added a comment.Jun 14 2020, 9:41 AM

In the repo where your git push fails, what does git remote -v say?

stefanocrocco added a comment.Jun 14 2020, 9:47 AM
In D29066#675217, @dfaure wrote:

In the repo where your git push fails, what does git remote -v say?

This is the output from git:

origin  https://invent.kde.org/network/konqueror.git (fetch)
origin  git@invent.kde.org:network/konqueror.git (push)
bcooksley added a subscriber: bcooksley.Jun 14 2020, 9:49 AM

@stefanocrocco Can you please confirm you have uploaded your SSH Keys to https://invent.kde.org/?
This has to be done separately to the upload on Identity (which no longer has any effect)

stefanocrocco added a comment.Jun 14 2020, 9:55 AM
In D29066#675219, @bcooksley wrote:

@stefanocrocco Can you please confirm you have uploaded your SSH Keys to https://invent.kde.org/?
This has to be done separately to the upload on Identity (which no longer has any effect)

You're right! It seems I either forgot to do so or I missed this piece of information.

Thanks

stefanocrocco closed this revision.Jun 14 2020, 1:07 PM

Revision Contents
Changeset List

PathPackages
Msrc/konqmainwindow.h (4 lines)
Msrc/konqmainwindow.cpp (79 lines)

Diff 83266

View Options

src/konqmainwindow.h

Loading...
View Options

src/konqmainwindow.cpp

Loading...