Refactor: Make MediaCaptureDevicesDispatcher have pluggable handlers.

Refactor: Make MediaCaptureDevicesDispatcher have pluggable handlers.

Codes of function ProcessMediaAccessRequest() are all located under class
MediaCaptureDevicesDispatcher at the moment, as a result,
MediaCaptureDevicesDispatcher looks quite heavy.

Add MediaAccessHandler interface for pluggable handlers for different types
of media requests. And move all logics of handling different types of
media requests to separate implementation classes.

BUG=386012
Committed: https://crrev.com/43fd86763b002480899f10184fbaba6c9581d889
Cr-Commit-Position: refs/heads/master@{#333450}

[changbin, 2015-04-24 06:53:34]

changbin.shao@intel.com changed reviewers:
+ sergeyu@chromium.org, thestig@chromium.org

[changbin, 2015-04-24 06:53:34]

Hi, PTAL. Thanks!

[Lei Zhang, 2015-04-24 18:39:17]

sergeyu@ should be the primary reviewer on this.

https://codereview.chromium.org/1095393004/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:3188:
'browser/media/extension_media_access_handler.cc',
Just put these in the 'chrome_browser_extensions_sources' section.

[Sergey Ulanov, 2015-04-24 22:43:51]

Thanks for working on this stuff!

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/deskto...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/deskto...
chrome/browser/media/desktop_capture_access_handler.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
nit: Here and in other files: please remove (c)
https://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
File chrome/browser/media/media_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_access_handler.cc:7:
MediaAccessHandler::MediaAccessHandler() {
These can be moved to .h file, so you can remove this .cc file.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
File chrome/browser/media/media_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_access_handler.h:11: #include
"content/public/browser/web_contents.h"
WebContents can be forward-declared

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_access_handler.h:18: class MediaAccessHandler {
Add comments to explain what this interface is used for.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_access_handler.h:29: struct PendingAccessRequest {
This doesn't need to be in this header - they are useful only in some
implementations of MediaAccessHandler. Move to RegularMediaAccessHandler?

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_capture_devices_dispatcher.cc:201: if
(request.video_type == content::MEDIA_DESKTOP_VIDEO_CAPTURE ||
This code shouldn't need to know or care about type of request. Instead it would
be better to have list of MediaAccessHandler implementations and
MediaAccessHandler should expose some mechanism to tell this function which
types of requests it supports. Essentially this logic should be moved to
MediaAccessHandler implementations. E.g. HandleRequest() could return some 
error code indicating that the specific request is not supported by that
specific MediaAccessHandler.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_capture_devices_dispatcher.cc:210:
media_access_handler_.reset(
RegularMediaAccessHandler keeps queue of pending requests and you are destroying
it here, dropping all pending requests.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/regula...
File chrome/browser/media/regular_media_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/regula...
chrome/browser/media/regular_media_access_handler.h:10: class
RegularMediaAccessHandler : public MediaAccessHandler {
This name is not very descriptive. Maybe call it
PermissionBubbleMediaAccessHandler?

[changbin, 2015-04-29 05:24:35]

Thanks for your comments:)
Update the CL, PTAL.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/deskto...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/deskto...
chrome/browser/media/desktop_capture_access_handler.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/04/24 22:43:51, Sergey Ulanov wrote:
> nit: Here and in other files: please remove (c)
> https://www.chromium.org/developers/coding-style#TOC-File-headers

Done.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
File chrome/browser/media/media_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_access_handler.h:11: #include
"content/public/browser/web_contents.h"
On 2015/04/24 22:43:51, Sergey Ulanov wrote:
> WebContents can be forward-declared

Done.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_access_handler.h:18: class MediaAccessHandler {
On 2015/04/24 22:43:51, Sergey Ulanov wrote:
> Add comments to explain what this interface is used for.

Done.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_access_handler.h:29: struct PendingAccessRequest {
On 2015/04/24 22:43:51, Sergey Ulanov wrote:
> This doesn't need to be in this header - they are useful only in some
> implementations of MediaAccessHandler. Move to RegularMediaAccessHandler?

Thanks, done. PTAL

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_capture_devices_dispatcher.cc:201: if
(request.video_type == content::MEDIA_DESKTOP_VIDEO_CAPTURE ||
This sounds better;) 
Use list of MediaAccessHandler instead, ptal.
Also I add function SupportsRequest() in MediaAccessHandler to tell the caller
which type of request it supports. wdyt?

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_capture_devices_dispatcher.cc:210:
media_access_handler_.reset(
On 2015/04/24 22:43:51, Sergey Ulanov wrote:
> RegularMediaAccessHandler keeps queue of pending requests and you are
destroying
> it here, dropping all pending requests.

That means it should pass pointer of RequestsQueues to initialize
RegularMediaAccessHandler, right?

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/regula...
File chrome/browser/media/regular_media_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/regula...
chrome/browser/media/regular_media_access_handler.h:10: class
RegularMediaAccessHandler : public MediaAccessHandler {
On 2015/04/24 22:43:51, Sergey Ulanov wrote:
> This name is not very descriptive. Maybe call it
> PermissionBubbleMediaAccessHandler?

Done.

https://codereview.chromium.org/1095393004/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:3188:
'browser/media/extension_media_access_handler.cc',
On 2015/04/24 18:39:17, Lei Zhang wrote:
> Just put these in the 'chrome_browser_extensions_sources' section.

Thanks, done.

[changbin, 2015-05-05 01:27:53]

ping.
Please take a look when you have time. thanks!

[Sergey Ulanov, 2015-05-06 16:54:11]

On 2015/05/05 01:27:53, changbin wrote:
> ping.
> Please take a look when you have time. thanks!

Will try to review this today. Sorry about delay.

[Sergey Ulanov, 2015-05-07 01:05:49]

The main issue is that CheckMediaAccessPermission() logic belongs in
MediaAccessHandler too.

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1095393004/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_capture_devices_dispatcher.cc:210:
media_access_handler_.reset(
On 2015/04/29 05:24:34, changbin wrote:
> On 2015/04/24 22:43:51, Sergey Ulanov wrote:
> > RegularMediaAccessHandler keeps queue of pending requests and you are
> destroying
> > it here, dropping all pending requests.
> 
> That means it should pass pointer of RequestsQueues to initialize
> RegularMediaAccessHandler, right?

No. It means you shouldn't be reset handler for every new request. It's already
fixed in the last patchset.

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

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.h:9: #include <map>
remove these two includes. They are not used in this header.

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.h:12: #include
"base/memory/scoped_ptr.h"
Remove these two includes.

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

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:298: bool
MediaCaptureDevicesDispatcher::CheckMediaAccessPermission(
This logic needs to be moved to MediaAccessHandler too. Also see
https://codereview.chromium.org/1125333003 where I removed one of the
CheckMediaAccessPermission() overrides.

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:195: RequestsQueues
pending_requests_;
Move this to PermissionBubbleMediaAccessHandler

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/pe...
File chrome/browser/media/permission_bubble_media_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.cc:20: namespace {
nit: empty line after this one

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.cc:52: bool res =
request.video_type == content::MEDIA_DEVICE_VIDEO_CAPTURE ||
s/res/result/

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.cc:55: res &=
(!extension || (!extension->is_platform_app() &&
I don't think you need this check here. ExtensionMediaAccessHandler already
handles this case and it's added in the list of MediaAccessHandlers before
PermissionBubbleMediaAccessHandler

https://codereview.chromium.org/1095393004/diff/20001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/1095393004/diff/20001/extensions/common/exten...
extensions/common/extension.h:357: bool is_media_request_white_listed() const;
I don't think the whitelist belongs in the extensions. Please keep it in
chrome/browser/media. You can move the whitelist to ExtensionMediaAccessHandler
if you address my other comments, particularly about CheckMediaAccessPermission

[changbin, 2015-05-22 05:10:59]

sergeyu@, sorry for replying late due to I was on business travel.
I have updated patch according to your review comments, ptal.

PS#3 mainly includes changes as
a) Move CheckMediaAccessPermission() logic into MediaAccessHandler and its
implementations.
b) Move pending_requests_ and notifications_registrar_ to 
PermissionBubbleMediaAccessHandler. Now  PermissionBubbleMediaAccessHandler will
observe Notification instead.
c) Move some codes from UpdateMediaRequestStateOnUIThread() to
MediaAccessHandler implementations.  desktop_capture_sessions_ is now owned by
DesktopCaptureAccessHandler.
d) MediaAccessHandler::SupportsRequest() is now renamed to SupportsStreamType()
with argument as MediaStreamType instead.

PTAL. Thanks!

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

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.h:9: #include <map>
On 2015/05/07 01:05:48, Sergey Ulanov wrote:
> remove these two includes. They are not used in this header.

Done.

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.h:12: #include
"base/memory/scoped_ptr.h"
On 2015/05/07 01:05:48, Sergey Ulanov wrote:
> Remove these two includes.
content::MediaResponseCallback is actually base::Callback. While I don't know
how to forward-declare base::Callback here?

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

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:298: bool
MediaCaptureDevicesDispatcher::CheckMediaAccessPermission(
On 2015/05/07 01:05:48, Sergey Ulanov wrote:
> This logic needs to be moved to MediaAccessHandler too. Also see
> https://codereview.chromium.org/1125333003 where I removed one of the
> CheckMediaAccessPermission() overrides.

Done.

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:195: RequestsQueues
pending_requests_;
On 2015/05/07 01:05:48, Sergey Ulanov wrote:
> Move this to PermissionBubbleMediaAccessHandler

Done.

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/pe...
File chrome/browser/media/permission_bubble_media_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.cc:20: namespace {
On 2015/05/07 01:05:48, Sergey Ulanov wrote:
> nit: empty line after this one

Done.

https://codereview.chromium.org/1095393004/diff/20001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.cc:55: res &=
(!extension || (!extension->is_platform_app() &&
On 2015/05/07 01:05:48, Sergey Ulanov wrote:
> I don't think you need this check here. ExtensionMediaAccessHandler already
> handles this case and it's added in the list of MediaAccessHandlers before
> PermissionBubbleMediaAccessHandler

I agree with that. While shouldn't we make the code ignorant of the order of
handlers?

https://codereview.chromium.org/1095393004/diff/20001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/1095393004/diff/20001/extensions/common/exten...
extensions/common/extension.h:357: bool is_media_request_white_listed() const;
On 2015/05/07 01:05:49, Sergey Ulanov wrote:
> I don't think the whitelist belongs in the extensions. Please keep it in
> chrome/browser/media. You can move the whitelist to
ExtensionMediaAccessHandler
> if you address my other comments, particularly about
CheckMediaAccessPermission

Thanks, i think it's done now:)

[changbin, 2015-05-28 01:11:46]

ping.
please take a look when you have time:)

[Sergey Ulanov, 2015-06-01 22:08:51]

On 2015/05/28 01:11:46, changbin wrote:
> ping.
> please take a look when you have time:)

Looking now. Sorry about the delay.

[Sergey Ulanov, 2015-06-01 23:39:18]

Looks mostly good now, but I have more comments.
Thanks!

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:29: #if
defined(ENABLE_EXTENSIONS)
This file doesn't need to be compiled when extensions are disabled. I.e. you can
put this file under chrome_browser_extensions_sources in the gyp file and remove
these ifdefs.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:14: class
DesktopCaptureAccessHandler : public MediaAccessHandler {
Add short comment for this and other classes describing what this class is used
for. E.g.:
// MediaAccessHandler for DesktopCapture API.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
File chrome/browser/media/media_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.h:27: virtual bool
SupportsStreamType(const content::MediaStreamType type,
add a short comment for each method to describe its purpose.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.h:38: virtual void
UpdateMediaRequest(int render_process_id,
call it UpdateMediaRequestState()?

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:93: #if
defined(ENABLE_EXTENSIONS)
Only PermissionBubbleMediaAccessHandler should be enabled when extensions are
disabled.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:150:
handler->HandleRequest(web_contents, request, callback, extension);
Return after this call. You don't want more than one handler to handle the
request.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:152: }
Reject the request if there is no appropriate handler:
  callback.Run(content::MediaStreamDevices(),
content::MEDIA_DEVICE_NOT_SUPPORTED, nullptr);

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:159: return
CheckMediaAccessPermission(web_contents, security_origin, type, NULL);
s/NULL/nullptr/

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:159: return
CheckMediaAccessPermission(web_contents, security_origin, type, NULL);
Please keep DCHECK_CURRENTLY_ON(BrowserThread::UI);

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:328:
handler->UpdateMediaRequest(render_process_id, render_frame_id,
break after this call.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:334: if
(IsOriginForCasting(security_origin) && IsVideoMediaType(stream_type)) {
I think this won't compile for chrome os because you moved IsOriginForCasting()
(try adding chromeos=1 in GYP_DEFINES). I think adding static
MediaCaptureDevicesDispatcher::IsOriginForCasting() is fine.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:17: #include
"chrome/browser/media/media_access_handler.h"
You can forward-declare MediaAccessHandler. Don't need this include.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:18: #include
"chrome/browser/media/permission_bubble_media_access_handler.h"
Don't need this.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/pe...
File chrome/browser/media/permission_bubble_media_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.cc:20: #include
"extensions/common/extension.h"
I don't think you need this include

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.cc:77:
ContentSettingsType contentSettingsType =
content_settings_type

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/pe...
File chrome/browser/media/permission_bubble_media_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.h:15: struct
PendingAccessRequest {
this can be nested in PermissionBubbleMediaAccessHandler. Also you don't need to
define the class in the header. Just declare it and move definition to the .cc
file.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.h:63: typedef
std::deque<PendingAccessRequest> RequestsQueue;
please move before other private methods:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_capture_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_capture_access_handler.cc:12: #if
defined(ENABLE_EXTENSIONS)
This file doesn't need to be compiled when extensions are disabled. I.e. you can
put this file under chrome_browser_extensions_sources in the gyp file and remove
these ifdefs.

[changbin, 2015-06-02 14:02:51]

Patchset #5 (id:80001) has been deleted

[changbin, 2015-06-02 14:03:49]

Patchset #4 (id:60001) has been deleted

[changbin, 2015-06-02 14:20:10]

Update CL to address review comments of PS #3, please take another look when you
have a cycle. Thanks!

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:29: #if
defined(ENABLE_EXTENSIONS)
On 2015/06/01 23:39:17, Sergey Ulanov wrote:
> This file doesn't need to be compiled when extensions are disabled. I.e. you
can
> put this file under chrome_browser_extensions_sources in the gyp file and
remove
> these ifdefs. 

Thanks! Remove ifdefs check from desktop_capture_access_handler.cc &
tab_capture_access_handler.cc. PTAL.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:14: class
DesktopCaptureAccessHandler : public MediaAccessHandler {
On 2015/06/01 23:39:17, Sergey Ulanov wrote:
> Add short comment for this and other classes describing what this class is
used
> for. E.g.:
> // MediaAccessHandler for DesktopCapture API.

Done.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
File chrome/browser/media/media_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_access_handler.h:27: virtual bool
SupportsStreamType(const content::MediaStreamType type,
On 2015/06/01 23:39:17, Sergey Ulanov wrote:
> add a short comment for each method to describe its purpose.

Done.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:93: #if
defined(ENABLE_EXTENSIONS)
On 2015/06/01 23:39:17, Sergey Ulanov wrote:
> Only PermissionBubbleMediaAccessHandler should be enabled when extensions are
> disabled.

I was wrongly thought desktop capturing should also work when ENABLE_EXTENSIONS
is false. We won't support desktop capturing without using extension apis,
right?

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:150:
handler->HandleRequest(web_contents, request, callback, extension);
On 2015/06/01 23:39:17, Sergey Ulanov wrote:
> Return after this call. You don't want more than one handler to handle the
> request.

Thanks, will remove.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:328:
handler->UpdateMediaRequest(render_process_id, render_frame_id,
On 2015/06/01 23:39:17, Sergey Ulanov wrote:
> break after this call.

Done.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:334: if
(IsOriginForCasting(security_origin) && IsVideoMediaType(stream_type)) {
On 2015/06/01 23:39:17, Sergey Ulanov wrote:
> I think this won't compile for chrome os because you moved
IsOriginForCasting()
> (try adding chromeos=1 in GYP_DEFINES). I think adding static
> MediaCaptureDevicesDispatcher::IsOriginForCasting() is fine.

Yeah, my fault then. Will fix this.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:17: #include
"chrome/browser/media/media_access_handler.h"
On 2015/06/01 23:39:18, Sergey Ulanov wrote:
> You can forward-declare MediaAccessHandler. Don't need this include.

Done.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/pe...
File chrome/browser/media/permission_bubble_media_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/pe...
chrome/browser/media/permission_bubble_media_access_handler.h:15: struct
PendingAccessRequest {
On 2015/06/01 23:39:18, Sergey Ulanov wrote:
> this can be nested in PermissionBubbleMediaAccessHandler. Also you don't need
to
> define the class in the header. Just declare it and move definition to the .cc
> file.

Done.

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_capture_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_capture_access_handler.cc:12: #if
defined(ENABLE_EXTENSIONS)
On 2015/06/01 23:39:18, Sergey Ulanov wrote:
> This file doesn't need to be compiled when extensions are disabled. I.e. you
can
> put this file under chrome_browser_extensions_sources in the gyp file and
remove
> these ifdefs. 

Done.

[Sergey Ulanov, 2015-06-03 22:31:58]

Please fix the bug in
MediaCaptureDevicesDispatcher::UpdateMediaRequestStateOnUIThread(). Otherwise
looks good.

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/d...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/d...
chrome/browser/media/desktop_capture_access_handler.cc:170:
screen_capture_enabled |= MediaCaptureDevicesDispatcher::IsOriginForCasting(
Now that you don't have the ifdef this doesn't have to be a separate statement

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/d...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/d...
chrome/browser/media/desktop_capture_access_handler.h:55: typedef
std::list<DesktopCaptureSession> DesktopCaptureSessions;
nit: move DesktopCaptureSession and this typedef above
ProcessScreenCaptureAccessRequest()

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/m...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/m...
chrome/browser/media/media_capture_devices_dispatcher.cc:353: return;
this should be break, not return.

[changbin, 2015-06-05 06:40:34]

PTAL.
sergeyu@, can you help to kick off the tryjob if the patch looks good to you? My
local testing only covers Linux OS at the moment.
Thank you a lot:)

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/d...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/d...
chrome/browser/media/desktop_capture_access_handler.cc:170:
screen_capture_enabled |= MediaCaptureDevicesDispatcher::IsOriginForCasting(
On 2015/06/03 22:31:57, Sergey Ulanov wrote:
> Now that you don't have the ifdef this doesn't have to be a separate statement

Done.

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/d...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/d...
chrome/browser/media/desktop_capture_access_handler.h:55: typedef
std::list<DesktopCaptureSession> DesktopCaptureSessions;
On 2015/06/03 22:31:57, Sergey Ulanov wrote:
> nit: move DesktopCaptureSession and this typedef above
> ProcessScreenCaptureAccessRequest()

Done.

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/m...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1095393004/diff/100001/chrome/browser/media/m...
chrome/browser/media/media_capture_devices_dispatcher.cc:353: return;
On 2015/06/03 22:31:58, Sergey Ulanov wrote:
> this should be break, not return.

Done.

[Sergey Ulanov, 2015-06-08 19:04:38]

lgtm

[Sergey Ulanov, 2015-06-08 19:04:44]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-06-08 19:06:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1095393004/120001

[commit-bot: I haz the power, 2015-06-08 19:21:38]

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

[commit-bot: I haz the power, 2015-06-08 19:21:41]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[changbin, 2015-06-09 03:12:11]

The CQ bit was checked by changbin.shao@intel.com

[changbin, 2015-06-09 03:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1095393004/#ps140001
(title: "Fix tryjob error.")

[commit-bot: I haz the power, 2015-06-09 03:12:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1095393004/140001

[commit-bot: I haz the power, 2015-06-09 05:13:59]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-06-09 05:15:00]

Patchset 6 (id:??) landed as
https://crrev.com/43fd86763b002480899f10184fbaba6c9581d889
Cr-Commit-Position: refs/heads/master@{#333450}