Check all settings when checking mic and camera access

Check all settings when checking mic and camera access.

We check if we have camera or mic access when devices are enumerated to determine if labels should be visible or not. We must ensure all settings are checked when doing this.

* Add interface to ContentBrowserClient for checking media access permission. Add implementation in //chrome.
* Refactor permission checking in MediaStreamDevicesController to be able to use them from elsewhere.
* Add checking policy and app permission to the existing content settings check.
* Remove old interfaces on ResourceContext and its implementations.
* Move the permission check from MediaStreamDispatcherHost to MediaStreamManager.
* Refactor MediaStreamManager to be able to mock the check function and update unit test to mock it.

TBR=jochen@chromium.org (for a few trivial changes)

BUG=406094
Committed: https://crrev.com/72dc4829a2a5bd7cbf42c956ac9ba6e8e744b004
Cr-Commit-Position: refs/heads/master@{#292868}

[Henrik Grunell, 2014-08-25 12:23:53]

This is a draft of the interface I plan to add. If OK, I'll proceed with
- implementation in //chrome,
- another function for Camera (or possibly argument),
- ideally have a general check that includes also the content settings (i.e.
tests if we already know access is allowed or not, without asking the user at
this time), and in that case remove those functions from ResourceContext,
- look over naming of the function.

Let me know if you have objections to the interface.

Tommi: General.
sky@: content/browser/*
creis@: content/public/*

[Henrik Grunell, 2014-08-25 12:24:44]

Oh, and this CL would make the CL https://codereview.chromium.org/502483002/
tommi@ have reviewed obsolete.

[Charlie Reis, 2014-08-25 18:35:43]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2014-08-25 18:35:44]

I don't see any big concerns given the similarity to
RequestMediaAccessPermission.  One naming nit and question below, but you'll
probably also want John's feedback on the content/public API change.

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_delegate.h:136: virtual void
AllowMicAccessBasedOnPolicyAndAppPermissions(
nit: Calling this "AllowFoo" makes it sound like it's granting the permission. 
Would RequestMicAccessPermission (similar to RequestMediaAccessPermission above)
work, or does that imply that it's going to query the user?  Maybe
CheckMicAccessPermission?

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_delegate.h:137: const GURL&
security_origin,
Security origins can be a bit subtle to get right.  Where will you be getting
this value from?

[Henrik Grunell, 2014-08-25 19:14:14]

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_delegate.h:136: virtual void
AllowMicAccessBasedOnPolicyAndAppPermissions(
On 2014/08/25 18:35:44, Charlie Reis wrote:
> nit: Calling this "AllowFoo" makes it sound like it's granting the permission.

> Would RequestMicAccessPermission (similar to RequestMediaAccessPermission
above)
> work, or does that imply that it's going to query the user?  Maybe
> CheckMicAccessPermission?

Agree. I think "Request..." implies query the user. I prefer "Check..." Will do
that change.

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_delegate.h:137: const GURL&
security_origin,
On 2014/08/25 18:35:44, Charlie Reis wrote:
> Security origins can be a bit subtle to get right.  Where will you be getting
> this value from?

From
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

[jam, 2014-08-25 19:18:24]

looks like this cl isn't ready for review yet?

https://codereview.chromium.org/483523006/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/483523006/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:451: const
MediaAccessResponseCallback& callback);
no need for MediaAccessResponseCallback here, just make this self contained with
a 
const base::Callback<void(bool)>& callback

[Charlie Reis, 2014-08-25 20:30:11]

SGTM, if you want to proceed with it.

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_delegate.h:137: const GURL&
security_origin,
On 2014/08/25 19:14:13, Henrik Grunell wrote:
> On 2014/08/25 18:35:44, Charlie Reis wrote:
> > Security origins can be a bit subtle to get right.  Where will you be
getting
> > this value from?
> 
> From
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Ok, sounds like there's an existing (presumably vetted) source.  Works for me.

[Henrik Grunell, 2014-08-26 16:56:32]

PTAL.

getSources path through MediaStreamCenter (renderer) and
DeviceRequestMessageFilter (browser) doesn't work at the moment due to missing
render frame id. This needs to be addressed - will look at tomorrow.

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/483523006/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_delegate.h:136: virtual void
AllowMicAccessBasedOnPolicyAndAppPermissions(
On 2014/08/25 19:14:13, Henrik Grunell wrote:
> On 2014/08/25 18:35:44, Charlie Reis wrote:
> > nit: Calling this "AllowFoo" makes it sound like it's granting the
permission.
> 
> > Would RequestMicAccessPermission (similar to RequestMediaAccessPermission
> above)
> > work, or does that imply that it's going to query the user?  Maybe
> > CheckMicAccessPermission?
> 
> Agree. I think "Request..." implies query the user. I prefer "Check..." Will
do
> that change.

Done.

https://codereview.chromium.org/483523006/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/483523006/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:451: const
MediaAccessResponseCallback& callback);
On 2014/08/25 19:18:23, jam wrote:
> no need for MediaAccessResponseCallback here, just make this self contained
with
> a 
> const base::Callback<void(bool)>& callback

Done.

[Charlie Reis, 2014-08-26 21:52:40]

content/ looks ok to me so far.  Let us know once it's working.

(One lament: so many layers to go through!  Don't know if it will help in this
case or not, but I look forward to Mojo cutting out some of this glue in the
future.)

[Henrik Grunell, 2014-08-27 05:36:03]

On 2014/08/26 21:52:40, Charlie Reis wrote:
> content/ looks ok to me so far.  Let us know once it's working.
> 
> (One lament: so many layers to go through!  Don't know if it will help in this
> case or not, but I look forward to Mojo cutting out some of this glue in the
> future.)

Yes, indeed. :\ One thing with this CL is that we shouldn't really have to go to
RenderFrameHostDelegate and check the access. I think it make sense to put it
there along with the RequestMediaAccessPermission function, but on the other
hand we don't need to know the tab that asked for this. (The request function
does; it may trigger an infobar/bubble.) Afaict as of yesterday night there's no
straight forward way of getting the frame associated with the request in the
getSources cases. (For EnumerateDevices in MediaStreamDispatcher we have it
already.) I'll check this and possibly move this function to another interface
surface.

Another thing is that it would be nice to harmonize the settings access check.
I've broken out the individual checks (always allow origin, device policy, api
permission and content settings) that makes sense, but I think move and modify
MediaStreamDevicesController::DismissInfoBarAndTakeActionOnSettings to a general
GetSettingsAccess that returns allow, deny or no_setting would be good. I might
do that in this CL.

[Henrik Grunell, 2014-08-28 12:51:31]

grunell@chromium.org changed reviewers:
+ lcwu@chromium.org, michaelbai@chromium.org, perkj@chromium.org

[Henrik Grunell, 2014-08-28 12:57:15]

Adding reviewers:
perkj@: Please do a general review, but particularly the */media/* parts.
michaelbai@: android_webview/*
lcwu@: chromecast/*

sky@, jam@ and creis@, ptal.
sky@: content/browser/* and chrome/*
creis@: content/public/*
jam@: creis added you for input on his parts afaik, feel free to have a general
look.

[perkj_chrome, 2014-08-28 15:09:40]

As far as I got today...

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.cc:261: if
(!security_origin.SchemeIs(extensions::kExtensionScheme))
Should this be CHECK( ?

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.cc:411: } else {
no need for else since you return above.

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.cc:428: security_origin,
same parameter twice? Why ? please comment in code.

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.h:87: // Checks if we have
 media access permission. Note that this only checks the
nit: extra space

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_util.h (right):

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_util.h:5: #ifndef
CHROME_BROWSER_MEDIA_MEDIA_STREAM_DEVICES_UTIL_H_
media_stream_device_policy.h ?

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_util.h:19: bool
ShouldAlwaysAllowOrigin(Profile* profile, const GURL& security_origin);
ShouldAlwaysAllowOrigin is a bit confusing name I think. What is it that is
allowed? From what origins?

https://codereview.chromium.org/483523006/diff/60001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/483523006/diff/60001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:325: // Checks for
media access. Overridden by unit tests.
media device permissions?

[lcwu1, 2014-08-28 17:03:45]

lgtm on chromecast/*.

[Charlie Reis, 2014-08-28 17:27:48]

This is much nicer than going through all the RFH->WebContents->Browser layers! 
Moving it from ResourceContext to BrowserContext also seems like an improvement,
rather than caching the mic/camera permissions on the IO thread.

content/public LGTM.

https://codereview.chromium.org/483523006/diff/60001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/483523006/diff/60001/content/public/browser/c...
content/public/browser/content_browser_client.h:642: // Checks if we have
permission to access the microphone or camera. Note that
nit: we have -> |security_origin| has

[Henrik Grunell, 2014-08-29 07:59:10]

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.cc:261: if
(!security_origin.SchemeIs(extensions::kExtensionScheme))
On 2014/08/28 15:09:40, perkj wrote:
> Should this be CHECK( ?

No, it may or may not be an extension. If not, it's signaled with NULL.

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.cc:411: } else {
On 2014/08/28 15:09:40, perkj wrote:
> no need for else since you return above.

Done.

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.cc:428: security_origin,
On 2014/08/28 15:09:40, perkj wrote:
> same parameter twice? Why ? please comment in code.

Done.

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.h:87: // Checks if we have
 media access permission. Note that this only checks the
On 2014/08/28 15:09:40, perkj wrote:
> nit: extra space

Done.

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_util.h (right):

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_util.h:5: #ifndef
CHROME_BROWSER_MEDIA_MEDIA_STREAM_DEVICES_UTIL_H_
On 2014/08/28 15:09:40, perkj wrote:
> media_stream_device_policy.h ? 

The ShouldAllow... function has nothing to do with policy so I think a general
name is better.

https://codereview.chromium.org/483523006/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_util.h:19: bool
ShouldAlwaysAllowOrigin(Profile* profile, const GURL& security_origin);
On 2014/08/28 15:09:40, perkj wrote:
> ShouldAlwaysAllowOrigin is a bit confusing name I think. What is it that is
> allowed? From what origins?  

Changed the name, hopefully better. (It's media stream content that is allowed
for that origin.)

https://codereview.chromium.org/483523006/diff/60001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/483523006/diff/60001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:325: // Checks for
media access. Overridden by unit tests.
On 2014/08/28 15:09:40, perkj wrote:
> media device permissions?

We have e.g. HandleAccessRequestResponse and FinalizeMediaAccessRequest, so I
chose the name to be in line with that. I'd prefer to stick to the current
naming convention. (It's also in other places like RenderFrameHostDelegate.)

https://codereview.chromium.org/483523006/diff/60001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/483523006/diff/60001/content/public/browser/c...
content/public/browser/content_browser_client.h:642: // Checks if we have
permission to access the microphone or camera. Note that
On 2014/08/28 17:27:48, Charlie Reis wrote:
> nit: we have -> |security_origin| has

Done.

[perkj_chrome, 2014-08-29 09:38:10]

Looks good.

https://codereview.chromium.org/483523006/diff/80001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_util.h (right):

https://codereview.chromium.org/483523006/diff/80001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_util.h:6: #define
CHROME_BROWSER_MEDIA_MEDIA_STREAM_DEVICES_UTIL_H_
CHROME_BROWSER_MEDIA_MEDIA_STREAM_DEVICE_PERMISSIONS_? 


We have so many files named something like media_stream_.... So I don't like a
general name. 
Can you try to come up with a name that motivates the existence of this file and
the methods within?

https://codereview.chromium.org/483523006/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/483523006/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1495: bool
MediaStreamManager::CheckMediaAccessOnUI(int render_process_id,
CheckMediaAccessPermissionsOnUi?

https://codereview.chromium.org/483523006/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1536: //
TODO(tommi):
Not this cl maybe, but is this todo still relevant? Should it rathter say that
EnumerateDevices should change name to reflect that its a subscriptions service
for device changes?

[Henrik Grunell, 2014-08-29 11:20:08]

https://codereview.chromium.org/483523006/diff/80001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_util.h (right):

https://codereview.chromium.org/483523006/diff/80001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_util.h:6: #define
CHROME_BROWSER_MEDIA_MEDIA_STREAM_DEVICES_UTIL_H_
On 2014/08/29 09:38:10, perkj wrote:
> CHROME_BROWSER_MEDIA_MEDIA_STREAM_DEVICE_PERMISSIONS_? 
> 
> 
> We have so many files named something like media_stream_.... So I don't like a
> general name. 
> Can you try to come up with a name that motivates the existence of this file
and
> the methods within? 

Yeah, sgtm. I changed it to your suggestion, I think that's fine.

https://codereview.chromium.org/483523006/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/483523006/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1495: bool
MediaStreamManager::CheckMediaAccessOnUI(int render_process_id,
On 2014/08/29 09:38:10, perkj wrote:
> CheckMediaAccessPermissionsOnUi?

Yes, that's good. Even ...OnUIThread() so that it's named exactly like existing
functions in the class. Done.

https://codereview.chromium.org/483523006/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1536: //
TODO(tommi):
On 2014/08/29 09:38:10, perkj wrote:
> Not this cl maybe, but is this todo still relevant? Should it rathter say that
> EnumerateDevices should change name to reflect that its a subscriptions
service
> for device changes?

I think we still should separate the notification from the request. In that case
it's relevant.

[perkj_chrome, 2014-08-29 11:42:50]

lgtm on content/renderer/media.

[Henrik Grunell, 2014-08-29 11:44:31]

On 2014/08/29 11:42:50, perkj wrote:
> lgtm on content/renderer/media.

You mean content/browser/renderer_host/media?

Did you review chrome/browser/media?

[Henrik Grunell, 2014-08-29 13:35:25]

On 2014/08/29 11:44:31, Henrik Grunell wrote:
> On 2014/08/29 11:42:50, perkj wrote:
> > lgtm on content/renderer/media.
> 
> You mean content/browser/renderer_host/media?
> 
> Did you review chrome/browser/media?

Talked to Per. Yes he means content/browser/renderer_host/media. And yes, he's
reviewed chrome/browser/media (but is not owner).

[Henrik Grunell, 2014-08-29 13:42:13]

grunell@chromium.org changed reviewers:
- sky@chromium.org

[Henrik Grunell, 2014-08-29 13:42:13]

Remaining reviewers, ptal:

michaelbai@: android/webview/*

tommi@: chrome/browser/media/*

jam@:
(At least)
chrome/browser/chrome_content_browser_client.h/cc
content/browser/loader/resource_scheduler_unittest.cc
content/browser/renderer_host/render_process_host_impl.cc
content/shell/browser/shell_browser_context.cc

[michaelbai, 2014-08-29 16:06:02]

lgtm android_webview

[tommi (sloooow) - chröme, 2014-08-30 13:57:15]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2014-08-30 13:57:16]

lgtm

[commit-bot: I haz the power, 2014-08-30 13:58:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grunell@chromium.org/483523006/100001

[commit-bot: I haz the power, 2014-08-30 14:10:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48499)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/53827)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-30 14:11:57]

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

[commit-bot: I haz the power, 2014-08-30 14:11:59]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Henrik Grunell, 2014-09-01 05:59:52]

grunell@chromium.org changed reviewers:
+ jochen@chromium.org

[Henrik Grunell, 2014-09-01 06:01:46]

Adding jochen@ as reviewer, please review:

chrome/browser/chrome_content_browser_client.h/cc
chrome/browser/profiles/profile_io_data.h/cc
content/browser/loader/resource_scheduler_unittest.cc
content/browser/renderer_host/render_process_host_impl.cc
content/shell/browser/shell_browser_context.cc

[Henrik Grunell, 2014-09-01 11:26:55]

On 2014/09/01 06:01:46, Henrik Grunell wrote:
> Adding jochen@ as reviewer, please review:
> 
> chrome/browser/chrome_content_browser_client.h/cc
> chrome/browser/profiles/profile_io_data.h/cc
> content/browser/loader/resource_scheduler_unittest.cc
> content/browser/renderer_host/render_process_host_impl.cc
> content/shell/browser/shell_browser_context.cc

I'm going to go ahead and TBR jochen@ on the remaining trivial changes.
(chrome/browser/chrome_content_browser_client.h/cc has been reviewed already I
noticed.)

[Henrik Grunell, 2014-09-01 11:27:03]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-09-01 11:27:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grunell@chromium.org/483523006/140001

[commit-bot: I haz the power, 2014-09-01 12:30:40]

Committed patchset #8 (id:140001) as b4a527348ab1b0e410f9da29b1a6d886205c9c69

[jam, 2014-09-02 19:56:43]

https://codereview.chromium.org/483523006/diff/140001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/483523006/diff/140001/content/public/browser/...
content/public/browser/content_browser_client.h:645: virtual bool
CheckMediaAccessPermission(BrowserContext* browser_context,
this should be on BrowserContext, since it's a method that is per-BrowserContext

ContentBrowserClient is a last-resort when a callback from content has no other
delegate interface

[commit-bot: I haz the power, 2014-09-10 03:16:46]

Patchset 8 (id:??) landed as
https://crrev.com/72dc4829a2a5bd7cbf42c956ac9ba6e8e744b004
Cr-Commit-Position: refs/heads/master@{#292868}

[Henrik Grunell, 2014-10-06 09:01:02]

On 2014/09/02 19:56:43, jam wrote:
>
https://codereview.chromium.org/483523006/diff/140001/content/public/browser/...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/483523006/diff/140001/content/public/browser/...
> content/public/browser/content_browser_client.h:645: virtual bool
> CheckMediaAccessPermission(BrowserContext* browser_context,
> this should be on BrowserContext, since it's a method that is
per-BrowserContext
> 
> ContentBrowserClient is a last-resort when a callback from content has no
other
> delegate interface

To close this: The check has been moved to RenderFrameHostDelegate and
WebContentsDelegate in https://codereview.chromium.org/562263002/.