[Webkit-unassigned] [Bug 51159] [Qt] Permit qrc resources to load in QWebSettings::setUserStyleSheetUrl()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=51159





--- Comment #13 from Jarred Nicholls <jarred at sencha.com>  2011-09-10 06:23:56 PST ---
(In reply to comment #11)
> (From update of attachment 106917 [details])
> 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.
> 

This is good, it was written 9 months ago and I didn't give the comments much attention.

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

Sure

> Also, the double occurrence of the "qrc" literal is ugly. Please rephrase this so we only need it once.

Agreed

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

Yeah the two patches are related.  I knew up front this would cause a merge conflict depending on which went first; but I figured a committer could resolve it easily.  I will rebase...

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

Perhaps, though that's not what we're testing.  This (like in other userStyleSheet test) is just a sanity check before accessing at(0), in my opinion.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.