Bug 33614 - [Qt] Missing fileSystemPath() method in Qt KURL implementation
: [Qt] Missing fileSystemPath() method in Qt KURL implementation
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Qt
:
: 33617
  Show dependency treegraph
 
Reported: 2010-01-13 12:19 PST by
Modified: 2010-02-10 01:22 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-01-13 12:47:40 PST -------
Created an attachment (id=46491) [details]
Proposed patch
------- Comment #2 From 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 From 2010-01-13 14:01:26 PST -------
Created an attachment (id=46502) [details]
Proposed patch v0.2

Make check of Qt version to user QUrl::fromUserInput method
------- Comment #4 From 2010-01-13 14:19:46 PST -------
Created an attachment (id=46506) [details]
Proposed patch v0.3

Use m_strig instead of non-existent input variable
------- Comment #5 From 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] [details]
> Proposed patch v0.3
> 
> Use m_strig instead of non-existent input variable
------- Comment #6 From 2010-01-14 03:01:10 PST -------
(From update of attachment 46506 [details])

>  
>  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 From 2010-01-14 05:20:16 PST -------
Created an attachment (id=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 From 2010-01-14 06:41:43 PST -------
(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"

>  
>  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 From 2010-01-14 07:17:16 PST -------
(In reply to comment #8)
> (From update of attachment 46558 [details] [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 From 2010-01-14 07:39:55 PST -------
Created an attachment (id=46564) [details]
Proposed patch v0.5

Using KURL methods instead of QUrl. Reviewed by Kenneth
------- Comment #11 From 2010-01-14 07:49:57 PST -------
Created an attachment (id=46567) [details]
Final patch

Correct patch
------- Comment #12 From 2010-01-14 08:08:01 PST -------
(From update of attachment 46567 [details])
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 From 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 From 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 From 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 From 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 From 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