Closed Bug 582783 Opened 10 years ago Closed 8 years ago

be more selective about #undef LoadImage

Categories

(Core :: ImageLib, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: joe, Assigned: almasry.mina)

Details

(Whiteboard: [mentor=joe@drew.ca])

Attachments

(1 file, 4 obsolete files)

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;
OS: Mac OS X → Windows 7
QA Contact: imagelib → joe
QA Contact: joe → imagelib
Whiteboard: [mentor=joe@drew.ca]
Hi I'm new here and I'd like to take a shot at this. Can I get assigned?
Hi Mina,

Absolutely! The bug is all yours.

Do you have any questions about this bug or where to look?
Assignee: nobody → almasry.mina
Attachment #727921 - Flags: review?(joe)
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.
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 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.
Attachment #727921 - Flags: review?(joe) → review+
Attached patch Second revision of patch (obsolete) — Splinter Review
Attachment #728668 - Flags: review?(joe)
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 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!
(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.
Attachment #728668 - Flags: review?(joe) → review+
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!
Attached patch Error building gfx (obsolete) — Splinter Review
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
Attachment #728785 - Flags: review?(joe)
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...
(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!
Attachment #728785 - Flags: review?(joe)
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"
Attachment #728801 - Flags: review+
(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.
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!
Attachment #727921 - Attachment is obsolete: true
Attachment #728801 - Attachment is obsolete: true
Attachment #728668 - Attachment is obsolete: true
Attachment #728785 - Attachment is obsolete: true
Attached patch Bug fix v2Splinter Review
Thanks Joe!
Attachment #729238 - Flags: checkin?(joe)
Attachment #729238 - Flags: checkin?(joe)
https://hg.mozilla.org/mozilla-central/rev/392c44e25cd2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.