[webkit-reviews] review denied: [Bug 51159] [Qt] Permit qrc resources to load in QWebSettings::setUserStyleSheetUrl() : [Attachment 106917] Proposed Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Sep 10 04:12:19 PDT 2011
Andreas Kling <kling at webkit.org> has denied Jarred Nicholls
<jarred at sencha.com>'s request for review:
Bug 51159: [Qt] Permit qrc resources to load in
QWebSettings::setUserStyleSheetUrl()
https://bugs.webkit.org/show_bug.cgi?id=51159
Attachment 106917: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=106917&action=review
------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106917&action=review
I like where this is going, but let me put my nitpick hat on for a sec.
> Source/WebCore/page/Page.cpp:649
> + // Allow any local file URI scheme to be loaded
URI here and URL elsewhere? I admit I'm a little foggy on the distinction, but
the usage here should be consistent, no?
Also, period at end of sentence.
> Source/WebCore/platform/qt/KURLQt.cpp:46
> + // Permit valid file or qrc URLs
Ditto.
> Source/WebCore/platform/qt/KURLQt.cpp:47
> + if (isValid() && (protocolIs("file") || protocolIs("qrc"))) {
I'd prefer isLocalFile() to protocolIs("file") here.
Also, the double occurrence of the "qrc" literal is ugly. Please rephrase this
so we only need it once.
> Source/WebCore/platform/qt/KURLQt.cpp:48
> + // A valid qrc resource path begins with a colon
Ditto.
> Source/WebCore/platform/qt/KURLQt.cpp:52
> + // Convert the file URL into a proper platform file path
Ditto.
> Source/WebCore/platform/qt/KURLQt.cpp:53
> + return static_cast<QUrl>(*this).toLocalFile();
Since this line was already changed by a recent patch of yours, this will need
a reupload.
> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:489
> + QVERIFY(networkManager->requestedUrls.count() >= 1);
Is there an exact value that we could check against instead of ">=1"?
More information about the webkit-reviews
mailing list