Bug 26425 – Small Refactoring to Consolidate Common GDI Calls

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

There are a number of places where BITMAPINFO structures and XFORM structures are created.  These are almost always the same, and seem like a good opportunity to abstract these structure filling routines to be shared between the various Windows ports.

This patch adds a new GDIUtilities.h/GDIUtilities.cpp file that contains implementations of routines to:
1.  Construct the typical BITMAPINFO structure we use in various operations.
2.  Clear the data in a BITMAP structure.
3.  Construct a Windows XFORM object from the input matrix components.

After this patch, several methods in GraphicsContextCGWin and GraphicsContextCairoWin can be pushed up to GraphicsContextWin.

[Brent Fulgham]

Created attachment 31320 [details]
Rev 1.

[Eric Seidel (no email)]

Looks great to me.  The patch is missing the new files, and Adam and Steve are the people you should really ask about windows changes.

[Eric Seidel (no email)]

Comment on attachment 31320 [details]
Rev 1.

Missing the added files.

[Brent Fulgham]

Created attachment 31325 [details]
Rev. 2.

Show missing (new) files.

[Adam Roben (:aroben)]

Comment on attachment 31325 [details]
Rev. 2.

> + #ifndef GdiUtilities_h

This should be GDIUtilities_h. We always match the case of the file name.

In general we've tried to resist adding *Utilities files, partly because they can become a catch-all. So I've been trying to think about how we could remove the need for such a file here.

> + XFORM makeXFORM(float, float, float, float, float, float);

I think it would be better to add an operator XFORM() to the TransformationMatrix class instead of adding the makeXFORM function. Then we can change the existing code to use TransformationMatrix::translate to convert the rect into a matrix, and convert that to an XFORM. Does that make sense?

> + BITMAPINFO bitmapInfoForSize(const IntSize&, bool invertHeight = false);

Boolean parameters make it hard to read code, and optional boolean parameters make it really easy to make mistakes when the function signature changes. You could replace the optional boolean with a required two-member enum, like this:

enum BitmapOrientation { TopDown, BottomUp };
BITMAPINFO bitmapInfoForSize(const IntSize&, BitmapOrientation).

Or you could just require the caller to negate the height themselves.

Another option, which would allow moving this function out of GDIUtilities (and get us a step closer to removing that file altogether), would be to create a WebCore::BitmapInfo struct that derives from BITMAPINFO. Something like:

struct BitmapInfo : public BITMAPINFO {
    static BitmapInfo create(const IntSize&);
    static BitmapInfo createBottomUp(const IntSize&);
};

> + void fillWithClearColor(HBITMAP);

I'm not sure what to do with this one.

[Brent Fulgham]

(In reply to comment #5)
> In general we've tried to resist adding *Utilities files, partly because they
> can become a catch-all. So I've been trying to think about how we could remove
> the need for such a file here.

Okay
 
> > + XFORM makeXFORM(float, float, float, float, float, float);
> 
> I think it would be better to add an operator XFORM() to the
> TransformationMatrix class instead of adding the makeXFORM function. Then we
> can change the existing code to use TransformationMatrix::translate to convert
> the rect into a matrix, and convert that to an XFORM. Does that make sense?

Yes.  I can do that.
 
> > + BITMAPINFO bitmapInfoForSize(const IntSize&, bool invertHeight = false);
> Another option, which would allow moving this function out of GDIUtilities (and
> get us a step closer to removing that file altogether), would be to create a
> WebCore::BitmapInfo struct that derives from BITMAPINFO. Something like:
> 
> struct BitmapInfo : public BITMAPINFO {
>     static BitmapInfo create(const IntSize&);
>     static BitmapInfo createBottomUp(const IntSize&);
> };

That's a good idea.  Should this be in its own set of files, or is there someplace  else where this would make sense?
 
> > + void fillWithClearColor(HBITMAP);
> 
> I'm not sure what to do with this one.

The only thing that uses this is GraphicsContextWinXXXX, so I can just make it a method of the class and put it there.

[Adam Roben (:aroben)]

(In reply to comment #6)
> (In reply to comment #5)
> > > + BITMAPINFO bitmapInfoForSize(const IntSize&, bool invertHeight = false);
> > Another option, which would allow moving this function out of GDIUtilities (and
> > get us a step closer to removing that file altogether), would be to create a
> > WebCore::BitmapInfo struct that derives from BITMAPINFO. Something like:
> > 
> > struct BitmapInfo : public BITMAPINFO {
> >     static BitmapInfo create(const IntSize&);
> >     static BitmapInfo createBottomUp(const IntSize&);
> > };
> 
> That's a good idea.  Should this be in its own set of files, or is there
> someplace  else where this would make sense?

I think having its own files would be best.

> > > + void fillWithClearColor(HBITMAP);
> > 
> > I'm not sure what to do with this one.
> 
> The only thing that uses this is GraphicsContextWinXXXX, so I can just make it
> a method of the class and put it there.

Adding a new function to GraphicsContext.h seems a little ugly for this platform-specific function, especially since the function doesn't involve the GraphicsContext at all.

It looks like the only caller to fillWithClearColor is getWindowsContext. Can we do some refactoring to share that implementation, so that we don't need to find a way to share just this one function?

[Brent Fulgham]

Created attachment 31453 [details]
Rev 3

Revised per Adam Roben's comments.

[Adam Roben (:aroben)]

Comment on attachment 31453 [details]
Rev 3

This is looking great.

Since the 4 changes are each pretty self-contained, it would be much better to get them reviewed and landed as separate patches.

I also think the BitmapInfo functions should return a WebCore::BitmapInfo, not a BITMAPINFO.

r- so that these changes can be split up. Thanks for the great work!

[Brent Fulgham]

Created attachment 31456 [details]
Part 1 (XFORM operator)

Add new XFORM operator to TransformationMatrix

[Brent Fulgham]

Created attachment 31457 [details]
Part 2: (BitmapInfo structure)

This patch adds the BitmapInfo structure for later use in some refactoring.

[Adam Roben (:aroben)]

Comment on attachment 31456 [details]
Part 1 (XFORM operator)

I don't think there's value in a patch that only adds the casting operator but doesn't use it. Can you put the code that uses it in this patch, too? Sorry for being unclear earlier.

[Adam Roben (:aroben)]

Comment on attachment 31457 [details]
Part 2: (BitmapInfo structure)

Again, please put the code that uses the new functions in this patch. Sorry for being unclear.

[Brent Fulgham]

Created attachment 31463 [details]
Part 3: (Use BitmapInfo Structure)

Depends on Patch #2.  Use BitmapInfo structure instead of manual structure assembly.

[Brent Fulgham]

Created attachment 31465 [details]
Part 1: BitmapInfo Refactoring

Add a new BitmapInfo structure and use to refactor some duplicated code

[Adam Roben (:aroben)]

Comment on attachment 31465 [details]
Part 1: BitmapInfo Refactoring

> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,31 @@
> [phone]  Brent Fulgham  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Refactor a few common routines in the various Windows ports
> +        and reduce some duplicated code.
> +
> +        Refactor use of BITMAPINFO for the new BitmapInfo structure.

You should mention the Bugzilla URL in your ChangeLog.

> +        BITMAPINFO bitmapInfo = BitmapInfo::create(dstRect.size());

Might as well use BitmapInfo here, too.

You could give BitmapInfo a constructor that initialized bmiHeader.biSize correctly and everything else to 0. That would follow the way ATL's C++ wrappers work. Or you could leave it like you have it now.

r=me

[Brent Fulgham]

Created attachment 31467 [details]
Part 2: (Refactor XFORM)

Add new XFORM casting operator to TransformationMatrix, and use it to remove some duplicated code.

Push the concatCTM up from the CG- and Cairo-specific files into the generic GraphicsContextWin file.

[Brent Fulgham]

Part 1 landed in https://trac.webkit.org/changeset/44789.

[Adam Roben (:aroben)]

Comment on attachment 31467 [details]
Part 2: (Refactor XFORM)

>          // Apply a translation to our context so that the drawing done will be at (0,0) of the bitmap.
> -        XFORM xform;
> -        xform.eM11 = 1.0f;
> -        xform.eM12 = 0.0f;
> -        xform.eM21 = 0.0f;
> -        xform.eM22 = 1.0f;
> -        xform.eDx = -dstRect.x();
> -        xform.eDy = -dstRect.y();
> +        TransformationMatrix translate(1.0f, 0.0f, 0.0f, 1.0f, -dstRect.x(), -dstRect.y());
> +        XFORM xform = translate;

Maybe this would be better as:

XFORM xform = TransformationMatrix().translate(-dstRect.x(), -dstRect.y());

> -    XFORM xform;
> -    xform.eM11 = size.width();
> -    xform.eM12 = 0.0f;
> -    xform.eM21 = 0.0f;
> -    xform.eM22 = size.height();
> -    xform.eDx = 0.0f;
> -    xform.eDy = 0.0f;
> +
> +    TransformationMatrix scale(size.width(), 0.0f, 0.0f, size.height(), 0.0f, 0.0f);
> +
> +    XFORM xform = scale;

Maybe this would be better as:

XFORM xform = TransformationMatrix().scale(size.width(), size.height());

> -    XFORM xform;
> -    xform.eM11 = cosAngle;
> -    xform.eM12 = -sinAngle;
> -    xform.eM21 = sinAngle;
> -    xform.eM22 = cosAngle;
> -    xform.eDx = 0.0f;
> -    xform.eDy = 0.0f;
> +
> +    TransformationMatrix rotate(cosAngle, -sinAngle, sinAngle, cosAngle, 0.0f, 0.0f);
> +
> +    XFORM xform = rotate;

You could probably use TransformationMatrix::rotate() here, but I don't know the exactly incantation (probably just TransformationMatrix().rotate(degreesAngle)).

...and so on for the other cases.

r=me

[Brent Fulgham]

Created attachment 31486 [details]
Part 3: (Move common logic to parent)

Add new 'flush' method to allow more common logic to be pushed to the parent file.

[Eric Seidel (no email)]

Comment on attachment 31486 [details]
Part 3: (Move common logic to parent)

Assuming you're sure that the two getWindowsContext implementations are identical, than  this looks great!  r=me

[Brent Fulgham]

Part 2 landed in http://trac.webkit.org/changeset/44793.

[Brent Fulgham]

Part 3 landed in http://trac.webkit.org/changeset/44821.

[Brent Fulgham]

Created attachment 31507 [details]
Part 4: (Final cleanups)

1. Move WinBitmap up so both ports can use it.
2. Use more concise TransformationMatrix syntax as suggested by Adam Roben in his earlier review.

[Eric Seidel (no email)]

Comment on attachment 31507 [details]
Part 4: (Final cleanups)

LGTM.

[Brent Fulgham]

Created attachment 31522 [details]
Part 5: (Some missed cleanups)

A handful of changes where the BitmapInfo struct can remove duplicated code.

[David Levin]

Assigned to bfulgham to make it clear who is landing it.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/44836.