Bug 75438

Summary: When showing results.html pass a correct URL to webbrowser.open
Product: WebKit Reporter: jochen
Component: New BugsAssignee: jochen
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description jochen 2012-01-02 08:48:18 PST
When showing results.html pass a correct URL to webbrowser.open
Comment 1 jochen 2012-01-02 08:48:57 PST
Created attachment 120882 [details]
Patch
Comment 2 jochen 2012-01-02 08:50:26 PST
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.
Comment 3 Eric Seidel (no email) 2012-01-02 09:53:29 PST
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?
Comment 4 Eric Seidel (no email) 2012-01-02 09:54:25 PST
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?
Comment 5 Adam Barth 2012-01-02 10:45:33 PST
I've had the same problem.  It seems to affect the Chromium port, not the Apple port.
Comment 6 Adam Barth 2012-01-02 10:46:00 PST
Comment on attachment 120882 [details]
Patch

Test?
Comment 7 jochen 2012-01-02 10:52:26 PST
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.
Comment 8 jochen 2012-01-03 05:21:21 PST
Created attachment 120936 [details]
Patch
Comment 9 Adam Barth 2012-01-03 11:46:38 PST
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?
Comment 10 jochen 2012-01-03 11:57:07 PST
(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 :-/
Comment 11 Eric Seidel (no email) 2012-01-03 11:58:01 PST
(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.
Comment 12 jochen 2012-01-03 12:14:05 PST
(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
Comment 13 WebKit Review Bot 2012-01-03 16:18:44 PST
Comment on attachment 120936 [details]
Patch

Clearing flags on attachment: 120936

Committed r103984: <http://trac.webkit.org/changeset/103984>
Comment 14 WebKit Review Bot 2012-01-03 16:18:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Dirk Pranke 2012-01-04 13:08:31 PST
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.
Comment 16 jochen 2012-01-04 13:10:47 PST
(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