Moved DCHECK() to prevent NULL deref of codec_context_.

Moved DCHECK() to prevent NULL deref of codec_context_.

CID=102266
BUG=
TEST=
TBR=annacc

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

[Tyler Breisacher (Chromium), 2012-07-25 21:53:03]

https://chromiumcodereview.appspot.com/10821029/diff/1/media/filters/audio_fi...
File media/filters/audio_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10821029/diff/1/media/filters/audio_fi...
media/filters/audio_file_reader.cc:122: if (!format_context_ || !codec_context_
|| !codec_) {
Isn't if () redundant then? Maybe it should just say:

DCHECK(format_context_ && codec_context_ && codec_) << "AudioFileReader::Read()
: reader is not opened!";

[Kyle Horimoto, 2012-07-25 21:57:25]

https://chromiumcodereview.appspot.com/10821029/diff/1/media/filters/audio_fi...
File media/filters/audio_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10821029/diff/1/media/filters/audio_fi...
media/filters/audio_file_reader.cc:122: if (!format_context_ || !codec_context_
|| !codec_) {
On 2012/07/25 21:53:03, Tyler Breisacher wrote:
> Isn't if () redundant then? Maybe it should just say:
> 
> DCHECK(format_context_ && codec_context_ && codec_) <<
"AudioFileReader::Read()
> : reader is not opened!";

If we did that, we wouldn't hit the "return false;". Don't think we should be
changing the behavior in this case.

[Tyler Breisacher (Chromium), 2012-07-25 22:14:23]

http://codereview.chromium.org/10821029/diff/1/media/filters/audio_file_reade...
File media/filters/audio_file_reader.cc (right):

http://codereview.chromium.org/10821029/diff/1/media/filters/audio_file_reade...
media/filters/audio_file_reader.cc:122: if (!format_context_ || !codec_context_
|| !codec_) {
The DCHECK is saying "none of these should ever be null" and this line is saying
"if one of these is null, then do the following". Check with someone who knows
this file better, to determine whether this is something that should NEVER
happen, or not.

[Kyle Horimoto, 2012-07-25 22:56:36]

On 2012/07/25 22:14:23, Tyler Breisacher wrote:
>
http://codereview.chromium.org/10821029/diff/1/media/filters/audio_file_reade...
> File media/filters/audio_file_reader.cc (right):
> 
>
http://codereview.chromium.org/10821029/diff/1/media/filters/audio_file_reade...
> media/filters/audio_file_reader.cc:122: if (!format_context_ ||
!codec_context_
> || !codec_) {
> The DCHECK is saying "none of these should ever be null" and this line is
saying
> "if one of these is null, then do the following". Check with someone who knows
> this file better, to determine whether this is something that should NEVER
> happen, or not.

Done.

[Tyler Breisacher (Chromium), 2012-07-25 22:59:26]

lgtm as far as I can see but please add an OWNER as a reviewer as well.

[Kyle Horimoto, 2012-07-25 23:01:55]

On 2012/07/25 22:59:26, Tyler Breisacher wrote:
> lgtm as far as I can see but please add an OWNER as a reviewer as well.

+annacc

[commit-bot: I haz the power, 2012-07-26 22:56:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10821029/2003

[commit-bot: I haz the power, 2012-07-27 01:41:46]

Change committed as 148691