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

Wed Oct 8 11:55:11 PDT 2008

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

------- Comment #22 from pol at apple.com  2008-10-08 11:55 PDT -------
(In reply to comment #21)
> 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 actually haven't touched this part of the DRT logic, it behaves the same as
before. The difference is that the path to the test is now stored in the layout
controller along with optional hash instead of being in a global.

The issue you raise make sense but I'd rather have it addressed in a separate
bug: the patch is already pretty big as is :)

> 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.

gLayoutTestController is created / destroyed for each test, which I like as
this way you know you don't have "stalled" parameters or data.

>  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>.

Fixed

>  91         snprintf(hashString, 33, "%s%02x", hashString, hash[i]);
> 
> That is not pretty! (But you did not write it.)

Yeah, I noticed too ;)

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

Fixed

>  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.

I'd rather not put the CGContext creation code in BitmapContext as it is
closely coupled to getBitmapContextFromWebView(): having all this setup code in
a single place makes it easier to maintain it (especially when you have to
properly deal with colorspace, endianness, pixel format and whatnot).

>  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.

Agreed, fixed

>  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))

I hadn't touched this part but fixed anyway

>  353 #if PLATFORM(MAC)
>  354     restoreMainDisplayColorProfile(0);
>  355 #endif
> 
> This is a Mac-only file, so you don't need the #if.

Fixed

>  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 '.

OK, changed

>  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;

Fixed

>  990         fprintf(stderr, "Failed to parse \"%s\" as an url\n",
> pathOrURL.c_str());
> 
> Typo: "an url" instead of "a URL".

Fixed

>  84     NSURL* url = [NSURL fileURLWithPath:[NSString
> stringWithUTF8String:PROFILE_PATH]];
> 
> Again, the * should be next to "url".

Fixed

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

Fixed

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

Fixed

>  146     NSGraphicsContext* nsContext = [NSGraphicsContext
> graphicsContextWithGraphicsPort:context flipped:NO];
> 
> "*" misplaced, through no fault of your own.

Fixed

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

Fixed

>  172         NSView* documentView = [[mainFrame frameView] documentView];
> 
> Another * placement issue.

Fixed

> 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?

Yes, this is a required change: the script expects DRT to outputs the expected
MD5 (even if the same script passes it to DRT in the first place), so I fixed
DRT and for consistency also fixed the script to use "expected" as everywhere
else in the pixel test system.

-- 
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.