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

[bugzilla-daemon at webkit.org]

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





------- Comment #33 from pol at apple.com  2008-10-23 13:44 PDT -------
(In reply to comment #28)

> This patch would be a lot easier to review if it were split up into a number of
> smaller changes (e.g., the move to use std::string in more places could have
> been its own patch). It's hard to review large patches, and when there's a
> straightforward way to break the work up into smaller pieces it's almost always
> better to do so.

That's how I had started, but unfortunately, everything became inter-dependent
rapidly.

> Normally in .cpp files we use using directives rather than continuing to use
> namespace scope operators. For example, in LayoutTestController.cpp, you could
> put "using namespace std;" just after all the #include directives, then all the
> places where you have "std::string" in that file could just become "string".
> (We don't use using directives in headers, however, so you'd still need the
> "std::" prefix there.)

Fixed

> Even though this is a patch-in-progress that will probably be further revised,
> having a detailed ChangeLog is still important. It makes the reviewer's job
> much easier because it explains *why* each change was made, not just *how*
> (which is all the code tells you).

I have one this time :)

> > +++ b/WebKitTools/DumpRenderTree/cg/ImageDiffCG.cpp
> > @@ -54,6 +54,8 @@ typedef float CGFloat;
> >  using namespace std;
> >  
> >  #if PLATFORM(WIN)
> > +#define strtof(x, y) strtod(x, y)
> > +#define roundf(x) ((x-floorf(x)) > 0.5 ? ceilf(x) : floorf(x))
> >  static const CFStringRef kUTTypePNG = CFSTR("public.png");
> >  #endif
> 
> You should use wtf/MathExtras.h to get a definition of roundf() for Windows. I
> think a static inline function would be better than a macro for strtof.

Fixed

> > +static RetainPtr<CGImageRef> compareImages(CGImageRef baseImage, CGImageRef testImage, float& difference)
> 
> I find the "compareImages" name a bit confusing. Maybe something like
> createDifferenceImage would be clearer? Or if the main purpose is to produce
> the difference value and the image is more of a side-effect, perhaps the image
> should be an out parameter and the difference value should be the return value.

There are both as important, so I just renamed the function

> There's no need to pre-declare the context variable. You can just do:
> 
> RefPtr<BitmapContext> context = createBitmapContextFromWebView(...);

The exact code (which I assume wasn't visible in the diff) is:

    RefPtr<BitmapContext> context;

#if PLATFORM(MAC)
    context =
createBitmapContextFromWebView(gLayoutTestController->testOnscreen(),
gLayoutTestController->testRepaint(),
gLayoutTestController->testRepaintSweepHorizontally(),
gLayoutTestController->dumpSelectionRect());
#endif
    ASSERT(context);

> > +class BitmapContext : public RefCounted<BitmapContext>
> > +{
> 
> This brace should go on the previous line.

Fixed

> It might be cleaner to make a typedef that is void* on Mac and HBITMAP on
> Windows.

Fixed

> We use 0 instead of NULL in C++ and Obj-C++ code.

Fixed

> > -    # If we got some PNG data, diff with expected
> > +    if($verbose && $pixelTests && !$resetResults && $actualPNGSize) {
> 
> You're missing a space after "if".

Fixed


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