Add is_encrypted() in VideoDecoderConfig.

Add is_encrypted() in VideoDecoderConfig.

This is needed so that decoders can check if the video stream is encrypted or not and decide if it can support decrypting and/or decodeing the stream.

BUG=141784
TEST=media_unittest, encrypted media demo.

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

[xhwang, 2012-09-14 23:22:09]

I am still debating on a few options and would like to hear your opinion.

1) Add is_encrypted() in DemuxerStream, which is what this patch is doing.
This better reflects the fact that encryption is added on top of encoding. It's
also a natural result of the fact that decoder config and encryption info come
from different places in the container (e.g. in WebM).

2) Add is_encrypted() in VideoDecoderConfig.
This is my original plan. Until I see how FFmpegConfigHelper is used to generate
the VideoDecoderConfig (http://goo.gl/GSiBy). I have the feeling  "ah,
encryption info isn't really a part of a 'decoder' config".

3) As proposed by acolwell@, having a new StreamConfig that contains
VideoDecoderConfig and is_encrypted.
I feel this is more or less similar to (1) but may come up with a cleaner
interface.

WDYT?

[acolwell GONE FROM CHROMIUM, 2012-09-15 00:25:24]

On 2012/09/14 23:22:09, xhwang wrote:
> I am still debating on a few options and would like to hear your opinion.
> 
> 1) Add is_encrypted() in DemuxerStream, which is what this patch is doing.
> This better reflects the fact that encryption is added on top of encoding.
It's
> also a natural result of the fact that decoder config and encryption info come
> from different places in the container (e.g. in WebM).
> 

I don't think downstream components should care that the information was
assembled from different places in the container.

The fact that changes need to be made at several layers of abstraction to
propagate this info leads me to believe we are doing something wrong here.

> 2) Add is_encrypted() in VideoDecoderConfig.
> This is my original plan. Until I see how FFmpegConfigHelper is used to
generate
> the VideoDecoderConfig (http://goo.gl/GSiBy). I have the feeling  "ah,
> encryption info isn't really a part of a 'decoder' config".

FFmpegConfigHelper is a hack to get things working quickly. It will eventually
go away and should not significantly influence the API design.

If you still plan to support "decrypt & decode" decoders then an argument could
be made that it actually makes sense to include this as part of the decoder
config.

> 
> 3) As proposed by acolwell@, having a new StreamConfig that contains
> VideoDecoderConfig and is_encrypted.
> I feel this is more or less similar to (1) but may come up with a cleaner
> interface.

Obviously I support this option, but if we are only talking about adding a
single bool then it would be overkill and trigger unnecessary work. If the
encryption info was more complicated, I'd prefer this path because it treats
encryption & decoder configuration as an atomic unit which completely reflects
the encoding of the stream. This information would always flow together so it
would be less likely to get out of sync.

http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h#ne...
media/base/demuxer_stream.h:73: virtual bool is_encrypted() = 0;
Why is this method necessary especially if it only signals that the content "is
potentially encrypted". It seems to me that the ChunkDemuxerStream should always
return true for this since it never knows when an encrypted initialization
segment could get appended.

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

http://codereview.chromium.org/10910293/diff/1/media/filters/chunk_demuxer.cc...
media/filters/chunk_demuxer.cc:371: is_encrypted_ = is_encrypted;
This is not correct. It will cause the is_encrypted_ state to be incorrect when
a config change happens. This method updates the "append side" config of the
SourceBufferStream and the DemuxerStream methods are supposed to reflect the
current playback position config. If you are currently playing encrypted content
and an unencrypted init segment is appended, the DemuxerStream will reflect that
the stream is not encrypted eventhough the current playback position hasn't
reached the config change yet.

http://codereview.chromium.org/10910293/diff/1/media/filters/chunk_demuxer.cc...
media/filters/chunk_demuxer.cc:379: is_encrypted_ = is_encrypted;
ditto

[ddorwin, 2012-09-15 00:39:40]

It might help the discussion to see [prototype] code showing how this would be
used. (Don't spend too much time on it, though.)

http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h#ne...
media/base/demuxer_stream.h:73: virtual bool is_encrypted() = 0;
Seems there should be some better indication of "potential", though I don't have
a concise suggestion.
is_potentially_encrypted(), has_encrypted_blocks(), may_be_encrypted().

http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h#ne...
media/base/demuxer_stream.h:73: virtual bool is_encrypted() = 0;
On 2012/09/15 00:25:24, acolwell wrote:
> Why is this method necessary especially if it only signals that the content
"is
> potentially encrypted". It seems to me that the ChunkDemuxerStream should
always
> return true for this since it never knows when an encrypted initialization
> segment could get appended.

We reinitialize decoders when the config changes between segments, right? That
would fit with the broader goal, which is to know whether we need to use a CDM
for decoding during initialization.

[ddorwin, 2012-09-15 03:12:04]

http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h#ne...
media/base/demuxer_stream.h:73: virtual bool is_encrypted() = 0;
On 2012/09/15 00:39:40, ddorwin wrote:
> On 2012/09/15 00:25:24, acolwell wrote:
> > Why is this method necessary especially if it only signals that the content
> "is
> > potentially encrypted". It seems to me that the ChunkDemuxerStream should
> always
> > return true for this since it never knows when an encrypted initialization
> > segment could get appended.
> 
> We reinitialize decoders when the config changes between segments, right? That
> would fit with the broader goal, which is to know whether we need to use a CDM
> for decoding during initialization.

After discussing this offline, I remembered that we decided that the decoder
must be selected once and used for the lifetime of the stream. The plan is to
enforce this in the spec, but for v0.1 we need some help to determine that we
should wait before initializing the decoder. That's where is_encrypted() comes
in.

So, addressing the initial comment:
1) We will not support that use case for now.
2) We hope to eliminate the need for this in the future.

[acolwell GONE FROM CHROMIUM, 2012-09-15 14:38:39]

On 2012/09/15 03:12:04, ddorwin wrote:
> http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h
> File media/base/demuxer_stream.h (right):
> 
>
http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h#ne...
> media/base/demuxer_stream.h:73: virtual bool is_encrypted() = 0;
> On 2012/09/15 00:39:40, ddorwin wrote:
> > On 2012/09/15 00:25:24, acolwell wrote:
> > > Why is this method necessary especially if it only signals that the
content
> > "is
> > > potentially encrypted". It seems to me that the ChunkDemuxerStream should
> > always
> > > return true for this since it never knows when an encrypted initialization
> > > segment could get appended.
> > 
> > We reinitialize decoders when the config changes between segments, right?
That
> > would fit with the broader goal, which is to know whether we need to use a
CDM
> > for decoding during initialization.
> 
> After discussing this offline, I remembered that we decided that the decoder
> must be selected once and used for the lifetime of the stream. The plan is to
> enforce this in the spec, but for v0.1 we need some help to determine that we
> should wait before initializing the decoder. That's where is_encrypted() comes
> in.
I do not agree with this restriction and I would not support any such language
in the MSE spec. I think it makes sense to support encrypted & non-encrypted
content in a single presentation. I can understand if this is not a v1
implementation goal, but I don't think we should put API's in place or add spec
text that restrict this.

I still haven't seen a compelling argument around why this should be separate
from the existing config structures. This change burdens several layers that
don't remotely care about encryption with propagating that info. This info also
follows the exact same path as the decoder configs so I don't understand the
benefit of keeping it separate.

> 
> So, addressing the initial comment:
> 1) We will not support that use case for now.
> 2) We hope to eliminate the need for this in the future.
This is fine. The existing mechanism allows rejection of config changes that are
not allowed. For example codec changes & any type of audio config change are
rejected by the current system. When the config change comes in the decoder can
simply check to see whether the config's has_encrypted_buffers() field has
changed and signal a decoder error if it has. The interactions between
DemuxerStream & the Decoder don't need to change to support this.

[xhwang, 2012-09-15 17:09:31]

On 2012/09/15 14:38:39, acolwell wrote:
> On 2012/09/15 03:12:04, ddorwin wrote:
> > http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h
> > File media/base/demuxer_stream.h (right):
> > 
> >
>
http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h#ne...
> > media/base/demuxer_stream.h:73: virtual bool is_encrypted() = 0;
> > On 2012/09/15 00:39:40, ddorwin wrote:
> > > On 2012/09/15 00:25:24, acolwell wrote:
> > > > Why is this method necessary especially if it only signals that the
> content
> > > "is
> > > > potentially encrypted". It seems to me that the ChunkDemuxerStream
should
> > > always
> > > > return true for this since it never knows when an encrypted
initialization
> > > > segment could get appended.
> > > 
> > > We reinitialize decoders when the config changes between segments, right?
> That
> > > would fit with the broader goal, which is to know whether we need to use a
> CDM
> > > for decoding during initialization.
> > 
> > After discussing this offline, I remembered that we decided that the decoder
> > must be selected once and used for the lifetime of the stream. The plan is
to
> > enforce this in the spec, but for v0.1 we need some help to determine that
we
> > should wait before initializing the decoder. That's where is_encrypted()
comes
> > in.
> I do not agree with this restriction and I would not support any such language
> in the MSE spec. I think it makes sense to support encrypted & non-encrypted
> content in a single presentation. I can understand if this is not a v1
> implementation goal, but I don't think we should put API's in place or add
spec
> text that restrict this.

I think the plan ddorwin mentioned is to change the EME spec such that the key
system
should be specified upfront instead of specified later using GenerateKeyRequest.
This
eliminates a bunch of headaches such as 1) user may call GKR multiple times w/
different
key systesm, 2) the pipeline needs to wait for GKR to pick a decryptor/decoder,
3) it's
not clear what the pipeline should do w/ unencrypted stream followed by
encrypted
stream. With this change, the pipeline will create/initialize the specified key
system
upfront and use it for decryption/decoding all the time.

ddorwin: how do we choose between decrypt-only and decrypt-and-decode? For
instance,
if a key system supports both, which one should be tried first? Should we always
try to do
decrypt-and-decode, and then decrypt-only if decrypt-and-decode doesn't work?
Even
worse, what if we choose decrypt-only first (e.g. the codec config isn't
supported by the key
system), then config changed and the key system can actually support
decrypt-and-decode
now? Should the spec say anything about this, or this should be up to the
implementation of
each browser?

> 
> I still haven't seen a compelling argument around why this should be separate
> from the existing config structures. This change burdens several layers that
> don't remotely care about encryption with propagating that info. This info
also
> follows the exact same path as the decoder configs so I don't understand the
> benefit of keeping it separate.

I think putting "is_encrypted" in VideoDecoderConfig makes sense. Will do that
if nobody
objects.
> 
> > 
> > So, addressing the initial comment:
> > 1) We will not support that use case for now.
> > 2) We hope to eliminate the need for this in the future.
> This is fine. The existing mechanism allows rejection of config changes that
are
> not allowed. For example codec changes & any type of audio config change are
> rejected by the current system. When the config change comes in the decoder
can
> simply check to see whether the config's has_encrypted_buffers() field has
> changed and signal a decoder error if it has. The interactions between
> DemuxerStream & the Decoder don't need to change to support this.

Will prevent encrypted/unencrypted switching with this for now. Thanks for the
tip.

[xhwang, 2012-09-16 18:23:02]

As discussed, moved is_encrypted() into VideoDecoderConfig. PTAL!

[ddorwin, 2012-09-17 04:46:43]

On 2012/09/15 17:09:31, xhwang wrote:
> On 2012/09/15 14:38:39, acolwell wrote:
> > On 2012/09/15 03:12:04, ddorwin wrote:
> > > http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h
> > > File media/base/demuxer_stream.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10910293/diff/1/media/base/demuxer_stream.h#ne...
> > > media/base/demuxer_stream.h:73: virtual bool is_encrypted() = 0;
> > > On 2012/09/15 00:39:40, ddorwin wrote:
> > > > On 2012/09/15 00:25:24, acolwell wrote:
> > > > > Why is this method necessary especially if it only signals that the
> > content
> > > > "is
> > > > > potentially encrypted". It seems to me that the ChunkDemuxerStream
> should
> > > > always
> > > > > return true for this since it never knows when an encrypted
> initialization
> > > > > segment could get appended.
> > > > 
> > > > We reinitialize decoders when the config changes between segments,
right?
> > That
> > > > would fit with the broader goal, which is to know whether we need to use
a
> > CDM
> > > > for decoding during initialization.
> > > 
> > > After discussing this offline, I remembered that we decided that the
decoder
> > > must be selected once and used for the lifetime of the stream. The plan is
> to
> > > enforce this in the spec, but for v0.1 we need some help to determine that
> we
> > > should wait before initializing the decoder. That's where is_encrypted()
> comes
> > > in.
> > I do not agree with this restriction and I would not support any such
language
> > in the MSE spec. I think it makes sense to support encrypted & non-encrypted
> > content in a single presentation. I can understand if this is not a v1
> > implementation goal, but I don't think we should put API's in place or add
> spec
> > text that restrict this.
> 
> I think the plan ddorwin mentioned is to change the EME spec such that the key
> system
> should be specified upfront instead of specified later using
GenerateKeyRequest.
> This
> eliminates a bunch of headaches such as 1) user may call GKR multiple times w/
> different
> key systesm, 2) the pipeline needs to wait for GKR to pick a
decryptor/decoder,
> 3) it's
> not clear what the pipeline should do w/ unencrypted stream followed by
> encrypted
> stream. With this change, the pipeline will create/initialize the specified
key
> system
> upfront and use it for decryption/decoding all the time.

Yes. If there is a point where MSE can allow decoders to change, then that's
fine. But EME cannot allow decoders to change at other times.

> ddorwin: how do we choose between decrypt-only and decrypt-and-decode? For
> instance,
> if a key system supports both, which one should be tried first? Should we
always
> try to do
> decrypt-and-decode, and then decrypt-only if decrypt-and-decode doesn't work?
> Even
> worse, what if we choose decrypt-only first (e.g. the codec config isn't
> supported by the key
> system), then config changed and the key system can actually support
> decrypt-and-decode
> now? Should the spec say anything about this, or this should be up to the
> implementation of
> each browser?

I think this is an implementation detail.
I think Chromium is going to have to do some work to support these two modes,
but we won't be able to do that until we move to the OO design and have a more
concrete implementation to evaluate. 

> 
> > 
> > I still haven't seen a compelling argument around why this should be
separate
> > from the existing config structures. This change burdens several layers that
> > don't remotely care about encryption with propagating that info. This info
> also
> > follows the exact same path as the decoder configs so I don't understand the
> > benefit of keeping it separate.
> 
> I think putting "is_encrypted" in VideoDecoderConfig makes sense. Will do that
> if nobody
> objects.
> > 
> > > 
> > > So, addressing the initial comment:
> > > 1) We will not support that use case for now.
> > > 2) We hope to eliminate the need for this in the future.
> > This is fine. The existing mechanism allows rejection of config changes that
> are
> > not allowed. For example codec changes & any type of audio config change are
> > rejected by the current system. When the config change comes in the decoder
> can
> > simply check to see whether the config's has_encrypted_buffers() field has
> > changed and signal a decoder error if it has. The interactions between
> > DemuxerStream & the Decoder don't need to change to support this.
> 
> Will prevent encrypted/unencrypted switching with this for now. Thanks for the
> tip.

[scherkus (not reviewing), 2012-09-17 15:54:10]

http://codereview.chromium.org/10910293/diff/8001/media/base/video_decoder_co...
File media/base/video_decoder_config.cc (right):

http://codereview.chromium.org/10910293/diff/8001/media/base/video_decoder_co...
media/base/video_decoder_config.cc:78:
UmaHistogramAspectRatio("Media.VideoVisibleAspectRatio", visible_rect);
do we want a histogram for is_encrypted?

http://codereview.chromium.org/10910293/diff/8001/media/base/video_decoder_co...
File media/base/video_decoder_config.h (right):

http://codereview.chromium.org/10910293/diff/8001/media/base/video_decoder_co...
media/base/video_decoder_config.h:135: void set_is_encrypted(bool is_encrypted);
note that there are no other setters on this class

it looks like the only uses are in ffmpeg_video_decoder_unittest.cc and
webm_stream_parser.cc but that they could both be changed to use Initialize()

[ddorwin, 2012-09-17 17:10:57]

LG for me. I'll defer to scherkus & acolwell.

[acolwell GONE FROM CHROMIUM, 2012-09-17 17:41:17]

looks pretty good. Just a few comments.

http://codereview.chromium.org/10910293/diff/8001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10910293/diff/8001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:104:
config_.set_is_encrypted(true);
Just copy the config_.Initialize() call from above and modify the encryption
bool so you don't need set_is_encrypted().

http://codereview.chromium.org/10910293/diff/8001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:255: NULL, false, false);
nit: I'm pretty sure this should be 0, false here and below.

http://codereview.chromium.org/10910293/diff/8001/media/filters/source_buffer...
File media/filters/source_buffer_stream_unittest.cc (right):

http://codereview.chromium.org/10910293/diff/8001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:1842: gfx::Rect(kNewCodedSize),
kNewCodedSize, NULL, false, false);
Should be 0, false

http://codereview.chromium.org/10910293/diff/8001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:1889: gfx::Rect(kNewCodedSize),
kNewCodedSize, NULL, false, false);
Should be 0, false

http://codereview.chromium.org/10910293/diff/8001/media/webm/webm_stream_pars...
File media/webm/webm_stream_parser.cc (right):

http://codereview.chromium.org/10910293/diff/8001/media/webm/webm_stream_pars...
media/webm/webm_stream_parser.cc:360: video_config.set_is_encrypted(true);
Use Initialize() here and CopyFrom() in an else so that you don't need
set_is_encrypted().

http://codereview.chromium.org/10910293/diff/8001/media/webm/webm_stream_pars...
media/webm/webm_stream_parser.cc:366: CHECK_LT(key_id_size, 2048);
This should not be a CHECK since it means an "Aw Snap" could be triggered from
JavaScript. If you want this to fail then you should return -1 here.

[xhwang, 2012-09-17 19:42:25]

Comments mostly resolved. PTAL!

http://codereview.chromium.org/10910293/diff/8001/media/base/video_decoder_co...
File media/base/video_decoder_config.cc (right):

http://codereview.chromium.org/10910293/diff/8001/media/base/video_decoder_co...
media/base/video_decoder_config.cc:78:
UmaHistogramAspectRatio("Media.VideoVisibleAspectRatio", visible_rect);
On 2012/09/17 15:54:10, scherkus wrote:
> do we want a histogram for is_encrypted?

Interesting point. But why not? The encryption is only on frame level and what
we are reporting here are not encrypted.

http://codereview.chromium.org/10910293/diff/8001/media/base/video_decoder_co...
File media/base/video_decoder_config.h (right):

http://codereview.chromium.org/10910293/diff/8001/media/base/video_decoder_co...
media/base/video_decoder_config.h:135: void set_is_encrypted(bool is_encrypted);
On 2012/09/17 15:54:10, scherkus wrote:
> note that there are no other setters on this class
> 
> it looks like the only uses are in ffmpeg_video_decoder_unittest.cc and
> webm_stream_parser.cc but that they could both be changed to use Initialize()

Done.

http://codereview.chromium.org/10910293/diff/8001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10910293/diff/8001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:104:
config_.set_is_encrypted(true);
On 2012/09/17 17:41:17, acolwell wrote:
> Just copy the config_.Initialize() call from above and modify the encryption
> bool so you don't need set_is_encrypted().

Done.

http://codereview.chromium.org/10910293/diff/8001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:255: NULL, false, false);
On 2012/09/17 17:41:17, acolwell wrote:
> nit: I'm pretty sure this should be 0, false here and below.

Oops, thanks for the catch. Done here and blow.

http://codereview.chromium.org/10910293/diff/8001/media/filters/source_buffer...
File media/filters/source_buffer_stream_unittest.cc (right):

http://codereview.chromium.org/10910293/diff/8001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:1842: gfx::Rect(kNewCodedSize),
kNewCodedSize, NULL, false, false);
On 2012/09/17 17:41:17, acolwell wrote:
> Should be 0, false

Done.

http://codereview.chromium.org/10910293/diff/8001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:1889: gfx::Rect(kNewCodedSize),
kNewCodedSize, NULL, false, false);
On 2012/09/17 17:41:17, acolwell wrote:
> Should be 0, false

Done.

http://codereview.chromium.org/10910293/diff/8001/media/webm/webm_stream_pars...
File media/webm/webm_stream_parser.cc (right):

http://codereview.chromium.org/10910293/diff/8001/media/webm/webm_stream_pars...
media/webm/webm_stream_parser.cc:360: video_config.set_is_encrypted(true);
On 2012/09/17 17:41:17, acolwell wrote:
> Use Initialize() here and CopyFrom() in an else so that you don't need
> set_is_encrypted().

Done.

http://codereview.chromium.org/10910293/diff/8001/media/webm/webm_stream_pars...
media/webm/webm_stream_parser.cc:366: CHECK_LT(key_id_size, 2048);
On 2012/09/17 17:41:17, acolwell wrote:
> This should not be a CHECK since it means an "Aw Snap" could be triggered from
> JavaScript. If you want this to fail then you should return -1 here.

Hmm, good catch. According to Frank, 2048bytes is the de facto upper limit of
key ID size, but it's not specified anywhere. I propose to remove this check
unless anybody objects.

Also change the CHECK on line 365 to DCHECK as it should always be true based on
our implementation in WebMContentEncodingsClient.

http://codereview.chromium.org/10910293/diff/13004/media/filters/ffmpeg_video...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10910293/diff/13004/media/filters/ffmpeg_video...
media/filters/ffmpeg_video_decoder_unittest.cc:142: EXPECT_EQ(VideoDecoder::kOk,
status);
Changed the order here since otherwise it gives funny results when the
expectation is not satisfied as follows:
 Value of: VideoDecoder::kOk
  Actual: 0
Expected: status
Which is: 1

[scherkus (not reviewing), 2012-09-17 21:42:59]

lgtm

[acolwell GONE FROM CHROMIUM, 2012-09-17 23:07:52]

LGTM

[commit-bot: I haz the power, 2012-09-17 23:18:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10910293/13004

[commit-bot: I haz the power, 2012-09-18 03:24:08]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-18 05:27:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10910293/13004

[commit-bot: I haz the power, 2012-09-18 05:36:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10910293/20002

[commit-bot: I haz the power, 2012-09-18 09:01:15]

Change committed as 157324