582783 - be more selective about #undef LoadImage

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

Spun off from bug 572520 comment 55:

for the LoadImage #undef, seems like that should be in gfxWindowsSurface.h. 
Also, we can probably get rid of the windows.h include in gfxWindowsSurface.h
by just providing local definitions of HDC and HWND; they're declared via
DECLARE_HANDLE in WinDef.h (with DECLARE_HANDLE coming from WinNT.h), so:

struct nameHWND__;
struct nameHDC__;
typedef struct nameHWND__ *HWND;
typedef struct nameHDC__ *HDC;

Comment 1

[Mina Almasry]

Hi I'm new here and I'd like to take a shot at this. Can I get assigned?

Comment 2

[Joe Drew (not getting mail)]

Hi Mina,

Absolutely! The bug is all yours.

Do you have any questions about this bug or where to look?

Comment 3

[Mina Almasry]

Attachment: First revision of patch - moved #undef LoadImage to gfxWindowsSurface.h

Comment 4

[Mina Almasry]

Hi Joe!

(I decided to take a stab at it before I ask you)

Sorry I seem to have a problem with line endings, I'm working on fixing that, but if I understand correctly you have #undef LoadImage in these files:

content/svg/content/src/nsSVGFilters.cpp
image/build/nsImageModule.cpp
image/src/imgRequest.cpp
image/src/imgLoader.cpp

and you would like to move it to gfxWindowsSurface.h, after #include <windows.h>

I made a patch that does just that and added it to this bug. If that's not what you wanted let me know.

Comment 5

[Joe Drew (not getting mail)]

This definitely seems right for the first part of this bug! Do you have access to the try server? https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 727921 [details] [diff] [review]
First revision of patch - moved #undef LoadImage to gfxWindowsSurface.h

r=me as long as that line ending stuff gets fixed; also add an explanatory comment in gfxWindowsSurface.h as to why that's the case.

Comment 7

[Mina Almasry]

Attachment: Second revision of patch

Comment 8

[Mina Almasry]

Hey again Joe,

No I actually don't have access to the try server, and would love to get some. I have 2 other bugs and if gecko keeps me interested I will be contributing more for the next 6 months. I was actually wondering how I'm supposed to test all this (I can't get incremental builds to work for me yet). Should I ask for access?

In addition to the first patch, this removes the #include windows.h, adds the declarations we need from that, and fixes the whitespace error. I am haven't tested it yet and I'm not sure how to. The incrmental build fails on my machine with or without the patch...

I'm stuck at testing the patch works. Where do I go from here? I'll work on getting the incrmental builds working, but for now do I do a full build, or wait to tryserver access? Is there another way to test patches?

Thanks,

Mina

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 728668 [details] [diff] [review]
Second revision of patch

Review of attachment 728668 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxWindowsSurface.h
@@ +9,4 @@
>  #include "gfxASurface.h"
>  #include "gfxImageSurface.h"
>  
> +/* define names we need from windows.h to avoid uncluding the file */

Note: Typo, "including"

@@ +15,5 @@
> +typedef struct nameHWND__ *HWND;
> +typedef struct nameHDC__ *HDC;
> +
> +/* prevent windows from redefining LoadImage */
> +#undef LoadImage

You actually shouldn't even need this now!

Comment 10

[Joe Drew (not getting mail)]

(In reply to Mina Almasry from comment #8)
> No I actually don't have access to the try server, and would love to get
> some. I have 2 other bugs and if gecko keeps me interested I will be
> contributing more for the next 6 months. I was actually wondering how I'm
> supposed to test all this (I can't get incremental builds to work for me
> yet). Should I ask for access?

I've pushed this to try for you: https://tbpl.mozilla.org/?tree=Try&rev=7a85d92ad780

If you're actively working on 2 other bugs, then yes, you should definitely ask for Try access.

> In addition to the first patch, this removes the #include windows.h, adds
> the declarations we need from that, and fixes the whitespace error. I am
> haven't tested it yet and I'm not sure how to. The incrmental build fails on
> my machine with or without the patch...

Incremental build shouldn't be failing; you should ask in #introduction about that problem.

> I'm stuck at testing the patch works. Where do I go from here? I'll work on
> getting the incrmental builds working, but for now do I do a full build, or
> wait to tryserver access? Is there another way to test patches?

In this case, Try is going to be your best bet, since it's a compile change and nothing else.

Comment 11

[Joe Drew (not getting mail)]

Mina,

Sad news - it seems we don't compile as-is. (This isn't terribly surprising, though.)

What's going to happen at this point is that you're going to need to go through the compile on your Windows computer and find all the places that were relying on #include <windows.h> - like nsDeviceContext.cpp, mentioned in the try log https://tbpl.mozilla.org/php/getParsedLog.php?id=21025725&tree=Try&full=1#error0 - and add that include manually.

Once your build works, re-upload a patch and flag me for review, and we can go from there!

Comment 12

[Mina Almasry]

Attachment: Error building gfx

I included windows.h manually in nsDeviceContext.cpp like you said, but then I got a make error: redefining HWND. Windows thinks that the definition we put in gfxWindowsSurface.h is different from the one defined in windef.h

Comment 13

[Mina Almasry]

Attachment: Keep #include <windows.h> in gfxWindowsSurface.h

Also, I tried to guess what you'll ask me to do so here's a patch that keeps #include <windows.h> in gfxWindowsSurface.h

Incremental build of gfx and image completes successfully on my windows 8 machine

I hope I'm not bombarding you with messages...

Comment 14

[Joe Drew (not getting mail)]

(In reply to Mina Almasry from comment #12)
> Created attachment 728785 [details] [diff] [review]
> Error building gfx
> 
> I included windows.h manually in nsDeviceContext.cpp like you said, but then
> I got a make error: redefining HWND. Windows thinks that the definition we
> put in gfxWindowsSurface.h is different from the one defined in windef.h

Can you show me the definition that Windows refers to? Maybe the type we put in there is just out of date.

(In reply to Mina Almasry from comment #13)
> I hope I'm not bombarding you with messages...

Not at all!

Comment 15

[Joe Drew (not getting mail)]

Comment on attachment 728801 [details] [diff] [review]
Keep #include <windows.h> in gfxWindowsSurface.h

Review of attachment 728801 [details] [diff] [review]:
-----------------------------------------------------------------

If we can't get not including windows.h working, we'll fall back to this. Thanks!

::: gfx/thebes/gfxWindowsSurface.h
@@ +9,4 @@
>  #include "gfxASurface.h"
>  #include "gfxImageSurface.h"
>  
> +/* include windows.h HWND and HDC definitions that we need. */

"for HWND and HDC"

Comment 16

[Mina Almasry]

(In reply to Joe Drew (:JOEDREW! \o/) (Vacation 26 March–11 April) from comment #14)
> (In reply to Mina Almasry from comment #12)
> > Created attachment 728785 [details] [diff] [review]
> > Error building gfx
> > 
> > I included windows.h manually in nsDeviceContext.cpp like you said, but then
> > I got a make error: redefining HWND. Windows thinks that the definition we
> > put in gfxWindowsSurface.h is different from the one defined in windef.h
> 
> Can you show me the definition that Windows refers to? Maybe the type we put
> in there is just out of date.
> 

Absolutely. windef.h declares:
DECLARE_HANDLE(HWND);
DECLARE_HANDLE(HDC);

And on my machine winnt.h declares:
#if 0 && (_MSC_VER > 1000)
#define DECLARE_HANDLE(name) struct name##__; typedef struct name##__ *name
#else
#define DECLARE_HANDLE(name) struct name##__{int unused;}; typedef struct name##__ *name
#endif

I see we could include winnt.h and then do 
DECLARE_HANDLE(HDC); 
DECLARE_HANDLE(HWND);

But I don't know if we want to declare HDC and HWND ourselves without DECLARE_HANDLE. That would mean that if windows folks change the definition of DECLARE_HANDLE, our code will break too, no? DECLARE_HANDLE could also be dependant on the version of windows/visual studio.

Comment 17

[Joe Drew (not getting mail)]

Yeah. This feels like it's going to put us in a world of pain. I think your attachment 728801 [details] [diff] [review] is the right way to go.

Can you update it to include the language fix, make sure it's formatted correctly <https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in>, mark the other patches as obsolete and then set checkin-needed? Then we can get this fix into mozilla-central!

Comment 18

[Mina Almasry]

Attachment: Bug fix v2

Thanks Joe!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/392c44e25cd2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/392c44e25cd2