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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jeff Walden [:Waldo]]

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:

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIConsoleMessage.idl#48

Comment 1

[Aiko]

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.

Comment 2

[Jacek Caban]

Attachment: fix v1.0

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:

https://tbpl.mozilla.org/?tree=Try&rev=4a9a9fd79eec

Comment 3

[Jacek Caban]

Comment on attachment 642219 [details] [diff] [review]
fix v1.0

Looks good on try

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 5

[Jonathan Watt [:jwatt]]

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?

Comment 6

[Jacek Caban]

I investigated it more and it's not a difference in includes. Following code:

#define TEST()
TEST(x)

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

http://msdn.microsoft.com/en-us/library/y37zb304%28v=vs.80%29.aspx
http://stackoverflow.com/questions/5248599/too-many-actual-parameters-for-macro

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.

Comment 7

[Jacek Caban]

Attachment: fix v2.0

This is sufficient for mingw compilation and fixes a warning on MSVC.

Comment 8

[Jacek Caban]

To give better context, the problemactic macro from winbase.h looks like this:

#define GetCurrentTime() GetTickCount()

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 642917 [details] [diff] [review]
fix v2.0

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

Much better, thanks!

Comment 10

[Jacek Caban]

Thanks for reviews, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/743be252e631

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/743be252e631