Diff
Modified: trunk/Tools/ChangeLog (98033 => 98034)
--- trunk/Tools/ChangeLog 2011-10-20 22:55:49 UTC (rev 98033)
+++ trunk/Tools/ChangeLog 2011-10-20 23:17:42 UTC (rev 98034)
@@ -1,3 +1,35 @@
+2011-10-20 Eric Seidel <e...@webkit.org>
+
+ REGRESSION(97879): Pixel tests no longer work with NRWT on Mac
+ https://bugs.webkit.org/show_bug.cgi?id=70492
+
+ Reviewed by Adam Barth.
+
+ The bug turned out to be that I was assuming the block.content
+ would be empty before the binary content following Content-Length
+ was read inside _read_block. Turns out its not, due to extra newlines
+ and "ExpectedHash" header.
+
+ In the process of trying to figure out what was going wrong I ended up
+ cleaning up our newline usage in DumpRenderTree a little. Moved
+ two error messages from stdout to stderr, and fixed a little code indent/whitespace.
+
+ I also fixed ServerProcess to use "deadline" everywhere instead of timeout
+ per Adam's request in the original bug.
+
+ * DumpRenderTree/PixelDumpSupport.cpp:
+ (dumpWebViewAsPixelsAndCompareWithExpected):
+ * DumpRenderTree/cg/ImageDiffCG.cpp:
+ (main):
+ * DumpRenderTree/mac/DumpRenderTree.mm:
+ (dump):
+ * DumpRenderTree/mac/PixelDumpSupportMac.mm:
+ (restoreMainDisplayColorProfile):
+ (setupMainDisplayColorProfile):
+ * Scripts/webkitpy/layout_tests/port/server_process.py:
+ * Scripts/webkitpy/layout_tests/port/webkit.py:
+ * Scripts/webkitpy/layout_tests/port/webkit_unittest.py:
+
2011-10-20 Tony Chang <t...@chromium.org>
[chromium] Remove <stdint.h> from ImageDiff and use
Modified: trunk/Tools/DumpRenderTree/PixelDumpSupport.cpp (98033 => 98034)
--- trunk/Tools/DumpRenderTree/PixelDumpSupport.cpp 2011-10-20 22:55:49 UTC (rev 98033)
+++ trunk/Tools/DumpRenderTree/PixelDumpSupport.cpp 2011-10-20 23:17:42 UTC (rev 98034)
@@ -57,21 +57,21 @@
// Compute the hash of the bitmap context pixels
char actualHash[33];
computeMD5HashStringForBitmapContext(context.get(), actualHash);
- printf("\nActualHash: %s\n", actualHash);
-
+ printf("\nActualHash: %s\n", actualHash); // FIXME: No need for the leading newline.
+
// Check the computed hash against the expected one and dump image on mismatch
bool dumpImage = true;
if (expectedHash.length() > 0) {
ASSERT(expectedHash.length() == 32);
+
+ printf("\nExpectedHash: %s\n", expectedHash.c_str()); // FIXME: No need for the leading newline.
- printf("\nExpectedHash: %s\n", expectedHash.c_str());
-
- if (expectedHash == actualHash) // FIXME: do case insensitive compare
+ if (expectedHash == actualHash) // FIXME: do case insensitive compare
dumpImage = false;
}
-
+
if (dumpImage)
- dumpBitmap(context.get(), actualHash);
+ dumpBitmap(context.get(), actualHash);
}
static void appendIntToVector(unsigned number, Vector<unsigned char>& vector)
Modified: trunk/Tools/DumpRenderTree/cg/ImageDiffCG.cpp (98033 => 98034)
--- trunk/Tools/DumpRenderTree/cg/ImageDiffCG.cpp 2011-10-20 22:55:49 UTC (rev 98033)
+++ trunk/Tools/DumpRenderTree/cg/ImageDiffCG.cpp 2011-10-20 23:17:42 UTC (rev 98034)
@@ -211,13 +211,13 @@
else if (imageSize > 0 && !baselineImage)
baselineImage = createImageFromStdin(imageSize);
else
- fputs("error, image size must be specified.\n", stdout);
+ fputs("error, image size must be specified.\n", stderr);
}
if (actualImage && baselineImage) {
RetainPtr<CGImageRef> diffImage;
float difference = 100.0f;
-
+
if ((CGImageGetWidth(actualImage.get()) == CGImageGetWidth(baselineImage.get())) && (CGImageGetHeight(actualImage.get()) == CGImageGetHeight(baselineImage.get())) && (imageHasAlpha(actualImage.get()) == imageHasAlpha(baselineImage.get()))) {
diffImage = createDifferenceImage(actualImage.get(), baselineImage.get(), difference); // difference is passed by reference
if (difference <= tolerance)
@@ -228,7 +228,7 @@
}
} else
fputs("error, test and reference image have different properties.\n", stderr);
-
+
if (difference > 0.0f) {
if (diffImage) {
RetainPtr<CFMutableDataRef> imageData(AdoptCF, CFDataCreateMutable(0, 0));
@@ -238,7 +238,7 @@
printf("Content-Length: %lu\n", CFDataGetLength(imageData.get()));
fwrite(CFDataGetBytePtr(imageData.get()), 1, CFDataGetLength(imageData.get()), stdout);
}
-
+
fprintf(stdout, "diff: %01.2f%% failed\n", difference);
} else
fprintf(stdout, "diff: %01.2f%% passed\n", difference);
Modified: trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm (98033 => 98034)
--- trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm 2011-10-20 22:55:49 UTC (rev 98033)
+++ trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm 2011-10-20 23:17:42 UTC (rev 98034)
@@ -1083,8 +1083,8 @@
printf("Content-Type: %s\n", [resultMimeType UTF8String]);
if (gLayoutTestController->dumpAsAudio())
- printf("Content-Transfer-Encoding: base64\n");
-
+ printf("Content-Transfer-Encoding: base64\n");
+
if (resultData) {
fwrite([resultData bytes], 1, [resultData length], stdout);
Modified: trunk/Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm (98033 => 98034)
--- trunk/Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm 2011-10-20 22:55:49 UTC (rev 98033)
+++ trunk/Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm 2011-10-20 23:17:42 UTC (rev 98034)
@@ -62,7 +62,7 @@
const CMDeviceScope scope = { kCFPreferencesCurrentUser, kCFPreferencesCurrentHost };
int error = CMSetDeviceProfile(cmDisplayDeviceClass, (CMDeviceID)kCGDirectMainDisplay, &scope, cmDefaultProfileID, &sInitialProfileLocation);
if (error)
- fprintf(stderr, "Failed to restore initial color profile for main display! Open System Preferences > Displays > Color and manually re-select the profile. (Error: %i)", error);
+ fprintf(stderr, "Failed to restore initial color profile for main display! Open System Preferences > Displays > Color and manually re-select the profile. (Error: %i)\n", error);
sInitialProfileLocation.locType = cmNoProfileBase;
}
}
@@ -80,7 +80,7 @@
CMCloseProfile(profile);
}
if (error) {
- fprintf(stderr, "Failed to retrieve current color profile for main display, thus it won't be changed. Many pixel tests may fail as a result. (Error: %i)", error);
+ fprintf(stderr, "Failed to retrieve current color profile for main display, thus it won't be changed. Many pixel tests may fail as a result. (Error: %i)\n", error);
sInitialProfileLocation.locType = cmNoProfileBase;
return;
}
@@ -90,7 +90,7 @@
strcpy(location.u.pathLoc.path, PROFILE_PATH);
error = CMSetDeviceProfile(cmDisplayDeviceClass, (CMDeviceID)kCGDirectMainDisplay, &scope, cmDefaultProfileID, &location);
if (error) {
- fprintf(stderr, "Failed to set color profile for main display! Many pixel tests may fail as a result. (Error: %i)", error);
+ fprintf(stderr, "Failed to set color profile for main display! Many pixel tests may fail as a result. (Error: %i)\n", error);
sInitialProfileLocation.locType = cmNoProfileBase;
return;
}
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/server_process.py (98033 => 98034)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/server_process.py 2011-10-20 22:55:49 UTC (rev 98033)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/server_process.py 2011-10-20 23:17:42 UTC (rev 98034)
@@ -48,7 +48,7 @@
class ServerProcess:
"""This class provides a wrapper around a subprocess that
implements a simple request/response usage model. The primary benefit
- is that reading responses takes a timeout, so that we don't ever block
+ is that reading responses takes a deadline, so that we don't ever block
indefinitely. The class also handles transparently restarting processes
as necessary to keep issuing commands."""
@@ -118,13 +118,13 @@
return self._proc.poll()
return None
- def write(self, input):
+ def write(self, bytes):
"""Write a request to the subprocess. The subprocess is (re-)start()'ed
if is not already running."""
if not self._proc:
self._start()
try:
- self._proc.stdin.write(input)
+ self._proc.stdin.write(bytes)
except IOError, e:
self.stop()
# stop() calls _reset(), so we have to set crashed to True after calling stop().
@@ -142,13 +142,13 @@
return self._pop_error_bytes(index_after_newline)
return None
- def read_stdout_line(self, timeout_seconds):
- return self._read(timeout_seconds, self._pop_stdout_line_if_ready)
+ def read_stdout_line(self, deadline):
+ return self._read(deadline, self._pop_stdout_line_if_ready)
- def read_stderr_line(self, timeout_seconds):
- return self._read(timeout_seconds, self._pop_stderr_line_if_ready)
+ def read_stderr_line(self, deadline):
+ return self._read(deadline, self._pop_stderr_line_if_ready)
- def read_either_stdout_or_stderr_line(self, timeout_seconds):
+ def read_either_stdout_or_stderr_line(self, deadline):
def retrieve_bytes_from_buffers():
stdout_line = self._pop_stdout_line_if_ready()
if stdout_line:
@@ -158,13 +158,13 @@
return None, stderr_line
return None # Instructs the caller to keep waiting.
- return_value = self._read(timeout_seconds, retrieve_bytes_from_buffers)
+ return_value = self._read(deadline, retrieve_bytes_from_buffers)
# FIXME: This is a bit of a hack around the fact that _read normally only returns one value, but this caller wants it to return two.
if return_value is None:
return None, None
return return_value
- def read_stdout(self, timeout_seconds, size):
+ def read_stdout(self, deadline, size):
if size <= 0:
raise ValueError('ServerProcess.read() called with a non-positive size: %d ' % size)
@@ -173,7 +173,7 @@
return self._pop_output_bytes(size)
return None
- return self._read(timeout_seconds, retrieve_bytes_from_stdout_buffer)
+ return self._read(deadline, retrieve_bytes_from_stdout_buffer)
def _check_for_crash(self):
if self._proc.poll() != None:
@@ -211,7 +211,7 @@
self._sample()
def _split_string_after_index(self, string, index):
- return string[0:index], string[index:]
+ return string[:index], string[index:]
def _pop_output_bytes(self, bytes_count):
output, self._output = self._split_string_after_index(self._output, bytes_count)
@@ -221,11 +221,11 @@
output, self._error = self._split_string_after_index(self._error, bytes_count)
return output
- def _wait_for_data_and_update_buffers(self, ms_to_wait):
+ def _wait_for_data_and_update_buffers(self, deadline):
out_fd = self._proc.stdout.fileno()
err_fd = self._proc.stderr.fileno()
select_fds = (out_fd, err_fd)
- read_fds, _, _ = select.select(select_fds, [], select_fds, ms_to_wait)
+ read_fds, _, _ = select.select(select_fds, [], select_fds, deadline - time.time())
try:
if out_fd in read_fds:
self._output += self._proc.stdout.read()
@@ -246,8 +246,7 @@
# This read function is a bit oddly-designed, as it polls both stdout and stderr, yet
# only reads/returns from one of them (buffering both in local self._output/self._error).
# It might be cleaner to pass in the file descriptor to poll instead.
- def _read(self, timeout, fetch_bytes_from_buffers_callback):
- deadline = time.time() + timeout
+ def _read(self, deadline, fetch_bytes_from_buffers_callback):
while True:
if self._check_for_abort(deadline):
return None
@@ -256,7 +255,7 @@
if bytes is not None:
return bytes
- self._wait_for_data_and_update_buffers(deadline - time.time())
+ self._wait_for_data_and_update_buffers(deadline)
def stop(self):
if not self._proc:
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py (98033 => 98034)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py 2011-10-20 22:55:49 UTC (rev 98033)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py 2011-10-20 23:17:42 UTC (rev 98034)
@@ -177,35 +177,39 @@
return process
def _read_image_diff(self, sp):
- timeout = 2.0
- deadline = time.time() + timeout
- output = sp.read_stdout_line(timeout)
+ deadline = time.time() + 2.0
+ output = None
output_image = ""
- diff_percent = 0
- while not sp.timed_out and not sp.crashed and output:
+
+ while True:
+ output = sp.read_stdout_line(deadline)
+ if sp.timed_out or sp.crashed or not output:
+ break
+
+ if output.startswith('diff'): # This is the last line ImageDiff prints.
+ break
+
if output.startswith('Content-Length'):
m = re.match('Content-Length: (\d+)', output)
content_length = int(m.group(1))
- timeout = deadline - time.time()
- output_image = sp.read_stdout(timeout, content_length)
- output = sp.read_stdout_line(timeout)
+ output_image = sp.read_stdout(deadline, content_length)
+ output = sp.read_stdout_line(deadline)
break
- elif output.startswith('diff'):
- break
- else:
- timeout = deadline - time.time()
- output = sp.read_stdout_line(deadline)
if sp.timed_out:
_log.error("ImageDiff timed out")
if sp.crashed:
_log.error("ImageDiff crashed")
+ # FIXME: There is no need to shut down the ImageDiff server after every diff.
sp.stop()
- if output.startswith('diff'):
+
+ diff_percent = 0
+ if output and output.startswith('diff'):
m = re.match('diff: (.+)% (passed|failed)', output)
if m.group(2) == 'passed':
return [None, 0]
diff_percent = float(m.group(1))
+
return (output_image, diff_percent)
def default_results_directory(self):
@@ -550,6 +554,7 @@
or self._read_header(block, line, 'Content-Length: ', '_content_length', int)
or self._read_header(block, line, 'ActualHash: ', 'content_hash')):
return
+ # Note, we're not reading ExpectedHash: here, but we could.
# If the line wasn't a header, we just append it to the content.
block.content += line
@@ -566,20 +571,18 @@
if out_seen_eof and (self.err_seen_eof or not wait_for_stderr_eof):
break
- if self._server_process.timed_out or self._detected_crash():
- break
-
- timeout = deadline - time.time()
-
if self.err_seen_eof:
- out_line = self._server_process.read_stdout_line(timeout)
+ out_line = self._server_process.read_stdout_line(deadline)
err_line = None
elif out_seen_eof:
out_line = None
- err_line = self._server_process.read_stderr_line(timeout)
+ err_line = self._server_process.read_stderr_line(deadline)
else:
- out_line, err_line = self._server_process.read_either_stdout_or_stderr_line(timeout)
+ out_line, err_line = self._server_process.read_either_stdout_or_stderr_line(deadline)
+ if self._server_process.timed_out or self._detected_crash():
+ break
+
if out_line:
assert not out_seen_eof
out_line, out_seen_eof = self._strip_eof(out_line)
@@ -588,11 +591,13 @@
err_line, self.err_seen_eof = self._strip_eof(err_line)
if out_line:
+ assert out_line[-1] == "\n", "Missing newline"
+ content_length_before_header_check = block._content_length
self._process_stdout_line(block, out_line)
# FIXME: Unlike HTTP, DRT dumps the content right after printing a Content-Length header.
# Don't wait until we're done with headers, just read the binary blob right now.
- if block._content_length is not None and not block.content:
- block.content = self._server_process.read_stdout(deadline - time.time(), block._content_length)
+ if content_length_before_header_check != block._content_length:
+ block.content = self._server_process.read_stdout(deadline, block._content_length)
if err_line:
if self._check_for_driver_crash(err_line):
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py (98033 => 98034)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py 2011-10-20 22:55:49 UTC (rev 98033)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py 2011-10-20 23:17:42 UTC (rev 98034)
@@ -205,12 +205,18 @@
self.crashed = False
self.lines = lines or []
- def read_stdout_line(self, timeout):
+ def read_stdout_line(self, deadline):
return self.lines.pop(0) + "\n"
- def read_either_stdout_or_stderr_line(self, timeout):
+ def read_stdout(self, deadline, size):
+ # read_stdout doesn't actually function on lines, but this is sufficient for our testing.
+ line = self.lines.pop(0)
+ assert len(line) == size
+ return line
+
+ def read_either_stdout_or_stderr_line(self, deadline):
# FIXME: We should have tests which intermix stderr and stdout lines.
- return self.read_stdout_line(timeout), None
+ return self.read_stdout_line(deadline), None
class WebKitDriverTest(unittest.TestCase):
@@ -227,3 +233,20 @@
self.assertEquals(content_block.content_type, 'my_type')
self.assertEquals(content_block.encoding, 'none')
self.assertEquals(content_block.content_hash, 'foobar')
+
+ def test_read_binary_block(self):
+ port = TestWebKitPort()
+ driver = WebKitDriver(port, 0)
+ driver._server_process = MockServerProcess([
+ 'ActualHash: actual',
+ 'ExpectedHash: expected',
+ 'Content-Type: image/png',
+ 'Content-Length: 8',
+ "12345678",
+ "#EOF",
+ ])
+ content_block = driver._read_block(0)
+ self.assertEquals(content_block.content_type, 'image/png')
+ self.assertEquals(content_block.content_hash, 'actual')
+ self.assertEquals(content_block.content, '12345678')
+ self.assertEquals(content_block.decoded_content, '12345678')
