Details

Reviewers

Commits

R226:81e224f91994: Display an error page for error URLs in WebEnginePart

Summary

When asked to display an error URL, WebEnginePart displayed another
error telling that the web page "error:/..." can't be reached.

To avoid this, add a scheme handler for the "error" scheme to the default
profile. The handler replies with the html code read from an error.html page
with placeholders replaced by the error details contained in the URL.

The code used to produce the error page is taken, with small changes, by
webpage.cpp from KWebKitPart. The error page, too is taken from KWebKitPart and
modified a little.

Test Plan

Entered an invalid URL in address bar and checked that the error message was
displayed correctly.

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.May 1 2018, 4:09 PM

stefanocrocco created this revision.

stefanocrocco added a reviewer: dfaure.May 1 2018, 4:10 PM

stefanocrocco edited the summary of this revision. (Show Details)May 1 2018, 4:28 PM

Nice! I remember that this was sorely missing when I was using that part :-)

webenginepart/src/webenginepart.cpp
90	missing space after if
webenginepart/src/webengineparterrorschemehandler.cpp
46	if you move this to the ctor init list, you can make the variable const.
52	no space before ( But I would split this; if the icon is not found, iconPath returns an empty string, and QFile doesn't like that (-> runtime warning) to better test that first.
61	unnecessary, the QFile destructor closes the file.
102	and suddenly from this point on, there are spaces inside parenthesis? ;-) weird...
webenginepart/src/webengineparterrorschemehandler.h
31	missing space before ':'

This revision now requires changes to proceed.May 1 2018, 5:09 PM

Made corrections as required

anthonyfieroni added a subscriber: anthonyfieroni.May 1 2018, 6:59 PM

anthonyfieroni added inline comments.

webenginepart/src/error.html
4	Why not use %1, %2, %3, other % should be doubled.

Thanks for the fixes, here are a few more nitpicks ;)

webenginepart/src/webengineparterrorschemehandler.cpp
52	missing space after if ;) (and before the opening curly brace)
108	remove space before first '('
109	remove space inside (), add one before {
128	isEmpty() would be better than isNull(), no?
141	no space before '('
142	isEmpty() would be better than isNull, no?
159	should be if (!causes.isEmpty()), as a general rule (we don't need the actual count)
167	same

This revision now requires changes to proceed.May 1 2018, 8:45 PM

Fixed issues requested by dfaure

stefanocrocco added inline comments.May 2 2018, 6:39 PM

webenginepart/src/error.html
4	Do you mean replacing TITLE, DIRECTION, ICON_PATH and TEXT with %1, %2 and so on, then using QString::arg to replace the placeholders with the actual values?

anthonyfieroni added inline comments.May 2 2018, 7:07 PM

webenginepart/src/error.html
4	Yes.

In both cases the same fragility exists, in case one of the replacement strings contains one of the placeholders --- unless you use multi-arg, i.e. myString.arg(title, direction, m_warningIconData, ...)

dfaure added inline comments.May 3 2018, 7:58 AM

webenginepart/src/webenginepart.cpp
90	missing space before {
webenginepart/src/webengineparterrorschemehandler.cpp
109	too many spaces inside parenthesis, on this line.
122	remove space before (

@dfaure you're right except that replaced string can contain other one, since TEXT is the last this isn't so impossible :)

I don't understand the "except" in your reply.

The fact that "replaced string can contain other one" is exactly what I'm pointing out... but that's only a problem with replace(a,b) or .arg(a).arg(b).arg(c). Multi-arg doesn't have that problem.

Used QString::arg instead of QString::replace and fixed spaces

I've received no more comments on this review request even if I believe I made all requested changes. Did I miss something? Should I check the "Done" checkbox for each change to notify that it has been made? It's not clear whether those checkboxes are for the reviewers' use or mine.

Sorry, I failed to re-read it after you made the fixes.

The standard procedure in this case is to "ping" the reviewer in the review, which you just did ;)

This revision is now accepted and ready to land.Jun 24 2018, 11:19 AM

Closed by commit R226:81e224f91994: Display an error page for error URLs in WebEnginePart (authored by stefanocrocco). · Explain WhyJun 25 2018, 3:29 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		webenginepart/src/CMakeLists.txt (4 lines)
A	M	webenginepart/src/error.html (61 lines)
M		webenginepart/src/webenginepart.h (1 line)
M		webenginepart/src/webenginepart.cpp (6 lines)
A	M	webenginepart/src/webengineparterrorschemehandler.h (59 lines)
A	M	webenginepart/src/webengineparterrorschemehandler.cpp (174 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	33404	e72de7f		May 1 2018, 4:09 PM	★	★
Diff 2	33421	e72de7f	Make corrections as required	May 1 2018, 6:41 PM	★	★
Diff 3	33508	e72de7f	Fixed issues requested by dfaure	May 2 2018, 6:32 PM	★	★
Diff 4	33570	e72de7f	Used QString::arg instead of QString::replace and fixed spaces	May 3 2018, 6:56 PM	★	★
Diff 5	36639	68482de	R226:81e224f919941ec39c23e4ef72ce1f78caf0aa74	Jun 25 2018, 3:29 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
ee545ad55753	cd2a58632990	1b9acdc88fdc	Stefano Crocco	Display an error page for error URLs in WebEnginePart (Show More…)	May 1 2018, 3:48 PM
1b9acdc88fdc	a9f6dcb5fc54	617ed6cd7a02	Stefano Crocco	Addded license information	May 1 2018, 3:45 PM
617ed6cd7a02	f739ee75ba1c	e72de7f9d998	Stefano Crocco	Display an error page for error URLs in WebEnginePart (Show More…)	May 1 2018, 3:32 PM

Display an error page for error URLs in WebEnginePart
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 36639

webenginepart/src/CMakeLists.txt

webenginepart/src/error.html

webenginepart/src/webenginepart.h

webenginepart/src/webenginepart.cpp

webenginepart/src/webengineparterrorschemehandler.h

webenginepart/src/webengineparterrorschemehandler.cpp

Display an error page for error URLs in WebEnginePartClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 36639

webenginepart/src/CMakeLists.txt

webenginepart/src/error.html

webenginepart/src/webenginepart.h

webenginepart/src/webenginepart.cpp

webenginepart/src/webengineparterrorschemehandler.h

webenginepart/src/webengineparterrorschemehandler.cpp

Display an error page for error URLs in WebEnginePart
ClosedPublic
Actions

Revision Contents
Changeset List