[webkit-reviews] review denied: [Bug 21322] DumpRenderTree pixel test improvements : [Attachment 24166] Revised patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Pierre-Olivier Latour <pol at apple.com>'s request for
review:
Bug 21322: DumpRenderTree pixel test improvements
https://bugs.webkit.org/show_bug.cgi?id=21322

Attachment 24166: Revised patch
https://bugs.webkit.org/attachment.cgi?id=24166&action=edit

------- Additional Comments from mitz at webkit.org
Thanks for working on making pixel tests better!

A general comment:
Propagating knowledge of the test's URL or path even deeper into DumpRenderTree
is not a good thing in my opinion. I think it would actually be good to move in
the opposite direction: since you are adding the ability to pass metadata (the
expected hash) along with the test path/URL, you should consider passing custom
WebView dimensions in a similar fashion and making it the responsibility of
run-webkit-tests to decide the dimensions based on the path (maybe in an even
nicer way than hard-coding "svg/W3C-SVG-1.1").

I am not sure it makes sense to add the expected hash as a LayoutTestController
member. We mostly use DumpRenderTree globals for things like that.

 34 #include <ctype.h>
 35 #include <algorithm>

Those should be in ASCII order along with the rest of the #includes, so just
before #include <wtf/Assertions.h>.

 91	    snprintf(hashString, 33, "%s%02x", hashString, hash[i]);

That is not pretty! (But you did not write it.)

 38 typedef struct CGContext* CGContextRef;
 39 
 40 
 41 class BitmapContext : public RefCounted<BitmapContext>

Extra newline there.

 78	    , m_context(AdoptCF, context)

That makes for a very unusual calling convention where BitmapContext::create()
takes ownership of the passed-in CGContext. I would rather avoid it.

 92 PassRefPtr<BitmapContext> getBitmapContextFromWebView(bool
incrementalRepaint, bool sweepHorizontally, bool drawSelectionRect);

This is an unfortunate name for a function with "create" semantics. Again, not
your fault.

 195	     if (URLString.compare(URLString.length() - curIgnore.length(),
curIgnore.length(), curIgnore) == 0)

WebKit coding style is to avoid comparison with 0, NULL and nil, so the above
should be written as

 195	     if (!URLString.compare(URLString.length() - curIgnore.length(),
curIgnore.length(), curIgnore))

 353 #if PLATFORM(MAC)
 354	 restoreMainDisplayColorProfile(0);
 355 #endif

This is a Mac-only file, so you don't need the #if.

 530 #if PLATFORM(MAC)
528531	   if (dumpPixels)
529	     restoreColorSpace(0);
 532	     restoreMainDisplayColorProfile(0);
 533 #endif

Ditto.

 967	 // Look for '|' as a separator between the path or URL, and the pixel
dump hash that follows.
 968	 // FIXME: this is a valid URL characters. Use a different separator?  


Typo: "characters". I think this is a problem. While | is a valid URL character
and almost any character is valid in a path in some platform, but the single
quote (') is not valid in the scheme part of a URL, so a format like
	'expectedhash'path/or/URL
will be unambiguous for the URL case, and only problematic for paths that begin
with a '.

 978	 NSString* pathOrURLString = [NSString
stringWithUTF8String:pathOrURL.c_str()];

WebKit coding style is for Objective-C objects to put the space before the *,
so "NSString *pathOrURLString = ...".

 984	 NSURL* url;

Ditto.

 990	     fprintf(stderr, "Failed to parse \"%s\" as an url\n",
pathOrURL.c_str());

Typo: "an url" instead of "a URL".

 84	NSURL* url = [NSURL fileURLWithPath:[NSString
stringWithUTF8String:PROFILE_PATH]];

Again, the * should be next to "url".

 88	    if(error == noErr)

Missing space after "if". I think you can write this as if (!error).

 130	 size_t rowBytes = (4 * pixelsWide + 63) & ~63; // Use a multiple of 64
bytes to improve CG performances

Typo? "performances".

 146	 NSGraphicsContext* nsContext = [NSGraphicsContext
graphicsContextWithGraphicsPort:context flipped:NO];

"*" misplaced, through no fault of your own.

 157	 }
 158	 else {

This should be all on one line as "} else {"

 172	     NSView* documentView = [[mainFrame frameView] documentView];

Another * placement issue.

644		 } elsif (/BaselineHash: ([a-f0-9]{32})/) {
 659		 } elsif (/ExpectedHash: ([a-f0-9]{32})/) {

Is this change correct? What is it fixing? Is that code even used?

r- for now, as I think you should address at least some of the comments.