Bug 107031 - [Qt][WK2] Pages / resources cannot be loaded from qrc files.
Summary: [Qt][WK2] Pages / resources cannot be loaded from qrc files.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Brüning
URL:
Keywords:
Depends on:
Blocks: 103747
  Show dependency treegraph
 
Reported: 2013-01-16 10:17 PST by Michael Brüning
Modified: 2013-01-24 04:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.19 KB, patch)
2013-01-18 05:57 PST, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (5.96 KB, patch)
2013-01-22 08:30 PST, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (5.92 KB, patch)
2013-01-23 06:25 PST, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2013-01-23 08:02 PST, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (8.05 KB, patch)
2013-01-24 04:10 PST, Michael Brüning
jturcotte: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Brüning 2013-01-16 10:17:47 PST
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.
Comment 1 Michael Brüning 2013-01-18 05:57:44 PST
Created attachment 183435 [details]
Patch
Comment 2 Andras Becsi 2013-01-22 05:33:34 PST
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
Comment 3 Simon Hausmann 2013-01-22 07:01:22 PST
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.
Comment 4 Jocelyn Turcotte 2013-01-22 07:08:16 PST
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"),...)
Comment 5 Jocelyn Turcotte 2013-01-22 07:09:09 PST
(In reply to comment #4)
Humm, that was pretty useless :P
Comment 6 Michael Brüning 2013-01-22 07:32:41 PST
(In reply to comment #5)
> (In reply to comment #4)
> Humm, that was pretty useless :P

As you both agree, it must be right ;)
Comment 7 Michael Brüning 2013-01-22 08:30:06 PST
Created attachment 183993 [details]
Patch
Comment 8 Jocelyn Turcotte 2013-01-23 05:39:21 PST
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.
Comment 9 Michael Brüning 2013-01-23 06:25:08 PST
Created attachment 184218 [details]
Patch
Comment 10 Jocelyn Turcotte 2013-01-23 06:34:19 PST
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.
Comment 11 Simon Hausmann 2013-01-23 07:23:59 PST
Comment on attachment 184218 [details]
Patch

What about a unit test? :)
Comment 12 Michael Brüning 2013-01-23 08:02:37 PST
Created attachment 184238 [details]
Patch
Comment 13 Michael Brüning 2013-01-23 08:04:12 PST
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.
Comment 14 Benjamin Poulain 2013-01-23 13:49:42 PST
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?
Comment 15 Michael Brüning 2013-01-24 04:10:01 PST
Created attachment 184460 [details]
Patch
Comment 16 Jocelyn Turcotte 2013-01-24 04:19:51 PST
Comment on attachment 184460 [details]
Patch

Amazing.
Comment 17 Michael Brüning 2013-01-24 04:26:54 PST
Committed r140676: <http://trac.webkit.org/changeset/140676>