Bug 40136 - webkit-patch upload doesn't show diff on cygwin
Summary: webkit-patch upload doesn't show diff on cygwin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 78408
  Show dependency treegraph
 
Reported: 2010-06-03 13:46 PDT by Tony Gentilcore
Modified: 2012-02-10 20:00 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.91 KB, patch)
2010-06-03 13:53 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2010-06-03 13:46:56 PDT
webkit-patch upload doesn't show diff on cygwin
Comment 1 Tony Gentilcore 2010-06-03 13:53:21 PDT
Created attachment 57808 [details]
Patch
Comment 2 David Levin 2010-06-03 13:56:11 PDT
Hmmm... no test. :( (Don't know how possible it is or not.)
Comment 3 Tony Gentilcore 2010-06-03 14:02:37 PDT
(In reply to comment #2)
> Hmmm... no test. :( (Don't know how possible it is or not.)

Manually, I tested this on osx to verify that the pretty diff is still displayed and cygwin to verify that the plain old diff is displayed. However, test-webkitpy hangs for me on cygwin so I haven't verified the full suite passes.

If you think a test is important, I can update the mock to control the return value of can_open_url() and add a test that exercises the new branch in confirmdiff. But I can't really test can_open_url() itself.

By the way, I'm starting to suspect that no one else is developing webkit on windows+cygwin. Should I just leave it be and switch to mac?
Comment 4 Tony Gentilcore 2010-06-03 14:21:44 PDT
By the way, the pretty diff file is successfully created in C:\cygwin\tmp. An alternative approach to adding can_open_url() would be to modify open_url() to test webbrowser.get() internally and on exception just output the file:///{{CYGWIN_PATH}}/tmp/{{TEMP_FILE}} URL to the console so the user can manually open it.
Comment 5 Adam Barth 2010-06-03 18:23:03 PDT
Comment on attachment 57808 [details]
Patch

Looks reasonable.  Generally, we don't have a good strategy for testing things in user.py.  In fact, that file exists to abstract away things that we don't know how to test.
Comment 6 WebKit Commit Bot 2010-06-04 01:56:02 PDT
Comment on attachment 57808 [details]
Patch

Clearing flags on attachment: 57808

Committed r60666: <http://trac.webkit.org/changeset/60666>
Comment 7 WebKit Commit Bot 2010-06-04 01:56:13 PDT
All reviewed patches have been landed.  Closing bug.