Closed
Bug 582783
Opened 14 years ago
Closed 11 years ago
be more selective about #undef LoadImage
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: joe, Assigned: almasry.mina)
Details
(Whiteboard: [mentor=joe@drew.ca])
Attachments
(1 file, 4 obsolete files)
3.48 KB,
patch
|
Details | Diff | Splinter Review |
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;
Updated•12 years ago
|
OS: Mac OS X → Windows 7
QA Contact: imagelib → joe
Reporter | ||
Updated•12 years ago
|
QA Contact: joe → imagelib
Whiteboard: [mentor=joe@drew.ca]
Assignee | ||
Comment 1•11 years ago
|
||
Hi I'm new here and I'd like to take a shot at this. Can I get assigned?
Reporter | ||
Comment 2•11 years ago
|
||
Hi Mina, Absolutely! The bug is all yours. Do you have any questions about this bug or where to look?
Assignee: nobody → almasry.mina
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #727921 -
Flags: review?(joe)
Assignee | ||
Comment 4•11 years ago
|
||
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.
Reporter | ||
Comment 5•11 years ago
|
||
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
Reporter | ||
Comment 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #728668 -
Flags: review?(joe)
Assignee | ||
Comment 8•11 years ago
|
||
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
Reporter | ||
Comment 9•11 years ago
|
||
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!
Reporter | ||
Comment 10•11 years ago
|
||
(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.
Reporter | ||
Updated•11 years ago
|
Attachment #728668 -
Flags: review?(joe) → review+
Reporter | ||
Comment 11•11 years ago
|
||
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!
Assignee | ||
Comment 12•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #728785 -
Flags: review?(joe)
Assignee | ||
Comment 13•11 years ago
|
||
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...
Reporter | ||
Comment 14•11 years ago
|
||
(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!
Reporter | ||
Updated•11 years ago
|
Attachment #728785 -
Flags: review?(joe)
Reporter | ||
Comment 15•11 years ago
|
||
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+
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Reporter | ||
Comment 17•11 years ago
|
||
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!
Assignee | ||
Updated•11 years ago
|
Attachment #727921 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #728801 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #728668 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #728785 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #729238 -
Flags: checkin?(joe)
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/392c44e25cd2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•