[webkit-reviews] review granted: [Bug 21322] DumpRenderTree pixel test improvements : [Attachment 24631] Updated patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted 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 24631: Updated patch
https://bugs.webkit.org/attachment.cgi?id=24631&action=edit

------- Additional Comments from mitz at webkit.org
> +	   - Ensure font smoothing is disabled (this is also called LCD
anti-aliasing and
> +	   is different from regular font CG anti-aliasing) as font-smoothing
settings
> +	   depends on the display and can also be changed by the user

The user setting was overridden by the old code by setting the user default
value to MediumFontSmoothing, so I don't think that was part of the problem.

> +	   - Use a new cleared buffer for each test instead of the reusing same
one to
> +	   avoid potential result corruption across tests

Is there any evidence that such corruption occurred or that it could occur? It
is OK to make this change, but it is not clear that it is solving a problem,
not even a potential one.

> +		   }
> +	       }
> +	       else
> +		   fputs("error, test and reference image have different
properties.\n", stderr);

The else should go on the same line with the brace.

> +    static PassRefPtr<BitmapContext>
createByAdoptingBitmapAndContext(BitmapContextBacking backing, CGContextRef
context)

I think "BitmapContextBacking" and "backing" are not ideal names. Variable
names should almost always be nouns, and the noun meaning of "backing" (the
gerund) does not make sense here. "Backing buffer" or "backing store" seem
better for the variable names, while something "PlatformBitmapBuffer" might be
better as the type name.

These are minor comments so I am going to say r=me.