Bug 107031

Created attachment 183435 [details] Patch

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 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 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"),...)

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

(In reply to comment #5) > (In reply to comment #4) > Humm, that was pretty useless :P As you both agree, it must be right ;)

Created attachment 183993 [details] Patch

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.

Created attachment 184218 [details] Patch

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 on attachment 184218 [details] Patch What about a unit test? :)

Created attachment 184238 [details] Patch

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 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?

Created attachment 184460 [details] Patch

Comment on attachment 184460 [details] Patch Amazing.

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