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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 26 23:11:02 PDT 2008

mitz at webkit.org has granted Pierre-Olivier Latour <pol at apple.com>'s request for
Bug 21322: DumpRenderTree pixel test improvements

Attachment 24631: Updated patch

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

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.

More information about the webkit-reviews mailing list