Comment on attachment 8334392 [details] [diff] [review] unify-image-src.patch Review of attachment 8334392 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/moz.build @@ +33,5 @@ > 'SVGDocumentWrapper.cpp', > 'VectorImage.cpp', > ] > > +SOURCES += [ Please add a comment saying why these files are not unified.

patch v2: I needed to #undef GetCurrentTime to fix some SVG link errors because winbase.h #defines GetCurrentTime GetTickCount, stomping on calls to SVG member function GetCurrentTime(). https://tbpl.mozilla.org/?tree=Try&rev=f3796a617b93

Comment on attachment 8335828 [details] [diff] [review] unify-image-src-v2.patch Review of attachment 8335828 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Thanks, Chris! ::: image/src/SVGDocumentWrapper.cpp @@ +28,5 @@ > #include "mozilla/dom/SVGAnimatedLength.h" > #include "nsMimeTypes.h" > > +// undef the GetCurrentTime macro defined in WinBase.h from the MS Platform SDK > +#undef GetCurrentTime If you know or can easily get the information, it'd be great if this comment noted which header file was responsible for pulling in WinBase.h. ::: image/src/SVGDocumentWrapper.h @@ +25,5 @@ > > #define OBSERVER_SVC_CID "@mozilla.org/observer-service;1" > > +// undef the GetCurrentTime macro defined in WinBase.h from the MS Platform SDK > +#undef GetCurrentTime (Same here.) ::: image/src/VectorImage.cpp @@ +26,5 @@ > #include "nsIDOMEventListener.h" > #include "SurfaceCache.h" > > +// undef the GetCurrentTime macro defined in WinBase.h from the MS Platform SDK > +#undef GetCurrentTime (Same here.) ::: image/src/moz.build @@ +33,5 @@ > 'SVGDocumentWrapper.cpp', > 'VectorImage.cpp', > ] > > +# These files can't be unified because of prlog.h #include order issues. It'd be good to file a followup about fixing this issue.

(In reply to Seth Fowler [:seth] from comment #4) > > +// undef the GetCurrentTime macro defined in WinBase.h from the MS Platform SDK > > +#undef GetCurrentTime > > If you know or can easily get the information, it'd be great if this comment > noted which header file was responsible for pulling in WinBase.h. Unfortunately, I don't know which header files is indirectly including WinBase.h because I don't have a Windows dev environment. I'm just testing my Windows fix on the try server. I just copy/pasted that WinBase.h comment from another header file that was #undef'ing GetCurrentTime. > > +# These files can't be unified because of prlog.h #include order issues. > > It'd be good to file a followup about fixing this issue. This prlog.h issue is not really bug. image/public/ImageLogging.h must #define FORCE_PR_LOG before prlog.h is included to enable logging in release builds. See bug 481962. In the unified .cpp file, prlog.h is indirectly included before ImageLogging.h. I'll change this comment to say ImageLogging.h instead of prlog.h.