Build breaks with Windows 8.1 SDK (installed by VS2013) and ac_add_options --enable-warnings-as-errors netwerk/protocol/http/nsHttpHandler.cpp(706) : warning C4996: 'GetVersionExW': was declared deprecated xpcom/base/nsWindowsHelpers.h(117) : warning C4996: 'GetVersionExW': was declared deprecated GetVersionEx on MSDN: http://msdn.microsoft.com/en-us/library/windows/desktop/ms724451%28v=vs.85%29.aspx It seems that a lot of people get version comparisons wrong, so Microsoft is promoting a new family of APIs to do the comparison for you: IsWindowsVistaOrGreater() etc. But the user agent code really does need to get the raw numeric values, so we may just want to suppress the warning for that one.

There are two potential ways to fix nsWindowsHelpers.h (http://hg.mozilla.org/mozilla-central/annotate/0e26e6f12ad9/xpcom/base/nsWindowsHelpers.h#l109). 1. Check the SDK version, and if we're compiling with 8.1 then use the new ::IsWindowsVistaOrGreater(). 2. Suppress the warning. As long as we still support older SDKs, then we have to keep using GetVersionEx with those SDKs anyway. We might as well keep the code consistent. Personally I lean towards 2. Normally I like fixing warnings, but I don't think this one gives us enough benefit since we'd still have to keep the old code path. (By the way, these two hits on GetVersionEx are the only thing preventing us from --enable-warnings-as-errors. Whew. I was worried it would be worse.)

3. Use ::VerifyVersionInfo(). The new version helper API is just a thin wrapper of this function.

(In reply to Masatoshi Kimura [:emk] from comment #2) > 3. Use ::VerifyVersionInfo(). The new version helper API is just a thin > wrapper of this function. You are right, it seems that VerifyVersionInfo is available on all SDKs that we need to support. #3 is probably the best option.

nsHttpHandler essentially requires GetVersionEx() to build the UA string.

Try runs: https://tbpl.mozilla.org/?tree=Try&rev=8ff05075562f https://tbpl.mozilla.org/?tree=Try&rev=85423c904e24

Comment on attachment 817545 [details] [diff] [review] Replace WinUtils::GetWindowsVersion() and GetWindowsServicePackVersion() Review of attachment 817545 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a fan of 'IsOsSomeVersionOrTest' helper functions in the global scope. We've been migrating away from these and toward the WinUtils versions for a while now. I'll default to other windows folks though if people like them. Generally maybe we should wrap everything in a deeper namespace (mozilla::winver or similar?) so we can just pull the whole name space in vs. pulling in individual functions. Or better yet, don't pull in the namespace at all so people know where these calls are coming from. ::: widget/windows/nsLookAndFeel.cpp @@ +26,5 @@ > + WIN8_VERSION = 0x602, > + WIN8_1_VERSION = 0x603 > +}; > + > +static WinVersion GetWindowsVersion() Can we move this into WindowsVersion.h, but modify it such that it works like the WinUtils helpers we had that allow value comparisons?

Also looks like we've lost the caching WinUtils performed. I'd really like to see that included in these new utils.

What's the benefit of pulling it out of widget/windows by the way? Is xpcomglue_s linked somewhere else than xul.dll that needs to use it? WinUtils.h is already exported.

> Is xpcomglue_s linked somewhere else than xul.dll that needs to use it? Yes. firefox.exe needs this, as do a few other places, AIUI.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #15) > > Is xpcomglue_s linked somewhere else than xul.dll that needs to use it? > > Yes. firefox.exe needs this, as do a few other places, AIUI. Is xpcomglue_s the ideal place for it? For example some apps like updater don't link to it but would benefit from functions like the windows version helpers. I've gotten around this in the past by making the self contained static/unnamed-namespace nsWindowsHelpers.h. But I think that would lead to larger file sizes. Maybe it should go in its own winglue library?

You don't need it to be static/unnamed: you can just make it inline and then the compiler will either inline or combine them.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #17) > You don't need it to be static/unnamed: you can just make it inline and then > the compiler will either inline or combine them. In C99 and possibly other places that doesn't hold true, but in general I think that doing both static and inline wouldn't hurt with link time optimizations. So should we make it header only code, and then it doesn't matter where it lives as long as it is added to the EXPORTS?

Not static inline, just inline.

They'll lea to the same file size, but that's fine. . As long as we never plan to include the new proposed, uncoded yet, header only version in .c files that's fine.

Comment on attachment 817542 [details] [diff] [review] Suppress warning in netwerk/ though you can't tell it from the directory structure, gerv is the owner of the UA code. gerv as this is code not content and you want to delegate it back to me then that would be fine by me.

(In reply to Jim Mathies [:jimm] from comment #12) > ::: widget/windows/nsLookAndFeel.cpp > @@ +26,5 @@ > > + WIN8_VERSION = 0x602, > > + WIN8_1_VERSION = 0x603 > > +}; > > + > > +static WinVersion GetWindowsVersion() > > Can we move this into WindowsVersion.h, but modify it such that it works > like the WinUtils helpers we had that allow value comparisons? I would like to discourage the use of GetVersionEx as much as possible. If I moved the function to a public header, people may find the function and start to use it. (In reply to Jim Mathies [:jimm] from comment #13) > Also looks like we've lost the caching WinUtils performed. I'd really like > to see that included in these new utils. I copy & pasted the function from WinUtils. How we've lost the caching? The |version| variable will be calculated only once because it is designated with |static| in the function.

(In reply to Jim Mathies [:jimm] from comment #13) > Also looks like we've lost the caching WinUtils performed. I'd really like > to see that included in these new utils. Ah, are you saying about IsWindowsVersionOrLater()? It will cache |minVersion| and |maxVersion| to avoid calling VerifyVersionInfo() as much as possible. |minVersion| is the lowest possible version number of the system (inclusive). |maxVersion| is the highest possible version number of the system (exclusive). These values will be updated every time we could narrow the possible version range. I couldn't think of any better caching method because VerifyVersionInfo() will not return the exact version number directly.

(In reply to Masatoshi Kimura [:emk] from comment #23) > (In reply to Jim Mathies [:jimm] from comment #13) > > Also looks like we've lost the caching WinUtils performed. I'd really like > > to see that included in these new utils. > > Ah, are you saying about IsWindowsVersionOrLater()? It will cache > |minVersion| and |maxVersion| to avoid calling VerifyVersionInfo() as much > as possible. > |minVersion| is the lowest possible version number of the system > (inclusive). |maxVersion| is the highest possible version number of the > system (exclusive). These values will be updated every time we could narrow > the possible version range. > I couldn't think of any better caching method because VerifyVersionInfo() > will not return the exact version number directly. Sorry I missed that minVersion/maxVersion while looking it over.

I made a single .cpp file to increase efficiency of the caching as much as possible. Can compilers really combine the function even if it includes |static| variables? (I admit I don't familiar with C++ black magic so much.)

For a single occurrence, you can also use #pragma warning(suppress:4996), it's a little more compact than push/disable/pop.

Comment on attachment 817542 [details] [diff] [review] Suppress warning in netwerk/ Patrick: thanks for thinking of me; I'm very happy to delegate to you review of all patches to the UA code which do not affect policy (i.e. what string it sends when). Gerv

Indeed adding |inline| keyword did the trick.

Hi guys) This code https://bitbucket.org/AnyCPU/findversion was written in a matter of keeping score/ for fun) Enjoy)

This part does not depend on other parts and needed to build with VS2013. https://hg.mozilla.org/integration/mozilla-inbound/rev/185202a68dc9

Comment on attachment 818400 [details] [diff] [review] Introduce version test functions using VerifyVersionInfo(), v2 Review of attachment 818400 [details] [diff] [review]: ----------------------------------------------------------------- Look great overall :) I think you missed one in http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/common/uachelper.cpp and the declaration in the .h file. ::: xpcom/base/WindowsVersion.h @@ +9,5 @@ > +#include <windows.h> > + > +namespace mozilla > +{ > + inline bool nit: MOZ_INLINE.... except if the updater file I mentioned gives you problems with the nscore.h include. If it does, then just use inline everywhere and get rid of the nscore.h include.

BTW, where UACHelper::IsVistaOrLater is used? I couldn't find any callers. > review?(netzen@gmail.com) → feedback+ Who do I have to ask for an additional review?

(In reply to Masatoshi Kimura [:emk] from comment #32) > BTW, where UACHelper::IsVistaOrLater is used? I couldn't find any callers. > > > review?(netzen@gmail.com) → feedback+ > > Who do I have to ask for an additional review? Great it doesn't look like it is used, so just remove it. I didn't r+ yet because I wanted to see what the next patch would be fixing the IsVistaOrLater in UACHelper. Since you're just removing it though I'll r+ since that's pretty trivial to remove :)

(In reply to Brian R. Bondy [:bbondy] from comment #31) > nit: MOZ_INLINE.... except if the updater file I mentioned gives you > problems with the nscore.h include. If it does, then just use inline > everywhere and get rid of the nscore.h include. I'd rather get rid of MOZ_INLINE from our tree. Filed bug 928210.

np

Try runs: https://tbpl.mozilla.org/?tree=Try&rev=f315c9e73043 https://tbpl.mozilla.org/?tree=Try&rev=df70bae708cf

Looks like the b-c orange is not my fault: https://tbpl.mozilla.org/?rev=4e7d1e2c93a6 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2eb79e83b3

Comment on attachment 817545 [details] [diff] [review] Replace WinUtils::GetWindowsVersion() and GetWindowsServicePackVersion() Review of attachment 817545 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Masatoshi Kimura [:emk] from comment #22) > (In reply to Jim Mathies [:jimm] from comment #12) > > ::: widget/windows/nsLookAndFeel.cpp > > @@ +26,5 @@ > > > + WIN8_VERSION = 0x602, > > > + WIN8_1_VERSION = 0x603 > > > +}; > > > + > > > +static WinVersion GetWindowsVersion() > > > > Can we move this into WindowsVersion.h, but modify it such that it works > > like the WinUtils helpers we had that allow value comparisons? > > I would like to discourage the use of GetVersionEx as much as possible. If I > moved the function to a public header, people may find the function and > start to use it. I'm not saying we should use GetVersionEx, I'm suggesting we keep using a value comparison helper available. ::: content/media/wmf/WMFUtils.cpp @@ +370,2 @@ > HRESULT > MFStartup() The check here doesn't match up, is this correct? if (WinUtils::GetWindowsVersion() == WinUtils::VISTA_VERSION) vs. f (!IsWin7OrLater())

(In reply to Jim Mathies [:jimm] from comment #40) > I'm not saying we should use GetVersionEx, I'm suggesting we keep using a > value comparison helper available. Sorry, I don't parse your suggestion. What "value comparison helper" means? GetWindowsVersion() and GetWindowsServicePackVersion() do not compare the version. They will just return the version value. And it is unimplementable using VerifyVersionInfo() unless doing brute-force bisect (such as comment #29). I didn't remove any other functions from WinUtils. > ::: content/media/wmf/WMFUtils.cpp > @@ +370,2 @@ > > HRESULT > > MFStartup() > > The check here doesn't match up, is this correct? > > if (WinUtils::GetWindowsVersion() == WinUtils::VISTA_VERSION) > > vs. > > f (!IsWin7OrLater()) WMF is unavaliable on WinXP anyway. So WMFDecoder::Init() will always fail before calling this function.

Comment on attachment 817540 [details] [diff] [review] Replace gfxWindowsPlatform::WindowsOSVersion() Bas is unresponsive.

Comment on attachment 817540 [details] [diff] [review] Replace gfxWindowsPlatform::WindowsOSVersion() Bas is actually a much better reviewer than me for this. Let's try again pinging him ;-)

Bas, if you're not checking your review requests, here's hoping that you are checking your needinfo requests!

Bas is currently on vacation, he should be back on next Monday.

Comment on attachment 817540 [details] [diff] [review] Replace gfxWindowsPlatform::WindowsOSVersion() Review of attachment 817540 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxWindowsPlatform.cpp @@ -629,5 @@ > -#ifdef CAIRO_HAS_DWRITE_FONT > -#define WINDOWS7_RTM_BUILD 7600 > - > -static bool > -AllowDirectWrite() Can we just remove this?

(In reply to Bas Schouten (:bas.schouten) from comment #46) > ::: gfx/thebes/gfxWindowsPlatform.cpp > @@ -629,5 @@ > > -#ifdef CAIRO_HAS_DWRITE_FONT > > -#define WINDOWS7_RTM_BUILD 7600 > > - > > -static bool > > -AllowDirectWrite() > > Can we just remove this? Preview versions of Windows 7 are all expired now. No one would hit this anymore.

Comment on attachment 817540 [details] [diff] [review] Replace gfxWindowsPlatform::WindowsOSVersion() Review of attachment 817540 [details] [diff] [review]: ----------------------------------------------------------------- I suppose I can live with that explanation :)

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=ac5cf3efa3df https://hg.mozilla.org/integration/mozilla-inbound/rev/2e503bef99d7 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1ed54cd96c https://hg.mozilla.org/integration/mozilla-inbound/rev/34be216f6d0e https://hg.mozilla.org/integration/mozilla-inbound/rev/a148cd4445aa https://hg.mozilla.org/integration/mozilla-inbound/rev/01e15a1abbb1

https://hg.mozilla.org/mozilla-central/rev/2e503bef99d7 https://hg.mozilla.org/mozilla-central/rev/6c1ed54cd96c https://hg.mozilla.org/mozilla-central/rev/34be216f6d0e https://hg.mozilla.org/mozilla-central/rev/a148cd4445aa https://hg.mozilla.org/mozilla-central/rev/01e15a1abbb1