Log PipelineImpl events to MediaLog.

Log PipelineImpl events to MediaLog.

BUG=
TEST=


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

[Scott Franklin, 2011-08-08 18:06:19]

I'm not entirely happy with having the PipelineStateToString function in
MediaLog. But I can't think of anywhere else to put it, and I do need the string
names at some point.

[scherkus (not reviewing), 2011-08-08 23:14:15]

q about the whole string/int/etc stuff

http://codereview.chromium.org/7584013/diff/1/media/base/media_log.h
File media/base/media_log.h (right):

http://codereview.chromium.org/7584013/diff/1/media/base/media_log.h#newcode17
media/base/media_log.h:17: 
nit: want to nuke this blank line?

http://codereview.chromium.org/7584013/diff/1/media/base/media_log_event.h
File media/base/media_log_event.h (right):

http://codereview.chromium.org/7584013/diff/1/media/base/media_log_event.h#ne...
media/base/media_log_event.h:40: // params: "pipeline_state": <intergral value
of the state>.
what if this was setting a string so that PipelineImpl could do the int ->
string conversion?

do we really need the pipeline states as ints?

[Scott Franklin, 2011-08-09 00:31:28]

http://codereview.chromium.org/7584013/diff/1/media/base/media_log.h
File media/base/media_log.h (right):

http://codereview.chromium.org/7584013/diff/1/media/base/media_log.h#newcode17
media/base/media_log.h:17: 
On 2011/08/08 23:14:15, scherkus wrote:
> nit: want to nuke this blank line?

I thought the rule was blank line before //?

http://codereview.chromium.org/7584013/diff/1/media/base/media_log_event.h
File media/base/media_log_event.h (right):

http://codereview.chromium.org/7584013/diff/1/media/base/media_log_event.h#ne...
media/base/media_log_event.h:40: // params: "pipeline_state": <intergral value
of the state>.
On 2011/08/08 23:14:15, scherkus wrote:
> what if this was setting a string so that PipelineImpl could do the int ->
> string conversion?
> 
> do we really need the pipeline states as ints?

oops, forgot to update the comment.
The event does actually take a string. It gets converted from a
PipelineImpl::State to a string in MediaLog because I didn't think PipelineImpl
should have to care about naming the states.
I could easily dump the giant switch statement into PipelineImpl::SetState() if
that would be better.

The original plan was to be similar to net-internals, which sends its page
integer values of events and a map to convert them to strings. Unfortunately,
that wasn't guaranteed to reflect changes to the State enum, while the large
switch statement throws a compiler error if you miss one.

[scherkus (not reviewing), 2011-08-09 00:58:07]

ok this looks mostly good -- care to rebase ontop of the other patch + update
the event creating shortcuts?

http://codereview.chromium.org/7584013/diff/1/media/base/media_log.h
File media/base/media_log.h (right):

http://codereview.chromium.org/7584013/diff/1/media/base/media_log.h#newcode17
media/base/media_log.h:17: 
On 2011/08/09 00:31:28, Scott Franklin wrote:
> On 2011/08/08 23:14:15, scherkus wrote:
> > nit: want to nuke this blank line?
> 
> I thought the rule was blank line before //?

Ah! I should clarify.

Blank line before // UNLESS it's the start of a new scope block (like { or
public:, etc... notice how you did The Right Thing with private: int32 id_
below)

[Scott Franklin, 2011-08-09 01:25:06]

rebasing. I had already done the Create*Event methods.

[scherkus (not reviewing), 2011-08-09 20:52:33]

LGTM

http://codereview.chromium.org/7584013/diff/5003/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/7584013/diff/5003/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:11: #include "media/base/filters.h"
thanks :)

[commit-bot: I haz the power, 2011-08-09 22:47:25]

Change committed as 96090