Closed Bug 472799 Opened 11 years ago Closed 7 years ago

Use binaryname=CurrentTimeMoz annotation rather than #undef GetCurrentTime to address Windows macro idiocy


(Core :: Audio/Video, defect, trivial)

Windows XP
Not set





(Reporter: Waldo, Assigned: jacek)




(1 file, 1 obsolete file)

Undefining GetCurrentTime breaks any third-party code that wants to interact with media elements while simultaneously using the Windows macro for whatever it does.  The binaryname annotation is the correct way to address this:
Just for the record, GetCurrentTime is an ancient and obsolete part of the Win16 API. I doubt there are many Windows 3.1 apps dying to be ported to XULRunner or whatever, so this problem is pretty hypothetic.

It sure is nice that there is a better way to handle this, though.
Attached patch fix v1.0 (obsolete) — Splinter Review
I found this bug when I was looking at current mingw build breakage due to GetCurrentTime macro. It looks like there is a difference in includes order between GCC and MSVC build causing problems with the macro during Navigator.cpp compilation. Instead of improving the hack, I decided to get rid of all GetCurrentTime usages in m-c. The attached patch changes IDLs to use GetCurrentTimeMoz and all other cases s/GetCurrentTime/GetTime/. It passes locally, waiting for try results:
Assignee: nobody → jacek
Comment on attachment 642219 [details] [diff] [review]
fix v1.0

Looks good on try
Attachment #642219 - Flags: review?(rjesup)
I work on media and I've spoken to the SVG and SMIL guys and we agree that we'd prefer to not rename our uses of GetCurrentTime. We think it would reduce our code clarity.

However we are interested in ensuring that we can build on Mingw/GCC... So can you work around this some other way? Can you instead re-order the includes so that you can build on Mingw/GCC? Or is there some other work around?
Comment on attachment 642219 [details] [diff] [review]
fix v1.0

Yeah, sorry, r- on the SVG name changes at least, unless it turns out this can't be worked around another way. What are the actual Mingw build errors being caused by? GetCurrentTime being redefined and breaking the SVG code? GetCurrentTime being undefined and then being needed by non-Mozilla source files?
Attachment #642219 - Flags: review-
I investigated it more and it's not a difference in includes. Following code:

#define TEST()

will cause an error on GCC, while it's only warning on (most) MSVCs. See:

for more info. To sum it up, MSVC ignores extra macro arguments, while GCC throws an error. We have #undef GetCurrentTime in a few places, but it's not sufficient. What happens in this case is that we attempt to undef it before winbase.h is included, then include winbase.h and use it in nsDOMMediaStream.h. In case of MSVC, it just causes a warning, When you look at preprocessed file, it contains bogus declaration of nsDOMMediaStream class with a function:

virtual nsresult __stdcall GetTickCount();

instead of GetCurrentTime(double *aCurrentTime). This happens to work because that function is not used in Navigator.cpp. The same happens to nsSMILTimeContainer's GetCurrentTime in Navigator.cpp. I expect this to happen in more places in the codebase, but it's unnoticed (no example to prove). Note that such cases work fine even on GCC as long as GetCurrentTime doesn't get arguments (and its non-IDL-based implementations don't take arguments). This may lead to strange behavior.

However, if you prefer the current solution, that's fine with me. I will attach a patch adding one more #undef to make GCC happy and we can deal with eventual future collisions when they happen.
Attached patch fix v2.0Splinter Review
This is sufficient for mingw compilation and fixes a warning on MSVC.
Attachment #642219 - Attachment is obsolete: true
Attachment #642219 - Flags: review?(rjesup)
Attachment #642917 - Flags: review?(cpearce)
To give better context, the problemactic macro from winbase.h looks like this:

#define GetCurrentTime() GetTickCount()
Comment on attachment 642917 [details] [diff] [review]
fix v2.0

Review of attachment 642917 [details] [diff] [review]:

Much better, thanks!
Attachment #642917 - Flags: review?(cpearce) → review+
Thanks for reviews, pushed to m-i:
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.