Clean up some API in trace_event_analyzer.h and add some getters.

Clean up some API in trace_event_analyzer.h and add some getters.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108672

[Paweł Hajdan Jr., 2011-11-03 10:09:19]

http://codereview.chromium.org/8438058/diff/2001/base/test/trace_event_analyz...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/8438058/diff/2001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:107: std::string
TraceEvent::GetKnownArgAsString(const std::string& name) const {
Are those accessors used? I'd prefer to avoid adding them before they're used.

[jbates, 2011-11-03 17:10:32]

http://codereview.chromium.org/8438058/diff/2001/base/test/trace_event_analyz...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/8438058/diff/2001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:107: std::string
TraceEvent::GetKnownArgAsString(const std::string& name) const {
On 2011/11/03 10:09:20, Paweł Hajdan Jr. wrote:
> Are those accessors used? I'd prefer to avoid adding them before they're used.

Yeah, some are used in a pending CL (http://codereview.chromium.org/7982007/)
and the rest are there for completeness. I split them because there was other
cleanup needed in this code.

To make sure they actually work, I added a test that verifies these methods.

[nduca, 2011-11-03 19:05:53]

wrt Pawel's comments, I think its perfectly reasonable to add these APIs. I'm
still neutral-negative about your non-normative approach to GetArgument but I'd
LGTM were I reviewing. :)

http://codereview.chromium.org/8438058/diff/4001/base/test/trace_event_analyz...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/8438058/diff/4001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.h:128: double GetAbsTimeToOtherEvent() const;
What happens if you forget to do this? DCHECK or a specific return value?

http://codereview.chromium.org/8438058/diff/4001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.h:147: 
I guess by moving to this CL, I didn't get your response to why you're doing
this and not

bool GetArg(name, value*)?

What's the behavior if GetKnownArgAsX fails?

[jbates, 2011-11-03 19:34:39]

http://codereview.chromium.org/8438058/diff/4001/base/test/trace_event_analyz...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/8438058/diff/4001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.h:128: double GetAbsTimeToOtherEvent() const;
On 2011/11/03 19:05:53, nduca wrote:
> What happens if you forget to do this? DCHECK or a specific return value?

The behavior is to crash when other_event doesn't exist so that the test will
not accidentally pass.

http://codereview.chromium.org/8438058/diff/4001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.h:147: 
On 2011/11/03 19:05:53, nduca wrote:
> I guess by moving to this CL, I didn't get your response to why you're doing
> this and not
> 
> bool GetArg(name, value*)?
> 
> What's the behavior if GetKnownArgAsX fails?

What I realized (tried to explain it in the comment above -- let me know if it's
not clear) is that the typical usage of the Query API makes additional checking
here redundant and it's often inconvenient to require named return parameters
(it breaks the functional flow of code).

The behavior is to assert when the API is not used correctly (so that tests
don't accidentally pass). Consider std::vector: it allows you to access data at
particular indices and crashes if the index is out of bounds.

[nduca, 2011-11-03 20:14:54]

Sounds good

[Paweł Hajdan Jr., 2011-11-04 07:56:23]

LGTM stamp.