Let Contraints Controll Mute/Unmute Audio Local Playback For Desktop Sharing

Let Contraints Controll Mute/Unmute Audio Local Playback For Desktop Sharing

In this CL, we did the following things:
1. Add an entry for audio constraints of getUserMedia API, namely
googDisableLocalEcho.

2. The constraint works for the case of desktop sharing.

3. If set googDisableLocalEcho = true, then we will mute the
local playback of the sharing audio. Namely for full screen share
we will mute the system audio, and for tab share, we will mute
the corresponding tab. Vice versa.

BUG=647801
Committed: https://crrev.com/f6ca1ed1a2d80ce5f22c10a2a5cda30b1c44bccd
Cr-Commit-Position: refs/heads/master@{#425190}

[qiangchen, 2016-08-31 16:49:12]

Description was changed from

==========
Let Contraints Controll Mute/Unmute Source Audio For Desktop Sharing (NonBlink
Part)

BUG=
==========

to

==========
Let Contraints Controll Mute/Unmute Source Audio For Desktop Sharing
BUG=
==========

[qiangchen, 2016-09-06 16:57:05]

Patchset #1 (id:1) has been deleted

[qiangchen, 2016-09-06 16:57:11]

Patchset #1 (id:20001) has been deleted

[qiangchen, 2016-09-06 16:57:15]

Patchset #1 (id:40001) has been deleted

[qiangchen, 2016-09-06 17:01:28]

qiangchen@chromium.org changed reviewers:
+ miu@chromium.org

[qiangchen, 2016-09-06 17:01:29]

miu@: Can you glance this CL? Is this what we want?

Thanks.

[miu, 2016-09-07 21:10:03]

Took a quick glance. Looks good at a high level.

I looked at this mostly from the perspective of the tab capture device IDs. I'm
not sure how the desktop/window capture device IDs look, but perhaps they could
behave similarly so that we don't have to pass around extra "bool mute" flags
everywhere? Would this work?

https://codereview.chromium.org/2291893002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2291893002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:261: video_type_,
mute_source));
Seems like the extra bool argument is not needed, since the device ID will
contain the mute option flag. That way, you won't need the |mute_source| member
added to this class either.

https://codereview.chromium.org/2291893002/diff/60001/content/public/browser/...
File content/public/browser/web_contents_media_capture_id.h (right):

https://codereview.chromium.org/2291893002/diff/60001/content/public/browser/...
content/public/browser/web_contents_media_capture_id.h:64: static bool
ExtractOptions(const std::string& device_id,
Looking at the calls to this function, only one of the two bool* is ever
provided. This suggests you should either: 1) Keep the
IsAutoThrottlingOptionSet() function and add a new IsMuteSourceOptionSet()
function; or 2) Delete the IsAudioThrottlingOptionSet() function and make all
callers just use Parse() and then read the bool field they are interested in
from the WebContentsMediaCaptureId struct.

I'll leave it up to you which you think fits cleanest. ;)

https://codereview.chromium.org/2291893002/diff/60001/content/public/common/m...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/2291893002/diff/60001/content/public/common/m...
content/public/common/media_stream_request.h:208: bool mute_source);
Since the mute option is a part of the device ID, do we need the extra "bool
mute_source" here?

[qiangchen, 2016-09-09 18:45:24]

Sorry, late responded. Was fixing several UI bugs.

Replied. 

And do you have suggestions on the constraint name? Do you need to prefix it by
"goog", because this is not W3C standard.

https://codereview.chromium.org/2291893002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2291893002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:261: video_type_,
mute_source));
On 2016/09/07 21:10:03, miu wrote:
> Seems like the extra bool argument is not needed, since the device ID will
> contain the mute option flag. That way, you won't need the |mute_source|
member
> added to this class either.

We need it. Because at this point, the device_id is still encrypted, we need to
pass |mute_source| info into the DesktopCaptureAccessHandler, and it is in the
DesktopCaptureAccessHandler we look up the registry and translate id into a
DesktopMediaID and prepare the |mute_source| information into the
DesktopMediaID.

https://codereview.chromium.org/2291893002/diff/60001/content/public/common/m...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/2291893002/diff/60001/content/public/common/m...
content/public/common/media_stream_request.h:208: bool mute_source);
On 2016/09/07 21:10:03, miu wrote:
> Since the mute option is a part of the device ID, do we need the extra "bool
> mute_source" here?

Ditto.

[qiangchen, 2016-09-09 23:30:11]

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

[commit-bot: I haz the power, 2016-09-09 23:30:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qiangchen, 2016-09-09 23:32:08]

https://codereview.chromium.org/2291893002/diff/60001/content/public/browser/...
File content/public/browser/web_contents_media_capture_id.h (right):

https://codereview.chromium.org/2291893002/diff/60001/content/public/browser/...
content/public/browser/web_contents_media_capture_id.h:64: static bool
ExtractOptions(const std::string& device_id,
On 2016/09/07 21:10:03, miu wrote:
> Looking at the calls to this function, only one of the two bool* is ever
> provided. This suggests you should either: 1) Keep the
> IsAutoThrottlingOptionSet() function and add a new IsMuteSourceOptionSet()
> function; or 2) Delete the IsAudioThrottlingOptionSet() function and make all
> callers just use Parse() and then read the bool field they are interested in
> from the WebContentsMediaCaptureId struct.
> 
> I'll leave it up to you which you think fits cleanest. ;)

Done.

[commit-bot: I haz the power, 2016-09-09 23:34:03]

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

[commit-bot: I haz the power, 2016-09-09 23:34:04]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qiangchen, 2016-09-12 19:35:11]

Patchset #3 (id:100001) has been deleted

[miu, 2016-09-12 23:13:12]

Looking good. Mostly small things:

https://codereview.chromium.org/2291893002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2291893002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:357: bool
mute_source;
naming nit: We're not really muting the source, we're muting output to the local
audio device. So, for clarity, this should be named something like
|disable_local_echo| or |disable_local_output|. (IMHO, the first one's better
since it also implies there's a capture/loop-back mechanism involved, similar to
how we describe "local echo" for console output.)

...and, please change everywhere else to match (including StreamControls, IPC
message field, WebContentsMediaCaptureId data member and method args,
MediaStreamRequest, etc).

Also, please add a comment here like: "Set to true to prevent
captured/looped-back audio from also playing out locally."

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.cc (right):

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:15: static char
kEnableThrottlingFlag[] = "throttling=auto";
C++11 update: Please make these new strings constexpr's instead:

  constexpr char kEnableThrottlingFlag[] = "throttling=auto";
  constexpr char kDisableLocalEchoFlag[] = "local_echo=false";
  ...

(Note: kWebContenstCaptureScheme cannot be a constexpr because storage must be
allocated for external use. The rest are internally used within this .cc file.)

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:17: static char
kOptionStart[] = "?";
For the separators, use a char instead of a char[]. This reduces space and
run-time overhead:

  constexpr char kOptionStart = '?';
  constexpr char kOptionSeparator = '&';

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:108: char* connector =
kOptionStart;
Just char (see prior comment).

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.h (right):

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
content/public/browser/web_contents_media_capture_id.h:49: static bool
TryParse(const std::string& str,
nit suggestion: Just "Parse()". It's a common pattern for Parse() operations to
be expected to fail (when false is returned).

https://codereview.chromium.org/2291893002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/2291893002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:86: const
char kMuteSourceAudio[] = "muteSourceAudio";
In answer to your question: Yes, this does need the "goog" prefix if it's not
part of the W3C spec. (And, given my prior comments, I suggest naming it
"googDisableLocalEcho" or "googDisableLocalOutput".)

[qiangchen, 2016-09-13 21:46:57]

https://codereview.chromium.org/2291893002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2291893002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:357: bool
mute_source;
On 2016/09/12 23:13:12, miu wrote:
> naming nit: We're not really muting the source, we're muting output to the
local
> audio device. So, for clarity, this should be named something like
> |disable_local_echo| or |disable_local_output|. (IMHO, the first one's better
> since it also implies there's a capture/loop-back mechanism involved, similar
to
> how we describe "local echo" for console output.)
> 
> ...and, please change everywhere else to match (including StreamControls, IPC
> message field, WebContentsMediaCaptureId data member and method args,
> MediaStreamRequest, etc).
> 
> Also, please add a comment here like: "Set to true to prevent
> captured/looped-back audio from also playing out locally."

Done.

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.cc (right):

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:15: static char
kEnableThrottlingFlag[] = "throttling=auto";
On 2016/09/12 23:13:12, miu wrote:
> C++11 update: Please make these new strings constexpr's instead:
> 
>   constexpr char kEnableThrottlingFlag[] = "throttling=auto";
>   constexpr char kDisableLocalEchoFlag[] = "local_echo=false";
>   ...
> 
> (Note: kWebContenstCaptureScheme cannot be a constexpr because storage must be
> allocated for external use. The rest are internally used within this .cc
file.)

Done.

I think we can constexpr for kWebContentsCaptureScheme, because no other files
access it.

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:17: static char
kOptionStart[] = "?";
On 2016/09/12 23:13:12, miu wrote:
> For the separators, use a char instead of a char[]. This reduces space and
> run-time overhead:
> 
>   constexpr char kOptionStart = '?';
>   constexpr char kOptionSeparator = '&';

Done.

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:108: char* connector =
kOptionStart;
On 2016/09/12 23:13:12, miu wrote:
> Just char (see prior comment).

Done.

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.h (right):

https://codereview.chromium.org/2291893002/diff/120001/content/public/browser...
content/public/browser/web_contents_media_capture_id.h:49: static bool
TryParse(const std::string& str,
On 2016/09/12 23:13:12, miu wrote:
> nit suggestion: Just "Parse()". It's a common pattern for Parse() operations
to
> be expected to fail (when false is returned).

Done.

https://codereview.chromium.org/2291893002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/2291893002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:86: const
char kMuteSourceAudio[] = "muteSourceAudio";
On 2016/09/12 23:13:12, miu wrote:
> In answer to your question: Yes, this does need the "goog" prefix if it's not
> part of the W3C spec. (And, given my prior comments, I suggest naming it
> "googDisableLocalEcho" or "googDisableLocalOutput".)

Done.

[qiangchen, 2016-09-16 20:42:41]

Description was changed from

==========
Let Contraints Controll Mute/Unmute Source Audio For Desktop Sharing
BUG=
==========

to

==========
Let Contraints Controll Mute/Unmute Audio Local Playback For Desktop Sharing

In this CL, we did the following things:
1. Add an entry for audio constraints of getUserMedia API, namely
googDisableLocalEcho.

2. The constraint works for the case of desktop sharing.

3. If set googDisableLocalEcho = true, then we will mute the 
local playback of the sharing audio. Namely for full screen share
we will mute the system audio, and for tab share, we will mute
the corresponding tab. Vice versa.

BUG=647801
==========

[qiangchen, 2016-09-16 20:42:41]

qiangchen@chromium.org changed reviewers:
+ brettw@chromium.org, dcheng@chromium.org

[qiangchen, 2016-09-16 20:48:51]

brettw@ and dcheng@ can you take a look at this CL?

Thanks.

If you think you are not the right reviewer, feel free to give me an
recommendation on who should look at this CL.

[miu, 2016-09-16 21:22:48]

lgtm % nits:

https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.cc (right):

https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:16: constexpr char
kMuteSourceFlag[] = "disable_local_echo=true";
nit: It's fine for the C++ variables to be named |disable_local_echo| (a decent
choice). However, following a general style guideline of boolean values assigned
to "negative naming" this flag should probably be:

  constexpr char kMuteSourceFlag[] = "local_echo=false";

The same may apply in the naming of the MediaStreamTrackConstraint
(googDisableLocalEcho vs. googLocalEcho or googEnableLocalEcho). You'll have to
work with the blink OWNER on that.

Also, please s/kMuteSourceFlag/kDisableLocalEchoFlag/.

https://codereview.chromium.org/2291893002/diff/140001/content/public/common/...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/2291893002/diff/140001/content/public/common/...
content/public/common/media_stream_request.h:259: // Flag for desktop share to
indicate whether to mute the source audio during
nit: s/desktop share/Desktop or WebContents capture/

and

s/mute the source audio/prevent playing the captured audio locally/

(note: "WebContents" may sound slightly weird, but "tab" is not a concept in
src/content code)

[qiangchen, 2016-09-16 22:02:24]

https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.cc (right):

https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:16: constexpr char
kMuteSourceFlag[] = "disable_local_echo=true";
On 2016/09/16 21:22:48, miu wrote:
> nit: It's fine for the C++ variables to be named |disable_local_echo| (a
decent
> choice). However, following a general style guideline of boolean values
assigned
> to "negative naming" this flag should probably be:
> 
>   constexpr char kMuteSourceFlag[] = "local_echo=false";
> 
> The same may apply in the naming of the MediaStreamTrackConstraint
> (googDisableLocalEcho vs. googLocalEcho or googEnableLocalEcho). You'll have
to
> work with the blink OWNER on that.
> 
> Also, please s/kMuteSourceFlag/kDisableLocalEchoFlag/.

Done.

https://codereview.chromium.org/2291893002/diff/140001/content/public/common/...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/2291893002/diff/140001/content/public/common/...
content/public/common/media_stream_request.h:259: // Flag for desktop share to
indicate whether to mute the source audio during
On 2016/09/16 21:22:48, miu wrote:
> nit: s/desktop share/Desktop or WebContents capture/
> 
> and
> 
> s/mute the source audio/prevent playing the captured audio locally/
> 
> (note: "WebContents" may sound slightly weird, but "tab" is not a concept in
> src/content code)

Done.

[dcheng, 2016-09-16 22:36:04]

https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
File content/public/browser/desktop_media_id.cc (right):

https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
content/public/browser/desktop_media_id.cc:129: //                        
window:"window_id:aura_id".
It doesn't have to be done in this CL... but can we use something more
structured than a std::string to transport this information? While I think the
current parsing code is safe, it'd make it even easier if I didn't have to read
any parsing code =)

[qiangchen, 2016-09-16 22:45:52]

replied. Thank you for your suggestion.

https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
File content/public/browser/desktop_media_id.cc (right):

https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
content/public/browser/desktop_media_id.cc:129: //                        
window:"window_id:aura_id".
On 2016/09/16 22:36:04, dcheng wrote:
> It doesn't have to be done in this CL... but can we use something more
> structured than a std::string to transport this information? While I think the
> current parsing code is safe, it'd make it even easier if I didn't have to
read
> any parsing code =)

Good suggestion. But I think one reason we use string is that when we first
wrote the media code, we support camera, microphone etc, and their device_ids
are strings we got from OS. Later when we add desktop capture functionality, we
made up some format to represent those IDs, to avoid changing all the parameter
signatures along the code paths.

[dcheng, 2016-09-20 00:46:43]

On 2016/09/16 22:45:52, qiangchenC wrote:
> replied. Thank you for your suggestion.
> 
>
https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
> File content/public/browser/desktop_media_id.cc (right):
> 
>
https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
> content/public/browser/desktop_media_id.cc:129: //                        
> window:"window_id:aura_id".
> On 2016/09/16 22:36:04, dcheng wrote:
> > It doesn't have to be done in this CL... but can we use something more
> > structured than a std::string to transport this information? While I think
the
> > current parsing code is safe, it'd make it even easier if I didn't have to
> read
> > any parsing code =)
> 
> Good suggestion. But I think one reason we use string is that when we first
> wrote the media code, we support camera, microphone etc, and their device_ids
> are strings we got from OS. Later when we add desktop capture functionality,
we
> made up some format to represent those IDs, to avoid changing all the
parameter
> signatures along the code paths.

OK, I'm not asking for it in this CL, but can we figure this out in a followup
CL? it's not very nice to parse options and stuff out of device IDs like this,
and it should be relatively easy to add a wrapper type to encapsulate all the
different types of IDs we now plumb through.

[qiangchen, 2016-09-20 17:44:50]

On 2016/09/20 00:46:43, dcheng wrote:
> On 2016/09/16 22:45:52, qiangchenC wrote:
> > replied. Thank you for your suggestion.
> > 
> >
>
https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
> > File content/public/browser/desktop_media_id.cc (right):
> > 
> >
>
https://codereview.chromium.org/2291893002/diff/140001/content/public/browser...
> > content/public/browser/desktop_media_id.cc:129: //                        
> > window:"window_id:aura_id".
> > On 2016/09/16 22:36:04, dcheng wrote:
> > > It doesn't have to be done in this CL... but can we use something more
> > > structured than a std::string to transport this information? While I think
> the
> > > current parsing code is safe, it'd make it even easier if I didn't have to
> > read
> > > any parsing code =)
> > 
> > Good suggestion. But I think one reason we use string is that when we first
> > wrote the media code, we support camera, microphone etc, and their
device_ids
> > are strings we got from OS. Later when we add desktop capture functionality,
> we
> > made up some format to represent those IDs, to avoid changing all the
> parameter
> > signatures along the code paths.
> 
> OK, I'm not asking for it in this CL, but can we figure this out in a followup
> CL? it's not very nice to parse options and stuff out of device IDs like this,
> and it should be relatively easy to add a wrapper type to encapsulate all the
> different types of IDs we now plumb through.

Added a TODO and filed a crbug for tracking.

[dcheng, 2016-09-20 22:58:28]

https://codereview.chromium.org/2291893002/diff/180001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.cc (right):

https://codereview.chromium.org/2291893002/diff/180001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:68: if (auto_throttling
&& component.compare(kEnableThrottlingFlag) == 0)
Why would either of these be null? We should just DCHECK() that we never pass a
non-null value in. Also, what is the default value if they are not specified? It
seems like they would be uninitialized.

[qiangchen, 2016-09-20 23:23:44]

Patchset #7 (id:200001) has been deleted

[qiangchen, 2016-09-20 23:25:33]

set default value to option parameters.

https://codereview.chromium.org/2291893002/diff/180001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.cc (right):

https://codereview.chromium.org/2291893002/diff/180001/content/public/browser...
content/public/browser/web_contents_media_capture_id.cc:68: if (auto_throttling
&& component.compare(kEnableThrottlingFlag) == 0)
On 2016/09/20 22:58:28, dcheng wrote:
> Why would either of these be null? We should just DCHECK() that we never pass
a
> non-null value in. Also, what is the default value if they are not specified?
It
> seems like they would be uninitialized.

Done.

[dcheng, 2016-09-20 23:34:40]

LGTM. I realize this is legacy, but I would appreciate it if we could prioritize
making the IDs into something with a bit more structure: different types of IDs
should have distinct C++ types, things that are bools should be bools, not
parsed out of an options string. Just serializing/deserializing bits like this
from a string leads to complicated security reviews (since I have to worried
about missing potential edge cases in logic, such as not initializing the bool
flags). If we had been using strongly typed IDs, this would have been a much
simpler and fast review =)

If you need help with the design of this, I'm happy to sit down and chat.

[miu, 2016-09-21 08:24:56]

https://codereview.chromium.org/2291893002/diff/220001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.h (right):

https://codereview.chromium.org/2291893002/diff/220001/content/public/browser...
content/public/browser/web_contents_media_capture_id.h:47: // string conversion
back and forth. See crbug/648666.
BTW--My memory has been jogged, and I think this is related to a few bugs:
http://crbug.com/576877 and http://crbug.com/163100

I'll link them, and try to follow-up on http://crbug.com/648666

[qiangchen, 2016-09-21 21:11:01]

qiangchen@chromium.org changed reviewers:
+ mkwst@chromium.org, sergeyu@chromium.org, sievers@chromium.org
- brettw@chromium.org

[qiangchen, 2016-09-21 21:15:21]

Can you take a look at this CL? 
We want to add an audio constraint for getUserMedia, it will control whether we
mute the local playback at sender side during desktop or tab sharing, because
both mute or unmute have use cases.

mkwst@: Can you mainly focus on the WebKit part of this CL?

Thanks.

https://codereview.chromium.org/2291893002/diff/220001/content/public/browser...
File content/public/browser/web_contents_media_capture_id.h (right):

https://codereview.chromium.org/2291893002/diff/220001/content/public/browser...
content/public/browser/web_contents_media_capture_id.h:47: // string conversion
back and forth. See crbug/648666.
On 2016/09/21 08:24:55, miu wrote:
> BTW--My memory has been jogged, and I think this is related to a few bugs:
> http://crbug.com/576877 and http://crbug.com/163100
> 
> I'll link them, and try to follow-up on http://crbug.com/648666

Acknowledged.

[no sievers, 2016-09-21 22:03:37]

sievers@chromium.org changed reviewers:
- sievers@chromium.org

[qiangchen, 2016-09-22 16:46:48]

qiangchen@chromium.org changed reviewers:
+ avi@chromium.org

[qiangchen, 2016-09-22 16:51:32]

avi@: Can you take a look at the content folder of this CL?

Thanks.

[Avi (use Gerrit), 2016-09-22 18:19:36]

https://codereview.chromium.org/2291893002/diff/220001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/2291893002/diff/220001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:963:
media_id.enable_auto_throttling);
If you return a new object, make it return a std::unique_ptr to enforce that the
caller must take ownership.

We have been lazy in this regard in the past, and we must step up and do the
right thing here. Please fix this, and make sure that in all future CLs you
properly return unique_ptrs when returning new objects.

https://codereview.chromium.org/2291893002/diff/220001/content/common/media/m...
File content/common/media/media_stream_options.h (right):

https://codereview.chromium.org/2291893002/diff/220001/content/common/media/m...
content/common/media/media_stream_options.h:59: bool hotword_enabled;  //
kMediaStreamAudioHotword = "googHotword";
What is this?!

I see the bug http://crbug.com/577627 about removing this layering violation.

You need to remove this layering violation.

[qiangchen, 2016-09-22 20:40:59]

Fixed the unique_ptr issue.

But I am not familiar with HotWord stuff. And I found a place the bool is in
use.

https://codereview.chromium.org/2291893002/diff/220001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/2291893002/diff/220001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:963:
media_id.enable_auto_throttling);
On 2016/09/22 18:19:36, Avi wrote:
> If you return a new object, make it return a std::unique_ptr to enforce that
the
> caller must take ownership.
> 
> We have been lazy in this regard in the past, and we must step up and do the
> right thing here. Please fix this, and make sure that in all future CLs you
> properly return unique_ptrs when returning new objects.

Done.

https://codereview.chromium.org/2291893002/diff/220001/content/common/media/m...
File content/common/media/media_stream_options.h (right):

https://codereview.chromium.org/2291893002/diff/220001/content/common/media/m...
content/common/media/media_stream_options.h:59: bool hotword_enabled;  //
kMediaStreamAudioHotword = "googHotword";
On 2016/09/22 18:19:36, Avi wrote:
> What is this?!
> 
> I see the bug http://crbug.com/577627 about removing this layering violation.
> 
> You need to remove this layering violation.

But I am not familiar with this HotWord stuff, and by code searching I found it
is in use at [1]. So simply removing it would break something existing.

And I think this worth a separate CL to do it.

But similar qustion: Is this place appropriate to place "disable_local_echo"? Or
is it better if I put it as a field in |audio|?

[1]https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/media_stream_manager.cc?q=hotword_enabled&sq=package:chromium&l=152&dr=C

[Avi (use Gerrit), 2016-09-22 20:44:38]

When I say "remove the layering violation" I don't mean remove the feature.

The problem is that the feature is a feature of Chrome. It should live in
chrome/. However, you have all of the implementation code in content/ and it
doesn't belong in content.

The bug is admitting that the code doesn't belong in content/. It's been half a
year since it was filed. Please take the time to clean up the code, to put
things where they belong.

[qiangchen, 2016-09-22 22:06:25]

On 2016/09/22 20:44:38, Avi wrote:
> When I say "remove the layering violation" I don't mean remove the feature.
> 
> The problem is that the feature is a feature of Chrome. It should live in
> chrome/. However, you have all of the implementation code in content/ and it
> doesn't belong in content.
> 
> The bug is admitting that the code doesn't belong in content/. It's been half
a
> year since it was filed. Please take the time to clean up the code, to put
> things where they belong.

Then my question is where should I put the variable? Do you have suggestions?

If I understand correctly, we need to create a struct (say
"ChromeStreamControl") in chrome/ folder, and take in HotWord parameter.
Then inevitably we have to touch the full code path where StreamControl gets
passed to take in one more parameter for ChromeStreamControl.

Is this the same as your opinion?

[qiangchen, 2016-09-22 22:19:41]

On 2016/09/22 22:06:25, qiangchenC wrote:
> On 2016/09/22 20:44:38, Avi wrote:
> > When I say "remove the layering violation" I don't mean remove the feature.
> > 
> > The problem is that the feature is a feature of Chrome. It should live in
> > chrome/. However, you have all of the implementation code in content/ and it
> > doesn't belong in content.
> > 
> > The bug is admitting that the code doesn't belong in content/. It's been
half
> a
> > year since it was filed. Please take the time to clean up the code, to put
> > things where they belong.
> 
> Then my question is where should I put the variable? Do you have suggestions?
> 
> If I understand correctly, we need to create a struct (say
> "ChromeStreamControl") in chrome/ folder, and take in HotWord parameter.
> Then inevitably we have to touch the full code path where StreamControl gets
> passed to take in one more parameter for ChromeStreamControl.
> 
> Is this the same as your opinion?

One more thing. Beyond the parameter passing, I did not find a way how to
completely separate the implementation of HotWord from content/ as it is mixed
with other "non chrome specific" parameter processing.

[qiangchen, 2016-09-23 16:04:40]

On 2016/09/22 20:44:38, Avi wrote:
> When I say "remove the layering violation" I don't mean remove the feature.
> 
> The problem is that the feature is a feature of Chrome. It should live in
> chrome/. However, you have all of the implementation code in content/ and it
> doesn't belong in content.
> 
> The bug is admitting that the code doesn't belong in content/. It's been half
a
> year since it was filed. Please take the time to clean up the code, to put
> things where they belong.

Let's make it clearer.

By "a feature of Chrome", did you mean hot word is "ChromeOS" only stuff, or did
you mean hot word is not W3C stuff, but unique to Chrome browser?

[Avi (use Gerrit), 2016-09-23 18:15:25]

On 2016/09/23 16:04:40, qiangchenC wrote:
> On 2016/09/22 20:44:38, Avi wrote:
> > When I say "remove the layering violation" I don't mean remove the feature.
> > 
> > The problem is that the feature is a feature of Chrome. It should live in
> > chrome/. However, you have all of the implementation code in content/ and it
> > doesn't belong in content.
> > 
> > The bug is admitting that the code doesn't belong in content/. It's been
half
> a
> > year since it was filed. Please take the time to clean up the code, to put
> > things where they belong.
> 
> Let's make it clearer.
> 
> By "a feature of Chrome", did you mean hot word is "ChromeOS" only stuff, or
did
> you mean hot word is not W3C stuff, but unique to Chrome browser?

That's what I mean. content is for web standards stuff, and stuff genuinely
useful for all embedders.

[qiangchen, 2016-09-23 18:30:17]

On 2016/09/23 18:15:25, Avi wrote:
> On 2016/09/23 16:04:40, qiangchenC wrote:
> > On 2016/09/22 20:44:38, Avi wrote:
> > > When I say "remove the layering violation" I don't mean remove the
feature.
> > > 
> > > The problem is that the feature is a feature of Chrome. It should live in
> > > chrome/. However, you have all of the implementation code in content/ and
it
> > > doesn't belong in content.
> > > 
> > > The bug is admitting that the code doesn't belong in content/. It's been
> half
> > a
> > > year since it was filed. Please take the time to clean up the code, to put
> > > things where they belong.
> > 
> > Let's make it clearer.
> > 
> > By "a feature of Chrome", did you mean hot word is "ChromeOS" only stuff, or
> did
> > you mean hot word is not W3C stuff, but unique to Chrome browser?
> 
> That's what I mean. content is for web standards stuff, and stuff genuinely
> useful for all embedders.

Then I think it will also apply to my field "disable_local_echo_". 

Then the problem is that it is just a parameter controlling one aspect of the
stream, and it must be process with other parameters (namely source_id), so I
cannot figure out a way to completely separate the implementation from
MediaStreamManager, which resides in content/. 

Any idea?

[Avi (use Gerrit), 2016-09-23 21:22:45]

But disabling local echo is a reasonable thing to have for embedders.

Anyway, after talking with some content people, I'm not going to hold you back
here. LGTM for this change.

However, hta@ filed http://crbug.com/577627 at the beginning of this year to
address that issue. Is there someone working on hotwording that would be able to
take that bug and make progress on it?

[qiangchen, 2016-09-23 21:47:54]

On 2016/09/23 21:22:45, Avi wrote:
> But disabling local echo is a reasonable thing to have for embedders.
> 
> Anyway, after talking with some content people, I'm not going to hold you back
> here. LGTM for this change.
> 
> However, hta@ filed http://crbug.com/577627 at the beginning of this year to
> address that issue. Is there someone working on hotwording that would be able
to
> take that bug and make progress on it?

I do not know who is working on hot word. Maybe, we can ask dalecurtis@, as he
reviewed the CL introducing hot word, and he is one of the owners of media.

[qiangchen, 2016-09-26 15:59:47]

Hi, sergeyu@ and mkwst@ can you take a look at this CL?

The main change is add one more constraint in audio part of getUserMedia and
pass that parameter to the class where we generate the audio stream. 

The implementation on how to mute or unmute local playback was done in previous
CLs. This CL is just parameter passing.

Thanks.

[Mike West, 2016-09-27 08:03:41]

https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:94: const
char kGoogDisableLocalEcho[] = "googDisableLocalEcho";
Is this web-exposed? That is, does a developer type this into JavaScript, or is
this just an internal name?

If it's web-exposed, then the name makes me sad (we should not be shipping new,
vendor-prefixed APIs), and there are no tests.

If it's not web-exposed, then I'm less concerned about the name, but I still
don't see any tests. You suggested that the implementation is in another CL: are
the tests there too?

[qiangchen, 2016-09-27 16:16:47]

replied.

https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:94: const
char kGoogDisableLocalEcho[] = "googDisableLocalEcho";
On 2016/09/27 08:03:41, Mike West (OOO until 26th) wrote:
> Is this web-exposed? That is, does a developer type this into JavaScript, or
is
> this just an internal name?
> 
> If it's web-exposed, then the name makes me sad (we should not be shipping
new,
> vendor-prefixed APIs), and there are no tests.
> 
> If it's not web-exposed, then I'm less concerned about the name, but I still
> don't see any tests. You suggested that the implementation is in another CL:
are
> the tests there too?

It is web exposed. But it is not an API function name, just an entry in audio
constraint. 

For example:
getUserMedia({audio: {googDisableLocalEcho: true}, ......})

We want to give the developer a freedom to override default behavior and choose
whether stop local playback or not during sharing. But we did not find there is
such a parameter defined in W3C. If this is not allowed, then we have to give up
this CL.

We do have tests for the real functionality. But those tests have no idea of
googDisableLocalEcho, and in those tests we pass the bool parameter directly
into the class processing audio stream, and then we test whether it behaves as
expected. 

For here, I think the test you mean is a test that verifies that we can pass
googDisableLocalEcho from Javascript to the audio stream generating class,
right?

[Avi (use Gerrit), 2016-09-27 16:20:34]

https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:94: const
char kGoogDisableLocalEcho[] = "googDisableLocalEcho";
Waaaaaait. I see lots of "googXXXX" constants in this file, around this line.

Are *all* of them web-exposed?

[qiangchen, 2016-09-27 16:32:50]

https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:94: const
char kGoogDisableLocalEcho[] = "googDisableLocalEcho";
On 2016/09/27 16:20:34, Avi wrote:
> Waaaaaait. I see lots of "googXXXX" constants in this file, around this line.
> 
> Are *all* of them web-exposed?

Yes. You can search them on google, and you will find the result is the
discussion of their usage. And the way to use them is much the same as 
getUserMedia({audio: {googXXXX: <value>}} ......)

[Mike West, 2016-09-27 18:02:13]

On 2016/09/27 at 16:32:50, qiangchen wrote:
>
https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):
> 
>
https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:94:
const char kGoogDisableLocalEcho[] = "googDisableLocalEcho";
> On 2016/09/27 16:20:34, Avi wrote:
> > Waaaaaait. I see lots of "googXXXX" constants in this file, around this
line.
> > 
> > Are *all* of them web-exposed?
> 
> Yes. You can search them on google, and you will find the result is the
discussion of their usage. And the way to use them is much the same as 
> getUserMedia({audio: {googXXXX: <value>}} ......)

In that case, please send an intent to ship to blink-dev@ (as per the process at
http://www.chromium.org/blink#launch-process). I don't think we should be
shipping new vendor-prefixed mechanisms without a spec behind them, but I'm not
the decider. :) I think we've gone around in circles on media stuff in the past,
but it's probably time to do it again.

Naming to the side, if it's web-exposed, you need to have tests somewhere that
verify that putting the new parameter in on the JavaScript side has the relevant
effect on the C++ side. These could take the form of layout tests with
appropriate `window.internals` accessors, browsertests that poke at the internal
state, etc.

[qiangchen, 2016-09-27 21:58:24]

On 2016/09/27 18:02:13, Mike West (OOO until 26th) wrote:
> On 2016/09/27 at 16:32:50, qiangchen wrote:
> >
>
https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2291893002/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:94:
> const char kGoogDisableLocalEcho[] = "googDisableLocalEcho";
> > On 2016/09/27 16:20:34, Avi wrote:
> > > Waaaaaait. I see lots of "googXXXX" constants in this file, around this
> line.
> > > 
> > > Are *all* of them web-exposed?
> > 
> > Yes. You can search them on google, and you will find the result is the
> discussion of their usage. And the way to use them is much the same as 
> > getUserMedia({audio: {googXXXX: <value>}} ......)
> 
> In that case, please send an intent to ship to blink-dev@ (as per the process
at
> http://www.chromium.org/blink#launch-process). I don't think we should be
> shipping new vendor-prefixed mechanisms without a spec behind them, but I'm
not
> the decider. :) I think we've gone around in circles on media stuff in the
past,
> but it's probably time to do it again.
> 
> Naming to the side, if it's web-exposed, you need to have tests somewhere that
> verify that putting the new parameter in on the JavaScript side has the
relevant
> effect on the C++ side. These could take the form of layout tests with
> appropriate `window.internals` accessors, browsertests that poke at the
internal
> state, etc.

ITI in progress now. 

For test, I tried to code search the test for other constraint keys, but
unfortunately, not exist.
One difficulty is that we can hardly let getUserMedia be fully executed, and
tell the result from the stream we get, because whether or not we mute the local
playback does not affect the stream generated. Instead, can we override
MediaStreamDispatcher, and in the mock class we make sure the code path from JS
to C++ is connected?

[Mike West, 2016-09-28 11:25:28]

On 2016/09/27 at 21:58:24, qiangchen wrote:
> ITI in progress now. 

Thank you!

> For test, I tried to code search the test for other constraint keys, but
unfortunately, not exist.

I'm sometimes surprised that anything in Chrome works at all. ;)

> One difficulty is that we can hardly let getUserMedia be fully executed, and
tell the result from the stream we get, because whether or not we mute the local
playback does not affect the stream generated. Instead, can we override
MediaStreamDispatcher, and in the mock class we make sure the code path from JS
to C++ is connected?

That sounds fine to me.

[qiangchen, 2016-10-03 17:42:54]

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

[commit-bot: I haz the power, 2016-10-03 17:43:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qiangchen, 2016-10-03 17:49:03]

On 2016/09/28 11:25:28, Mike West (OOO until the 4th) wrote:
> On 2016/09/27 at 21:58:24, qiangchen wrote:
> > ITI in progress now. 
> 
> Thank you!
> 
> > For test, I tried to code search the test for other constraint keys, but
> unfortunately, not exist.
> 
> I'm sometimes surprised that anything in Chrome works at all. ;)
> 
> > One difficulty is that we can hardly let getUserMedia be fully executed, and
> tell the result from the stream we get, because whether or not we mute the
local
> playback does not affect the stream generated. Instead, can we override
> MediaStreamDispatcher, and in the mock class we make sure the code path from
JS
> to C++ is connected?
> 
> That sounds fine to me.

mkwst@, can you take a look again? I've done the following things
1. According to blink review, remove prefix "goog".
2. According to blink review, just do Intent To Implement first, and move on
this CL behind a flag of RuntimeEnabledFeature.
   Push Intent To Ship afterwards, and use another CL to remove the flag once
approved.
3. Added a browser test case for getUserMedia parameter parsing.

[commit-bot: I haz the power, 2016-10-03 18:04:02]

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

[commit-bot: I haz the power, 2016-10-03 18:04:03]

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

[qiangchen, 2016-10-03 18:37:35]

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

[qiangchen, 2016-10-03 18:37:45]

Patchset #11 (id:300001) has been deleted

[commit-bot: I haz the power, 2016-10-03 18:38:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 19:06:04]

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

[commit-bot: I haz the power, 2016-10-03 19:06:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[qiangchen, 2016-10-03 20:10:04]

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

[commit-bot: I haz the power, 2016-10-03 20:10:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 21:15:47]

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

[commit-bot: I haz the power, 2016-10-03 21:15:49]

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

[qiangchen, 2016-10-03 22:01:34]

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

[commit-bot: I haz the power, 2016-10-03 22:03:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 23:17:35]

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

[commit-bot: I haz the power, 2016-10-03 23:17:36]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[qiangchen, 2016-10-03 23:40:37]

Patchset #12 (id:340001) has been deleted

[qiangchen, 2016-10-04 17:13:59]

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

[qiangchen, 2016-10-04 17:14:08]

Patchset #13 (id:380001) has been deleted

[commit-bot: I haz the power, 2016-10-04 17:14:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 22:10:11]

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

[commit-bot: I haz the power, 2016-10-04 22:10:13]

Dry run: This issue passed the CQ dry run.

[qiangchen, 2016-10-04 22:13:50]

On 2016/10/03 17:49:03, qiangchenC wrote:
> On 2016/09/28 11:25:28, Mike West (OOO until the 4th) wrote:
> > On 2016/09/27 at 21:58:24, qiangchen wrote:
> > > ITI in progress now. 
> > 
> > Thank you!
> > 
> > > For test, I tried to code search the test for other constraint keys, but
> > unfortunately, not exist.
> > 
> > I'm sometimes surprised that anything in Chrome works at all. ;)
> > 
> > > One difficulty is that we can hardly let getUserMedia be fully executed,
and
> > tell the result from the stream we get, because whether or not we mute the
> local
> > playback does not affect the stream generated. Instead, can we override
> > MediaStreamDispatcher, and in the mock class we make sure the code path from
> JS
> > to C++ is connected?
> > 
> > That sounds fine to me.
> 
> mkwst@, can you take a look again? I've done the following things
> 1. According to blink review, remove prefix "goog".
> 2. According to blink review, just do Intent To Implement first, and move on
> this CL behind a flag of RuntimeEnabledFeature.
>    Push Intent To Ship afterwards, and use another CL to remove the flag once
> approved.
> 3. Added a browser test case for getUserMedia parameter parsing.

mkwst@: can you look at this CL again? I've just resolved the browser test
issues.

[Mike West, 2016-10-05 04:11:03]

Thanks for following up on blink-dev@. :)

//third_party/WebKit LGTM. The new tests in //content/browser/webrtc look
reasonable, but it would be good to find an owner there to skim through them as
well.

[qiangchen, 2016-10-05 16:54:23]

qiangchen@chromium.org changed reviewers:
+ boliu@chromium.org, tnagel@chromium.org

[qiangchen, 2016-10-05 16:55:23]

qiangchen@chromium.org changed reviewers:
+ pastarmovj@chromium.org
- tnagel@chromium.org

[qiangchen, 2016-10-05 16:58:34]

Sergeyu@ Can you take a look at 
chrome/browser/media/webrtc/desktop_capture_access_handler.cc

This file is not covered by other owners. 
We essentially pass the parameter value into the function to control whether let
local playback run.

[qiangchen, 2016-10-05 16:59:50]

boliu@ can you take a look at 
android_webview/native/permission/media_access_permission_request_unittest.cc?

It is a trivial change due to MediaStreamRequest signature change.

[qiangchen, 2016-10-05 17:00:43]

pastarmovj@ can you take a look at 	chrome/browser/policy/policy_browsertest.cc?

It is a trivial change due to MediaStreamRequest signature change.

[boliu, 2016-10-05 17:31:25]

rs lgtm

https://codereview.chromium.org/2291893002/diff/400001/android_webview/native...
File
android_webview/native/permission/media_access_permission_request_unittest.cc
(right):

https://codereview.chromium.org/2291893002/diff/400001/android_webview/native...
android_webview/native/permission/media_access_permission_request_unittest.cc:56:
content::MEDIA_DEVICE_VIDEO_CAPTURE, false);
nit: maybe add an inline comment to make this a bit easier to read

false /* disable_local_echo */

[pastarmovj, 2016-10-05 18:07:18]

policy lgtm

[pastarmovj, 2016-10-05 18:07:19]

policy lgtm

[qiangchen, 2016-10-05 18:13:58]

https://codereview.chromium.org/2291893002/diff/400001/android_webview/native...
File
android_webview/native/permission/media_access_permission_request_unittest.cc
(right):

https://codereview.chromium.org/2291893002/diff/400001/android_webview/native...
android_webview/native/permission/media_access_permission_request_unittest.cc:56:
content::MEDIA_DEVICE_VIDEO_CAPTURE, false);
On 2016/10/05 17:31:25, boliu wrote:
> nit: maybe add an inline comment to make this a bit easier to read
> 
> false /* disable_local_echo */

Done.

[miu, 2016-10-05 18:43:23]

still lgtm (as of PS14)

[qiangchen, 2016-10-07 19:43:39]

ping sergeyu@

[qiangchen, 2016-10-11 16:59:53]

Hi, sergeyu@:

  Can you take a look at 
chrome/browser/media/webrtc/desktop_capture_access_handler.cc?

  That is the only file not covered by other reviewers.

[Sergey Ulanov, 2016-10-12 22:21:02]

lgtm

https://codereview.chromium.org/2291893002/diff/420001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/2291893002/diff/420001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:961: return
std::unique_ptr<media::VideoCaptureDevice>(
nit:: use base::MakeUnique<WebContentsVideoCaptureDevice>

https://codereview.chromium.org/2291893002/diff/420001/content/renderer/media...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2291893002/diff/420001/content/renderer/media...
content/renderer/media/user_media_client_impl.cc:89: bool exact =
request.audioConstraints().basic().disableLocalEcho.exact();
this doesn't look like a good variable name. Also it looks wrong to call exact()
if hasExcat() returns false. Why can't this code follow the same pattern that's
used above for hotword_enabled:
  if (request.audioConstraints().basic().disableLocalEcho.hasExact()) {
    controls->disable_local_echo =
         request.audioConstraints().basic().disableLocalEcho.exact();
  } else {
    controls->disable_local_echo =
        controls->audio.stream_source != kMediaStreamSourceDesktop;
  }

Or even this:
  controls->disable_local_echo =
     request.audioConstraints().basic().disableLocalEcho.hasExact()
        ? request.audioConstraints().basic().disableLocalEcho.exact()
        : controls->audio.stream_source != kMediaStreamSourceDesktop;

[qiangchen, 2016-10-12 22:49:43]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-10-12 22:49:44]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
avi@chromium.org, boliu@chromium.org, mkwst@chromium.org, sergeyu@chromium.org,
miu@chromium.org, pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2291893002/#ps440001
(title: "Nit")

[qiangchen, 2016-10-12 22:49:54]

https://codereview.chromium.org/2291893002/diff/420001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/2291893002/diff/420001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:961: return
std::unique_ptr<media::VideoCaptureDevice>(
On 2016/10/12 22:21:01, Sergey Ulanov wrote:
> nit:: use base::MakeUnique<WebContentsVideoCaptureDevice>

Not work. As media::VideoCaptureDevice is an abstract class, and it does not
allow base::MakeUnique<media::VideoCaptureDevice>.

https://codereview.chromium.org/2291893002/diff/420001/content/renderer/media...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2291893002/diff/420001/content/renderer/media...
content/renderer/media/user_media_client_impl.cc:89: bool exact =
request.audioConstraints().basic().disableLocalEcho.exact();
On 2016/10/12 22:21:01, Sergey Ulanov wrote:
> this doesn't look like a good variable name. Also it looks wrong to call
exact()
> if hasExcat() returns false. Why can't this code follow the same pattern
that's
> used above for hotword_enabled:
>   if (request.audioConstraints().basic().disableLocalEcho.hasExact()) {
>     controls->disable_local_echo =
>          request.audioConstraints().basic().disableLocalEcho.exact();
>   } else {
>     controls->disable_local_echo =
>         controls->audio.stream_source != kMediaStreamSourceDesktop;
>   }
> 
> Or even this:
>   controls->disable_local_echo =
>      request.audioConstraints().basic().disableLocalEcho.hasExact()
>         ? request.audioConstraints().basic().disableLocalEcho.exact()
>         : controls->audio.stream_source != kMediaStreamSourceDesktop;

Done.

[commit-bot: I haz the power, 2016-10-12 22:50:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 22:53:40]

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

[commit-bot: I haz the power, 2016-10-12 22:53:42]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qiangchen, 2016-10-12 23:07:56]

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

[commit-bot: I haz the power, 2016-10-12 23:08:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 23:24:29]

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

[commit-bot: I haz the power, 2016-10-12 23:24:30]

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

[qiangchen, 2016-10-12 23:44:57]

Patchset #16 (id:460001) has been deleted

[qiangchen, 2016-10-12 23:45:05]

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

[commit-bot: I haz the power, 2016-10-12 23:45:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 23:53:18]

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

[commit-bot: I haz the power, 2016-10-12 23:53:20]

Dry run: Exceeded global retry quota

[qiangchen, 2016-10-13 21:34:29]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-10-13 21:34:30]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
sergeyu@chromium.org, boliu@chromium.org, dcheng@chromium.org,
mkwst@chromium.org, miu@chromium.org, pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2291893002/#ps480001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-13 21:35:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 22:25:26]

Description was changed from

==========
Let Contraints Controll Mute/Unmute Audio Local Playback For Desktop Sharing

In this CL, we did the following things:
1. Add an entry for audio constraints of getUserMedia API, namely
googDisableLocalEcho.

2. The constraint works for the case of desktop sharing.

3. If set googDisableLocalEcho = true, then we will mute the 
local playback of the sharing audio. Namely for full screen share
we will mute the system audio, and for tab share, we will mute
the corresponding tab. Vice versa.

BUG=647801
==========

to

==========
Let Contraints Controll Mute/Unmute Audio Local Playback For Desktop Sharing

In this CL, we did the following things:
1. Add an entry for audio constraints of getUserMedia API, namely
googDisableLocalEcho.

2. The constraint works for the case of desktop sharing.

3. If set googDisableLocalEcho = true, then we will mute the 
local playback of the sharing audio. Namely for full screen share
we will mute the system audio, and for tab share, we will mute
the corresponding tab. Vice versa.

BUG=647801
==========

[commit-bot: I haz the power, 2016-10-13 22:25:28]

Committed patchset #16 (id:480001)

[commit-bot: I haz the power, 2016-10-13 22:29:33]

Description was changed from

==========
Let Contraints Controll Mute/Unmute Audio Local Playback For Desktop Sharing

In this CL, we did the following things:
1. Add an entry for audio constraints of getUserMedia API, namely
googDisableLocalEcho.

2. The constraint works for the case of desktop sharing.

3. If set googDisableLocalEcho = true, then we will mute the 
local playback of the sharing audio. Namely for full screen share
we will mute the system audio, and for tab share, we will mute
the corresponding tab. Vice versa.

BUG=647801
==========

to

==========
Let Contraints Controll Mute/Unmute Audio Local Playback For Desktop Sharing

In this CL, we did the following things:
1. Add an entry for audio constraints of getUserMedia API, namely
googDisableLocalEcho.

2. The constraint works for the case of desktop sharing.

3. If set googDisableLocalEcho = true, then we will mute the
local playback of the sharing audio. Namely for full screen share
we will mute the system audio, and for tab share, we will mute
the corresponding tab. Vice versa.

BUG=647801

Committed: https://crrev.com/f6ca1ed1a2d80ce5f22c10a2a5cda30b1c44bccd
Cr-Commit-Position: refs/heads/master@{#425190}
==========

[commit-bot: I haz the power, 2016-10-13 22:29:35]

Patchset 16 (id:??) landed as
https://crrev.com/f6ca1ed1a2d80ce5f22c10a2a5cda30b1c44bccd
Cr-Commit-Position: refs/heads/master@{#425190}