Bug 75438 – When showing results.html pass a correct URL to webbrowser.open

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

When showing results.html pass a correct URL to webbrowser.open

Created attachment 120882 [details]
Patch

Eric, can you please review?

Without this patch, I get the following error on my Lion box:


2012-01-02 17:08:14.094 osascript[95311:f07] CFURLCreateWithString was passed this invalid URL string: '/Volumes/Data/WebKit/Source/WebKit/chromium/webkit/Debug/layout-test-results/results.html' (a file system path instead of an URL string). The URL created will not work with most file URL functions. CFURLCreateWithFileSystemPath or CFURLCreateWithFileSystemPathRelativeToBase should be used instead.

[Eric Seidel (no email)]

Comment on attachment 120882 [details]
Patch

It would be better to add this in a mockable fashion.  like adding a file_url_for_path to FileSystem or just making open_url do this automatically?  I'm surprised this only affects chromium?

[Eric Seidel (no email)]

I believe I've run the tests on Lion before and had it open the results correctly.  So I'm surprised that this is a new issue?

[Adam Barth]

I've had the same problem.  It seems to affect the Chromium port, not the Apple port.

[Adam Barth]

Comment on attachment 120882 [details]
Patch

Test?

Some ports override the show_results_html method.

I was a bit hesitant to add this to open_url, because there might be other call sites that pass in correct URLs. I'd then first have to try to guess whether it's a filesystem path or already a valid URL.

Adding file_path_as_url to FileSystem sounds like a good idea. I'll do that and write a test for it.

Created attachment 120936 [details]
Patch

[Adam Barth]

Comment on attachment 120936 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:66
> +        self.assertEqual(fs.file_path_as_url('/foo'),
> +                         'file:///foo')

Is this going to pass on Windows?

(In reply to comment #9)
> (From update of attachment 120936 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120936&action=review
> 
> > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:66
> > +        self.assertEqual(fs.file_path_as_url('/foo'),
> > +                         'file:///foo')
> 
> Is this going to pass on Windows?

I don't know. I don't have a windows checkout of webkit handy to try and no bots execute those tests :-/

[Eric Seidel (no email)]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 120936 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=120936&action=review
> > 
> > > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:66
> > > +        self.assertEqual(fs.file_path_as_url('/foo'),
> > > +                         'file:///foo')
> > 
> > Is this going to pass on Windows?
> 
> I don't know. I don't have a windows checkout of webkit handy to try and no bots execute those tests :-/

All the bots execute tehse tests. :)  I don't think the EWS bots do today... but they used to.

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 120936 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=120936&action=review
> > > 
> > > > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:66
> > > > +        self.assertEqual(fs.file_path_as_url('/foo'),
> > > > +                         'file:///foo')
> > > 
> > > Is this going to pass on Windows?
> > 
> > I don't know. I don't have a windows checkout of webkit handy to try and no bots execute those tests :-/
> 
> All the bots execute tehse tests. :)  I don't think the EWS bots do today... but they used to.

ah, ok, I was mistaken, just the slow scm tests aren't executed. I'll watch the buildbots for the result then

[WebKit Review Bot]

Comment on attachment 120936 [details]
Patch

Clearing flags on attachment: 120936

Committed r103984: <http://trac.webkit.org/changeset/103984>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

Hm. We already had a path module in webkitpy.common.system that provided an abspath_to_uri() method that did the same thing. It's not clear to me that this should really live on filesystem instead (although at least that would give you predictable behavior using a mock filesystem on windows) but we probably shouldn't have both.

(In reply to comment #15)
> Hm. We already had a path module in webkitpy.common.system that provided an abspath_to_uri() method that did the same thing. It's not clear to me that this should really live on filesystem instead (although at least that would give you predictable behavior using a mock filesystem on windows) but we probably shouldn't have both.

Oh, I didn't see that

I'll try to come up with a way to unify this