[ChromeDriver] Implementing PerfLoggingPrefs for perf log

chrome/test/chromedriver/capabilities.h

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_TEST_CHROMEDRIVER_CAPABILITIES_H_
 #define CHROME_TEST_CHROMEDRIVER_CAPABILITIES_H_
 
 #include <map>
 #include <set>
 #include <string>
@@ skipping 42 matching lines @@
   void AppendToCommandLine(base::CommandLine* command) const;
   std::string ToString() const;
 
  private:
   typedef std::map<std::string, NativeString> SwitchMap;
   SwitchMap switch_map_;
 };
 
 typedef std::map<std::string, Log::Level> LoggingPrefs;
 
+struct PerfLoggingPrefs {
+  PerfLoggingPrefs();
+  ~PerfLoggingPrefs();

[samuong, 2014/08/04 06:08:22]

This probably doesn't matter since no one will ever subclass this, but should we
just make the destructor virtual anyway?

[johnmoore, 2014/08/04 17:37:57]

For consistency, I think we shouldn't, since neither the Capabilities nor
Switches classes have virtual destructors, since like you said they have no
reason to be subclassed.

+
+  // We must distinguish between a log domain being set by default and being
+  // explicitly set. Otherwise, |PerformanceLogger| could only handle 3 of 4
+  // possible combinations (tracing enabled/disabled + Timeline on/off).
+  enum InspectorDomainStatus {
+    kDefaultEnabled,  // enabled by default
+    kDefaultDisabled,  // disabled by default
+    kExplicitlyEnabled,  // explicitly enabled
+    kExplicitlyDisabled  // explicitly disabled

[samuong, 2014/08/04 06:08:22]

I think people will be able to figure out what the enum names mean even without
these four comments :)

[johnmoore, 2014/08/04 17:37:57]

Done.

+  };
+
+  // True if |domain_status| is kDefaultEnabled or kExplicitlyEnabled.
+  static bool IsEnabled(const InspectorDomainStatus& domain_status);

[samuong, 2014/08/04 06:08:22]

structs aren't usually supposed to have any logic in them, and since this is
static anyway you could easily just pull this out into a non-member function

[johnmoore, 2014/08/04 17:37:58]

Done.

+
+  InspectorDomainStatus network;
+  InspectorDomainStatus page;
+  InspectorDomainStatus timeline;
+
+  std::string trace_categories;  // Non-empty string enables tracing.
+};
+
 struct Capabilities {
   Capabilities();
   ~Capabilities();
 
   // Return true if remote host:port session is to be used.
   bool IsRemoteBrowser() const;
 
   // Return true if android package is specified.
   bool IsAndroid() const;
 
@@ skipping 34 matching lines @@
 
   scoped_ptr<base::DictionaryValue> local_state;
 
   std::string log_path;
 
   LoggingPrefs logging_prefs;
 
   // If set, enable minidump for chrome crashes and save to this directory.
   std::string minidump_path;
 
+  PerfLoggingPrefs perf_logging_prefs;
+
   scoped_ptr<base::DictionaryValue> prefs;
 
   Switches switches;
 };
 
 #endif  // CHROME_TEST_CHROMEDRIVER_CAPABILITIES_H_