[ChromeDriver] Implementing PerfLoggingPrefs for perf log

[ChromeDriver] Implementing PerfLoggingPrefs for perf log

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287613

[johnmoore, 2014-07-29 20:37:59]

Hi Sam, Shuotao,

I have decided to break the last ChromeDriver tracing CL into two pieces: this
CL, which just implements the ChromeOptions parsing (and also fixes
ParseInspectorMessage per previous discussion), and a final CL, which should
only affect the PerformanceLogger implementation and tests. This way the reviews
can be more focused.

For reference, the tracing design document now reflects what we agreed on in our
ChromeDriver sync meeting a few weeks ago (in terms of ChromeOptions and
performance logging preferences). This CL follows that design specification.

As always, thanks for the reviews :)

[samuong, 2014-07-30 00:01:23]

The CQ bit was checked by samuong@chromium.org

[commit-bot: I haz the power, 2014-07-30 00:03:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnmoore@google.com/429693003/20001

[commit-bot: I haz the power, 2014-07-30 00:03:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-30 00:03:43]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[samuong, 2014-08-04 06:08:23]

Thanks John, comments are below.

Also, we should document the changes you're making to the capabilities
somewhere. In addition to the next code review, could you start a doc so that we
can review the documentation of this feature? It can be based on the following
sites page:

https://sites.google.com/a/chromium.org/chromedriver/logging/performance-log

Or let me know if you've already written one and I've missed it.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:311: return Status(kUnknownError, "must
be a boolean");
Please also include what parameter was set incorrectly, to make it easier for
the user to debug.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:316: }
Don't need the braces in this if-else statement.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:324: return Status(kUnknownError, "must
be a dictionary");
Please also include what parameter was set incorrectly, to make it easier for
the user to debug.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:525: trace_categories("") {}
the default constructor for std::string creates an empty string, so you can just
write trace_categories()

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:65: ~PerfLoggingPrefs();
This probably doesn't matter since no one will ever subclass this, but should we
just make the destructor virtual anyway?

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:74: kExplicitlyDisabled  // explicitly
disabled
I think people will be able to figure out what the enum names mean even without
these four comments :)

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:78: static bool IsEnabled(const
InspectorDomainStatus& domain_status);
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

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome_launcher.cc:222: if
(perf_logging_prefs.trace_categories != "") {
!perf_logging_prefs.trace_categories.empty()

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/performance_logger.cc (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/performance_logger.cc:40: if (prefs_.trace_categories
!= "") {
!prefs_.trace_categories.empty()

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/performance_logger.cc:64: if (prefs_.trace_categories
== "" || prefs_.timeline ==
use prefs_.trace_categories.empty()

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/performance_logger.h (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/performance_logger.h:32: explicit
PerformanceLogger(Log* log, const PerfLoggingPrefs& prefs);
don't need to make this explicit

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

Thanks for the review, Sam! I have addressed your comments.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:311: return Status(kUnknownError, "must
be a boolean");
On 2014/08/04 06:08:22, samuong wrote:
> Please also include what parameter was set incorrectly, to make it easier for
> the user to debug.

The parameter name will be appended to the status message by the for loop in
ParsePerfLoggingPrefs.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:316: }
On 2014/08/04 06:08:22, samuong wrote:
> Don't need the braces in this if-else statement.

Done.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:324: return Status(kUnknownError, "must
be a dictionary");
On 2014/08/04 06:08:22, samuong wrote:
> Please also include what parameter was set incorrectly, to make it easier for
> the user to debug.

Similar to above, the parameter name will be appended to the status message by
the for loop in ParseChromeOptions.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:525: trace_categories("") {}
On 2014/08/04 06:08:22, samuong wrote:
> the default constructor for std::string creates an empty string, so you can
just
> write trace_categories()

Done.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:65: ~PerfLoggingPrefs();
On 2014/08/04 06:08:22, samuong wrote:
> This probably doesn't matter since no one will ever subclass this, but should
we
> just make the destructor virtual anyway?

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.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:74: kExplicitlyDisabled  // explicitly
disabled
On 2014/08/04 06:08:22, samuong wrote:
> I think people will be able to figure out what the enum names mean even
without
> these four comments :)

Done.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.h:78: static bool IsEnabled(const
InspectorDomainStatus& domain_status);
On 2014/08/04 06:08:22, samuong wrote:
> 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

Done.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome_launcher.cc:222: if
(perf_logging_prefs.trace_categories != "") {
On 2014/08/04 06:08:22, samuong wrote:
> !perf_logging_prefs.trace_categories.empty()

Done.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/performance_logger.cc (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/performance_logger.cc:40: if (prefs_.trace_categories
!= "") {
On 2014/08/04 06:08:22, samuong wrote:
> !prefs_.trace_categories.empty()

Done.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/performance_logger.cc:64: if (prefs_.trace_categories
== "" || prefs_.timeline ==
On 2014/08/04 06:08:22, samuong wrote:
> use prefs_.trace_categories.empty()

Done.

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/performance_logger.h (right):

https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/performance_logger.h:32: explicit
PerformanceLogger(Log* log, const PerfLoggingPrefs& prefs);
On 2014/08/04 06:08:22, samuong wrote:
> don't need to make this explicit

Done.

[samuong, 2014-08-04 18:18:22]

On 2014/08/04 17:37:58, johnmoore wrote:
> Thanks for the review, Sam! I have addressed your comments.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> File chrome/test/chromedriver/capabilities.cc (right):
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/capabilities.cc:311: return Status(kUnknownError,
"must
> be a boolean");
> On 2014/08/04 06:08:22, samuong wrote:
> > Please also include what parameter was set incorrectly, to make it easier
for
> > the user to debug.
> 
> The parameter name will be appended to the status message by the for loop in
> ParsePerfLoggingPrefs.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/capabilities.cc:316: }
> On 2014/08/04 06:08:22, samuong wrote:
> > Don't need the braces in this if-else statement.
> 
> Done.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/capabilities.cc:324: return Status(kUnknownError,
"must
> be a dictionary");
> On 2014/08/04 06:08:22, samuong wrote:
> > Please also include what parameter was set incorrectly, to make it easier
for
> > the user to debug.
> 
> Similar to above, the parameter name will be appended to the status message by
> the for loop in ParseChromeOptions.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/capabilities.cc:525: trace_categories("") {}
> On 2014/08/04 06:08:22, samuong wrote:
> > the default constructor for std::string creates an empty string, so you can
> just
> > write trace_categories()
> 
> Done.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> File chrome/test/chromedriver/capabilities.h (right):
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/capabilities.h:65: ~PerfLoggingPrefs();
> On 2014/08/04 06:08:22, samuong wrote:
> > This probably doesn't matter since no one will ever subclass this, but
should
> we
> > just make the destructor virtual anyway?
> 
> 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.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/capabilities.h:74: kExplicitlyDisabled  // explicitly
> disabled
> On 2014/08/04 06:08:22, samuong wrote:
> > I think people will be able to figure out what the enum names mean even
> without
> > these four comments :)
> 
> Done.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/capabilities.h:78: static bool IsEnabled(const
> InspectorDomainStatus& domain_status);
> On 2014/08/04 06:08:22, samuong wrote:
> > 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
> 
> Done.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> File chrome/test/chromedriver/chrome_launcher.cc (right):
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/chrome_launcher.cc:222: if
> (perf_logging_prefs.trace_categories != "") {
> On 2014/08/04 06:08:22, samuong wrote:
> > !perf_logging_prefs.trace_categories.empty()
> 
> Done.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> File chrome/test/chromedriver/performance_logger.cc (right):
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/performance_logger.cc:40: if (prefs_.trace_categories
> != "") {
> On 2014/08/04 06:08:22, samuong wrote:
> > !prefs_.trace_categories.empty()
> 
> Done.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/performance_logger.cc:64: if (prefs_.trace_categories
> == "" || prefs_.timeline ==
> On 2014/08/04 06:08:22, samuong wrote:
> > use prefs_.trace_categories.empty()
> 
> Done.
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> File chrome/test/chromedriver/performance_logger.h (right):
> 
>
https://codereview.chromium.org/429693003/diff/20001/chrome/test/chromedriver...
> chrome/test/chromedriver/performance_logger.h:32: explicit
> PerformanceLogger(Log* log, const PerfLoggingPrefs& prefs);
> On 2014/08/04 06:08:22, samuong wrote:
> > don't need to make this explicit
> 
> Done.

thanks, lgtm

[stgao, 2014-08-04 21:15:09]

lgtm if the memory leak issue is fixed.

https://codereview.chromium.org/429693003/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/429693003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:338: return Status(kUnknownError,
"unrecognized option: " + it.key());
performance option?

https://codereview.chromium.org/429693003/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/devtools_client_impl.cc (right):

https://codereview.chromium.org/429693003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/devtools_client_impl.cc:484: unscoped_result =
new base::DictionaryValue();
Seems like a memory leak, because |unscoped_result| was not deleted.

Maybe integrate this into the if/else block below.

[johnmoore, 2014-08-04 21:40:49]

Fixed memory leak. Shuotao, could you let me know if you approve of my fix?
Thanks!

https://codereview.chromium.org/429693003/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/429693003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/capabilities.cc:338: return Status(kUnknownError,
"unrecognized option: " + it.key());
On 2014/08/04 21:15:09, Shuotao wrote:
> performance option?

Done.

https://codereview.chromium.org/429693003/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/devtools_client_impl.cc (right):

https://codereview.chromium.org/429693003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/devtools_client_impl.cc:484: unscoped_result =
new base::DictionaryValue();
On 2014/08/04 21:15:09, Shuotao wrote:
> Seems like a memory leak, because |unscoped_result| was not deleted.
> 
> Maybe integrate this into the if/else block below.

Done.

[stgao, 2014-08-04 22:15:06]

John, thanks for the quick fix!

lgtm

[johnmoore, 2014-08-05 15:41:43]

The CQ bit was checked by johnmoore@google.com

[commit-bot: I haz the power, 2014-08-05 15:42:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnmoore@google.com/429693003/60001

[commit-bot: I haz the power, 2014-08-05 18:36:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42655)

[commit-bot: I haz the power, 2014-08-05 18:52:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-05 18:52:28]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42698)

[johnmoore, 2014-08-05 20:45:51]

The CQ bit was checked by johnmoore@google.com

[commit-bot: I haz the power, 2014-08-05 20:47:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnmoore@google.com/429693003/60001

[commit-bot: I haz the power, 2014-08-05 22:17:36]

Change committed as 287613