Bug 34826

Created attachment 49671 [details] first draft of patch - still buggy This isn't ready to land yet, but it's close. The test script is hanging occasionally, and I think it's due to a bug in the diff_image() implementation but I haven't tracked it down yet. I'm posting the patch so others know it's in progress.

Created attachment 51582 [details] updated patch - this should work correctly and safely

Created attachment 51675 [details] split server_process.read() into two calls for clearer usage

Drive-by nit: ChangeLog looks not quite right.

Created attachment 51676 [details] fix ChangeLog comments

Can you split the style changes off into a separate page? That would make reviewing the code changes considerably easier.

Created attachment 51706 [details] back out PEP-8 reformatting to minimize confusion

(In reply to comment #6) > Can you split the style changes off into a separate page? That would make > reviewing the code changes considerably easier. Done.

Comment on attachment 51706 [details] back out PEP-8 reformatting to minimize confusion > + def diff_image(self, expected_filename, actual_filename, > + diff_filename=None): > + """Compare two image files and produce a delta image file. > + > + Return 1 if the two files are different, 0 if they are the same. Why not return a boolean? > + actual_file = open(actual_filename).read() > + input = 'Content-Length: %d\n' % actual_length > + input += actual_file > + input += 'Content-Length: %d\n' % expected_length > + input += expected_file How about using a single format string here? > + cmd = [self._path_to_image_diff(), '--tolerance', '0.1'] Doesn't this need to be controlled via the commandline? Also, I'm pretty sure tiger, leopard and snow-leopard have different default tolerances. > cmd += self._options.wrapper.split() > - # FIXME: Using arch here masks any possible file-not-found errors from a non-existant driver executable. > - cmd += ['arch', '-i386', port._path_to_driver(), '-'] Is this FIXME no longer true? > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/server_process.py > +class ServerProcess: I like the concept of this class, but not so much the name. There's not really a server involved. ReaderProcess? ReadLineProcess? ReadWriteProcess? > + """This class provides a wrapper around a subprocess that > + implements a simple read/write usage model. The primary benefit > + is that reading responses takes a timeout, so that we don't ever block > + indefinitely. The class also handles transparently restarting processes > + as necessary to keep issuing commands.""" > + def read_line(self, deadline): > + """Read a single line from the subprocess, waiting until the deadline. > + If the deadline passes, the call times out. Note that even if the > + subprocess has crashed or the deadline has passed, if there is output > + pending, it will be returned. > + > + Args: > + deadline: timestamp that the read must complete before Passing in a timestamp is weird. Can you have it be a timeout and then have this code deal with calculating what the deadline should be? > + Returns: > + output: data returned, if any. If no data is available and the > + call times out or crashes, an empty string is returned. Note > + that the returned string includes the newline ('\n').""" > + return self._read(deadline, 0) > + > + def read(self, deadline, size): > + """Attempts to read size characters from the subprocess, waiting until > + the deadline passes. If the deadline passes, any available data will be > + returned. Note that even if the deadline has passed or if the > + subprocess has crashed, any available data will still be returned. > + > + Args: > + deadline: timestamp that the read must complete before > + size: amount of data to read. > + Returns: > + output: data returned, if any. If no data is available, an empty > + string is returned. > + """ > + return self._read(deadline, size) Why does this function exist? Why not just make _read public? > + def _read(self, deadline, size): > + """Internal routine that actually does the read.""" > + idx = -1 Can you give this a more descriptive name? > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py b/WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py > index 42928ba..3a3d8a6 100644 > --- a/WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py > @@ -90,6 +90,7 @@ class ImageDiff(test_type_base.TestTypeBase): > Args: > filename: the name of the test > target: Debug or Release > + Returns 1 if the files are different, 0 if they match > """ I know you didn't make it this way, but can we make this return a boolean? Then we can make the above code do the same?

(In reply to comment #9) > (From update of attachment 51706 [details]) > > + def diff_image(self, expected_filename, actual_filename, > > + diff_filename=None): > > + """Compare two image files and produce a delta image file. > > + > > + Return 1 if the two files are different, 0 if they are the same. > > Why not return a boolean? You saw below why not; I'll change the two files to return booleans instead. > > > + actual_file = open(actual_filename).read() > > + input = 'Content-Length: %d\n' % actual_length > > + input += actual_file > > + input += 'Content-Length: %d\n' % expected_length > > + input += expected_file > > How about using a single format string here? > Will do. > > + cmd = [self._path_to_image_diff(), '--tolerance', '0.1'] > > Doesn't this need to be controlled via the commandline? Also, I'm pretty sure > tiger, leopard and snow-leopard have different default tolerances. Theoretically. I think the whole "tolerance" concept needs to go away, and we should only allow exact pixel matches. > > cmd += self._options.wrapper.split() > > - # FIXME: Using arch here masks any possible file-not-found errors from a non-existant driver executable. > > - cmd += ['arch', '-i386', port._path_to_driver(), '-'] > > Is this FIXME no longer true? > That is correct; we now detect and report if the driver is missing properly. > > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/server_process.py > > +class ServerProcess: > > I like the concept of this class, but not so much the name. There's not really > a server involved. ReaderProcess? ReadLineProcess? ReadWriteProcess? I agree with you that ServerProcess isn't great, but I don't like any of your suggestions any better. RpcProcess? > Passing in a timestamp is weird. Can you have it be a timeout and then have > this code deal with calculating what the deadline should be? I agree that this is a little weird (compared to select, etc), but I think it's actually easier to get right on the caller side than the normal API design pattern. I can change it if you think following established (IMO, bad) API conventions is better here. > > + """ > > + return self._read(deadline, size) > > Why does this function exist? Why not just make _read public? > There are two public use cases supported, <read one line of unknown length> and <read an exact number of bytes>). Combining those into a single function means you have to use some sort of a magic value to indicate which you want. I thought having two public functions was better. However, as you saw below, implementing them separately would be silly, so I combined them into a single internal routine. > > + def _read(self, deadline, size): > > + """Internal routine that actually does the read.""" > > + idx = -1 > > Can you give this a more descriptive name? Suggestions? > I know you didn't make it this way, but can we make this return a boolean? Then > we can make the above code do the same? will do.

(In reply to comment #10) > (In reply to comment #9) > > > + cmd = [self._path_to_image_diff(), '--tolerance', '0.1'] > > > > Doesn't this need to be controlled via the commandline? Also, I'm pretty sure > > tiger, leopard and snow-leopard have different default tolerances. > > Theoretically. I think the whole "tolerance" concept needs to go away, and we > should only allow exact pixel matches. I'm inclined to agree, but for now this should at least have a FIXME. > > > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/server_process.py > > > +class ServerProcess: > > > > I like the concept of this class, but not so much the name. There's not really > > a server involved. ReaderProcess? ReadLineProcess? ReadWriteProcess? > > I agree with you that ServerProcess isn't great, but I don't like any of your > suggestions any better. RpcProcess? How about IpcProcess? RPC is about actually executing code on the remote process. This is more about communicating with the remote process. > > Passing in a timestamp is weird. Can you have it be a timeout and then have > > this code deal with calculating what the deadline should be? > > I agree that this is a little weird (compared to select, etc), but I think it's > actually easier to get right on the caller side than the normal API design > pattern. I can change it if you think following established (IMO, bad) API > conventions is better here. I think the confusion of this violating established API conventions is more likely to cause bugs. I'd rather it just be a timeout. > > > + """ > > > + return self._read(deadline, size) > > > > Why does this function exist? Why not just make _read public? > > There are two public use cases supported, <read one line of unknown length> and > <read an exact number of bytes>). Combining those into a single function means > you have to use some sort of a magic value to indicate which you want. I > thought having two public functions was better. However, as you saw below, > implementing them separately would be silly, so I combined them into a single > internal routine. Sorry, I phrased poorly. I'm suggesting read_line just call the public read function. No need for a private one. > > > + def _read(self, deadline, size): > > > + """Internal routine that actually does the read.""" > > > + idx = -1 > > > > Can you give this a more descriptive name? > > Suggestions? Even just "index" would be fine.

Created attachment 51785 [details] revise w/ review feedback from eric and ojan

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > > + cmd = [self._path_to_image_diff(), '--tolerance', '0.1'] > > > > > > Doesn't this need to be controlled via the commandline? Also, I'm pretty sure > > > tiger, leopard and snow-leopard have different default tolerances. > > > > Theoretically. I think the whole "tolerance" concept needs to go away, and we > > should only allow exact pixel matches. > > I'm inclined to agree, but for now this should at least have a FIXME. > FIXME added. > > > > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/server_process.py > > > > +class ServerProcess: > > > > > > I like the concept of this class, but not so much the name. There's not really > > > a server involved. ReaderProcess? ReadLineProcess? ReadWriteProcess? > > > > I agree with you that ServerProcess isn't great, but I don't like any of your > > suggestions any better. RpcProcess? > > How about IpcProcess? RPC is about actually executing code on the remote > process. This is more about communicating with the remote process. > The intent of the api is that there is a request followed by a reply. Unfortunately due to the way ImageDiff and DRT produce output, you can't easily encapsulate this into a single reply() method, and so I was left with the more generic read() and write() routines. However, IPC is too vague to capture the intent. Per face-to-face discussion w/ Ojan, we agreed to leave it as ServerProcess until we could think of something better. > > > Passing in a timestamp is weird. Can you have it be a timeout and then have > > > this code deal with calculating what the deadline should be? > > > > I agree that this is a little weird (compared to select, etc), but I think it's > > actually easier to get right on the caller side than the normal API design > > pattern. I can change it if you think following established (IMO, bad) API > > conventions is better here. > > I think the confusion of this violating established API conventions is more > likely to cause bugs. I'd rather it just be a timeout. Okay, I've changed it to be a timeout. Judging by the fact that I introduced half a dozen bugs while change this, and by the fact that the code got longer on both sides, I think I was right. For a relevant discussion of the merits of API design in select() and read APIs, see http://cacm.acm.org/magazines/2009/5/24646-api-design-matters/fulltext , although he repeats the error of not being able to distinguish between "don't wait", "wait for X", and "wait forever" in a delta timestamp. Using a deadline is clearer still. > > > > > + """ > > > > + return self._read(deadline, size) > > > > > > Why does this function exist? Why not just make _read public? > > > > There are two public use cases supported, <read one line of unknown length> and > > <read an exact number of bytes>). Combining those into a single function means > > you have to use some sort of a magic value to indicate which you want. I > > thought having two public functions was better. However, as you saw below, > > implementing them separately would be silly, so I combined them into a single > > internal routine. > > Sorry, I phrased poorly. I'm suggesting read_line just call the public read > function. No need for a private one. As we discussed face-to-face, read() probably should've raised an exception if called with a non-positive number for size. I have added that, and you were okay with the change. > > > > > + def _read(self, deadline, size): > > > > + """Internal routine that actually does the read.""" > > > > + idx = -1 > > > > > > Can you give this a more descriptive name? > > > > Suggestions? > > Even just "index" would be fine. Ah, sorry, I thought you were complaining about "_read". I think "idx" is pretty obvious and shorter, but I've changed it to "index".

Created attachment 51791 [details] revise w/ more feedback from ojan

(In reply to comment #13) > > > > > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/server_process.py > > > > > +class ServerProcess: > > > > > > > > I like the concept of this class, but not so much the name. There's not really > > > > a server involved. ReaderProcess? ReadLineProcess? ReadWriteProcess? > > > > > > I agree with you that ServerProcess isn't great, but I don't like any of your > > > suggestions any better. RpcProcess? > > > > How about IpcProcess? RPC is about actually executing code on the remote > > process. This is more about communicating with the remote process. > > > > The intent of the api is that there is a request followed by a reply. > Unfortunately due to the way ImageDiff and DRT produce output, you can't easily > encapsulate this into a single reply() method, and so I was left with the more > generic read() and write() routines. However, IPC is too vague to capture the > intent. Per face-to-face discussion w/ Ojan, we agreed to leave it as > ServerProcess until we could think of something better. > Originally when I wrote this my idea was to have ImageDiff and DumpRenderTree derive from ServerProcess, and then the complexity in parsing the responses would be hidden. Unfortunately, since they return such weird sets of arguments, even trying to fit them both into a single reply() method was awkward, and so I abandoned that idea.

Comment on attachment 51791 [details] revise w/ more feedback from ojan I'm told that you and Ojan reached a happy place. This looks OK to me. I think this new architecture is much cleaner. Two letter variable names make my brain kink. But I don't think it's worth another round of changes to fix them. In general I'd rather write out full english names and phrases to describe variables. Then again, I come from a non-80 column limited world. :) Thanks for all your hard work!

Comment on attachment 51791 [details] revise w/ more feedback from ojan Clearing flags on attachment: 51791 Committed r56647: <http://trac.webkit.org/changeset/56647>

All reviewed patches have been landed. Closing bug.

This killed the chromium windows canary: http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20(webkit.org)/builds/25766/steps/webkit_tests/logs/stdio The failure is: Traceback (most recent call last): File "C:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\new-run-webkit-tests", line 37, in <module> sys.exit(run_webkit_tests.main(options, args)) File "C:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 1401, in main port_obj = port.get(options.platform, options) File "C:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\factory.py", line 84, in get import chromium_win File "C:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\chromium_win.py", line 39, in <module> import chromium File "C:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\chromium.py", line 43, in <module> import webkit File "C:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\webkit.py", line 46, in <module> import webkitpy.layout_tests.port.server_process as server_process File "C:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\server_process.py", line 32, in <module> import fcntl ImportError: No module named fcntl program finished with exit code 1 I think the 'import fcntl' line needs to be guarded. Can you take a look at this Dirk? If you don't have time I can try to fix it later today. Thanks.

(In reply to comment #19) > This killed the chromium windows canary: > http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20(webkit.org)/builds/25766/steps/webkit_tests/logs/stdio > This whole module won't work on Windows, because the non-blocking I/O model is totally different. We need to do much more invasive surgery than that. I've checked in a fix to webkit.py to not pull the file in if we're on windows, which fixes this for now.