Last Comment Bug 582783 - be more selective about #undef LoadImage
: be more selective about #undef LoadImage
Status: RESOLVED FIXED
[mentor=joe@drew.ca]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla22
Assigned To: Mina Almasry
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-28 15:46 PDT by Joe Drew (not getting mail)
Modified: 2013-03-27 14:12 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First revision of patch - moved #undef LoadImage to gfxWindowsSurface.h (6.88 KB, patch)
2013-03-21 15:19 PDT, Mina Almasry
joe: review+
Details | Diff | Splinter Review
Second revision of patch (2.85 KB, patch)
2013-03-23 16:02 PDT, Mina Almasry
joe: review+
Details | Diff | Splinter Review
Error building gfx (17.60 KB, patch)
2013-03-24 15:36 PDT, Mina Almasry
no flags Details | Diff | Splinter Review
Keep #include <windows.h> in gfxWindowsSurface.h (2.28 KB, patch)
2013-03-24 17:26 PDT, Mina Almasry
joe: review+
Details | Diff | Splinter Review
Bug fix v2 (3.48 KB, patch)
2013-03-25 15:10 PDT, Mina Almasry
no flags Details | Diff | Splinter Review

Description Joe Drew (not getting mail) 2010-07-28 15:46:31 PDT
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 2013-03-20 12:05:07 PDT
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) 2013-03-20 12:08:27 PDT
Hi Mina,

Absolutely! The bug is all yours.

Do you have any questions about this bug or where to look?
Comment 3 Mina Almasry 2013-03-21 15:19:46 PDT
Created attachment 727921 [details] [diff] [review]
First revision of patch - moved #undef LoadImage to gfxWindowsSurface.h
Comment 4 Mina Almasry 2013-03-21 15:51:30 PDT
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) 2013-03-22 04:56:34 PDT
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) 2013-03-22 14:07:10 PDT
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 2013-03-23 16:02:36 PDT
Created attachment 728668 [details] [diff] [review]
Second revision of patch
Comment 8 Mina Almasry 2013-03-23 16:19:56 PDT
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) 2013-03-23 19:50:21 PDT
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) 2013-03-23 19:53:35 PDT
(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) 2013-03-23 20:59:02 PDT
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 2013-03-24 15:36:50 PDT
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
Comment 13 Mina Almasry 2013-03-24 17:26:32 PDT
Created attachment 728801 [details] [diff] [review]
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) 2013-03-25 09:26:21 PDT
(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) 2013-03-25 09:27:59 PDT
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 2013-03-25 11:13:24 PDT
(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) 2013-03-25 11:18:20 PDT
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 2013-03-25 15:10:15 PDT
Created attachment 729238 [details] [diff] [review]
Bug fix v2

Thanks Joe!
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-03-27 08:36:01 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/392c44e25cd2
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-03-27 14:12:25 PDT
https://hg.mozilla.org/mozilla-central/rev/392c44e25cd2

Note You need to log in before you can comment on or make changes to this bug.