Move ChromeOS login media access logic into a MediaAccessHandler

Move ChromeOS login media access logic into a MediaAccessHandler

Currently the ChromeOS login screen media access logic is handled as a part of
the PermissionBubbleMediaAccessHandler. However its permission handling logic
is totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler. WebUILoginView::RequestMediaAccessPermission
is changed to go through the MediaCaptureDevicesDispatcher which matches the
codepath for other media permission requests.

BUG=596786
Review-Url: https://codereview.chromium.org/2746873004
Cr-Commit-Position: refs/heads/master@{#463454}
Committed: https://chromium.googlesource.com/chromium/src/+/2edae96f04b07205ce7b3ad83af879bcec2318a2

[raymes, 2017-03-14 03:40:39]

Description was changed from

==========
Move SAML media access logic into a MediaAccessHandler

Currently the SAML media access logic is handled as a part of the
PermissionBubbleMediaAccessHandler. However its permission handling logic is
totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler.

BUG=596786
==========

to

==========
Move SAML media access logic into a MediaAccessHandler

Currently the SAML media access logic is handled as a part of the
PermissionBubbleMediaAccessHandler. However its permission handling logic is
totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler. WebUILoginView::RequestMediaAccessPermission
is changed to go through the MediaCaptureDevicesDispatcher which matches the
codepath for other media permission requests.

BUG=596786
==========

[raymes, 2017-03-14 03:44:02]

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

[commit-bot: I haz the power, 2017-03-14 03:44:11]

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

[raymes, 2017-03-14 03:46:33]

raymes@chromium.org changed reviewers:
+ sergeyu@chromium.org

[raymes, 2017-03-14 03:46:33]

sergeyu: could you ptal? Thanks!

[commit-bot: I haz the power, 2017-03-14 03:48:56]

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

[commit-bot: I haz the power, 2017-03-14 03:48:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2017-03-14 03:51:52]

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

[commit-bot: I haz the power, 2017-03-14 03:52:07]

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, 2017-03-14 04:57:13]

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

[commit-bot: I haz the power, 2017-03-14 04:57:14]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-03-15 22:48:31]

lgtm

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:1455: EXPECT_EQ(
EXPECT_TRUE(..)
Same for other EXPECT_EQ() in this file.

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:1465: EXPECT_EQ(
EXPECT_FALSE

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.cc:20: //
MediaStreamDevicesController::Accept(). Move this code into a shared method
That code is in MediaStreamDevicesController::GetDevices() now. Update this TODO
please

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
File chrome/browser/media/saml_media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.cc:59: if (pattern ==
ContentSettingsPattern::Wildcard()) {
Maybe add a comment to explain why this is necessary.

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.cc:60: LOG(WARNING) << "Ignoring
wildcard URL pattern: " << value;
Does this need to be a WARNING? Can it be a VLOG(0)?

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
File chrome/browser/media/saml_media_access_handler.h (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.h:10: // MediaAccessHandler for
media requests coming from SAML login pages on
I have no idea what "SAML" is. Is it "Security Assertion Markup Language" or
something else? It should be clear from the comments.

[raymes, 2017-03-16 02:30:36]

raymes@chromium.org changed reviewers:
+ achuith@chromium.org

[raymes, 2017-03-16 02:30:37]

+achuith for chrome/browser/chromeos/login/ OWNERS

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:1455: EXPECT_EQ(
On 2017/03/15 22:48:30, Sergey Ulanov wrote:
> EXPECT_TRUE(..)
> Same for other EXPECT_EQ() in this file.

Thanks, done.

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:1465: EXPECT_EQ(
On 2017/03/15 22:48:30, Sergey Ulanov wrote:
> EXPECT_FALSE

Done.

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.cc:20: //
MediaStreamDevicesController::Accept(). Move this code into a shared method
On 2017/03/15 22:48:30, Sergey Ulanov wrote:
> That code is in MediaStreamDevicesController::GetDevices() now. Update this
TODO
> please

Done.

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
File chrome/browser/media/saml_media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.cc:59: if (pattern ==
ContentSettingsPattern::Wildcard()) {
On 2017/03/15 22:48:31, Sergey Ulanov wrote:
> Maybe add a comment to explain why this is necessary.

Done.

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.cc:60: LOG(WARNING) << "Ignoring
wildcard URL pattern: " << value;
On 2017/03/15 22:48:31, Sergey Ulanov wrote:
> Does this need to be a WARNING? Can it be a VLOG(0)?

I would say so. Done.

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
File chrome/browser/media/saml_media_access_handler.h (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.h:10: // MediaAccessHandler for
media requests coming from SAML login pages on
On 2017/03/15 22:48:31, Sergey Ulanov wrote:
> I have no idea what "SAML" is. Is it "Security Assertion Markup Language" or
> something else? It should be clear from the comments.

Me neither actually :) +achuith to clarify.

[raymes, 2017-03-16 02:31:11]

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

[commit-bot: I haz the power, 2017-03-16 02:31:22]

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, 2017-03-16 03:46:01]

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

[commit-bot: I haz the power, 2017-03-16 03:46:02]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-03-16 03:46:04]

lgtm

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
File chrome/browser/media/saml_media_access_handler.h (right):

https://codereview.chromium.org/2746873004/diff/20001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.h:10: // MediaAccessHandler for
media requests coming from SAML login pages on
On 2017/03/16 02:30:37, raymes wrote:
> On 2017/03/15 22:48:31, Sergey Ulanov wrote:
> > I have no idea what "SAML" is. Is it "Security Assertion Markup Language" or
> > something else? It should be clear from the comments.
> 
> Me neither actually :) +achuith to clarify.

Yes, it's a scheme for federated identity providers. This is useful for
enterprises who want to use their own identity providers.

https://codereview.chromium.org/2746873004/diff/40001/chrome/browser/media/sa...
File chrome/browser/media/saml_media_access_handler.h (right):

https://codereview.chromium.org/2746873004/diff/40001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.h:1: // Copyright 2017 The
Chromium Authors. All rights reserved.
Don't think this should be changed.

https://codereview.chromium.org/2746873004/diff/40001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.h:5: #ifndef
CHROME_BROWSER_MEDIA_SAML_MEDIA_ACCESS_HANDLER_H_
Should this file be moved to chrome/browser/chromeos?

[raymes, 2017-04-03 03:19:50]

Description was changed from

==========
Move SAML media access logic into a MediaAccessHandler

Currently the SAML media access logic is handled as a part of the
PermissionBubbleMediaAccessHandler. However its permission handling logic is
totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler. WebUILoginView::RequestMediaAccessPermission
is changed to go through the MediaCaptureDevicesDispatcher which matches the
codepath for other media permission requests.

BUG=596786
==========

to

==========
Move ChromeOS login media access logic into a MediaAccessHandler

Currently the ChromeOS login screen media access logic is handled as a part of
the PermissionBubbleMediaAccessHandler. However its permission handling logic
is totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler. WebUILoginView::RequestMediaAccessPermission
is changed to go through the MediaCaptureDevicesDispatcher which matches the
codepath for other media permission requests.

BUG=596786
==========

[raymes, 2017-04-03 03:23:54]

Patchset #4 (id:60001) has been deleted

[raymes, 2017-04-03 03:24:01]

Patchset #4 (id:80001) has been deleted

[raymes, 2017-04-03 03:30:11]

achuithb: could you PTAL. I've updated this based on the bug fix in
crbug.com/701700.

Specifically the changes are:
1) Renamed SAMLMediaAccessHandler to ChromeOSLoginMediaAccessHandler to better
reflect its purpose
2) Added a browsertest in webview_login_browsertest.cc to verify that requesting
media capture works properly from the login screen for ChromeOS.

https://codereview.chromium.org/2746873004/diff/40001/chrome/browser/media/sa...
File chrome/browser/media/saml_media_access_handler.h (right):

https://codereview.chromium.org/2746873004/diff/40001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.h:1: // Copyright 2017 The
Chromium Authors. All rights reserved.
On 2017/03/16 03:46:04, achuithb wrote:
> Don't think this should be changed.

This is a new file being added so it's appropriate to update the date.

https://codereview.chromium.org/2746873004/diff/40001/chrome/browser/media/sa...
chrome/browser/media/saml_media_access_handler.h:5: #ifndef
CHROME_BROWSER_MEDIA_SAML_MEDIA_ACCESS_HANDLER_H_
On 2017/03/16 03:46:04, achuithb wrote:
> Should this file be moved to chrome/browser/chromeos?

I can move it, but all the other media access handlers are here, including
extensions ones. I defer to sergeyu@ on that.

[raymes, 2017-04-03 03:30:25]

Description was changed from

==========
Move ChromeOS login media access logic into a MediaAccessHandler

Currently the ChromeOS login screen media access logic is handled as a part of
the PermissionBubbleMediaAccessHandler. However its permission handling logic
is totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler. WebUILoginView::RequestMediaAccessPermission
is changed to go through the MediaCaptureDevicesDispatcher which matches the
codepath for other media permission requests.

BUG=596786
==========

to

==========
Move ChromeOS login media access logic into a MediaAccessHandler

Currently the ChromeOS login screen media access logic is handled as a part of
the PermissionBubbleMediaAccessHandler. However its permission handling logic
is totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler. WebUILoginView::RequestMediaAccessPermission
is changed to go through the MediaCaptureDevicesDispatcher which matches the
codepath for other media permission requests.

BUG=596786
==========

[raymes, 2017-04-03 03:30:58]

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

[commit-bot: I haz the power, 2017-04-03 03:31:10]

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, 2017-04-03 05:13:22]

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

[commit-bot: I haz the power, 2017-04-03 05:13:23]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-04-04 21:52:53]

achuithb: friendly ping :)

[achuithb, 2017-04-05 00:55:35]

achuith@chromium.org changed reviewers:
+ emaxx@chromium.org, krishnargv@chromium.org

[achuithb, 2017-04-05 00:55:36]

Max, could you also take a look? 

Krishna, we should test SAML camera login once it lands and makes it into the
ChromeOS tree.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/webview_login_browsertest.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:138:
IN_PROC_BROWSER_TEST_F(WebviewLoginTest, RequestCamera) {
Thanks for adding this.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
File chrome/browser/media/chromeos_login_media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:23:
chromeos::LoginDisplayHost* login_display_host =
Can we add a DCHECK(web_contents);

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:66: VLOG(0) <<
"Ignoring wildcard URL pattern: " << value;
Should this be VLOG(1)?

[achuithb, 2017-04-05 00:56:00]

lgtm from me; please wait for Maksim if possible.

[krishnargv, 2017-04-05 01:06:27]

On 2017/04/05 00:56:00, achuithb wrote:
> lgtm from me; please wait for Maksim if possible.

Will execute a regression of the LoginVideoCaptureAllowedUrls for SAML/SSO based
logins after this CL lands.

[krishnargv, 2017-04-05 01:06:32]

On 2017/04/05 00:56:00, achuithb wrote:
> lgtm from me; please wait for Maksim if possible.

Will execute a regression of the LoginVideoCaptureAllowedUrls for SAML/SSO based
logins after this CL lands.

[krishnargv, 2017-04-05 01:06:36]

On 2017/04/05 00:56:00, achuithb wrote:
> lgtm from me; please wait for Maksim if possible.

Will execute a regression of the LoginVideoCaptureAllowedUrls for SAML/SSO based
logins after this CL lands.

[emaxx, 2017-04-06 02:41:47]

LGTM with nits, though I'm not super familiar with the modified code.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:1447: {
nit: The brackets are no more required.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/webview_login_browsertest.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:151:
ASSERT_TRUE(getUserMediaSuccess);
nit: Replace with EXPECT_TRUE here and in other places? As the usual approach,
AFAIU, is to use ASSERT_* only when the subsequent checks will crash or
something.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
File chrome/browser/media/chromeos_login_media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:42: if
(security_origin.spec() == chrome::kChromeUIOobeURL)
nit: #include "url/gurl.h"

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:59: std::string
value;
nit #include <string>

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:66: VLOG(0) <<
"Ignoring wildcard URL pattern: " << value;
nit: #include "base/logging.h"

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/m...
File chrome/browser/media/media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/m...
chrome/browser/media/media_access_handler.cc:22: // between the two classes.
nit: Reformat the comment.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/m...
chrome/browser/media/media_access_handler.cc:80: callback.Run(devices, result,
std::move(ui));
nit: #include <utility>

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:75: return
content::WebContents::FromRenderFrameHost(
It's not entirely clear to me whether we should care about RenderFrameHost or
WebContents being NULL here. Could you please add a DCHECK on that and/or drop a
comment explaining why it's safe to not do any checks here?

[raymes, 2017-04-10 00:16:49]

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:1447: {
On 2017/04/06 02:41:47, emaxx wrote:
> nit: The brackets are no more required.

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/webview_login_browsertest.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:151:
ASSERT_TRUE(getUserMediaSuccess);
On 2017/04/06 02:41:47, emaxx wrote:
> nit: Replace with EXPECT_TRUE here and in other places? As the usual approach,
> AFAIU, is to use ASSERT_* only when the subsequent checks will crash or
> something.

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
File chrome/browser/media/chromeos_login_media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:23:
chromeos::LoginDisplayHost* login_display_host =
On 2017/04/05 00:55:36, achuithb wrote:
> Can we add a DCHECK(web_contents);

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:42: if
(security_origin.spec() == chrome::kChromeUIOobeURL)
On 2017/04/06 02:41:47, emaxx wrote:
> nit: #include "url/gurl.h"

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:59: std::string
value;
On 2017/04/06 02:41:47, emaxx wrote:
> nit #include <string>

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:66: VLOG(0) <<
"Ignoring wildcard URL pattern: " << value;
On 2017/04/06 02:41:47, emaxx wrote:
> nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:66: VLOG(0) <<
"Ignoring wildcard URL pattern: " << value;
On 2017/04/05 00:55:36, achuithb wrote:
> Should this be VLOG(1)?

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/m...
File chrome/browser/media/media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/m...
chrome/browser/media/media_access_handler.cc:22: // between the two classes.
On 2017/04/06 02:41:47, emaxx wrote:
> nit: Reformat the comment.

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/m...
chrome/browser/media/media_access_handler.cc:80: callback.Run(devices, result,
std::move(ui));
On 2017/04/06 02:41:47, emaxx wrote:
> nit: #include <utility>

Done.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:75: return
content::WebContents::FromRenderFrameHost(
On 2017/04/06 02:41:47, emaxx wrote:
> It's not entirely clear to me whether we should care about RenderFrameHost or
> WebContents being NULL here. Could you please add a DCHECK on that and/or drop
a
> comment explaining why it's safe to not do any checks here?

That's a good point. Added a DCHECK for now.

[raymes, 2017-04-10 00:16:55]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-04-10 00:16:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, achuith@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2746873004/#ps140001
(title: "Move ChromeOS login media access logic into a MediaAccessHandler")

[commit-bot: I haz the power, 2017-04-10 00:17:08]

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, 2017-04-10 01:07:38]

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

[commit-bot: I haz the power, 2017-04-10 01:07:40]

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

[raymes, 2017-04-10 02:42:28]

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
File chrome/browser/media/chromeos_login_media_access_handler.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/c...
chrome/browser/media/chromeos_login_media_access_handler.cc:23:
chromeos::LoginDisplayHost* login_display_host =
On 2017/04/10 00:16:49, raymes wrote:
> On 2017/04/05 00:55:36, achuithb wrote:
> > Can we add a DCHECK(web_contents);
> 
> Done.

It turns out this can be nullptr, so I've added a check to return false in that
case.

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/2746873004/diff/120001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:75: return
content::WebContents::FromRenderFrameHost(
On 2017/04/10 00:16:49, raymes wrote:
> On 2017/04/06 02:41:47, emaxx wrote:
> > It's not entirely clear to me whether we should care about RenderFrameHost
or
> > WebContents being NULL here. Could you please add a DCHECK on that and/or
drop
> a
> > comment explaining why it's safe to not do any checks here?
> 
> That's a good point. Added a DCHECK for now.

This can be nullptr, so I've added a check in the handler to handle that
behavior.

[raymes, 2017-04-10 02:42:34]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-04-10 02:42:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, achuith@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2746873004/#ps160001
(title: "Move ChromeOS login media access logic into a MediaAccessHandler")

[commit-bot: I haz the power, 2017-04-10 02:42:50]

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, 2017-04-10 04:02:57]

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

[commit-bot: I haz the power, 2017-04-10 04:02:58]

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

[raymes, 2017-04-10 22:20:56]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-04-10 22:20:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, achuith@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2746873004/#ps180001
(title: "Move ChromeOS login media access logic into a MediaAccessHandler")

[commit-bot: I haz the power, 2017-04-10 22:21:34]

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, 2017-04-10 23:56:07]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491862856902640,
"parent_rev": "1767498665393e3c72cf7b48918f8be008094ea6", "commit_rev":
"2edae96f04b07205ce7b3ad83af879bcec2318a2"}

[commit-bot: I haz the power, 2017-04-10 23:56:56]

Description was changed from

==========
Move ChromeOS login media access logic into a MediaAccessHandler

Currently the ChromeOS login screen media access logic is handled as a part of
the PermissionBubbleMediaAccessHandler. However its permission handling logic
is totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler. WebUILoginView::RequestMediaAccessPermission
is changed to go through the MediaCaptureDevicesDispatcher which matches the
codepath for other media permission requests.

BUG=596786
==========

to

==========
Move ChromeOS login media access logic into a MediaAccessHandler

Currently the ChromeOS login screen media access logic is handled as a part of
the PermissionBubbleMediaAccessHandler. However its permission handling logic
is totally different to how permission requests are handled for drive-by pages.
This moves the logic into its own MediaAccessHandler. Some code is shared with
the ExtensionMediaAccessHandler. WebUILoginView::RequestMediaAccessPermission
is changed to go through the MediaCaptureDevicesDispatcher which matches the
codepath for other media permission requests.

BUG=596786

Review-Url: https://codereview.chromium.org/2746873004
Cr-Commit-Position: refs/heads/master@{#463454}
Committed:
https://chromium.googlesource.com/chromium/src/+/2edae96f04b07205ce7b3ad83af8...
==========

[commit-bot: I haz the power, 2017-04-10 23:56:57]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/2edae96f04b07205ce7b3ad83af8...