[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