Make sure that all OpenDevice requests are scrutinized against the audio and video policies.

Make sure that all OpenDevice requests are scrutinized against the audio and video policies.

BUG=159404
TEST=unit_tests,browser_tests,Manually by setting the policy and observing flapper obey it.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172877

[pastarmovj, 2012-12-06 17:18:17]

Hi Shijing,
Can you take a look at that CL. It is an attempt to wire the OpenDevice call to
the policies I just committed. I hope you are ok with this approach. It will
also pave to way to unify the UI for Flash and WebRTC because eventually we can
run the checks against the media content settings.
Thanks in Advance!

[no longer working on chromium, 2012-12-07 14:01:05]

Hi Julian,

Sorry for the delay on reviewing, we had our christmas party yesterday.

Thanks for doing this, and I am fine to add a bool to MediaStreamRequest to
treat the OpenDevice differently, but just wonder if it is a better idea move
enum RequestType from MediaStreamManager to MediaStreamRequest, and we treat the
request with different ways based on the request type.

I am a bit favoring this approach because we use MediaStreamDeviceType to
separate the cases from tab capture and now we are adding a bool to separate
OpenDevice with others. It might be a good idea if we can merge two approaches
in the future.

Since Wei was involved with the OpenDevice code before, lets leave the decision
to him. Wei, how do you think here?

[wjia(left Chromium), 2012-12-07 18:47:19]

+yzshen@ for his opinion since this will affect Pepper.

[pastarmovj, 2012-12-10 09:54:44]

Hi guys,
Now that we have agreement on this change I would like to get full review on
that one. 
I have also implemented Shijing's proposal to replace the bool param for the
request type.
Best,
Julian

[no longer working on chromium, 2012-12-10 10:17:56]

Thanks for sorting it out.
lgtm with two nits.

https://codereview.chromium.org/11446042/diff/8001/chrome/browser/media/media...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/11446042/diff/8001/chrome/browser/media/media...
chrome/browser/media/media_stream_devices_controller.cc:85: if
(request_.request_type == content::OPEN_DEVICE) {
thanks, the code is much clearer now since we are sure it is only for
OPEN_DEVICE request.

https://codereview.chromium.org/11446042/diff/8001/content/public/common/medi...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11446042/diff/8001/content/public/common/medi...
content/public/common/media_stream_request.h:34: DEVICE_ACCESS = 0,
It will be used by multiple files now, how do you think adding a MEDIA_ prefix?

[pastarmovj, 2012-12-12 09:57:03]

Addressed the comment. If there aren't any objections I will click the CQ once
the trybots are over.
Thanks for reviewing that!

https://codereview.chromium.org/11446042/diff/8001/content/public/common/medi...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11446042/diff/8001/content/public/common/medi...
content/public/common/media_stream_request.h:34: DEVICE_ACCESS = 0,
On 2012/12/10 10:17:56, xians1 wrote:
> It will be used by multiple files now, how do you think adding a MEDIA_
prefix? 

Done.

[commit-bot: I haz the power, 2012-12-12 14:07:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/11446042/18008

[commit-bot: I haz the power, 2012-12-12 14:07:28]

Presubmit check for 11446042-18008 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public

Presubmit checks took 1.1s to calculate.

[pastarmovj, 2012-12-12 14:09:05]

Adding Joi for OWNER approval for content/public. Thanks!

[Jói, 2012-12-12 14:18:18]

https://codereview.chromium.org/11446042/diff/18008/content/public/common/med...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11446042/diff/18008/content/public/common/med...
content/public/common/media_stream_request.h:89: // Whether the request should
be verified against the rules for denial only or
It's not clear here what happens for each of the different
MediaStreamRequestType values.  Suggest documenting each of their meanings
above, where the enum is defined.

[pastarmovj, 2012-12-12 14:52:52]

PTAL.

https://codereview.chromium.org/11446042/diff/18008/content/public/common/med...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11446042/diff/18008/content/public/common/med...
content/public/common/media_stream_request.h:89: // Whether the request should
be verified against the rules for denial only or
On 2012/12/12 14:18:18, Jói wrote:
> It's not clear here what happens for each of the different
> MediaStreamRequestType values.  Suggest documenting each of their meanings
> above, where the enum is defined.

Done.

[Jói, 2012-12-12 14:58:12]

LGTM

https://codereview.chromium.org/11446042/diff/28001/content/public/common/med...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11446042/diff/28001/content/public/common/med...
content/public/common/media_stream_request.h:92: // later should not be subject
to user approval but only to policy check.
later -> latter

[commit-bot: I haz the power, 2012-12-12 15:00:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/11446042/18018

[commit-bot: I haz the power, 2012-12-12 16:30:12]

Retried try job too often on linux_rel for step(s) content_browsertests

[commit-bot: I haz the power, 2012-12-12 16:34:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/11446042/18018

[commit-bot: I haz the power, 2012-12-12 16:34:51]

Failed to apply patch for
chrome/browser/media/media_stream_devices_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/media/media_stream_devices_controller.cc
  Hunk #1 FAILED at 80.
  Hunk #2 FAILED at 141.
  2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/media/media_stream_devices_controller.cc.rej

Patch:       chrome/browser/media/media_stream_devices_controller.cc
Index: chrome/browser/media/media_stream_devices_controller.cc
diff --git a/chrome/browser/media/media_stream_devices_controller.cc
b/chrome/browser/media/media_stream_devices_controller.cc
index
793254723bf621dab999885ccfe95006933fd484..ba61d5dddf1555770a294d5c401c672f815b262c
100644
--- a/chrome/browser/media/media_stream_devices_controller.cc
+++ b/chrome/browser/media/media_stream_devices_controller.cc
@@ -80,6 +80,18 @@ void
MediaStreamDevicesController::RegisterUserPrefs(PrefService* prefs) {
 
 
 bool MediaStreamDevicesController::DismissInfoBarAndTakeActionOnSettings() {
+  // If this is a no UI check for policies only go straight to accept - policy
+  // check will be done automatically on the way.
+  if (request_.request_type == content::MEDIA_OPEN_DEVICE) {
+    std::string audio, video;
+    if (has_audio_)
+      audio = GetFirstDeviceId(content::MEDIA_DEVICE_AUDIO_CAPTURE);
+    if (has_video_)
+      video = GetFirstDeviceId(content::MEDIA_DEVICE_VIDEO_CAPTURE);
+    Accept(audio, video, false);
+    return true;
+  }
+
   // For tab media requests, we need to make sure the request came from the
   // extension API, so we check the registry here.
   content::MediaStreamDeviceMap::const_iterator tab_video =
@@ -141,7 +153,6 @@ bool
MediaStreamDevicesController::DismissInfoBarAndTakeActionOnSettings() {
       return true;
     }
 
-    // Show the infobar.
     return false;
   }