210951 - Use a helper class instead of nsImageWin::BlitImage

Status: RESOLVED WONTFIX

(Core Graveyard :: GFX: Win32, defect)

Description

[Simon Montagu :smontagu]

Comment 1

[Simon Montagu :smontagu]

Attachment: wip patch

This is a totally untested work-in-progress patch.

Comment 2

[Simon Montagu :smontagu]

Attachment: Getting closer

This isn't ready for review without more testing, but I would welcome
high-level comments (like "this is a good idea" or "this is a stupid waste of
time") already.

Comment 3

[Simon Montagu :smontagu]

Attachment: Patch for review

Comment 4

[Ere Maijala (slow)]

I guess I was too slow with the review as the patch won't apply anymore. Hunk 4
failed.

Comment 5

[Simon Montagu :smontagu]

I don't quite understand that, since nsImageWin.cpp hasn't changed for about 2
months, and the patch applies for me on a clean tree.

Comment 6

[Ere Maijala (slow)]

My bad, I managed to somehow break the patch file. Now to the review..

Comment 7

[Ere Maijala (slow)]

Comment on attachment 127061 [details] [diff] [review]
Patch for review

Ok, here we go, finally. The code looks basically ok to me, but here are some
small things I found. I'm quite sure I wasn't able to find everything :)

The classes don't have any safeguards against incorrect use. Looks to me like
the code works correctly as it is, but I would like to see some assertions at
least against overwriting non-null pointers (like if someone called
nsBlitter::CreateImage() twice or didn't call Init() first).  

Why not do the stuff Cleanup() does in the destructor? That would make it clear
it's not supposed to be called by anything else. Otherwise I think it should
nullify the variables.

>    /* Call GetProcAddress() on MaskBlt() */
>    PRBool InitMaskBlt();

Why not just call ::MaskBlt()? The version check should be enough. I believe
MaskBlt is stubbed at least since Win95.

In nsBlitterWithAlpha::CreateTemporary():
>        if (mTmpMaskBits) {
>          mOldTmpMaskBits = (HBITMAP)::SelectObject(mTmpMaskDC, mTmpMaskBits);
>        } else {
>          return PR_FALSE;
>        }

How about just "if(!) return"?

>  if (!mTmpImgBits) {
>    return PR_FALSE;
>  }

An earlier return (well, two of them) would make it a lot easier to read and
and it would also eliminate need for "if (mTmpImgBits) {".


Style nits:

>class nsBlitter
>{
>  public:
>    nsBlitter();

"public:" should not be indented and others just indented once. 

>    void     Blit(HDC aDstDC,

Other definitions are not indented like this and BlitImageAndMask.

>  if(0!=memDC){

>    if(0 != tmpBitmap){

>      if(0!=oldBitmap) {

Spaces..

>  mMaskBits  = ::CreateDIBSection(mHDC, (LPBITMAPINFO)&bmi,
>                                  DIB_RGB_COLORS, &screenBits, NULL, 0);

Extra space.

Incorrect indentations:

>    PRBool CreateImage(LPBITMAPINFOHEADER aBHead,
>                         PRUint8* aImageBits, PRInt16 aNumColors,
>                         PRInt32 aSrcX, PRInt32 aSrcY,
>                         PRInt32 aWidth, PRInt32 aHeight);

>nsBlitter::nsBlitter()
>{
> mImgDC = nsnull;


Typo:

>// Helper class to blit images according to platform capabilites

"capabilities"

Comment 8

[Simon Montagu :smontagu]

>I believe MaskBlt is stubbed at least since Win95.

I have a patch addressing this and your other concerns and will attach it after
confirming that it builds on Win9x. If so, do you think we can do the same thing
with AlphaBlend()? The #defines for ALPHABLENDPROC in nsImageWin.h are commented
/* for compatibility with VC++ 5 */ but VC++ 5 has not been supported for some
years.

Comment 9

[Ere Maijala (slow)]

I think we could get rid of the defines, but as long as Windows 95 is supported,
we can't link to msimg32 as it doesn't exist in Windows 95. 

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch addressing ere's concerns

I think I have addressed all your comments, though sometimes they are expressed
rather briefly and I hope I have understood them correctly.

Exceptions:
>>  if(0!=memDC){
>>    if(0 != tmpBitmap){
>>	if(0!=oldBitmap) {
>
>Spaces..

I didn't change these lines...

Comment 11

[Ere Maijala (slow)]

> I didn't change these lines...

Oops, sorry about that. I got distracted. 

Comment 12

[Ere Maijala (slow)]

Comment on attachment 128899 [details] [diff] [review]
Patch addressing ere's concerns

It all looked good until I noticed it turns scrollbar arrows into black blocks.

Comment 13

[ArronM (:paper)]

Comment on attachment 128899 [details] [diff] [review]
Patch addressing ere's concerns

I can get this patch to work by changing:
+    if (!((nsBlitterWithAlpha*)blitter)->CreateMask(0, mBHead->biWidth,
to
+    if (!((nsBlitterWithAlpha*)blitter)->CreateMask(0, 0,

and changing the two location of
+	     MAKEROP4(SRCPAINT, SRCCOPY));
to
+	     MAKEROP4(SRCCOPY, SRCPAINT));



HOWEVER, there's still an issue here in nsMaskBlitter::CreateMask

+  if (aSrcX == 0 && aSrcY == aHeight) {

That line doesn't make sense.  Why only use ::CreateBitmap is aSrcY == aHeight?
 With the changes above, this always returns false, and  ::CreateBitmap never
gets called.  The function falls back to nsBlitterWithAlpha::CreateMask.  So
unless there's some purpose for using ::CreateBitmap, I would also remove
nsMaskingBlitter::CreateMask alltogether

Comment 14

[ArronM (:paper)]

sorry for the bugspam, two typos in my last comment.  
Re: "two location" means there's two locations in the code that must be changed
(in nsMaskingBlitter::ImageToDC and in nsMaskingBlitter::TemporaryToDC)
Re: "Why only use ::CreateBitmap is aSrcY == aHeight?" = "Why only use
::CreateBitmap if aSrcY == aHeight?"

Comment 15

[Simon Montagu :smontagu]

Mass reassign my image bugs to [e-mail]

Comment 16

[Aiko]

nsImageWin doesn't exist any more.