Improve input handling for WaveAudioHandler.

Improve input handling for WaveAudioHandler.

Improperly-formatted WAV data causes this class to attempt to divide by
zero in the constructor. Check for this condition, and invalidate
internal state if the WAV data does not parse properly. It should be able
to accept any data without crashing. Add unittests to verify.

Bug: b/21759728

BUG=
TEST=wav_audio_handler_unittest.cc

Committed: https://crrev.com/344528fe16c4daf1d34bd59e09fc8e07d12a27f4
Cr-Commit-Position: refs/heads/master@{#361147}

[slan, 2015-11-17 19:59:09]

slan@chromium.org changed reviewers:
+ liamog@chromium.org, tommi@chromium.org, wzhong@chromium.org

[slan, 2015-11-17 19:59:10]

Hey all,

Chromecast is having an issue in the ctor of WavAudioHandler. Add more robust
checks to handle malformed data.

Will add a media OWNER for media.gyp before submit.

[tommi (sloooow) - chröme, 2015-11-17 20:48:39]

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:86: if (num_channels_ == 0u ||
bits_per_sample_ == 0u || sample_rate_ == 0u) {
nit: could this expression be is_valid() or do we really need is_valid_?

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:103: return is_valid_ ? data_.size() <=
cursor : true;
nit: could also do
return !is_valid() || data_.size() <= cursor;

Actually, is it expected or should it be, that AtEnd() is called if the handler
is not valid?  Maybe there should just be a DCHECK(is_valid()); at the top of
each function?

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:170: data_ = base::StringPiece();
another potential implementation for is_valid();

return !data_.empty();

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler_unittest.cc (right):

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler_unittest.cc:69: auto data =
std::string(kTestAudioData, arraysize(kTestAudioData));
std::string data(kTestAudioData, arraysize(kTestAudioData));

[slan, 2015-11-17 21:50:16]

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:86: if (num_channels_ == 0u ||
bits_per_sample_ == 0u || sample_rate_ == 0u) {
On 2015/11/17 20:48:39, tommi wrote:
> nit: could this expression be is_valid() or do we really need is_valid_?

Done.

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:103: return is_valid_ ? data_.size() <=
cursor : true;
On 2015/11/17 20:48:39, tommi wrote:
> nit: could also do
> return !is_valid() || data_.size() <= cursor;
> 
> Actually, is it expected or should it be, that AtEnd() is called if the
handler
> is not valid?  Maybe there should just be a DCHECK(is_valid()); at the top of
> each function?

In practice, this object should not be used if is_valid() returns false. Adding
IsValid() seemed defensive and has precedent (AudioParameters), where I don't
see many cases of an object with methods that can't be touched after the ctor.

Alternatively, what do you think about a factory method that returns a
scoped_ptr<WavAudioHandler> iff it parses successfully (and null ptr otherwise)?
Thoughts?

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:170: data_ = base::StringPiece();
On 2015/11/17 20:48:39, tommi wrote:
> another potential implementation for is_valid();
> 
> return !data_.empty();

I like the idea you left above. Theoretically, we could have an emppty WAV bit,
no?

[tommi (sloooow) - chröme, 2015-11-18 11:52:58]

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/20001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:103: return is_valid_ ? data_.size() <=
cursor : true;
On 2015/11/17 21:50:16, slan wrote:
> On 2015/11/17 20:48:39, tommi wrote:
> > nit: could also do
> > return !is_valid() || data_.size() <= cursor;
> > 
> > Actually, is it expected or should it be, that AtEnd() is called if the
> handler
> > is not valid?  Maybe there should just be a DCHECK(is_valid()); at the top
of
> > each function?
> 
> In practice, this object should not be used if is_valid() returns false.
Adding
> IsValid() seemed defensive and has precedent (AudioParameters), where I don't
> see many cases of an object with methods that can't be touched after the ctor.
> 
> Alternatively, what do you think about a factory method that returns a
> scoped_ptr<WavAudioHandler> iff it parses successfully (and null ptr
otherwise)?
> Thoughts?

That sounds like a good idea to me and would simplify error handling quite a bit
in this class. We might also be able to make some members const. I don't see any
thread checks, so I think that would be a very good thing actually.

[slan, 2015-11-18 22:00:04]

Factory method is implemented, and most data members in WavAudioHandler are now
const. PTAL

[wzhong, 2015-11-18 22:21:35]

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:74: DLOG(ERROR) << "Data size " <<
data.size() << " is too short.";
Why DLOG?

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.h (right):

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.h:22: static scoped_ptr<WavAudioHandler>
Create(const base::StringPiece& wav_data);
static methods should be after dtor.

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.h:45: explicit WavAudioHandler(const
base::StringPiece& audio_data,
explicit not needed.

[slan, 2015-11-19 00:19:44]

Added a few more comments and more test cases.

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:74: DLOG(ERROR) << "Data size " <<
data.size() << " is too short.";
On 2015/11/18 22:21:35, wzhong wrote:
> Why DLOG?

Done.

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.h (right):

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.h:22: static scoped_ptr<WavAudioHandler>
Create(const base::StringPiece& wav_data);
On 2015/11/18 22:21:35, wzhong wrote:
> static methods should be after dtor.

Done.

https://codereview.chromium.org/1453233002/diff/80001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.h:45: explicit WavAudioHandler(const
base::StringPiece& audio_data,
On 2015/11/18 22:21:35, wzhong wrote:
> explicit not needed.

Whoops, leftover. Done.

[wzhong, 2015-11-19 01:07:40]

lgtm

[slan, 2015-11-19 01:58:46]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 02:00:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453233002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453233002/120001

[commit-bot: I haz the power, 2015-11-19 02:00:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 02:00:44]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tommi (sloooow) - chröme, 2015-11-19 09:44:06]

https://codereview.chromium.org/1453233002/diff/120001/media/audio/simple_sou...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/simple_sou...
media/audio/simple_sources.cc:173: reinterpret_cast<const
char*>(wav_file_data_.get()), file_length));
hmm... I guess shared_ptr could have helped here. As is, it's not exactly
obvious why the wav_file_data_ member variable exists in the first place (it's
only used in this scope and just used to pass the data to this constructor).
Alternatively it might make sense at some point to pass ownership of the data to
the WavAudioHandler (and then possibly change the type to scoped_ptr<uint8[]>). 
I don't know if any caller needs to also hold on to this data.

No action needed in this CL but if you agree with me, you could add a TODO to
WavAudioHandler for either myself or you to do that.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/aud...
File media/audio/sounds/audio_stream_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/aud...
media/audio/sounds/audio_stream_handler.cc:188: // safely be accessed on this
thread via wav_audio_handler().
that's unfortunate.  Can you file a bug for it?  How much work is it to make
callers of wav_audio_handler() access it on the AudioManager's thread?

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/aud...
File media/audio/sounds/audio_stream_handler.h (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/aud...
media/audio/sounds/audio_stream_handler.h:57: const WavAudioHandler*
wav_audio_handler() const { return wav_audio_.get(); }
can you add a DCHECK here for the current thread? (see cc file for usage of
base::NonThreadSafe)

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
media/audio/sounds/wav_audio_handler.cc:159:
WavAudioHandler::WavAudioHandler(const base::StringPiece& audio_data,
FYI - it's preferred to pas StringPiece by value.
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
Feel free to change or skip.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
media/audio/sounds/wav_audio_handler.cc:177: const base::StringPiece& wav_data)
{
Since this is a new function, it would be good to pass by value here at least.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
File media/audio/sounds/wav_audio_handler_unittest.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
media/audio/sounds/wav_audio_handler_unittest.cc:59: auto data =
std::string(kTestAudioData, arraysize(kTestAudioData) - 1);
please fix these to not use auto.  It's not needed.  Just do:

std::string data(kTestAudioData, arraysize(kTestAudioData) - 1);

[slan, 2015-11-19 18:57:16]

Okay, added some more tests and addressed comments. PTAL.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/simple_sou...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/simple_sou...
media/audio/simple_sources.cc:173: reinterpret_cast<const
char*>(wav_file_data_.get()), file_length));
On 2015/11/19 09:44:06, tommi wrote:
> hmm... I guess shared_ptr could have helped here. As is, it's not exactly
> obvious why the wav_file_data_ member variable exists in the first place (it's
> only used in this scope and just used to pass the data to this constructor).
> Alternatively it might make sense at some point to pass ownership of the data
to
> the WavAudioHandler (and then possibly change the type to
scoped_ptr<uint8[]>). 
> I don't know if any caller needs to also hold on to this data.
> 
> No action needed in this CL but if you agree with me, you could add a TODO to
> WavAudioHandler for either myself or you to do that.

Seems reasonable to remove it as a member. Done. Refactored the reading function
and added some test coverage.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/aud...
File media/audio/sounds/audio_stream_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/aud...
media/audio/sounds/audio_stream_handler.cc:188: // safely be accessed on this
thread via wav_audio_handler().
On 2015/11/19 09:44:06, tommi wrote:
> that's unfortunate.  Can you file a bug for it?  How much work is it to make
> callers of wav_audio_handler() access it on the AudioManager's thread?

Done. There was only one caller that needed it, and only for the duration, so I
added a duration() call to AudioStreamManager.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/aud...
File media/audio/sounds/audio_stream_handler.h (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/aud...
media/audio/sounds/audio_stream_handler.h:57: const WavAudioHandler*
wav_audio_handler() const { return wav_audio_.get(); }
On 2015/11/19 09:44:06, tommi wrote:
> can you add a DCHECK here for the current thread? (see cc file for usage of
> base::NonThreadSafe)

Does this class needs these? All data members are set in the ctor, and all work
is posted to the AudioManagerThread, which guarantees ordering. 

But to be consistent, I've added such a DCHECK to duration(), the replacement
for this method. There are should not be threading concerns, but this will help
find any nonsense and is consistent with the rest of the methods.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
media/audio/sounds/wav_audio_handler.cc:159:
WavAudioHandler::WavAudioHandler(const base::StringPiece& audio_data,
On 2015/11/19 09:44:06, tommi wrote:
> FYI - it's preferred to pas StringPiece by value.
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> Feel free to change or skip.

TIL - thank you!

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
media/audio/sounds/wav_audio_handler.cc:177: const base::StringPiece& wav_data)
{
On 2015/11/19 09:44:06, tommi wrote:
> Since this is a new function, it would be good to pass by value here at least.

Done.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
File media/audio/sounds/wav_audio_handler_unittest.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
media/audio/sounds/wav_audio_handler_unittest.cc:59: auto data =
std::string(kTestAudioData, arraysize(kTestAudioData) - 1);
On 2015/11/19 09:44:06, tommi wrote:
> please fix these to not use auto.  It's not needed.  Just do:
> 
> std::string data(kTestAudioData, arraysize(kTestAudioData) - 1);

Done. Also relied on implicit casting when passing |data| to Create()

[slan, 2015-11-19 19:05:27]

https://codereview.chromium.org/1453233002/diff/120001/media/audio/simple_sou...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/simple_sou...
media/audio/simple_sources.cc:173: reinterpret_cast<const
char*>(wav_file_data_.get()), file_length));
On 2015/11/19 18:57:16, slan wrote:
> On 2015/11/19 09:44:06, tommi wrote:
> > hmm... I guess shared_ptr could have helped here. As is, it's not exactly
> > obvious why the wav_file_data_ member variable exists in the first place
(it's
> > only used in this scope and just used to pass the data to this constructor).
> > Alternatively it might make sense at some point to pass ownership of the
data
> to
> > the WavAudioHandler (and then possibly change the type to
> scoped_ptr<uint8[]>). 
> > I don't know if any caller needs to also hold on to this data.
> > 
> > No action needed in this CL but if you agree with me, you could add a TODO
to
> > WavAudioHandler for either myself or you to do that.
> 
> Seems reasonable to remove it as a member. Done. Refactored the reading
function
> and added some test coverage.

Whoops, just read your comment more thoroughly. I don't see the copy in the ctor
as a huge deal, but I am willing to pass ownership if you'd like. StringPiece
makes it awfully nice to parse the headers out, though, so I might prefer we
keep that data type.

[tommi (sloooow) - chröme, 2015-11-20 09:45:23]

Lots of really good changes :)  If you have time to do the ownership change then
that would be great of course, but it's up to you.  One question about freeing
up memory.

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
File media/audio/sounds/wav_audio_handler_unittest.cc (right):

https://codereview.chromium.org/1453233002/diff/120001/media/audio/sounds/wav...
media/audio/sounds/wav_audio_handler_unittest.cc:59: auto data =
std::string(kTestAudioData, arraysize(kTestAudioData) - 1);
On 2015/11/19 18:57:16, slan wrote:
> On 2015/11/19 09:44:06, tommi wrote:
> > please fix these to not use auto.  It's not needed.  Just do:
> > 
> > std::string data(kTestAudioData, arraysize(kTestAudioData) - 1);
> 
> Done. Also relied on implicit casting when passing |data| to Create()

Acknowledged.

https://codereview.chromium.org/1453233002/diff/140001/media/audio/sounds/aud...
File media/audio/sounds/audio_stream_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/140001/media/audio/sounds/aud...
media/audio/sounds/audio_stream_handler.cc:195: if (IsInitialized()) {
nit: instead of IsInitialize(), can we check stream_?  I think we could also
lose the initialized_ member.

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
media/audio/simple_sources.cc:48: data_out->set(data.release(),
wav_file_length);
how is this memory freed?

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
File media/audio/simple_sources_unittest.cc (right):

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
media/audio/simple_sources_unittest.cc:89: ASSERT_EQ(arraysize(kTestAudioData) -
1, (size_t)temp.GetLength());
static_cast

[DaleCurtis, 2015-11-20 17:31:38]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2015-11-20 17:31:39]

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
media/audio/simple_sources.cc:48: data_out->set(data.release(),
wav_file_length);
On 2015/11/20 09:45:23, tommi wrote:
> how is this memory freed?

Drive by, StringPiece does not make a copy of the data, so if you free this
you'll now have a use-after-free :)

Presumably you're switching away from scoped_ptr because you want the size
information? I think instead you want something like
scoped_ptr<std::vector<uint8_t>> to get length and avoid copies.

[tommi (sloooow) - chröme, 2015-11-20 18:01:27]

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
media/audio/simple_sources.cc:48: data_out->set(data.release(),
wav_file_length);
On 2015/11/20 17:31:39, DaleCurtis wrote:
> On 2015/11/20 09:45:23, tommi wrote:
> > how is this memory freed?
> 
> Drive by, StringPiece does not make a copy of the data, so if you free this
> you'll now have a use-after-free :)
> 
> Presumably you're switching away from scoped_ptr because you want the size
> information? I think instead you want something like
> scoped_ptr<std::vector<uint8_t>> to get length and avoid copies.

At the moment I don't think this is ever freed actually.

I was thinking something along those lines, but the CL has already grown well
beyond the initial intent :)

[slan, 2015-11-20 18:09:37]

I've addressed some of the comments. 

I actually implemented passing ownership of the data into WavAudioHandler, but
it turns out that it is a non-trivial change (read: yak shave), and is not
correct IMHO. SoundsManager passes StringPieces from persistent resource classes
all the way down the stack to this class. You can see all of the separate
callsites for SoundsManager::Initialize(): 
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/sounds...

I have restored ownership of the data in FileSource, and added a comment to
prevent further confusion.

https://codereview.chromium.org/1453233002/diff/140001/media/audio/sounds/aud...
File media/audio/sounds/audio_stream_handler.cc (right):

https://codereview.chromium.org/1453233002/diff/140001/media/audio/sounds/aud...
media/audio/sounds/audio_stream_handler.cc:195: if (IsInitialized()) {
On 2015/11/20 09:45:23, tommi wrote:
> nit: instead of IsInitialize(), can we check stream_?  I think we could also
> lose the initialized_ member.

IsInitialized() now checks stream_, eliminating initialized_.

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
media/audio/simple_sources.cc:48: data_out->set(data.release(),
wav_file_length);
On 2015/11/20 17:31:39, DaleCurtis wrote:
> On 2015/11/20 09:45:23, tommi wrote:
> > how is this memory freed?
> 
> Drive by, StringPiece does not make a copy of the data, so if you free this
> you'll now have a use-after-free :)
> 
> Presumably you're switching away from scoped_ptr because you want the size
> information? I think instead you want something like
> scoped_ptr<std::vector<uint8_t>> to get length and avoid copies.

Thanks, Dale. This was a case of me not understanding StringPiece. I've changed
this back to its original signature (return a scoped_ptr<char[]> and taking
size_t* out param.

tommi@, if you would prefer scoped_ptr<vector<char>> to eliminate the size
param, we can do that too. I am impartial.

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
File media/audio/simple_sources_unittest.cc (right):

https://codereview.chromium.org/1453233002/diff/160001/media/audio/simple_sou...
media/audio/simple_sources_unittest.cc:89: ASSERT_EQ(arraysize(kTestAudioData) -
1, (size_t)temp.GetLength());
On 2015/11/20 09:45:23, tommi wrote:
> static_cast

Done, here and below.

[tommi (sloooow) - chröme, 2015-11-20 19:54:23]

lgtm! Thanks for going above and beyond on this one :)

[slan, 2015-11-20 20:03:33]

On 2015/11/20 19:54:23, tommi wrote:
> lgtm! Thanks for going above and beyond on this one :)

Happy to do it. Kicking off a dry-run. 

Dale, I will need your stamp before submitting. Please take a look at your
convenience.

Thanks.

[slan, 2015-11-20 20:03:53]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 20:05:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453233002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453233002/200001

[DaleCurtis, 2015-11-20 20:18:50]

Deferring to tommi@ for main review, lgtm for simple_sources and media.gyp

[commit-bot: I haz the power, 2015-11-20 20:19:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 20:19:49]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[slan, 2015-11-22 23:46:49]

The CQ bit was checked by slan@chromium.org

[slan, 2015-11-22 23:46:50]

The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org,
tommi@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1453233002/#ps220001
(title: "Export FileSource for component build + rebase")

[commit-bot: I haz the power, 2015-11-22 23:47:02]

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

[commit-bot: I haz the power, 2015-11-22 23:54:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-22 23:54:59]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[slan, 2015-11-23 17:11:05]

The CQ bit was checked by slan@chromium.org

[slan, 2015-11-23 17:11:06]

The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org,
tommi@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1453233002/#ps260001
(title: "Formatting")

[commit-bot: I haz the power, 2015-11-23 17:11:17]

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

[commit-bot: I haz the power, 2015-11-23 19:22:46]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-11-23 19:23:26]

Patchset 14 (id:??) landed as
https://crrev.com/344528fe16c4daf1d34bd59e09fc8e07d12a27f4
Cr-Commit-Position: refs/heads/master@{#361147}