Bug 34884 - [Qt] QWebSettings::setUserStyleSheetUrl() does not work with windows paths that contain drive letters
Summary: [Qt] QWebSettings::setUserStyleSheetUrl() does not work with windows paths th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-02-12 03:01 PST by Lev Golod
Modified: 2011-09-10 06:15 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch (1.33 KB, patch)
2010-12-15 17:29 PST, Jarred Nicholls
webkit.review.bot: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (3.98 KB, patch)
2011-09-09 12:40 PDT, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Proposed Patch (4.34 KB, patch)
2011-09-09 13:46 PDT, Jarred Nicholls
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lev Golod 2010-02-12 03:01:39 PST
When QWebSettings::setUserStyleSheetUrl() is used to set user style sheet nothing happens.
It starts from Qt 4.6.0
Qt 4.5.x works correctly.

To reproduce it's possible to start Qt demo Browser and setup style sheet URL in Edit -> Preferences -> Advanced -> Style Sheet
Comment 1 Benjamin Poulain 2010-03-05 15:16:29 PST
That seems broken as well with Qt 4.7.
Comment 2 Antonio Gomes 2010-03-07 05:51:55 PST
I remember diego  playing w/ this stuff. Diegou could you please verify if it is a dup of the bug you solved or a real problem ? if the later, a diagnostic would be great
Comment 3 Simon Hausmann 2010-03-08 07:23:05 PST
This works for me. Note that you can only specify a path to a local file or a base64 encoded url.

From the documentation:

/*!
    Specifies the location of a user stylesheet to load with every web page.

    The \a location must be either a path on the local filesystem, or a data URL
    with UTF-8 and Base64 encoded data, such as:

    "data:text/css;charset=utf-8;base64,cCB7IGJhY2tncm91bmQtY29sb3I6IHJlZCB9Ow=="

    \sa userStyleSheetUrl()
*/
Comment 4 tracy.rees 2010-03-19 13:01:52 PDT
It seems that
"The location must be either a path on the local filesystem" does not work.

I've tried very hard on 4.6.2 and it just doesn't work.
Comment 5 Benjamin Poulain 2010-03-19 14:09:25 PDT
Could(In reply to comment #4)
> It seems that
> "The location must be either a path on the local filesystem" does not work.
> 
> I've tried very hard on 4.6.2 and it just doesn't work.

Could you please attach a simple test case to reproduce the problem?
So we can quickly check if something is wrong is your code or in WebKit?
Comment 6 Lev Golod 2010-03-20 01:52:38 PDT
#include <QtGui/QApplication>
#include <QWebView>
#include <QUrl>

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    QWebView view;
    QByteArray css("div { background-color: red }");
    
    // that's  works fine
    //view.settings()->setUserStyleSheetUrl(QUrl("data:text/css;charset=utf-8;base64," + css.toBase64()));    
    
    // doesn't work
    view.settings()->setUserStyleSheetUrl(QUrl::fromLocalFile("d:/test.css"));    
    
    view.load(QUrl("http://qtsoftware.com/"));
    view.show();
    return a.exec();
}

It seems like 
String KURL::fileSystemPath() const 
returns "/d:/test.css" that QFileInfo can't understand.
Comment 7 Simon Hausmann 2010-03-21 16:27:06 PDT
(In reply to comment #6)
> #include <QtGui/QApplication>
> #include <QWebView>
> #include <QUrl>
> 
> int main(int argc, char *argv[])
> {
>     QApplication a(argc, argv);
>     QWebView view;
>     QByteArray css("div { background-color: red }");
> 
>     // that's  works fine
>    
> //view.settings()->setUserStyleSheetUrl(QUrl("data:text/css;charset=utf-8;base64,"
> + css.toBase64()));    
> 
>     // doesn't work
>     view.settings()->setUserStyleSheetUrl(QUrl::fromLocalFile("d:/test.css"));  
> 
>     view.load(QUrl("http://qtsoftware.com/"));
>     view.show();
>     return a.exec();
> }
> 
> It seems like 
> String KURL::fileSystemPath() const 
> returns "/d:/test.css" that QFileInfo can't understand.

Well spotted. It seems KURL::fileSystemPath() should use toLocalFile() instead of simply path().
Comment 8 Simon Hausmann 2010-03-21 16:29:30 PDT
(In reply to comment #7)
> Well spotted. It seems KURL::fileSystemPath() should use toLocalFile() instead
> of simply path().

Which argh isn't correct either. QUrl::toLocalFile() will return an empty string if the scheme is qrc (or != file && !isEmpty().

Looks like we need a special case and a unit test.
Comment 9 Kalle Vahlman 2010-03-22 11:19:54 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Well spotted. It seems KURL::fileSystemPath() should use toLocalFile() instead
> > of simply path().
> 
> Which argh isn't correct either. QUrl::toLocalFile() will return an empty
> string if the scheme is qrc (or != file && !isEmpty().
> 
> Looks like we need a special case and a unit test.

For what it's worth, the example doesn't work on Linux either, with a seemingly valid file URL:
 QUrl( "file:///tmp/test.css" )
Comment 10 Diego Gonzalez 2010-03-22 11:40:46 PDT
> For what it's worth, the example doesn't work on Linux either, with a seemingly
> valid file URL:
>  QUrl( "file:///tmp/test.css" )

Actually it works in this case since r53276. Which WebKit revision are you using?
Comment 11 Kalle Vahlman 2010-03-23 05:27:52 PDT
(In reply to comment #10)
> > For what it's worth, the example doesn't work on Linux either, with a seemingly
> > valid file URL:
> >  QUrl( "file:///tmp/test.css" )
> 
> Actually it works in this case since r53276. Which WebKit revision are you
> using?

Apparently something before that, from the 4.6.1 Qt release.

Checking the changeset confirms that I don't have that change here, so sorry for the noise :)
Comment 12 Jarred Nicholls 2010-12-15 17:29:30 PST
Created attachment 76715 [details]
Proposed patch

QUrl::toLocalFile() is perfectly sufficient right here.  KURL::path() returns a extra leading slash for Windows file URIs, but QUrl::toLocalFile() will handle these well.

A separate bug will be filed for allowing style sheet files as qrc resource URIs, which involves changing WebCore/page/Page.cpp.
Comment 13 WebKit Review Bot 2010-12-17 12:51:35 PST
Comment on attachment 76715 [details]
Proposed patch

Rejecting attachment 76715 [details] from review queue.

jarred.nicholls@gmail.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 14 WebKit Review Bot 2010-12-17 12:53:28 PST
Comment on attachment 76715 [details]
Proposed patch

Rejecting attachment 76715 [details] from commit-queue.

jarred.nicholls@gmail.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 15 Andreas Kling 2010-12-18 07:39:42 PST
Comment on attachment 76715 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76715&action=review

> WebCore/ChangeLog:10
> +        No new tests added.

Why not?

> WebCore/platform/qt/KURLQt.cpp:3
> + * Copyright (C) 2010 Sencha, Inc. All rights reserved.

This change is a bit too skinny to warrant a copyright line. :-)
Comment 16 Antonio Gomes 2010-12-18 12:09:01 PST
> > WebCore/platform/qt/KURLQt.cpp:3
> > + * Copyright (C) 2010 Sencha, Inc. All rights reserved.
> 
> This change is a bit too skinny to warrant a copyright line. :-)

Although I agree with you, kling, any change can warrant a copyright addition, so it is up to the developer, but definitively he is not wrong.
Comment 17 Jarred Nicholls 2011-09-09 12:40:30 PDT
Created attachment 106905 [details]
Proposed Patch
Comment 18 Jarred Nicholls 2011-09-09 13:46:25 PDT
Created attachment 106915 [details]
Proposed Patch
Comment 19 Andreas Kling 2011-09-10 03:59:00 PDT
Comment on attachment 106915 [details]
Proposed Patch

Great!
Comment 20 WebKit Review Bot 2011-09-10 06:14:55 PDT
Comment on attachment 106915 [details]
Proposed Patch

Clearing flags on attachment: 106915

Committed r94906: <http://trac.webkit.org/changeset/94906>
Comment 21 WebKit Review Bot 2011-09-10 06:15:01 PDT
All reviewed patches have been landed.  Closing bug.