Rename WebSourceBuffer::abort -> resetParserState

Rename WebSourceBuffer::abort -> resetParserState

As discussed in https://codereview.chromium.org/1013923002/, we could
rename WebSourceBuffer::abort into WebSourceBuffer::resetParserState to
avoid confusion that it's not the same thing as SourceBuffer::abort, and
just one of the steps of the MSE abort algorithm.

BUG=529497
Committed: https://crrev.com/0751b5f3fc75f72a438b7764ba5a59877b27fa84
Cr-Commit-Position: refs/heads/master@{#348308}

[servolk, 2015-09-08 23:28:07]

servolk@chromium.org changed reviewers:
+ wolenetz@chromium.org

[servolk, 2015-09-08 23:37:07]

On 2015/09/08 23:28:12, servolk wrote:

Next step: https://codereview.chromium.org/1326373002/

[wolenetz, 2015-09-09 18:20:42]

https://codereview.chromium.org/1312763014/diff/1/media/test/pipeline_integra...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1312763014/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:562: void Abort() {
Hmm. This mock of the SourceBuffer API's Abort() does something different than
SourceBuffer::abort().
In the spirit of this CL cleaning up MSE "Abort" ambiguity, either change this
method name to something like "ShutdownChunkDemuxer", or (preferred somewhat),
keep it called "Abort", but change behavior in two ways:
1) Have it call chunk_demuxer_->ResetParserState(...) [I'm unsure if tests rely
also on post-MockMediaSource::Abort() |last_timestamp_offset_| not being reset,
but it would also be good to reset it to align with SourceBuffer::Abort() and
fix the tests if needed to match their original intent], and
2) override PipelineIntegrationTestBase::Stop() to: a) shutdown and reset to
nullptr chunk_demuxer_ if it wasn't previously nullptr, and b) then call the
base class's Stop().

WDYT?

[servolk, 2015-09-10 00:50:11]

On 2015/09/09 18:20:42, wolenetz wrote:
>
https://codereview.chromium.org/1312763014/diff/1/media/test/pipeline_integra...
> File media/test/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/1312763014/diff/1/media/test/pipeline_integra...
> media/test/pipeline_integration_test.cc:562: void Abort() {
> Hmm. This mock of the SourceBuffer API's Abort() does something different than
> SourceBuffer::abort().
> In the spirit of this CL cleaning up MSE "Abort" ambiguity, either change this
> method name to something like "ShutdownChunkDemuxer", or (preferred somewhat),
> keep it called "Abort", but change behavior in two ways:
> 1) Have it call chunk_demuxer_->ResetParserState(...) [I'm unsure if tests
rely
> also on post-MockMediaSource::Abort() |last_timestamp_offset_| not being
reset,
> but it would also be good to reset it to align with SourceBuffer::Abort() and
> fix the tests if needed to match their original intent], and
> 2) override PipelineIntegrationTestBase::Stop() to: a) shutdown and reset to
> nullptr chunk_demuxer_ if it wasn't previously nullptr, and b) then call the
> base class's Stop().
> 
> WDYT?

AFAIU MockMediaSource::Abort is a completely separate method and has really
nothing to do with SourceBuffer::abort, it just shuts down/destroys the
ChunkDemuxer being used by the test and doesn't try to run the SB::abort
algorithm at all. So it's almost a coincidence that they have the same name. Now
the question is how do we want to fix this?
We can't do the option #2 described above directly, since chunk_demuxer_ is in a
separate MockMediaSource class and PipelineIntegrationTest*::Stop won't be able
to access that.
How about we rename MockMediaSource::Abort into Shutdown, to avoid name
confusion and also call ChunkDemuxer::Abort/ResetParserState from there to
provide test coverage for the ResetParserState logic as well?
https://codereview.chromium.org/1318643005/

[servolk, 2015-09-10 22:34:05]

On 2015/09/10 00:50:11, servolk wrote:
> On 2015/09/09 18:20:42, wolenetz wrote:
> >
>
https://codereview.chromium.org/1312763014/diff/1/media/test/pipeline_integra...
> > File media/test/pipeline_integration_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/1312763014/diff/1/media/test/pipeline_integra...
> > media/test/pipeline_integration_test.cc:562: void Abort() {
> > Hmm. This mock of the SourceBuffer API's Abort() does something different
than
> > SourceBuffer::abort().
> > In the spirit of this CL cleaning up MSE "Abort" ambiguity, either change
this
> > method name to something like "ShutdownChunkDemuxer", or (preferred
somewhat),
> > keep it called "Abort", but change behavior in two ways:
> > 1) Have it call chunk_demuxer_->ResetParserState(...) [I'm unsure if tests
> rely
> > also on post-MockMediaSource::Abort() |last_timestamp_offset_| not being
> reset,
> > but it would also be good to reset it to align with SourceBuffer::Abort()
and
> > fix the tests if needed to match their original intent], and
> > 2) override PipelineIntegrationTestBase::Stop() to: a) shutdown and reset to
> > nullptr chunk_demuxer_ if it wasn't previously nullptr, and b) then call the
> > base class's Stop().
> > 
> > WDYT?
> 
> AFAIU MockMediaSource::Abort is a completely separate method and has really
> nothing to do with SourceBuffer::abort, it just shuts down/destroys the
> ChunkDemuxer being used by the test and doesn't try to run the SB::abort
> algorithm at all. So it's almost a coincidence that they have the same name.
Now
> the question is how do we want to fix this?
> We can't do the option #2 described above directly, since chunk_demuxer_ is in
a
> separate MockMediaSource class and PipelineIntegrationTest*::Stop won't be
able
> to access that.
> How about we rename MockMediaSource::Abort into Shutdown, to avoid name
> confusion and also call ChunkDemuxer::Abort/ResetParserState from there to
> provide test coverage for the ResetParserState logic as well?
> https://codereview.chromium.org/1318643005/

ping. PTAL at this + https://codereview.chromium.org/1318643005/

[wolenetz, 2015-09-10 23:43:24]

lgtm % other CL's usage of ChunkDemuxer::Abort() (in l.565 of
https://codereview.chromium.org/1318643005/diff/20001/media/test/pipeline_int...)
will also need rename to ::ResetParserState()

[servolk, 2015-09-10 23:55:36]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-09-10 23:55:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1312763014/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-10 23:56:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312763014/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312763014/40001

[commit-bot: I haz the power, 2015-09-11 01:29:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-11 01:30:03]

Patchset 3 (id:??) landed as
https://crrev.com/0751b5f3fc75f72a438b7764ba5a59877b27fa84
Cr-Commit-Position: refs/heads/master@{#348308}

[commit-bot: I haz the power, 2015-09-23 12:17:30]

Patchset 3 (id:??) landed as
https://crrev.com/0751b5f3fc75f72a438b7764ba5a59877b27fa84
Cr-Commit-Position: refs/heads/master@{#348308}