Bug 33614 – [Qt] Missing fileSystemPath() method in Qt KURL implementation

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Diego Gonzalez]

The method fileSystemPath() is not implemented in Qt KURL side.

This method is currently return a empty string which can fails some
operations like set a user remote css.

[Diego Gonzalez]

Created attachment 46491 [details]
Proposed patch

[WebKit Review Bot]

Attachment 46491 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/185686

[Diego Gonzalez]

Created attachment 46502 [details]
Proposed patch v0.2

Make check of Qt version to user QUrl::fromUserInput method

[Diego Gonzalez]

Created attachment 46506 [details]
Proposed patch v0.3

Use m_strig instead of non-existent input variable

[Antonio Gomes]

LGTM !

@kenneth, could you please officially review ?

good work.

(In reply to comment #4)
> Created an attachment (id=46506) [details]
> Proposed patch v0.3
> 
> Use m_strig instead of non-existent input variable

[Kenneth Rohde Christiansen]

Comment on attachment 46506 [details]
Proposed patch v0.3


>  
>  String KURL::fileSystemPath() const
>  {
> -    notImplemented();
> +    if (m_string.isEmpty())
> +        return String();
> +
> +#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
> +    QUrl url = QUrl::fromUserInput(m_string);

Why use this? This might change an invalid URL into a valid one. This doesn't seem right and at least needs a comment

> +#else
> +    QUrl url = QUrl(m_string);
> +#endif
> +    if (url.isValid())
> +        return String(url.path());
> +
>      return String();
>  }
>

[Diego Gonzalez]

Created attachment 46558 [details]
Proposed patch v0.4

Not using the QUrl::fromUserInput method anymore. It's really
does not make sense to use it  in this situation as commented by Kenneth.

[Kenneth Rohde Christiansen]

Comment on attachment 46558 [details]
Proposed patch v0.4

Shouldn't you somehow verify that m_string is a file path, like comparing url.scheme() (or what it is called in KURL) with "file"

>  
>  String KURL::fileSystemPath() const
>  {
> -    notImplemented();
> +    if (m_string.isEmpty())
> +        return String();
> +
> +    QUrl url = QUrl(m_string);
> +    if (url.isValid())
> +        return String(url.path());
> +
>      return String();
>  }
>

[Antonio Gomes]

(In reply to comment #8)
> (From update of attachment 46558 [details])
> Shouldn't you somehow verify that m_string is a file path, like comparing
> url.scheme() (or what it is called in KURL) with "file"

hum, diego , kenneth , does not isValid take care of this?

> 
> >  
> >  String KURL::fileSystemPath() const
> >  {
> > -    notImplemented();
> > +    if (m_string.isEmpty())
> > +        return String();
> > +
> > +    QUrl url = QUrl(m_string);
> > +    if (url.isValid())
> > +        return String(url.path());
> > +
> >      return String();
> >  }
> >

[Diego Gonzalez]

Created attachment 46564 [details]
Proposed patch v0.5

Using KURL methods instead of QUrl. Reviewed by Kenneth

[Diego Gonzalez]

Created attachment 46567 [details]
Final patch

Correct patch

[WebKit Commit Bot]

Comment on attachment 46567 [details]
Final patch

Rejecting patch 46567 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/qt/KURLQt.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 558


Full output: http://webkit-commit-queue.appspot.com/results/186439

[Antonio Gomes]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/qt/KURLQt.cpp
Committed r53276

[Eric Seidel (no email)]

Ideally the "no new tests" line should be replaced either by pointers to new tests, or explanations why testing is impossible or infeasible. :)
http://webkit.org/coding/contributing.html#changelogs

http://trac.webkit.org/changeset/53276

[Antonio Gomes]

(In reply to comment #14)
> Ideally the "no new tests" line should be replaced either by pointers to new
> tests, or explanations why testing is impossible or infeasible. :)
> http://webkit.org/coding/contributing.html#changelogs
> 
> http://trac.webkit.org/changeset/53276

eric, i see your concern. But specifically in this case, no tests were added, *but* one existent will be unskipped. see bug 33617 , which i am landing manually.

[Eric Seidel (no email)]

Sounds fine. :)  Ideally we should mention that the one will be unskipped.  None of this is a big deal.  I just saw this commit-queue failure, pulled up the bug, noticed the nit and decided to comment.

[Simon Hausmann]

(In reply to comment #13)
> Committing to http://svn.webkit.org/repository/webkit/trunk ...
>     M    WebCore/ChangeLog
>     M    WebCore/platform/qt/KURLQt.cpp
> Committed r53276

Cherry-picked into qtwebkit-4.6 with commit 36fe058a9001e6d47f0fd41c6304cdfdf3a735ed