Make ChunkDemuxer error handling more consistent and robust.

Make ChunkDemuxer error handling more consistent and robust. 

This fixes a variety of error cases exposed by the WebKit LayoutTests I created to test this code. The ChunkDemuxer unit tests were updated to expose these cases as well.


BUG=86536
TEST=ChunkDemuxerTest.*

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

[Ami GONE FROM CHROMIUM, 2011-08-03 21:06:02]

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

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:421: break;
This break; is necessary?

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:680: void
ChunkDemuxer::ReportError_Locked(PipelineStatus status) {
FWIW, when you know status is not OK, I prefer naming the variable 'error'.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:693: if (state_ != PARSE_ERROR)
When can this happen?

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:714: cb.Run(status);
Seems strange that in this case host()->SetError() isn't called.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:78: message_loop_.RunAllPending();
This makes me worry.  Why do you have it?

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:159: EXPECT_CALL(mock_filter_host_,
SetNetworkActivity(true)).Times(AnyNumber());
Ugh to the above three lines, and the other green EXPECT_CALLs in this change. 
Can you say "brittle"?

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:232: MessageLoop message_loop_;
I don't get where this is populated, only drained.  What?

[acolwell GONE FROM CHROMIUM, 2011-08-03 22:08:11]

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

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:421: break;
On 2011/08/03 21:06:02, Ami Fischman wrote:
> This break; is necessary?

No. Done.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:680: void
ChunkDemuxer::ReportError_Locked(PipelineStatus status) {
On 2011/08/03 21:06:02, Ami Fischman wrote:
> FWIW, when you know status is not OK, I prefer naming the variable 'error'.

Done.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:693: if (state_ != PARSE_ERROR)
On 2011/08/03 21:06:02, Ami Fischman wrote:
> When can this happen?
This can happen if Stop() is called before this task executes. In that case we
don't want to report the error because we've been shutdown already.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:714: cb.Run(status);
On 2011/08/03 21:06:02, Ami Fischman wrote:
> Seems strange that in this case host()->SetError() isn't called.
SetError() is only used for error reporting when we don't have an outstanding
async call. In the context of this class it means that SetError() will only be
called during playback. 

Eventually I'd like to get rid of SetError() and have Play() take an error
callback so that the scope of this alternative error reporting path is easier to
see. Once this happens we'll always call cb to report an error and the code will
look more consistent.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:78: message_loop_.RunAllPending();
On 2011/08/03 21:06:02, Ami Fischman wrote:
> This makes me worry.  Why do you have it?
It was just catching the error on EndOfStream() cases. I've removed this and
added calls at the appropriate locations.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:159: EXPECT_CALL(mock_filter_host_,
SetNetworkActivity(true)).Times(AnyNumber());
On 2011/08/03 21:06:02, Ami Fischman wrote:
> Ugh to the above three lines, and the other green EXPECT_CALLs in this change.

> Can you say "brittle"?
Yes. Yes I can...These are on the must die list along with other FilterHost
behavior.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:232: MessageLoop message_loop_;
On 2011/08/03 21:06:02, Ami Fischman wrote:
> I don't get where this is populated, only drained.  What?
The constructor is magical and initializes the thread local storage. This
declaration causes MessageLoop::current() to no longer be null.

I can add a message_loop_ member to ChunkDemuxer & pass this in if you'd prefer
that to just using MessageLoop::current() in the one place I need it.

[Ami GONE FROM CHROMIUM, 2011-08-03 23:41:34]

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

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:693: if (state_ != PARSE_ERROR)
On 2011/08/03 22:08:11, acolwell wrote:
> On 2011/08/03 21:06:02, Ami Fischman wrote:
> > When can this happen?
> This can happen if Stop() is called before this task executes. In that case we
> don't want to report the error because we've been shutdown already.

Add a comment?

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:232: MessageLoop message_loop_;
On 2011/08/03 22:08:11, acolwell wrote:
> On 2011/08/03 21:06:02, Ami Fischman wrote:
> > I don't get where this is populated, only drained.  What?
> The constructor is magical and initializes the thread local storage. This
> declaration causes MessageLoop::current() to no longer be null.
> 
> I can add a message_loop_ member to ChunkDemuxer & pass this in if you'd
prefer
> that to just using MessageLoop::current() in the one place I need it.

Per offline convo, I think you can avoid ML entirely.

[acolwell GONE FROM CHROMIUM, 2011-08-04 00:22:16]

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

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:693: if (state_ != PARSE_ERROR)
On 2011/08/03 23:41:35, Ami Fischman wrote:
> On 2011/08/03 22:08:11, acolwell wrote:
> > On 2011/08/03 21:06:02, Ami Fischman wrote:
> > > When can this happen?
> > This can happen if Stop() is called before this task executes. In that case
we
> > don't want to report the error because we've been shutdown already.
> 
> Add a comment?
No longer necessary now that we aren't posting a task.

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/7538027/diff/6001/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:232: MessageLoop message_loop_;
On 2011/08/03 23:41:35, Ami Fischman wrote:
> On 2011/08/03 22:08:11, acolwell wrote:
> > On 2011/08/03 21:06:02, Ami Fischman wrote:
> > > I don't get where this is populated, only drained.  What?
> > The constructor is magical and initializes the thread local storage. This
> > declaration causes MessageLoop::current() to no longer be null.
> > 
> > I can add a message_loop_ member to ChunkDemuxer & pass this in if you'd
> prefer
> > that to just using MessageLoop::current() in the one place I need it.
> 
> Per offline convo, I think you can avoid ML entirely.

Removed

[Ami GONE FROM CHROMIUM, 2011-08-04 16:12:16]

LGTM