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

RESOLVED FIXED in mozilla17

Status

()

Core
Audio/Video
--
trivial
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Jacek Caban)

Tracking

Trunk
mozilla17
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

895 bytes, patch
cpearce
: review+
Details | Diff | Splinter Review
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

9 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 642219 [details] [diff] [review]
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
Assignee: nobody → jacek
(Assignee)

Comment 3

5 years ago
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-
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 642917 [details] [diff] [review]
fix v2.0

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)
(Assignee)

Comment 8

5 years ago
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+
(Assignee)

Comment 10

5 years ago
Thanks for reviews, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/743be252e631
https://hg.mozilla.org/mozilla-central/rev/743be252e631
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.