Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1784623003:
Add histograms to compare GetVersionEx() with VerQueryValue() of kernel32 (Closed)
|
Created:
4 years, 9 months ago by scottmg Modified:
4 years, 9 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org, grt+watch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histograms to compare GetVersionEx() with VerQueryValue() of kernel32
Checking VerQueryValue() of kernel32 reports the "real" OS, rather than
the potentially shimmed one that GetVersionEx() reports.
Normally it's better to use GetVersionEx() because that'll determine the
APIs that are available and how they behave. However, we'd like to know
if there are a substantial percentage of users in compatibility mode, as
there have been complaints of users seeing the "XP and Vista are no
longer supported" infobar on Windows 7.
R=asvitkine@chromium.org, robliao@chromium.org, wfh@chromium.org, nick@chromium.org
BUG=581499
Committed: https://crrev.com/d68b1e1c89ab3d8055046883b1bf726084673959
Cr-Commit-Position: refs/heads/master@{#380793}
Patch Set 1 #
Total comments: 8
Patch Set 2 : . #
Total comments: 23
Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 15
Patch Set 6 : . #
Total comments: 6
Patch Set 7 : . #Patch Set 8 : . #
Messages
Total messages: 28 (8 generated)
Description was changed from ========== Add histograms to compare GetVersionEx() with VerQueryValue() of kernel32 R=asvitkine@chromium.org, robliao@chromium.org, wfh@chromium.org BUG=581499 ========== to ========== Add histograms to compare GetVersionEx() with VerQueryValue() of kernel32 R=asvitkine@chromium.org, robliao@chromium.org, wfh@chromium.org BUG=581499 ==========
scottmg@chromium.org changed reviewers: + nick@chromium.org
Description was changed from ========== Add histograms to compare GetVersionEx() with VerQueryValue() of kernel32 R=asvitkine@chromium.org, robliao@chromium.org, wfh@chromium.org BUG=581499 ========== to ========== Add histograms to compare GetVersionEx() with VerQueryValue() of kernel32 Checking VerQueryValue() of kernel32 reports the "real" OS, rather than the potentially shimmed one that GetVersionEx() reports. Normally it's better to use GetVersionEx() because that'll determine the APIs that are available and how they behave. However, we'd like to know if there are a substantial percentage of users in compatibility mode, as there have been complaints of users seeing the "XP and Vista are no longer supported" infobar on Windows 7. R=asvitkine@chromium.org, robliao@chromium.org, wfh@chromium.org, nick@chromium.org BUG=581499 ==========
asvitkine: histograms.xml nick: content/browser/ robliao and wfh: base/win
test? even a test that just compares the two versions would catch when we accidently forget to update our manifest file at a a later date. I understand actually testing compatibility mode is probably quite hard to automate though... did you do manual testing? https://codereview.chromium.org/1784623003/diff/1/base/win/windows_version.cc File base/win/windows_version.cc (right): https://codereview.chromium.org/1784623003/diff/1/base/win/windows_version.cc... base/win/windows_version.cc:22: DWORD size = ::GetFileVersionInfoSize(path.value().c_str(), nullptr); maybe assert IO is allowed for thread, given this does a lot of IO. https://codereview.chromium.org/1784623003/diff/1/base/win/windows_version.h File base/win/windows_version.h (right): https://codereview.chromium.org/1784623003/diff/1/base/win/windows_version.h#... base/win/windows_version.h:89: Version kernel32_version() const { return kernel32_version_; } how are none of these functions commented...? :) https://codereview.chromium.org/1784623003/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1784623003/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.cc:706: base::win::OSInfo::GetInstance()->kernel32_version(), given this call can loadlibrary, should it be moved off the hotpath for startup e.g. into a posttaskwithdelay? https://codereview.chromium.org/1784623003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1784623003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:58389: +<histogram name="Windows.InCompatibilityMode"> I think you need a enum="Boolean*" type e.g. BooleanEnabled or just Boolean.
Yes, I manually tested on Win10 and it correctly reports when in compat mode for Win7 (and when not). Added a unittest that confirms we've updated our manifest. https://codereview.chromium.org/1784623003/diff/1/base/win/windows_version.cc File base/win/windows_version.cc (right): https://codereview.chromium.org/1784623003/diff/1/base/win/windows_version.cc... base/win/windows_version.cc:22: DWORD size = ::GetFileVersionInfoSize(path.value().c_str(), nullptr); On 2016/03/09 23:02:50, Will Harris wrote: > maybe assert IO is allowed for thread, given this does a lot of IO. I don't think I can do that because there's version checks before base::ThreadRestrictions is initialized. Hmm. I guess I could make the kernel32_version an on-demand+cached, rather than in the ctor so that it's pushed later? It seems a bit pointless since we're only using it with kernel32. But.. OK. Done. https://codereview.chromium.org/1784623003/diff/1/base/win/windows_version.h File base/win/windows_version.h (right): https://codereview.chromium.org/1784623003/diff/1/base/win/windows_version.h#... base/win/windows_version.h:89: Version kernel32_version() const { return kernel32_version_; } On 2016/03/09 23:02:50, Will Harris wrote: > how are none of these functions commented...? :) Doesn't seem like comments would be very useful. https://codereview.chromium.org/1784623003/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1784623003/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.cc:706: base::win::OSInfo::GetInstance()->kernel32_version(), On 2016/03/09 23:02:50, Will Harris wrote: > given this call can loadlibrary, should it be moved off the hotpath for startup > e.g. into a posttaskwithdelay? Done. (It wouldn't have done IO here before, but now it will since Kernel32Version() retrieves on demand) https://codereview.chromium.org/1784623003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1784623003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:58389: +<histogram name="Windows.InCompatibilityMode"> On 2016/03/09 23:02:50, Will Harris wrote: > I think you need a enum="Boolean*" type e.g. BooleanEnabled or just Boolean. I copied from some other bools, dunno. Maybe it just wouldn't have a "nice" enum name on the dashboard? Anyway, done.
https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1353: void BrowserMainLoop::RecordWindowsVersionInformation() { Does this instrumentation belong in content or chrome? Did you consider putting this in chrome_browser_main_win.cc instead?
https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1353: void BrowserMainLoop::RecordWindowsVersionInformation() { On 2016/03/10 18:19:27, ncarter wrote: > Does this instrumentation belong in content or chrome? > > Did you consider putting this in chrome_browser_main_win.cc instead? Also, perhaps chrome_browser_main_extra_parts_metrics.cc is an even better fit?
On 2016/03/10 18:21:40, ncarter wrote: > https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... > File content/browser/browser_main_loop.cc (right): > > https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... > content/browser/browser_main_loop.cc:1353: void > BrowserMainLoop::RecordWindowsVersionInformation() { > On 2016/03/10 18:19:27, ncarter wrote: > > Does this instrumentation belong in content or chrome? > > > > Did you consider putting this in chrome_browser_main_win.cc instead? > > Also, perhaps chrome_browser_main_extra_parts_metrics.cc is an even better fit? why not chrome_browser_main_extra_parts_metrics_win.cc? :)
Windows code looks good in general. Some nits. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... File base/win/windows_version.cc (right): https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:39: UINT ffi_size; Nit: Given that we don't actually care about this, you could go with UINT ignored; https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:98: VS_FIXEDFILEINFO ffi; Nit: fixed_file_info. This looks very hungarian :-).
grt@chromium.org changed reviewers: + grt@chromium.org
drive-by https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... File base/win/windows_version.cc (right): https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:21: bool GetModuleVersionAndType(const base::FilePath& path, i propose avoiding the FilePath for this private function. it's only used to extract the string it's constructed with. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:28: << "GetFileVersionInfoSize: " << base::UTF16ToUTF8(path.value()); in the future, base::UTF16ToUTF8 isn't needed in cases like this; path.value() works by itself. for cross-platformness, i believe path.AsUTF8Unsafe() is the right choice when you do need to turn a path into UTF-8. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:59: base::win::Version MajorMinorBuildToVersion(int major, int minor, int build) { is base::win:: needed to disambiguate? this is within the namespace already. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:97: const wchar_t kSystemDll[] = L"kernel32.dll"; nit: static const wchar_t to avoid the weight of putting the string on the stack. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:99: if (::GetModuleVersionAndType(base::FilePath(kSystemDll), &ffi)) { can you use FileVersionInfoWin to get rid of the duplication of code here? https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:100: const int major = ffi.dwFileVersionMS >> 16; HIWORD and LOWORD? https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... File base/win/windows_version_unittest.cc (right): https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version_unittest.cc:5: #include <windows.h> iwyu here and below https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1356: base::win::VERSION_WIN_LAST); please add a doc comment to enum Version in windows_version.h stating that it's used for a histogram and values must not be reordered. while you're at it, add an explicit " = N" value to each element so that this is crystal clear.
Thanks https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... File base/win/windows_version.cc (right): https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:21: bool GetModuleVersionAndType(const base::FilePath& path, On 2016/03/10 19:43:44, grt (very slow) wrote: > i propose avoiding the FilePath for this private function. it's only used to > extract the string it's constructed with. Done. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:28: << "GetFileVersionInfoSize: " << base::UTF16ToUTF8(path.value()); On 2016/03/10 19:43:44, grt (very slow) wrote: > in the future, base::UTF16ToUTF8 isn't needed in cases like this; path.value() > works by itself. It works in an ugly way (injecting into operator<< into std::), so I'd rather be explicit. > for cross-platformness, i believe path.AsUTF8Unsafe() is the > right choice when you do need to turn a path into UTF-8. Noted, thanks. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:39: UINT ffi_size; On 2016/03/10 19:28:51, robliao wrote: > Nit: Given that we don't actually care about this, you could go with UINT > ignored; Done. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:59: base::win::Version MajorMinorBuildToVersion(int major, int minor, int build) { On 2016/03/10 19:43:44, grt (very slow) wrote: > is base::win:: needed to disambiguate? this is within the namespace already. Done. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:97: const wchar_t kSystemDll[] = L"kernel32.dll"; On 2016/03/10 19:43:44, grt (very slow) wrote: > nit: static const wchar_t to avoid the weight of putting the string on the > stack. Done. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:98: VS_FIXEDFILEINFO ffi; On 2016/03/10 19:28:51, robliao wrote: > Nit: fixed_file_info. This looks very hungarian :-). Done. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:99: if (::GetModuleVersionAndType(base::FilePath(kSystemDll), &ffi)) { On 2016/03/10 19:43:44, grt (very slow) wrote: > can you use FileVersionInfoWin to get rid of the duplication of code here? Done. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version.cc:100: const int major = ffi.dwFileVersionMS >> 16; On 2016/03/10 19:43:44, grt (very slow) wrote: > HIWORD and LOWORD? Done. https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... File base/win/windows_version_unittest.cc (right): https://codereview.chromium.org/1784623003/diff/20001/base/win/windows_versio... base/win/windows_version_unittest.cc:5: #include <windows.h> On 2016/03/10 19:43:44, grt (very slow) wrote: > iwyu here and below Done. https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1353: void BrowserMainLoop::RecordWindowsVersionInformation() { On 2016/03/10 18:21:40, ncarter wrote: > On 2016/03/10 18:19:27, ncarter wrote: > > Does this instrumentation belong in content or chrome? > > > > Did you consider putting this in chrome_browser_main_win.cc instead? > > Also, perhaps chrome_browser_main_extra_parts_metrics.cc is an even better fit? Done. https://codereview.chromium.org/1784623003/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1356: base::win::VERSION_WIN_LAST); On 2016/03/10 19:43:44, grt (very slow) wrote: > please add a doc comment to enum Version in windows_version.h stating that it's > used for a histogram and values must not be reordered. while you're at it, add > an explicit " = N" value to each element so that this is crystal clear. Done.
lgtm w/ nits https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... File base/win/windows_version.cc (right): https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:63: base::win::Version GetVersionFromKernel32() { base::win:: here, too https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:65: reinterpret_cast<FileVersionInfoWin*>( barf https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:68: if (file_version_info.get()) { nit: omit ".get()" https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... File base/win/windows_version_unittest.cc (right): https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version_unittest.cc:5: #include "base/win/windows_version.h" nit: blank line between this header and those below (the .h for the thing under test always comes first and all by itself); see https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes.
https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... File base/win/windows_version.cc (right): https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:9: #include "base/file_version_info_win.h" FileVersionInfoWin seems asks for more information than we care about, and fails if translation info isn't available (this is probably okay). Something to note if for some reason we get failures obtaining kernel32 version info. https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:65: reinterpret_cast<FileVersionInfoWin*>( On 2016/03/11 16:13:42, grt (very slow) wrote: > barf Shouldn't a static_cast work here? That's done here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/enu... https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_version.h File base/win/windows_version.h (right): https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.h:21: // syntactic sugar reasons; see the declaration of GetVersion() below. NOTE: Nit: This NOTE should probably still remain on the next line. https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.h:22: // Keep these in order so callers can do things like "if "if should be on the next line.
lgtm
lgtm % comments https://codereview.chromium.org/1784623003/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/1784623003/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:154: base::win::OSInfo::GetInstance()->Kernel32Version()); Nit: make a local var for base::win::OSInfo::GetInstance() since you use it 4 times above, so it will make the code a bit more concise. https://codereview.chromium.org/1784623003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1784623003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58474: + The Windows version (base::win::Version) as reported by GetVersionEx(). Nit: For all of these, mention when this is logged. (Startup?) https://codereview.chromium.org/1784623003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58478: +<histogram name="Windows.InCompatibilityMode" enum="Boolean"> Nit: Use a more specific boolean, like BooleanMatches (add it if it doesn't exist yet).
https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... File base/win/windows_version.cc (right): https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:9: #include "base/file_version_info_win.h" On 2016/03/11 17:04:46, robliao wrote: > FileVersionInfoWin seems asks for more information than we care about, and fails > if translation info isn't available (this is probably okay). Something to note > if for some reason we get failures obtaining kernel32 version info. Yeah, I'm not sure if it's better or not, but it's less code in this file at least. I think for kernel32 it should be fine. https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:63: base::win::Version GetVersionFromKernel32() { On 2016/03/11 16:13:42, grt (very slow) wrote: > base::win:: here, too Done. https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:65: reinterpret_cast<FileVersionInfoWin*>( On 2016/03/11 17:04:46, robliao wrote: > On 2016/03/11 16:13:42, grt (very slow) wrote: > > barf > > Shouldn't a static_cast work here? That's done here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/enu... Done. https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.cc:68: if (file_version_info.get()) { On 2016/03/11 16:13:42, grt (very slow) wrote: > nit: omit ".get()" Done. https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_version.h File base/win/windows_version.h (right): https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.h:21: // syntactic sugar reasons; see the declaration of GetVersion() below. NOTE: On 2016/03/11 17:04:47, robliao wrote: > Nit: This NOTE should probably still remain on the next line. Done. https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version.h:22: // Keep these in order so callers can do things like "if On 2016/03/11 17:04:47, robliao wrote: > "if > should be on the next line. Done. https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... File base/win/windows_version_unittest.cc (right): https://codereview.chromium.org/1784623003/diff/80001/base/win/windows_versio... base/win/windows_version_unittest.cc:5: #include "base/win/windows_version.h" On 2016/03/11 16:13:42, grt (very slow) wrote: > nit: blank line between this header and those below (the .h for the thing under > test always comes first and all by itself); see > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes. Done. https://codereview.chromium.org/1784623003/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/1784623003/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:154: base::win::OSInfo::GetInstance()->Kernel32Version()); On 2016/03/11 20:16:51, Alexei Svitkine (very slow) wrote: > Nit: make a local var for base::win::OSInfo::GetInstance() since you use it 4 > times above, so it will make the code a bit more concise. Done. https://codereview.chromium.org/1784623003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1784623003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58474: + The Windows version (base::win::Version) as reported by GetVersionEx(). On 2016/03/11 20:16:51, Alexei Svitkine (very slow) wrote: > Nit: For all of these, mention when this is logged. (Startup?) Done. https://codereview.chromium.org/1784623003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58478: +<histogram name="Windows.InCompatibilityMode" enum="Boolean"> On 2016/03/11 20:16:51, Alexei Svitkine (very slow) wrote: > Nit: Use a more specific boolean, like BooleanMatches (add it if it doesn't > exist yet). Done.
scottmg@chromium.org changed reviewers: + thakis@chromium.org
+thakis for base/ build files.
lgtm
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, grt@chromium.org, asvitkine@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1784623003/#ps140001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1784623003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1784623003/140001
Non-owner LGTM
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add histograms to compare GetVersionEx() with VerQueryValue() of kernel32 Checking VerQueryValue() of kernel32 reports the "real" OS, rather than the potentially shimmed one that GetVersionEx() reports. Normally it's better to use GetVersionEx() because that'll determine the APIs that are available and how they behave. However, we'd like to know if there are a substantial percentage of users in compatibility mode, as there have been complaints of users seeing the "XP and Vista are no longer supported" infobar on Windows 7. R=asvitkine@chromium.org, robliao@chromium.org, wfh@chromium.org, nick@chromium.org BUG=581499 ========== to ========== Add histograms to compare GetVersionEx() with VerQueryValue() of kernel32 Checking VerQueryValue() of kernel32 reports the "real" OS, rather than the potentially shimmed one that GetVersionEx() reports. Normally it's better to use GetVersionEx() because that'll determine the APIs that are available and how they behave. However, we'd like to know if there are a substantial percentage of users in compatibility mode, as there have been complaints of users seeing the "XP and Vista are no longer supported" infobar on Windows 7. R=asvitkine@chromium.org, robliao@chromium.org, wfh@chromium.org, nick@chromium.org BUG=581499 Committed: https://crrev.com/d68b1e1c89ab3d8055046883b1bf726084673959 Cr-Commit-Position: refs/heads/master@{#380793} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/d68b1e1c89ab3d8055046883b1bf726084673959 Cr-Commit-Position: refs/heads/master@{#380793} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
