[Qt][WK2] Pages / resources cannot be loaded from qrc files.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Michael Brüning]

Subject says most. This is an issue when someone would like e.g. to package a default page as part of the resources or package an offline page for a web app.

[Michael Brüning]

Created attachment 183435 [details]
Patch

[Andras Becsi]

Comment on attachment 183435 [details]
Patch

LGTM.

Note that this makes it possible to fix: https://bugreports.qt-project.org/browse/QTWEBKIT-388 and is blocking
https://codereview.qt-project.org/#change,45329

[Simon Hausmann]

Comment on attachment 183435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183435&action=review

> Source/WebKit2/UIProcess/API/qt/qquickurlschemedelegate_p.h:68
> +    QFile m_file;
> +    QFileInfo m_fileInfo;

I suggest to make these to variables local to readResourceAndSend().

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1354
> +    QString qrcString = QStringLiteral("qrc");
> +    if (!qrcString.compare(QString(req->data().m_scheme), Qt::CaseInsensitive)) {

I think you can eliminate the QString conversions and the local qrcString variable by simply writing:

if (req->data().m_scheme.startsWith("qrc", /*case sensitive: */false)) {

WTFString appears to have a nice templatized ASCII fast-path for startsWith.

[Jocelyn Turcotte]

Comment on attachment 183435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183435&action=review

> Source/WebKit2/UIProcess/API/qt/qquickurlschemedelegate_p.h:68
> +    QFile m_file;
> +    QFileInfo m_fileInfo;

I think those two could be local variables in readResourceAndSend.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1354
> +    QString qrcString = QStringLiteral("qrc");
> +    if (!qrcString.compare(QString(req->data().m_scheme), Qt::CaseInsensitive)) {

QString::compare has a QLatin1String overload, so I think you could avoid the temporary variable by doing something like QString(req->data().m_scheme).compare(QLatin1String("qrc"),...)

[Jocelyn Turcotte]

(In reply to comment #4)
Humm, that was pretty useless :P

[Michael Brüning]

(In reply to comment #5)
> (In reply to comment #4)
> Humm, that was pretty useless :P

As you both agree, it must be right ;)

[Michael Brüning]

Created attachment 183993 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 183993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183993&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:331
> +    webPageProxy->registerApplicationScheme(QStringLiteral("qrc"));

This ones takes a String, so it would be better to use ASCIILiteral("qrc").

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1316
> +    if (!scheme->scheme().compare(QStringLiteral("qrc"), Qt::CaseInsensitive)) {

Nitpick: I think that you can use QLatin1String instead of QStringLiteral and avoid the QString construction here since there is a QString::compare(QLatin1String&...) overload. For the amount of times that this method would be called though, it doesn't matter, so feel free to ignore.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1317
> +        qWarning("WARNING: The qrc scheme is reserved to be handled internally. Handler will be ignored.");

"Handlers" or "The handler" (I think)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1318
> +        scheme->setParent(0);

I think that QObject::~QObject will take care of it. It's when we are about to delete a parent that we have to unparent the children since they would be deleted.

[Michael Brüning]

Created attachment 184218 [details]
Patch

[Jocelyn Turcotte]

LGTM.

CCing Benjamin since somebody needs to stamp this as per the "At this point, we ask that all completely non-trivial patches be reviewed by an owner, even if in port specific code" directive.

[Simon Hausmann]

Comment on attachment 184218 [details]
Patch

What about a unit test? :)

[Michael Brüning]

Created attachment 184238 [details]
Patch

[Michael Brüning]

Comment on attachment 184238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184238&action=review

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_applicationScheme.qml:122
> +

Sorry for the extra whitespace, I'll remove it when landing.

[Benjamin Poulain]

Comment on attachment 184238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184238&action=review

I sign off on this for WK2. Feel free to r+ if the patch is correct.

> Source/WebKit2/UIProcess/API/qt/qquickurlschemedelegate.cpp:29
>  #include "qquicknetworkreply_p.h"
>  #include "qquicknetworkrequest_p.h"
> +#include <QMimeDatabase>
> +#include <QtCore/QFile>
> +#include <QtCore/QFileInfo>
> +#include <QtCore/QUrl>

include order?

[Michael Brüning]

Created attachment 184460 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 184460 [details]
Patch

Amazing.

[Michael Brüning]

Committed r140676: <http://trac.webkit.org/changeset/140676>