Bug 33614 - [Qt] Missing fileSystemPath() method in Qt KURL implementation
: [Qt] Missing fileSystemPath() method in Qt KURL implementation
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Qt
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
: Qt
Depends on:
Blocks: 33617
  Show dependency treegraph
 
Reported: 2010-01-13 12:19 PST by Diego Gonzalez
Modified: 2010-02-10 01:22 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (1.04 KB, patch)
2010-01-13 12:47 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch v0.2 (1.12 KB, patch)
2010-01-13 14:01 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch v0.3 (1.13 KB, patch)
2010-01-13 14:19 PST, Diego Gonzalez
kenneth: review-
kenneth: commit‑queue-
Details | Formatted Diff | Diff
Proposed patch v0.4 (1.04 KB, patch)
2010-01-14 05:20 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch v0.5 (2.03 KB, patch)
2010-01-14 07:39 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Final patch (1016 bytes, patch)
2010-01-14 07:49 PST, Diego Gonzalez
kenneth: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Gonzalez 2010-01-13 12:19:17 PST
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.
Comment 1 Diego Gonzalez 2010-01-13 12:47:40 PST
Created attachment 46491 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-01-13 13:26:29 PST
Attachment 46491 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/185686
Comment 3 Diego Gonzalez 2010-01-13 14:01:26 PST
Created attachment 46502 [details]
Proposed patch v0.2

Make check of Qt version to user QUrl::fromUserInput method
Comment 4 Diego Gonzalez 2010-01-13 14:19:46 PST
Created attachment 46506 [details]
Proposed patch v0.3

Use m_strig instead of non-existent input variable
Comment 5 Antonio Gomes 2010-01-13 19:46:08 PST
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
Comment 6 Kenneth Rohde Christiansen 2010-01-14 03:01:10 PST
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();
>  }
>
Comment 7 Diego Gonzalez 2010-01-14 05:20:16 PST
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.
Comment 8 Kenneth Rohde Christiansen 2010-01-14 06:41:43 PST
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();
>  }
>
Comment 9 Antonio Gomes 2010-01-14 07:17:16 PST
(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();
> >  }
> >
Comment 10 Diego Gonzalez 2010-01-14 07:39:55 PST
Created attachment 46564 [details]
Proposed patch v0.5

Using KURL methods instead of QUrl. Reviewed by Kenneth
Comment 11 Diego Gonzalez 2010-01-14 07:49:57 PST
Created attachment 46567 [details]
Final patch

Correct patch
Comment 12 WebKit Commit Bot 2010-01-14 08:08:01 PST
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
Comment 13 Antonio Gomes 2010-01-14 11:21:43 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/qt/KURLQt.cpp
Committed r53276
Comment 14 Eric Seidel 2010-01-14 11:40:27 PST
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
Comment 15 Antonio Gomes 2010-01-14 11:43:26 PST
(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.
Comment 16 Eric Seidel 2010-01-14 11:49:27 PST
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.
Comment 17 Simon Hausmann 2010-02-10 01:22:50 PST
(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