Implement sourceAddId() & sourceRemoveId()

Implement sourceAddId() & sourceRemoveId()

BUG=122897
TEST=Covered by LayoutTests


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

[acolwell GONE FROM CHROMIUM, 2012-04-23 16:56:18]

PTAL. I'm pretty sure I know why the 3 previous commits failed now, but I want
another set of eyes look at this before I try landing it again. 

I'm pretty sure the previous commits failed because the unit tests were making
the AddId() calls inside a DCHECK so they could verify the return value. This
caused them not go get executed on some of the bots that apparently are doing
release builds. :/

[scherkus (not reviewing), 2012-04-23 19:24:17]

LGTM w/ nits

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:495: return false;
documentation states false is returned when in an invalid state (i.e,. below)
but these seem more of the DCHECK/CHECK variety, no?

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:62: bool RemoveId(const std::string& id);
what does true/false mean? could we have void?

is it CHECK-failable if you remove an ID that doesn't exist?

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:66: bool AppendData(const std::string& id, const
uint8* data, size_t length);
CHECK-fail instead?

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:118: // TODO(acolwell): Remove this when fixing
http://crbug.com/122909 .
nit: I'd just leave off that trailing period in this case :)

[acolwell GONE FROM CHROMIUM, 2012-04-23 22:15:23]

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:495: return false;
On 2012/04/23 19:24:17, scherkus wrote:
> documentation states false is returned when in an invalid state (i.e,. below)
> but these seem more of the DCHECK/CHECK variety, no?

Done.

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:62: bool RemoveId(const std::string& id);
On 2012/04/23 19:24:17, scherkus wrote:
> what does true/false mean? could we have void?
Yes we could have void. Eric wanted a bool returned in the WebKit code, "just in
case" we needed to signal an error. Since the WebKit call had a bool I figured
it made sense to pipe it all the way to here. I'll remove it and put the return
true in WebMediaPlayerImpl until we actually have a reason to plumb it all the
way to ChunkDemuxer.
> 
> is it CHECK-failable if you remove an ID that doesn't exist?
Yes. Done

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:66: bool AppendData(const std::string& id, const
uint8* data, size_t length);
On 2012/04/23 19:24:17, scherkus wrote:
> CHECK-fail instead?
No. It is possible for appends to happen when the ChunkDemuxer is in an error
state. We need to be able to return an error here. An error could have occurred,
but WebKit doesn't know about it yet.

http://codereview.chromium.org/10164017/diff/6002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:118: // TODO(acolwell): Remove this when fixing
http://crbug.com/122909 .
On 2012/04/23 19:24:17, scherkus wrote:
> nit: I'd just leave off that trailing period in this case :)

Done.

[commit-bot: I haz the power, 2012-04-23 22:15:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/10164017/15001

[commit-bot: I haz the power, 2012-04-23 23:46:40]

Change committed as 133568