[Webkit-unassigned] [Bug 21322] DumpRenderTree pixel test improvements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21322


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24166|review?                     |review-
               Flag|                            |




------- Comment #21 from mitz at webkit.org  2008-10-08 11:16 PDT -------
(From update of attachment 24166)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.