Summary: | When showing results.html pass a correct URL to webbrowser.open | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||
Component: | New Bugs | Assignee: | 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
jochen
2012-01-02 08:48:18 PST
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. 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?
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? I've had the same problem. It seems to affect the Chromium port, not the Apple port. 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
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 :-/ (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 Comment on attachment 120936 [details] Patch Clearing flags on attachment: 120936 Committed r103984: <http://trac.webkit.org/changeset/103984> All reviewed patches have been landed. Closing bug. 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 |