Implement ConstraintNotSatisfiedError for getusermedia

The old implementation didn't distinguish 'ConstraintNotSatisfiedError' in content side.
    It uses TrackStartError to summarize all the track error. This is inconsistent
    with the spec. In this Cl, I add the name of the constraint which cause the ConstraintNotSatisfiedError

R=tommi@chromium.org, yzshen@chromium.org, creis@chromium.org, miu@chromium.org
BUG=397057
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288921

[jiajia.qin, 2014-07-21 07:52:46]

Please help review this CL.

[jiajia.qin, 2014-07-23 10:24:47]

Hi, Is there anyone to review this CL? Please help to review this patch. Thank
you very much!

[jiajia.qin, 2014-07-24 01:52:28]

Could you please help to find the right person to review this CL? Thank you very
much.

[DaleCurtis, 2014-07-24 22:13:15]

I've updated the reviewers list with people more familiar

[miu, 2014-07-25 08:24:21]

Taking myself of the reviewer list.  While I am familiar with a lot of this
code, xians@ and/or his teammates would be best for making sure Chromium adheres
to the spec.

[no longer working on chromium, 2014-07-25 16:09:00]

Thanks for improvement, I will take my first look tonight.

[yzshen1, 2014-07-25 17:48:49]

I only reviewed .*pepper.* files. I think xians@ is the right person to comment
on other files. (Thanks!)

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_video_track_host.cc:541:
MediaStreamSource* source, content::MediaStreamRequestResult result) {
content:: is not needed.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_video_track_host.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_video_track_host.h:89:
content::MediaStreamRequestResult result,
content:: is not needed.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_video_track_host.h:90: const
blink::WebString& result_name = blink::WebString());
Do you really need this default value?
In general default value is not recommended.

[no longer working on chromium, 2014-07-25 19:27:36]

Looking good, I will take another closer look in the weekend.

https://codereview.chromium.org/391703002/diff/40001/content/public/common/me...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/public/common/me...
content/public/common/media_stream_request.h:78:
MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED,
curiously, could you please explain why MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED is
chosen? instead of MEDIA_DEVICE_CONSTRAINT_NOT_SUPPORTED ?

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl.h:102: const blink::WebString&
result_name = blink::WebString());
I don't understand why you need a result_name string here? why result is not
enough?
Also, adding a default param is not chromium style, what is your plan of
removing it?

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:145:
MEDIA_DEVICE_TRACK_START_FAILURE);
why isn't it MEDIA_DEVICE_SOURCE_START_FAILURE ?

[jiajia.qin, 2014-07-28 04:46:53]

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl.h:102: const blink::WebString&
result_name = blink::WebString());
On 2014/07/25 19:27:35, xians1 wrote:
> I don't understand why you need a result_name string here? why result is not
> enough?

The spec says if it is a constraint failure. Let error be a new MediaSteamError
object whose name attribute has the value ConstraintNotSatisfiedError and whose
constraintName attribute is set to the name of the constraint that caused the
error. Here, result is used to distinguish which error it is and result_name is
used to specify the constraintName if it is a ConstraintNotSatisfiedError.
Otherwise, the result_name will be set empty.
 
> Also, adding a default param is not chromium style, what is your plan of
> removing it?

Here, result_name is used if the result is CONSTRAINT_NOT_SATISFIED_ERROR.
Otherwise, if the result is other error cases, we don't need to set result_name.
So, here I add a default param.

[jiajia.qin, 2014-07-28 05:10:20]

https://codereview.chromium.org/391703002/diff/40001/content/public/common/me...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/public/common/me...
content/public/common/media_stream_request.h:78:
MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED,
On 2014/07/25 19:27:35, xians1 wrote:
> curiously, could you please explain why MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED
is
> chosen? instead of MEDIA_DEVICE_CONSTRAINT_NOT_SUPPORTED ? 

I think it's the same meaning. The final error name is
'ConstraintNotSatisfiedError', so I use MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED to
specify 'ConstraintNotSatisfiedError'. Do you think
'MEDIA_DEVICE_CONSTRAINT_NOT_SUPPORTED' is better?

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_video_track_host.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_video_track_host.h:90: const
blink::WebString& result_name = blink::WebString());
On 2014/07/25 17:48:49, yzshen1 wrote:
> Do you really need this default value?
> In general default value is not recommended.

In the spec, it says if it is the ConstraintNotSatisfiedError, we need to set
the constraintName that caused the error. But for other error type, it doesn't
say. So here I use a default value. If the result is
CONSTRAINT_NOT_SATISFIED_ERROR, I will transfer the result_name(constraint_name)
too. Otherwise, I only transfer the result and use the default value.

[jiajia.qin, 2014-07-28 05:52:24]

>
https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
> content/renderer/media/media_stream_video_capturer_source.cc:145:
> MEDIA_DEVICE_TRACK_START_FAILURE);
> why isn't it MEDIA_DEVICE_SOURCE_START_FAILURE ?

Good question. Actually, I don't know the differ between
MEDIA_DEVICE_SOURCE_START_FAILURE and MEDIA_DEVICE_TRACK_START_FAILURE.
I add MEDIA_DEVICE_SOURCE_START_FAILURE error type is because that in
mock_media_stream_video_source.cc FailToStartMockedSource()
function, I think calling 'OnStartDone(MEDIA_DEVICE_SOURCE_START_FAILURE)' is
better than OnStartDone(MEDIA_DEVICE_TRACK_START_FAILURE);
I think it's confused to have this two type of error. So, I will remove
MEDIA_DEVICE_SOURCE_START_FAILURE in my CL and uniformly
use MEDIA_DEVICE_TRACK_START_FAILURE.

[jiajia.qin, 2014-07-28 06:07:32]

Hi, xians@chromium.org / yzshen@chromium.org
  This Cl is mainly to support 'ConstraintNotSatisfiedError' in content side.
The result_name param is also served for that type error. You can find that from
file content/renderer/media/media_stream_video_source.cc. Could you please look
at that file and give your opinion if it is ok to use the current method? I will
wait for your
comment and then update the CL correspondingly (including your previous
comments). Thank you very much.

[yzshen1, 2014-07-28 17:37:18]

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_video_track_host.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_video_track_host.h:90: const
blink::WebString& result_name = blink::WebString());
On 2014/07/28 05:10:20, jiajia.qin wrote:
> On 2014/07/25 17:48:49, yzshen1 wrote:
> > Do you really need this default value?
> > In general default value is not recommended.
> 
> In the spec, it says if it is the ConstraintNotSatisfiedError, we need to set
> the constraintName that caused the error. But for other error type, it doesn't
> say. So here I use a default value. If the result is
> CONSTRAINT_NOT_SATISFIED_ERROR, I will transfer the
result_name(constraint_name)
> too. Otherwise, I only transfer the result and use the default value.

Could we pass an empty string in the cases that we don't need it, instead of
using a default value?

[no longer working on chromium, 2014-07-28 19:53:36]

On 2014/07/28 05:52:24, jiajia.qin wrote:
> >
>
https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
> > content/renderer/media/media_stream_video_capturer_source.cc:145:
> > MEDIA_DEVICE_TRACK_START_FAILURE);
> > why isn't it MEDIA_DEVICE_SOURCE_START_FAILURE ?
> 
> Good question. Actually, I don't know the differ between
> MEDIA_DEVICE_SOURCE_START_FAILURE and MEDIA_DEVICE_TRACK_START_FAILURE.
> I add MEDIA_DEVICE_SOURCE_START_FAILURE error type is because that in
> mock_media_stream_video_source.cc FailToStartMockedSource()
> function, I think calling 'OnStartDone(MEDIA_DEVICE_SOURCE_START_FAILURE)' is
> better than OnStartDone(MEDIA_DEVICE_TRACK_START_FAILURE);
> I think it's confused to have this two type of error. So, I will remove
> MEDIA_DEVICE_SOURCE_START_FAILURE in my CL and uniformly
> use MEDIA_DEVICE_TRACK_START_FAILURE.

You should not add MEDIA_DEVICE_SOURCE_START_FAILURE for the test code. please
remove.

[no longer working on chromium, 2014-07-28 19:55:08]

I have some comments, please address them.

Dale or Yuri, I will be on parental staring from tomorrow, can either of you
please take over the review?

Thanks,
SX

https://codereview.chromium.org/391703002/diff/40001/content/public/common/me...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/public/common/me...
content/public/common/media_stream_request.h:78:
MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED,
On 2014/07/28 05:10:20, jiajia.qin wrote:
> On 2014/07/25 19:27:35, xians1 wrote:
> > curiously, could you please explain why
MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED
> is
> > chosen? instead of MEDIA_DEVICE_CONSTRAINT_NOT_SUPPORTED ? 
> 
> I think it's the same meaning. The final error name is
> 'ConstraintNotSatisfiedError', so I use MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED
to
> specify 'ConstraintNotSatisfiedError'. Do you think
> 'MEDIA_DEVICE_CONSTRAINT_NOT_SUPPORTED' is better? 
> 

OK, then keep using MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED, I was not aware of
ConstraintNotSatisfiedError before.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl.h:154: void OnTrackStarted(
Avoid using default param.

If OnTrackStarted is used in many places, probably you should make a overload
OnTrackStarted method with const blink::WebString& result_name so that it won't
force all the clients to pass an empty string.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:223: blink::WebString*
constraintName) {
constraint_name or unsatisfied_constraint

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:111: void
OnStartDone(content::MediaStreamRequestResult result);
nit, you don't need content::

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:144: void
FinalizeAddTrack(content::MediaStreamRequestResult result);
ditto

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:150: blink::WebString
constraintName_;
how about changing the name to unsatisfied_constraint_

[miu, 2014-07-28 20:20:00]

On 2014/07/28 19:55:08, xians1 wrote:
> Dale or Yuri, I will be on parental staring from tomorrow, can either of you
> please take over the review?

Okay, I will do my best from what I know.

[jiajia.qin, 2014-07-30 05:08:30]

On 2014/07/28 17:37:18, yzshen1 wrote:
>
https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_media_stream_video_track_host.h (right):
> 
>
https://codereview.chromium.org/391703002/diff/40001/content/renderer/pepper/...
> content/renderer/pepper/pepper_media_stream_video_track_host.h:90: const
> blink::WebString& result_name = blink::WebString());
> On 2014/07/28 05:10:20, jiajia.qin wrote:
> > On 2014/07/25 17:48:49, yzshen1 wrote:
> > > Do you really need this default value?
> > > In general default value is not recommended.
> > 
> > In the spec, it says if it is the ConstraintNotSatisfiedError, we need to
set
> > the constraintName that caused the error. But for other error type, it
doesn't
> > say. So here I use a default value. If the result is
> > CONSTRAINT_NOT_SATISFIED_ERROR, I will transfer the
> result_name(constraint_name)
> > too. Otherwise, I only transfer the result and use the default value.
> 
> Could we pass an empty string in the cases that we don't need it, instead of
> using a default value?

Done

[jiajia.qin, 2014-07-30 05:08:50]

On 2014/07/28 19:53:36, xians1 wrote:
> On 2014/07/28 05:52:24, jiajia.qin wrote:
> > >
> >
>
https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
> > > content/renderer/media/media_stream_video_capturer_source.cc:145:
> > > MEDIA_DEVICE_TRACK_START_FAILURE);
> > > why isn't it MEDIA_DEVICE_SOURCE_START_FAILURE ?
> > 
> > Good question. Actually, I don't know the differ between
> > MEDIA_DEVICE_SOURCE_START_FAILURE and MEDIA_DEVICE_TRACK_START_FAILURE.
> > I add MEDIA_DEVICE_SOURCE_START_FAILURE error type is because that in
> > mock_media_stream_video_source.cc FailToStartMockedSource()
> > function, I think calling 'OnStartDone(MEDIA_DEVICE_SOURCE_START_FAILURE)'
is
> > better than OnStartDone(MEDIA_DEVICE_TRACK_START_FAILURE);
> > I think it's confused to have this two type of error. So, I will remove
> > MEDIA_DEVICE_SOURCE_START_FAILURE in my CL and uniformly
> > use MEDIA_DEVICE_TRACK_START_FAILURE.
> 
> You should not add MEDIA_DEVICE_SOURCE_START_FAILURE for the test code. please
> remove.

Done

[jiajia.qin, 2014-07-30 05:18:08]

Rebased and modified the code according reviews' comments. Please have a look
again.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl.h:154: void OnTrackStarted(
On 2014/07/28 19:55:08, xians1 wrote:
> Avoid using default param.
> 
> If OnTrackStarted is used in many places, probably you should make a overload
> OnTrackStarted method with const blink::WebString& result_name so that it
won't
> force all the clients to pass an empty string.
I have made a overload for GetUserMediaRequestFailed. For trackstarterror, it
will call GetUserMediaRequestFailed with const blink::WebString& result_name.
For other errors, keep the default function call.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:223: blink::WebString*
constraintName) {
On 2014/07/28 19:55:08, xians1 wrote:
> constraint_name or unsatisfied_constraint

Done.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:111: void
OnStartDone(content::MediaStreamRequestResult result);
On 2014/07/28 19:55:08, xians1 wrote:
> nit, you don't need content::

Done.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:144: void
FinalizeAddTrack(content::MediaStreamRequestResult result);
On 2014/07/28 19:55:08, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/391703002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:150: blink::WebString
constraintName_;
On 2014/07/28 19:55:08, xians1 wrote:
> how about changing the name to unsatisfied_constraint_

Done.

[yzshen1, 2014-07-30 15:39:24]

.*pepper.* LGTM

Please wait for other reviewers' LGTM.

[miu, 2014-07-30 19:37:23]

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:639: default:
It's generally not good practice to use "default" clauses when handling enum
values.  Please add handling for all values so that the compiler will force
someone to account for a new result value (if added in the future).

In addition, if there are cases that should not be reached, use NOTREACHED()
before the request_info->requestFailed() call, to ensure debug builds will crash
on a logic error.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:645: void
MediaStreamImpl::GetUserMediaRequestFailed(
IMHO, this shouldn't be a separate method.  Can you just add the new 3rd arg to
the existing one, and add these additional cases?

If you strongly disagree then, at the least, the name of this method should be
changed to indicate its specific meaning/difference from the other one. 
Something to make it more obvious when this method is supposed to handle a
failure versus the other.  Suggestion: StartUserMediaRequestFailed() or
CompleteUserMediaRequestFailed().

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:92: result_name_ =
result_name;
This is good, but where are the unit tests that check the value of
|result_name_| (and the code path that has to execute to reach this point)?

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:28: const
StreamDeviceInfo& device_info);
Since you're modifying this file, can you put this ctor decl on one line, as it
fits the 80 char limit?

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:276: // Here we record the
name of constraint that causes
This comment is stating the obvious, so you should just get rid of it.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:280: return candidates;
Sanity-check: With this change, you're now aborting the loop early on the first
constraint that eliminates all candidates.  Before, the code would loop through
all constraints unconditionally.  Are you sure there aren't any INTENDED side
effects that you've eliminated with this behavior change?

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:470:
FinalizeAddTrack(MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED);
re: My comment in the .h file:

The call to FindBestFormatWithConstraints() (a few lines above) should have an
unsatisfied_constraint string output argument, and then you can pass it to
FinalizeAddTrack() here.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:154: blink::WebString
unsatisfied_constraint_;
You don't want this here, as it obfuscates the lifetime of this string and the
code path that uses it.

Instead, you should pass it along the call stack.  See comment in .cc file for
more details.

[jiajia.qin, 2014-07-31 13:43:01]

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:639: default:
On 2014/07/30 19:37:23, miu wrote:
> It's generally not good practice to use "default" clauses when handling enum
> values.  Please add handling for all values so that the compiler will force
> someone to account for a new result value (if added in the future).
> 
> In addition, if there are cases that should not be reached, use NOTREACHED()
> before the request_info->requestFailed() call, to ensure debug builds will
crash
> on a logic error.

Done.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:645: void
MediaStreamImpl::GetUserMediaRequestFailed(
On 2014/07/30 19:37:23, miu wrote:
> IMHO, this shouldn't be a separate method.  Can you just add the new 3rd arg
to
> the existing one, and add these additional cases?
> 
> If you strongly disagree then, at the least, the name of this method should be
> changed to indicate its specific meaning/difference from the other one. 
> Something to make it more obvious when this method is supposed to handle a
> failure versus the other.  Suggestion: StartUserMediaRequestFailed() or
> CompleteUserMediaRequestFailed().
The new 3rd arg is only for 'TrackStartError'. For other errors, we don't need
to pass the 3rd arg. So I separate this method. I use
'GetUserMediaRequestTrackStartedFailed' to rename this new function. It seems
more meaningful.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:92: result_name_ =
result_name;
On 2014/07/30 19:37:23, miu wrote:
> This is good, but where are the unit tests that check the value of
> |result_name_| (and the code path that has to execute to reach this point)?
TEST_F(MediaStreamImplTest, MediaVideoSourceFailToStart) will reach this point.
But it's only for TrackStartError. So the result_name will be set empty string.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:28: const
StreamDeviceInfo& device_info);
On 2014/07/30 19:37:23, miu wrote:
> Since you're modifying this file, can you put this ctor decl on one line, as
it
> fits the 80 char limit?

Done.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:276: // Here we record the
name of constraint that causes
On 2014/07/30 19:37:23, miu wrote:
> This comment is stating the obvious, so you should just get rid of it.

Done.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:280: return candidates;
On 2014/07/30 19:37:23, miu wrote:
> Sanity-check: With this change, you're now aborting the loop early on the
first
> constraint that eliminates all candidates.  Before, the code would loop
through
> all constraints unconditionally.  Are you sure there aren't any INTENDED side
> effects that you've eliminated with this behavior change?
Yes, I am sure. According to the spec, 'ConstraintNotSatisfiedError: One of the
mandatory constraints could not be satisfied' says if one of mandatory
constraints is not satisfied, ConstraintNotSatisfiedError will be thrown.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:470:
FinalizeAddTrack(MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED);
On 2014/07/30 19:37:23, miu wrote:
> re: My comment in the .h file:
> 
> The call to FindBestFormatWithConstraints() (a few lines above) should have an
> unsatisfied_constraint string output argument, and then you can pass it to
> FinalizeAddTrack() here.

Done.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:154: blink::WebString
unsatisfied_constraint_;
On 2014/07/30 19:37:23, miu wrote:
> You don't want this here, as it obfuscates the lifetime of this string and the
> code path that uses it.
> 
> Instead, you should pass it along the call stack.  See comment in .cc file for
> more details.

Done.

[miu, 2014-07-31 19:30:10]

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:639: default:
On 2014/07/31 13:43:00, jiajia.qin wrote:
> On 2014/07/30 19:37:23, miu wrote:
> > It's generally not good practice to use "default" clauses when handling enum
> > values.  Please add handling for all values so that the compiler will force
> > someone to account for a new result value (if added in the future).
> > 
> > In addition, if there are cases that should not be reached, use NOTREACHED()
> > before the request_info->requestFailed() call, to ensure debug builds will
> crash
> > on a logic error.
> 
> Done.

You should still call request_info->requestFailed() and break after the
NOTREACHED(), since non-debug binaries will pass by the NOTREACHED().

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:92: result_name_ =
result_name;
On 2014/07/31 13:43:00, jiajia.qin wrote:
> On 2014/07/30 19:37:23, miu wrote:
> > This is good, but where are the unit tests that check the value of
> > |result_name_| (and the code path that has to execute to reach this point)?
> TEST_F(MediaStreamImplTest, MediaVideoSourceFailToStart) will reach this
point.
> But it's only for TrackStartError. So the result_name will be set empty
string.
> 

So, you can copy-and-paste that test, and modify it a little so the result will
be MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED with a non-empty string, right?  ;-)

https://codereview.chromium.org/391703002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/391703002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:658: NOTREACHED();
ditto: call request_info->requestFailed() after NOTREACHED()

[jiajia.qin, 2014-08-01 05:25:33]

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:639: default:
On 2014/07/31 19:30:09, miu wrote:
> On 2014/07/31 13:43:00, jiajia.qin wrote:
> > On 2014/07/30 19:37:23, miu wrote:
> > > It's generally not good practice to use "default" clauses when handling
enum
> > > values.  Please add handling for all values so that the compiler will
force
> > > someone to account for a new result value (if added in the future).
> > > 
> > > In addition, if there are cases that should not be reached, use
NOTREACHED()
> > > before the request_info->requestFailed() call, to ensure debug builds will
> > crash
> > > on a logic error.
> > 
> > Done.
> 
> You should still call request_info->requestFailed() and break after the
> NOTREACHED(), since non-debug binaries will pass by the NOTREACHED().
> 

Done.

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/391703002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:92: result_name_ =
result_name;
On 2014/07/31 19:30:09, miu wrote:
> On 2014/07/31 13:43:00, jiajia.qin wrote:
> > On 2014/07/30 19:37:23, miu wrote:
> > > This is good, but where are the unit tests that check the value of
> > > |result_name_| (and the code path that has to execute to reach this
point)?
> > TEST_F(MediaStreamImplTest, MediaVideoSourceFailToStart) will reach this
> point.
> > But it's only for TrackStartError. So the result_name will be set empty
> string.
> > 
> 
> So, you can copy-and-paste that test, and modify it a little so the result
will
> be MEDIA_DEVICE_CONSTRAINT_NOT_SATISFIED with a non-empty string, right?  ;-)

Done.

https://codereview.chromium.org/391703002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/391703002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:658: NOTREACHED();
On 2014/07/31 19:30:10, miu wrote:
> ditto: call request_info->requestFailed() after NOTREACHED()

Done.

[miu, 2014-08-01 05:27:40]

lgtm

[jiajia.qin, 2014-08-01 05:36:17]

The CQ bit was checked by jiajia.qin@intel.com

[commit-bot: I haz the power, 2014-08-01 05:39:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiajia.qin@intel.com/391703002/100001

[jiajia.qin, 2014-08-01 08:07:50]

Please hold on reviewing this patch. I just found this Cl has some error which
may disable the media stream. When I solve it, I will ping you. Thank you.

[commit-bot: I haz the power, 2014-08-01 13:38:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 13:43:24]

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

[commit-bot: I haz the power, 2014-08-01 13:43:25]

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

[jiajia.qin, 2014-08-04 07:41:11]

There are two errors in patch set 6. 
The first error is in media_stream_impl.cc UserMediaRequestInfo::OnTrackStarted.
I broke the original implementation and didn't wait for all tracks done. That
will result a new track' result will over the last one' result which is wrong
(specially if the first track failed and the second track succeeded, the whole
result won't happen error which was wrong).
The other error is I misunderstand  MediaStreamVideoSource::FinalizeAddTrack. In
patch 6, I passed two params to FinalizeAddTrack. However, based the original
implementation, FinalizeAddTrack means checking all the tracks in one source and
return the result. I shouldn't use one result to represent all tracks' results.

The newest patch set 7 has resolved the above two problems. Please review again.
Thank you very much!

[miu, 2014-08-05 03:35:53]

lgtm, but please address these two things before committing:

https://codereview.chromium.org/391703002/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/391703002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:548: blink::WebString
unsatisfied_constraint;
nit: Please swap this line and the next line, for consistency with the ordering
of these two values throughout the rest of the code.

https://codereview.chromium.org/391703002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:553:
&unsatisfied_constraint).empty())
nit: indentation: Align this to the right of the opening parenthesis on the line
above.

[jiajia.qin, 2014-08-05 05:39:25]

https://codereview.chromium.org/391703002/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/391703002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:548: blink::WebString
unsatisfied_constraint;
On 2014/08/05 03:35:53, miu wrote:
> nit: Please swap this line and the next line, for consistency with the
ordering
> of these two values throughout the rest of the code.

Done.

https://codereview.chromium.org/391703002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:553:
&unsatisfied_constraint).empty())
On 2014/08/05 03:35:53, miu wrote:
> nit: indentation: Align this to the right of the opening parenthesis on the
line
> above.

Done.

[jiajia.qin, 2014-08-05 05:40:41]

The CQ bit was checked by jiajia.qin@intel.com

[commit-bot: I haz the power, 2014-08-05 05:41:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiajia.qin@intel.com/391703002/140001

[commit-bot: I haz the power, 2014-08-05 07:09:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-05 07:13:05]

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

[commit-bot: I haz the power, 2014-08-05 07:13:06]

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

[jiajia.qin, 2014-08-05 10:12:31]

The CQ bit was checked by jiajia.qin@intel.com

[commit-bot: I haz the power, 2014-08-05 10:13:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiajia.qin@intel.com/391703002/160001

[commit-bot: I haz the power, 2014-08-05 12:57:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-05 12:59:45]

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

[commit-bot: I haz the power, 2014-08-05 12:59:46]

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

[jiajia.qin, 2014-08-06 01:16:45]

Hi miu,
I checked the failed trybots. The error is caused by missing LGTM from an OWNER
for these files. Could you please help find the proper people to give 'LGTM' so
that I can commit this CL successfully?

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/browser/media/webrtc_getusermedia_browsertest.cc
    content/public/common/media_stream_request.h
    content/renderer/media/media_stream_audio_source.cc
    content/renderer/media/media_stream_impl.cc
    content/renderer/media/media_stream_impl.h
    content/renderer/media/media_stream_impl_unittest.cc
    content/renderer/media/media_stream_source.h
    content/renderer/media/media_stream_video_capture_source_unittest.cc
    content/renderer/media/media_stream_video_capturer_source.cc
    content/renderer/media/media_stream_video_capturer_source.h
    content/renderer/media/media_stream_video_source.cc
    content/renderer/media/media_stream_video_source.h
    content/renderer/media/media_stream_video_source_unittest.cc
    content/renderer/media/mock_media_stream_video_source.cc
    content/renderer/media/webrtc/media_stream_remote_video_source.cc
    content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc
    content/renderer/media/webrtc/video_destination_handler.cc

[miu, 2014-08-06 01:25:28]

tommi: PTAL for OWNERS approval.

[jiajia.qin, 2014-08-08 01:24:48]

On 2014/08/06 01:25:28, miu wrote:
> tommi: PTAL for OWNERS approval.

Seems that tommi is not available currently.

miu, could you please find another person to look at this cl? or you think we
can just wait until tommi or xians come back?

[tommi (sloooow) - chröme, 2014-08-08 13:42:55]

rs lgtm.  sorry for the delay, things are a bit crazy right now.

[tommi (sloooow) - chröme, 2014-08-08 13:43:01]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-08-08 13:44:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiajia.qin@intel.com/391703002/160001

[commit-bot: I haz the power, 2014-08-08 13:47:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-08 13:49:22]

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

[commit-bot: I haz the power, 2014-08-08 13:49:23]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jiajia.qin, 2014-08-11 03:19:48]

Rebase the code to resolve the conflict.
Add creis to have a look the missing owner reviewers for file:
content/public/common/media_stream_request.h.

[jiajia.qin, 2014-08-11 09:02:20]

The CQ bit was checked by jiajia.qin@intel.com

[commit-bot: I haz the power, 2014-08-11 09:02:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiajia.qin@intel.com/391703002/180001

[commit-bot: I haz the power, 2014-08-11 12:37:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-11 12:40:34]

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

[commit-bot: I haz the power, 2014-08-11 12:40:36]

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

[Charlie Reis, 2014-08-11 17:19:07]

[+jam as FYI for content/public change]

content/public/common/media_stream_request.h LGTM.

nit: Please update the "R=" line in your CL description, since you ended up with
a different set of reviewers.

[jiajia.qin, 2014-08-12 01:15:03]

The CQ bit was checked by jiajia.qin@intel.com

[commit-bot: I haz the power, 2014-08-12 01:33:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiajia.qin@intel.com/391703002/200001

[commit-bot: I haz the power, 2014-08-12 05:48:38]

Change committed as 288921