add "did it run on GPU" check to frame_rate_tests.cc using AutomationProxy tracing feature

chrome/test/perf/frame_rate/frame_rate_tests.cc

Index: chrome/test/perf/frame_rate/frame_rate_tests.cc
diff --git a/chrome/test/perf/frame_rate/frame_rate_tests.cc b/chrome/test/perf/frame_rate/frame_rate_tests.cc
index 0df47c95942f4a820cddd0e4c42d1cf5dd6eac57..f0b218305f5781d266cb5ae66a3e66f7db46b4cb 100644
--- a/chrome/test/perf/frame_rate/frame_rate_tests.cc
+++ b/chrome/test/perf/frame_rate/frame_rate_tests.cc
@@ -8,13 +8,16 @@
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #include "base/test/test_timeouts.h"
+#include "base/test/trace_event_analyzer.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/test/automation/automation_proxy.h"
 #include "chrome/test/automation/tab_proxy.h"
 #include "chrome/test/ui/javascript_test_util.h"
 #include "chrome/test/ui/ui_perf_test.h"
 #include "net/base/net_util.h"
+#include "ui/gfx/gl/gl_implementation.h"
 #include "ui/gfx/gl/gl_switches.h"
 
 namespace {
@@ -53,6 +56,10 @@ class FrameRateTest
     disable_accelerated_compositing_ = false;
   }
 
+  virtual bool IsReferenceBuild() const {
+    return (GetParam() & kUseReferenceBuild);
+  }
+
   virtual FilePath GetDataPath(const std::string& name) {
     // Make sure the test data is checked out.
     FilePath test_path;
@@ -69,7 +76,7 @@ class FrameRateTest
   }
 
   virtual void SetUp() {
-    if (GetParam() & kUseReferenceBuild) {
+    if (IsReferenceBuild()) {
       UseReferenceBuild();
     }
 
@@ -92,6 +99,7 @@ class FrameRateTest
   }
 
   void RunTest(const std::string& name) {
+    using namespace trace_analyzer;

[nduca, 2011/11/01 19:17:49]

why using here only? That feels anomalous.

[jbates, 2011/11/03 00:51:07]

This is actually done in lots of other chromium code (helps to reduce namespace
clutter where symbols aren't used). I don't mind either way though.

     FilePath test_path = GetDataPath(name);
     ASSERT_TRUE(file_util::DirectoryExists(test_path))
         << "Missing test directory: " << test_path.value();
@@ -109,6 +117,12 @@ class FrameRateTest
         GURL("javascript:__make_body_composited();")));
     }
 
+    // TODO(jbates): remove this check when ref builds are updated.
+    if (!IsReferenceBuild()) {

[nduca, 2011/11/01 19:17:49]

Why not just update the ref builds now?

[jbates, 2011/11/03 00:51:07]

Either this needs to be split up into the test changes and the trace event
additions or we commit this with these checks, update ref build, then remove
checks. Either way, it consists of three steps.

In general, all tests will have this tedious 3-step problem:
1. add trace events.
2. update ref build.
3. add/update test.

That, or they need to be designed to handle the case of missing events until the
ref build is updated.

I'm not sure what the best solution is, it seems icky with the reference build
involved no matter what.

[jbates, 2011/11/04 22:30:39]

Actually, thinking this over more... we probably won't need to many more trace
events for performance tests, since we can test a lot of perf characteristics
with only a few basic rendering and input trace events. I think we should get
this in with ref build checks, update the bots, and then do a followup patch to
remove the checks. The reason I think that's the best solution is:

It's cleaner to write and commit the test + the new trace events in one CL, even
if it requires some code that is conditional on IsReferenceBuild, because the
test can't be verified without the trace events present. If they go in in
stages, it's more likely that the trace events are discovered to be incorrect
for some platform after commit or during the try-bot run. I also think it's a
more efficient development flow to decouple the reference build update from
writing new tests.

+      // Start tracing category "test_frame_rate_tests".
+      ASSERT_TRUE(automation()->BeginTracing("test_frame_rate_tests"));
+    }
+
     // Block until initialization completes.
     ASSERT_TRUE(WaitUntilJavaScriptCondition(
         tab, L"", L"window.domAutomationController.send(__initialized);",
@@ -122,6 +136,13 @@ class FrameRateTest
         tab, L"", L"window.domAutomationController.send(!__running_all);",
         TestTimeouts::large_test_timeout_ms()));
 
+    std::string json_events;
+    // TODO(jbates): remove this check when ref builds are updated.
+    if (!IsReferenceBuild()) {
+      // Stop tracing.
+      ASSERT_TRUE(automation()->EndTracing(&json_events));
+    }
+
     // Read out the results.
     std::wstring json;
     ASSERT_TRUE(tab->ExecuteAndExtractString(
@@ -139,8 +160,35 @@ class FrameRateTest
     ASSERT_TRUE(results.find("means") != results.end());
     ASSERT_TRUE(results.find("sigmas") != results.end());
 
+    std::string gpu_suffix;
+    // TODO(jbates): remove this check when ref builds are updated.
+    if (!IsReferenceBuild()) {
+      // Check trace for GPU accleration.

[nduca, 2011/11/01 19:17:49]

I can haz new function?

[jbates, 2011/11/03 00:51:07]

Done.

+      scoped_ptr<TraceAnalyzer> analyzer(TraceAnalyzer::Create(json_events));
+      const TraceEvent* gpu_event = analyzer->FindOneEvent(
+          Query(EVENT_NAME) == Query::String("SwapBuffers") &&
+          Query(EVENT_HAS_ARG, "GLImpl"));
+      gfx::GLImplementation gl_impl = gfx::kGLImplementationNone;
+      if (gpu_event)
+        gpu_event->GetArgAsIntType("GLImpl", &gl_impl);
+      bool is_GPU = (gl_impl == gfx::kGLImplementationDesktopGL ||
+                     gl_impl == gfx::kGLImplementationEGLGLES2);
+
+      // Verify that sanity-check DoDeferredUpdate event exists.
+      ASSERT_TRUE(analyzer->FindOneEvent(Query(EVENT_NAME) ==
+                                         Query::String("DoDeferredUpdate")));
+
+      // If GPU acceleration is requested, set the appropriate suffix.
+      // _swfb means software fallback (GPU acceleration requested but not
+      // available).
+      if (is_GPU)
+        gpu_suffix = "_gpu";
+      else if (GetParam() & kMakeBodyComposited)
+        gpu_suffix = "_swfb";

[nduca, 2011/11/01 19:17:49]

Will we still have the _comp suffix? WE have an explosion of suffixes here and I
think that means we're doing something wrong.

[jbates, 2011/11/03 00:51:07]

I got rid of swfb, because we can infer that from a missing _gpu. I think we
still need _comp as long as there is a chance of mesa or other software GL API
(in which case we would see _comp as opposed to _comp_gpu).

+    }
+
     std::string trace_name = "fps";
-    trace_name += GetSuffixForTestFlags(GetParam());
+    trace_name += GetSuffixForTestFlags(GetParam()) + gpu_suffix;

[nduca, 2011/11/01 19:17:49]

Merge this with the GetSuffixForTest bit? E.g. GetSuffixForTest(GetParams(),
did_use_gpu)

[jbates, 2011/11/03 00:51:07]

Done.

     printf("GESTURES %s: %s= [%s] [%s] [%s]\n", name.c_str(),
                                                 trace_name.c_str(),
                                                 results["gestures"].c_str(),