Log BufferedResourceLoader events to MediaLog.

Log BufferedResourceLoader events to MediaLog.

BUG=
TEST=


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

[scherkus (not reviewing), 2011-08-08 23:18:25]

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

http://codereview.chromium.org/7591001/diff/1/media/base/media_log.h#newcode25
media/base/media_log.h:25: void SetBufferedExtents(size_t start, size_t current,
size_t end);
hrmm.. I find these method names misleading

would it be so bad have them named:
AddLoadEvent()
AddBufferedExtentsChangedEvent()

...or break them out into helper methods:

AddEvent(CreateLoadEvent(...))
AddEvent(CreateBufferedExtentsEvent(...))

http://codereview.chromium.org/7591001/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/7591001/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:702: if (buffer_.get()) {
so is media_log a required param or not?

if it's refcounted, when would it be NULL?

http://codereview.chromium.org/7591001/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/7591001/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.h:219: // Log everything interesting
to media_log_.
|| around params

[Scott Franklin, 2011-08-09 00:27:27]

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

http://codereview.chromium.org/7591001/diff/1/media/base/media_log.h#newcode25
media/base/media_log.h:25: void SetBufferedExtents(size_t start, size_t current,
size_t end);
On 2011/08/08 23:18:25, scherkus wrote:
> hrmm.. I find these method names misleading
> 
> would it be so bad have them named:
> AddLoadEvent()
> AddBufferedExtentsChangedEvent()
> 
> ...or break them out into helper methods:
> 
> AddEvent(CreateLoadEvent(...))
> AddEvent(CreateBufferedExtentsEvent(...))

I was trying to make calling log functions concise, but the names did end up
sounding quite awkward.

Changed the others while I was at it.

http://codereview.chromium.org/7591001/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/7591001/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:702: if (buffer_.get()) {
On 2011/08/08 23:18:25, scherkus wrote:
> so is media_log a required param or not?
> 
> if it's refcounted, when would it be NULL?

That's a check for buffer_, which is null surprisingly often. At this point the
check is only necessary on the second Log() invocation above, so it could go
there instead, but that could conceivably change in the future.

http://codereview.chromium.org/7591001/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/7591001/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.h:219: // Log everything interesting
to media_log_.
On 2011/08/08 23:18:25, scherkus wrote:
> || around params

Done.

[scherkus (not reviewing), 2011-08-09 00:59:40]

LGTM w/ nits

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

http://codereview.chromium.org/7591001/diff/4002/media/base/media_log.h#newco...
media/base/media_log.h:16: 
nit: nuke blank line here

http://codereview.chromium.org/7591001/diff/4002/media/base/media_log.h#newco...
media/base/media_log.h:24: MediaLogEvent*
CreateBufferedExtentsChangedEvent(size_t start, size_t current,
nit: move these down to CreateEvent() + update .cc

http://codereview.chromium.org/7591001/diff/4002/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/7591001/diff/4002/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:72:
MockBufferedResourceLoader() : BufferedResourceLoader(GURL(), 0, 0,
in this case I'd just drop the : bit to next line

[Scott Franklin, 2011-08-09 01:16:08]

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

http://codereview.chromium.org/7591001/diff/4002/media/base/media_log.h#newco...
media/base/media_log.h:16: 
On 2011/08/09 00:59:40, scherkus wrote:
> nit: nuke blank line here

Done.

http://codereview.chromium.org/7591001/diff/4002/media/base/media_log.h#newco...
media/base/media_log.h:24: MediaLogEvent*
CreateBufferedExtentsChangedEvent(size_t start, size_t current,
On 2011/08/09 00:59:40, scherkus wrote:
> nit: move these down to CreateEvent() + update .cc

Done.

http://codereview.chromium.org/7591001/diff/4002/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/7591001/diff/4002/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:72:
MockBufferedResourceLoader() : BufferedResourceLoader(GURL(), 0, 0,
On 2011/08/09 00:59:40, scherkus wrote:
> in this case I'd just drop the : bit to next line

Done.

[commit-bot: I haz the power, 2011-08-09 17:53:48]

Change committed as 96014