Improve GPU software rendering list histograms.

content/browser/gpu/gpu_data_manager.cc

Index: content/browser/gpu/gpu_data_manager.cc
===================================================================
--- content/browser/gpu/gpu_data_manager.cc	(revision 96780)
+++ content/browser/gpu/gpu_data_manager.cc	(working copy)
@@ -12,6 +12,9 @@
 #include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
 #include "base/string_number_conversions.h"
+#include "base/sys_info.h"
+#include "base/values.h"
+#include "base/version.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/gpu/gpu_blacklist.h"
 #include "content/browser/gpu/gpu_process_host.h"
@@ -79,6 +82,36 @@
 
 #endif  // OS_WIN
 
+std::string GetOSString() {
+  std::string rt;
+#if defined(OS_CHROMEOS)
+  rt = "ChromeOS";
+#elif defined(OS_WIN)
+  rt = "Win";
+  std::string version_str = base::SysInfo::OperatingSystemVersion();
+  size_t pos = version_str.find_first_not_of("0123456789.");
+  if (pos != std::string::npos)
+    version_str = version_str.substr(0, pos);
+  scoped_ptr<Version> os_version(Version::GetVersionFromString(version_str));
+  if (os_version.get() && os_version->components().size() >= 2) {
+    const std::vector<uint16>& version_numbers = os_version->components();
+    if (version_numbers[0] == 5)
+      rt += "XP";
+    else if (version_numbers[0] == 6 && version_numbers[1] == 0)
+      rt += "Vista";
+    else if (version_numbers[0] == 6 && version_numbers[1] == 1)
+      rt += "7";
+  }
+#elif defined(OS_LINUX)
+  rt = "Linux";
+#elif defined(OS_MACOSX)
+  rt = "Mac";
+#else
+  rt = "UnknownOS";
+#endif
+  return rt;
+}
+
 }  // namespace anonymous
 
 GpuDataManager::GpuDataManager()
@@ -361,24 +394,51 @@
         GpuBlacklist::kOsAny, NULL, gpu_info_);
   }
 
+  // Notify clients that GpuInfo state has changed
+  RunGpuInfoUpdateCallbacks();
+
+  const GpuFeatureFlags::GpuFeatureType kGpuFeatures[] = {
+      GpuFeatureFlags::kGpuFeatureAccelerated2dCanvas,
+      GpuFeatureFlags::kGpuFeatureAcceleratedCompositing,
+      GpuFeatureFlags::kGpuFeatureWebgl
+  };
+  const size_t kNumFeatures =
+      sizeof(kGpuFeatures) / sizeof(GpuFeatureFlags::GpuFeatureType);
+
+  uint32 flags = gpu_feature_flags_.flags();
   uint32 max_entry_id = gpu_blacklist->max_entry_id();
-  if (!gpu_feature_flags_.flags()) {
+  if (flags == 0) {

[vangelis, 2011/08/18 06:06:44]

shouldn't you also include here the case where flags == kDisableMultisampling ? 

[Zhenyao Mo, 2011/08/18 18:16:10]

We have an entry which counts the number of use-cases that multisampling is
disabled.  So we don't need to include the case here.  It should be easy to add
this allowed-case and the multisampling-disabled-case in the histogram.

In the histograms that counts allowed/disallowed gpu features, since we do it
per feature cases, so multisampling won't affect those histograms.

[vangelis, 2011/08/18 21:05:41]

Why do we then just explicitly skip the multisampling flag when you set the
value for features in the loop below?  It looks like you should be using
features = kGpuFeaturesAll to get results for multisampling blacklisting too.

[Zhenyao Mo, 2011/08/18 23:47:18]

Ah you are right.  Fixed.

     UMA_HISTOGRAM_ENUMERATION("GPU.BlacklistTestResultsPerEntry",
         0, max_entry_id + 1);
-    return;
+  } else {
+    std::vector<uint32> flag_entries;
+    uint32 features = 0;
+    // We don't use GpuFeatureFlags::kGpuFeatureAll because we want to leave
+    // disabled multisampling out.
+    for (size_t i = 0; i < kNumFeatures; ++i)
+      features |= kGpuFeatures[i];
+    gpu_blacklist->GetGpuFeatureFlagEntries(
+        static_cast<GpuFeatureFlags::GpuFeatureType>(features), flag_entries);
+    DCHECK_GT(flag_entries.size(), 0u);
+    for (size_t i = 0; i < flag_entries.size(); ++i) {
+      UMA_HISTOGRAM_ENUMERATION("GPU.BlacklistTestResultsPerEntry",
+          flag_entries[i], max_entry_id + 1);
+    }
   }
 
-  // Notify clients that GpuInfo state has changed
-  RunGpuInfoUpdateCallbacks();
-
-  // TODO(zmo): move histograming to GpuBlacklist::DetermineGpuFeatureFlags.
-  std::vector<uint32> flag_entries;
-  gpu_blacklist->GetGpuFeatureFlagEntries(
-      GpuFeatureFlags::kGpuFeatureAll, flag_entries);
-  DCHECK_GT(flag_entries.size(), 0u);
-  for (size_t i = 0; i < flag_entries.size(); ++i) {
-    UMA_HISTOGRAM_ENUMERATION("GPU.BlacklistTestResultsPerEntry",
-        flag_entries[i], max_entry_id + 1);
+  const std::string kOSString = GetOSString();
+  const std::string kGpuBlacklistFeatureHistogramNames[] = {
+      "GPU.BlacklistAccelerated2dCanvasTestResults" + kOSString,
+      "GPU.BlacklistAcceleratedCompositingTestResults" + kOSString,
+      "GPU.BlacklistWebglTestResults" + kOSString
+  };
+  for (size_t i = 0; i < kNumFeatures; ++i) {
+    // We can't use UMA_HISTOGRAM_ENUMERATION here because the same name is
+    // expected if the macro is used within a loop.
+    base::Histogram* histogram_pointer = base::LinearHistogram::FactoryGet(
+        kGpuBlacklistFeatureHistogramNames[i], 1, 2, 3,
+        base::Histogram::kUmaTargetedHistogramFlag);
+    histogram_pointer->Add(((flags & kGpuFeatures[i]) == 0) ? 0 : 1);

[vangelis, 2011/08/18 06:06:44]

Could the expression above be simplified as (flags & kGpuFeatures[i])  ? 
Potentially with a static cast to the expected integer value.

[Zhenyao Mo, 2011/08/18 18:16:10]

Done.

[Zhenyao Mo, 2011/08/18 20:12:28]

The Windows try bot fails to compile because of the implicit conversion from
bool to uint32.  So we have to cast it to bool then cast it to int again.

Maybe the original ((flags & kGpuFeatures[i]) == 0) ? 0 : 1 is easier to read
than two nested cast.

   }
 }