Implement Chrome Extension TabCapture API.

Implement chrome extension tab capture API as specified here:
http://dev.chromium.org/developers/design-documents/extensions/proposed-changes/apis-under-development/webrtc-tab-content-capture

BUG=153388
TBR=sky@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163902

[justinlin, 2012-10-02 17:23:32]

Hi, please take a look at this CL. Will add tests shortly.

[Aaron Boodman, 2012-10-04 07:21:43]

Why isn't alpha on this review? I think he should probably take a pass on the
media parts, and he would have caught the style issues as well.

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:16: class
TabCaptureAsyncApiFunction : public AsyncApiFunction {
Is it possible to use IOThreadExtensionFunction instead? It is more efficient as
it avoid a round-trip from the UI thread. Also I'm not sure that
AsyncApiFunction should exist, so I'd rather not add dependencies.

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
File chrome/common/extensions/api/experimental_tab_capture.idl (right):

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental_tab_capture.idl:7: namespace
experimental.tabCapture {
Don't make this experimental. Make it a real API, but use
_permission_features.json to restrict it to 'trunk'.

It may also be useful to add the ability to c/b/extensions/features/feature.cc
to restrict to command line flags. Then you could add a command line flag and
require that to be set too.

We're trying to reduce our usage of 'experimental'. It has so many things in it,
and people turn it on to access one, but it gives access to them all.

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental_tab_capture.idl:40: // the "tabs"
permission to use this method.
Why do they need the 'tabs' permission? The tabs permission grants access to the
browsing activity of the user. I don't think that's what this API does (unless
it persists across navigations?)

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental_tab_capture.idl:44: static void
getTabMedia(optional long tabId,
Maybe just name this begin()? or capture()?

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental_tab_capture.idl:61: static void
onTabCaptured(CaptureInfo info);
Seems like name should be something like onStatusChange then.

http://codereview.chromium.org/11038021/diff/1/chrome/renderer/resources/exte...
File
chrome/renderer/resources/extensions/experimental.tab_capture_custom_bindings.js
(right):

http://codereview.chromium.org/11038021/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/experimental.tab_capture_custom_bindings.js:50:
navigator.webkitGetUserMedia(options, successFunc, errorFunc);
What prevents bad extensions from just calling this with whatever tabId they
want? Is permission checked somewhere behind this webkit call?

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc
(right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:21: void
RegisterAsMediaObserver(TabCaptureEventRouter* tabCaptureRouter) {
unix_hacker_style

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:21: void
RegisterAsMediaObserver(TabCaptureEventRouter* tabCaptureRouter) {
Free module-local functions should be defined in an anonymous namespace.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:42:
base::Bind(&UnRegisterAsMediaObserver, this));
Are you sure about the lifetime issues here? It seems like during shutdown it
will be possible for MediaInternals to try and poke at this dead object.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:69: int
tabId = -1;
unix_hacker_style!

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h
(right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h:22: class
TabCaptureEventRouter : public MediaInternalsObserver {
You will be the first to do this (sorry), but can we try making this class a
ProfileKeyedService that depends on ExtensionSystem? I would like to better
decouple clients of the extension system from the system itself.

[Alpha Left Google, 2012-10-04 20:52:00]

This is an initial pass for style check.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:12: #include
"chrome/browser/profiles/profile.h"
Fix order of include.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:99: std::map<int,
tab_capture::TabCaptureState> capture_states = profile()->
Is there a typedef for this map?

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:103: for
(std::map<int, tab_capture::TabCaptureState>::iterator it =
Same here, better if there's a typedef for this tab.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:23: //
AsyncApiFunction:
Period at the end of comments.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:35: //
AsyncApiFunction:
Period at end of line.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:40:
scoped_ptr<api::experimental_tab_capture::GetTabMedia::Params> params_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:56: };
This is empty and needs DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc
(right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:13:
#include "chrome/browser/extensions/event_router.h"
Reorder includes.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:31:
base::Bind(&RegisterAsMediaObserver, this));
indent by 4 spaces when you wrap a line

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:34: void
UnRegisterAsMediaObserver(TabCaptureEventRouter* tabCaptureRouter) {
like aa@ suggested, hacker_style.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:42:
base::Bind(&UnRegisterAsMediaObserver, this));
indent by 4 spaces when you wrap a line

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:49:
base::Bind(&TabCaptureEventRouter::HandleRequestUpdate,
indent by 4 spaces when you wrap a line

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:50:
base::Unretained(this), device, new_state));
indent by 4 spaces when you wrap a line

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:71:
base::StringPiece(device.device_id.data(), sep_pos), &tabId))
indentation is wrong

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:119:
std::map<int, tab_capture::TabCaptureState>
Use a typedef it will be look better.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h
(right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h:27:
std::map<int, tab_capture::TabCaptureState> GetCapturedTabs();
Use a typedef for this map.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h:40:
std::map<int, tab_capture::TabCaptureState> requests_;
Use typedef.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_id_map.cc (right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_id_map.cc:194: iter++) {
indentation is wrong here

[justinlin, 2012-10-08 09:58:31]

thanks! Went through most of the comments. Left to do: suggested api method
renames, fix registry/event_router media_observer deletion issue, most tests.

aa: I think one of your comments suggested adding a flag around this. Did you
mean similar to a flag in about:flags or a flag that can only be turned on via
command-line?

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:16: class
TabCaptureAsyncApiFunction : public AsyncApiFunction {
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> Is it possible to use IOThreadExtensionFunction instead? It is more efficient
as
> it avoid a round-trip from the UI thread. Also I'm not sure that
> AsyncApiFunction should exist, so I'd rather not add dependencies.

I can't seem to return a value when I use that interface. It crashes here:
https://cs.corp.google.com/#chrome/src/chrome/browser/extensions/extension_fu...

AsyncApiFunction seems like what I want though.

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
File chrome/common/extensions/api/experimental_tab_capture.idl (right):

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental_tab_capture.idl:7: namespace
experimental.tabCapture {
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> Don't make this experimental. Make it a real API, but use
> _permission_features.json to restrict it to 'trunk'.
> 
> It may also be useful to add the ability to c/b/extensions/features/feature.cc
> to restrict to command line flags. Then you could add a command line flag and
> require that to be set too.
> 
> We're trying to reduce our usage of 'experimental'. It has so many things in
it,
> and people turn it on to access one, but it gives access to them all.

Done. By command-line do you mean like one of the flags in about:flags, or
something that can only be enabled by giving the command line flag on startup?

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental_tab_capture.idl:40: // the "tabs"
permission to use this method.
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> Why do they need the 'tabs' permission? The tabs permission grants access to
the
> browsing activity of the user. I don't think that's what this API does (unless
> it persists across navigations?)

It does persist across navigations. Added this permission check in the code.

http://codereview.chromium.org/11038021/diff/1/chrome/renderer/resources/exte...
File
chrome/renderer/resources/extensions/experimental.tab_capture_custom_bindings.js
(right):

http://codereview.chromium.org/11038021/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/experimental.tab_capture_custom_bindings.js:50:
navigator.webkitGetUserMedia(options, successFunc, errorFunc);
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> What prevents bad extensions from just calling this with whatever tabId they
> want? Is permission checked somewhere behind this webkit call?

Do you mean what prevents any normal page calling this method? I added a check
so that the mediaSource constraints are only respected if called from a chrome
extension, so normal pages can't use tabCapture, only extensions can. However,
the purpose of this API is to be able to capture any tab, so you should be able
to specify any tabId you want for the extension API.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:12: #include
"chrome/browser/profiles/profile.h"
On 2012/10/04 20:52:01, Alpha wrote:
> Fix order of include.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:99: std::map<int,
tab_capture::TabCaptureState> capture_states = profile()->
On 2012/10/04 20:52:01, Alpha wrote:
> Is there a typedef for this map?

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:103: for
(std::map<int, tab_capture::TabCaptureState>::iterator it =
On 2012/10/04 20:52:01, Alpha wrote:
> Same here, better if there's a typedef for this tab.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:23: //
AsyncApiFunction:
On 2012/10/04 20:52:01, Alpha wrote:
> Period at the end of comments.

I think this is what they do in other files to specify overriden interfaces.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:35: //
AsyncApiFunction:
On 2012/10/04 20:52:01, Alpha wrote:
> Period at end of line.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:40:
scoped_ptr<api::experimental_tab_capture::GetTabMedia::Params> params_;
On 2012/10/04 20:52:01, Alpha wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:56: };
On 2012/10/04 20:52:01, Alpha wrote:
> This is empty and needs DISALLOW_COPY_AND_ASSIGN

Removed it.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc
(right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:13:
#include "chrome/browser/extensions/event_router.h"
On 2012/10/04 20:52:01, Alpha wrote:
> Reorder includes.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:21: void
RegisterAsMediaObserver(TabCaptureEventRouter* tabCaptureRouter) {
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> unix_hacker_style

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:21: void
RegisterAsMediaObserver(TabCaptureEventRouter* tabCaptureRouter) {
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> Free module-local functions should be defined in an anonymous namespace.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:31:
base::Bind(&RegisterAsMediaObserver, this));
On 2012/10/04 20:52:01, Alpha wrote:
> indent by 4 spaces when you wrap a line

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:34: void
UnRegisterAsMediaObserver(TabCaptureEventRouter* tabCaptureRouter) {
On 2012/10/04 20:52:01, Alpha wrote:
> like aa@ suggested, hacker_style.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:42:
base::Bind(&UnRegisterAsMediaObserver, this));
On 2012/10/04 20:52:01, Alpha wrote:
> indent by 4 spaces when you wrap a line

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:49:
base::Bind(&TabCaptureEventRouter::HandleRequestUpdate,
On 2012/10/04 20:52:01, Alpha wrote:
> indent by 4 spaces when you wrap a line

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:50:
base::Unretained(this), device, new_state));
On 2012/10/04 20:52:01, Alpha wrote:
> indent by 4 spaces when you wrap a line

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:69: int
tabId = -1;
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> unix_hacker_style!

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:71:
base::StringPiece(device.device_id.data(), sep_pos), &tabId))
On 2012/10/04 20:52:01, Alpha wrote:
> indentation is wrong

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:119:
std::map<int, tab_capture::TabCaptureState>
On 2012/10/04 20:52:01, Alpha wrote:
> Use a typedef it will be look better.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h
(right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h:22: class
TabCaptureEventRouter : public MediaInternalsObserver {
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> You will be the first to do this (sorry), but can we try making this class a
> ProfileKeyedService that depends on ExtensionSystem? I would like to better
> decouple clients of the extension system from the system itself.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h:27:
std::map<int, tab_capture::TabCaptureState> GetCapturedTabs();
On 2012/10/04 20:52:01, Alpha wrote:
> Use a typedef for this map.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.h:40:
std::map<int, tab_capture::TabCaptureState> requests_;
On 2012/10/04 20:52:01, Alpha wrote:
> Use typedef.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_id_map.cc (right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_id_map.cc:194: iter++) {
On 2012/10/04 20:52:01, Alpha wrote:
> indentation is wrong here

Done.

[Alpha Left Google, 2012-10-10 23:58:16]

Style check looks good to me. aa: please look again.

[justinlin, 2012-10-11 00:05:45]

On 2012/10/10 23:58:16, Alpha wrote:
> Style check looks good to me. aa: please look again.

Thanks! I have the rest of the changes promised almost done as well as a few
fixes, please take a look when I upload the 3rd patchset.

[justinlin, 2012-10-11 07:30:13]

Hi, please take another look!
Thanks

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
File chrome/common/extensions/api/experimental_tab_capture.idl (right):

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental_tab_capture.idl:44: static void
getTabMedia(optional long tabId,
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> Maybe just name this begin()? or capture()?

Done. I think I prefer capture() because begin() sounds like it implies this can
only be called once to me.

http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental_tab_capture.idl:61: static void
onTabCaptured(CaptureInfo info);
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> Seems like name should be something like onStatusChange then.

Done.

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc
(right):

http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:42:
base::Bind(&UnRegisterAsMediaObserver, this));
On 2012/10/04 07:21:43, Aaron Boodman wrote:
> Are you sure about the lifetime issues here? It seems like during shutdown it
> will be possible for MediaInternals to try and poke at this dead object.

Done, from what I can tell. Used a proxy class.

[justinlin, 2012-10-12 21:44:56]

aa: ping. this is ready for another look, thanks!

On 2012/10/11 07:30:13, justinlin wrote:
> Hi, please take another look!
> Thanks
> 
>
http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
> File chrome/common/extensions/api/experimental_tab_capture.idl (right):
> 
>
http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
> chrome/common/extensions/api/experimental_tab_capture.idl:44: static void
> getTabMedia(optional long tabId,
> On 2012/10/04 07:21:43, Aaron Boodman wrote:
> > Maybe just name this begin()? or capture()?
> 
> Done. I think I prefer capture() because begin() sounds like it implies this
can
> only be called once to me.
> 
>
http://codereview.chromium.org/11038021/diff/1/chrome/common/extensions/api/e...
> chrome/common/extensions/api/experimental_tab_capture.idl:61: static void
> onTabCaptured(CaptureInfo info);
> On 2012/10/04 07:21:43, Aaron Boodman wrote:
> > Seems like name should be something like onStatusChange then.
> 
> Done.
> 
>
http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc
> (right):
> 
>
http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a...
> chrome/browser/extensions/api/tab_capture/tab_capture_event_router.cc:42:
> base::Bind(&UnRegisterAsMediaObserver, this));
> On 2012/10/04 07:21:43, Aaron Boodman wrote:
> > Are you sure about the lifetime issues here? It seems like during shutdown
it
> > will be possible for MediaInternals to try and poke at this dead object.
> 
> Done, from what I can tell. Used a proxy class.

[Aaron Boodman, 2012-10-16 04:19:27]

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:16: class
TabCaptureAsyncApiFunction : public AsyncApiFunction {
On 2012/10/08 09:58:31, justinlin wrote:
> On 2012/10/04 07:21:43, Aaron Boodman wrote:
> > Is it possible to use IOThreadExtensionFunction instead? It is more
efficient
> as
> > it avoid a round-trip from the UI thread. Also I'm not sure that
> > AsyncApiFunction should exist, so I'd rather not add dependencies.
> 
> I can't seem to return a value when I use that interface. It crashes here:
>
https://cs.corp.google.com/#chrome/src/chrome/browser/extensions/extension_fu...
> 
> AsyncApiFunction seems like what I want though.

ExtensionFunctionDispatcher is handling this function for some reason, but it
can only handle UI thread functions. The IO thread functions are supposed to be
handled differently (see ExtensionFunctionDispatcher::DispatchOnIOThread).

I can't remember what causes one path or the other to be taken. You can take a
look at some other classes that inherit IOThreadExtensionFunction do and see if
you can do that as well?

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:40: if
(!GetExtension()->HasAPIPermission(APIPermission::kTab)) {
Two things:

(1) The 'tabs' permission is not the right level of access. Tabs permission just
means that the extension can see the URLs and titles in your browsing history.
This API also provides access to the content of those pages. So it should be
gated by 'host permissions' instead:
http://developer.chrome.com/extensions/manifest.html#permissions (the ones with
the URL patterns - we call these host permissions in the codebase).

(2) One interesting question is whether it would ever be useful for an extension
to be able to capture tab contents on some URLs but not others. The reason I ask
is that if that case is never interesting, then we can just make the tabCapture
permission 'imply' host permissions to all URLs. There are many other APIs we do
this with - for example the proxy API. You don't need to separately declare the
URLs you want access to in that case, we just assume it is all.

However, if it is the case that it would be useful to be able to capture on some
URLs but not others, then we could simply ask the developer to declare the
appropriate host permissions along with 'tabCapture'.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:50: void
TabCaptureCaptureFunction::Work() {
Wait a second... Why are you using AsyncApiFunction in the first place? There's
nothing in ::Work() that needs to be on the IO thread!

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:69:
(TabCaptureRegistry::TabCaptureRequest) { GetExtension()->id(), tab_id,
Is the cast necessary? Seems like it wouldn't be.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:75: if
(params_->options->audio.get()) {
Nit: this brace is not necessary. It's optional though, if you prefer it.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:83:
base::DictionaryValue* result = new base::DictionaryValue();
JSON Schema Compiler generates structs you can use for the result too, instead
of doing this manual JSON stuff.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:85:
result->SetString(std::string("videoConstraints.mandatory.") +
Question: why "constraints". Seem more like "details"?

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.h (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:36: typedef
std::map<std::string, TabCaptureRequest> DeviceCaptureRequestMap;
This one can be private.

http://codereview.chromium.org/11038021/diff/36001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/public/extensions/tabCapture.html
(right):

http://codereview.chromium.org/11038021/diff/36001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/public/extensions/tabCapture.html:1:
{{+partials.standard_extensions_api api:apis.tab_capture}}
I don't think it makes sense to doc this feature yet. I'd propose 'nodoc', but I
don't think it works for IDL.

http://codereview.chromium.org/11038021/diff/36001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set.cc (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set.cc:50: "tabCapture",
I don't think we should do this. I think it should have to be listed.

http://codereview.chromium.org/11038021/diff/36001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/tab_capture_custom_bindings.js
(right):

http://codereview.chromium.org/11038021/diff/36001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/tab_capture_custom_bindings.js:27:
navigator.webkitGetUserMedia(options, successFunc, errorFunc);
Silly question: Does the implementation of webKitGetUserMedia() check
TabCaptureRegistry? Otherwise it seems like people could bypass the permission
checks.

http://codereview.chromium.org/11038021/diff/36001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/tab_capture/experimental/test.js
(right):

http://codereview.chromium.org/11038021/diff/36001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/tab_capture/experimental/test.js:5: var
tabCapture = chrome.tabCapture;
It would be good to have a test somewhere that you can't call
webkitGetUserMedia() without first going through chrome.tabCapture.

[Aaron Boodman, 2012-10-16 04:22:28]

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:16: class
TabCaptureAsyncApiFunction : public AsyncApiFunction {
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> On 2012/10/08 09:58:31, justinlin wrote:
> > On 2012/10/04 07:21:43, Aaron Boodman wrote:
> > > Is it possible to use IOThreadExtensionFunction instead? It is more
> efficient
> > as
> > > it avoid a round-trip from the UI thread. Also I'm not sure that
> > > AsyncApiFunction should exist, so I'd rather not add dependencies.
> > 
> > I can't seem to return a value when I use that interface. It crashes here:
> >
>
https://cs.corp.google.com/#chrome/src/chrome/browser/extensions/extension_fu...
> > 
> > AsyncApiFunction seems like what I want though.
> 
> ExtensionFunctionDispatcher is handling this function for some reason, but it
> can only handle UI thread functions. The IO thread functions are supposed to
be
> handled differently (see ExtensionFunctionDispatcher::DispatchOnIOThread).
> 
> I can't remember what causes one path or the other to be taken. You can take a
> look at some other classes that inherit IOThreadExtensionFunction do and see
if
> you can do that as well?

Note: This comment only applies if
http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
is wrong for some reason.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:85:
result->SetString(std::string("videoConstraints.mandatory.") +
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> Question: why "constraints". Seem more like "details"?

I take this back. 'constraints' is fine.

[justinlin, 2012-10-17 08:32:26]

Hi Aaron, thanks for the review! Addressed most of the suggested changes. Please
see the comment in tab_capture_custom_bindings.js, which I don't have a good
solution for. Also, added the command-line flag to protect this API and 1 change
in feature_switch{h,cc}. Please take another look. If you think this cl needs to
be broken up, let me know, thanks.

xians@: Please take a look at the media_stream_manager.cc changes, thanks.

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:16: class
TabCaptureAsyncApiFunction : public AsyncApiFunction {
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> On 2012/10/08 09:58:31, justinlin wrote:
> > On 2012/10/04 07:21:43, Aaron Boodman wrote:
> > > Is it possible to use IOThreadExtensionFunction instead? It is more
> efficient
> > as
> > > it avoid a round-trip from the UI thread. Also I'm not sure that
> > > AsyncApiFunction should exist, so I'd rather not add dependencies.
> > 
> > I can't seem to return a value when I use that interface. It crashes here:
> >
>
https://cs.corp.google.com/#chrome/src/chrome/browser/extensions/extension_fu...
> > 
> > AsyncApiFunction seems like what I want though.
> 
> ExtensionFunctionDispatcher is handling this function for some reason, but it
> can only handle UI thread functions. The IO thread functions are supposed to
be
> handled differently (see ExtensionFunctionDispatcher::DispatchOnIOThread).
> 
> I can't remember what causes one path or the other to be taken. You can take a
> look at some other classes that inherit IOThreadExtensionFunction do and see
if
> you can do that as well?

OK, so from what I can tell, this would make the function entirely on the IO
thread (is this correct?). I need to be on the IO thread to figure out the
process and routing id, but then the UI thread to add it to the registry. I
could use ExtensionUI/IOFunction and post a task, but I think it ends up being
the same as AsyncApiFunction. That class is also used for pretty fundamental
things like the socket/usb/etc api's. If those migrate, it should be easy to
change this class too. If you don't think so, I'll take a closer look.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:40: if
(!GetExtension()->HasAPIPermission(APIPermission::kTab)) {
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> Two things:
> 
> (1) The 'tabs' permission is not the right level of access. Tabs permission
just
> means that the extension can see the URLs and titles in your browsing history.
> This API also provides access to the content of those pages. So it should be
> gated by 'host permissions' instead:
> http://developer.chrome.com/extensions/manifest.html#permissions (the ones
with
> the URL patterns - we call these host permissions in the codebase).
> 
> (2) One interesting question is whether it would ever be useful for an
extension
> to be able to capture tab contents on some URLs but not others. The reason I
ask
> is that if that case is never interesting, then we can just make the
tabCapture
> permission 'imply' host permissions to all URLs. There are many other APIs we
do
> this with - for example the proxy API. You don't need to separately declare
the
> URLs you want access to in that case, we just assume it is all.
> 
> However, if it is the case that it would be useful to be able to capture on
some
> URLs but not others, then we could simply ask the developer to declare the
> appropriate host permissions along with 'tabCapture'.

Changed it back to kTabCapture permission as suggested in a different comment.

Interesting. Yes, I do think for this feature we should just imply "host
permissions" for all URLs by default. Actually, I'm not sure if it should even
require host permissions yet. The issue is that once the capture starts, the
user can navigate to any URL they want because the actual video capturing code
doesn't care about URL changes or checking permissions after it's started. I
think maybe it should just be a normal API permission. Also, this would
eventually just become a WebKit API (we are currently prototyping with
extensions), in which case we won't have the extension framework to check url
permissions.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:50: void
TabCaptureCaptureFunction::Work() {
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> Wait a second... Why are you using AsyncApiFunction in the first place?
There's
> nothing in ::Work() that needs to be on the IO thread!

Converting process and tab id to process and routing id needs to be done on the
IO thread: see ExtensionTabIdMap::GetProcessAndRoutingId or any method in that
class.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:69:
(TabCaptureRegistry::TabCaptureRequest) { GetExtension()->id(), tab_id,
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> Is the cast necessary? Seems like it wouldn't be.

Done.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:75: if
(params_->options->audio.get()) {
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> Nit: this brace is not necessary. It's optional though, if you prefer it.

Done.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:83:
base::DictionaryValue* result = new base::DictionaryValue();
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> JSON Schema Compiler generates structs you can use for the result too, instead
> of doing this manual JSON stuff.

These are sort of intermediate results that we are creating here, which the
tab_capture_custom_bindings.js will make use of. I don't think it makes sense to
put these structs in the IDL file, if that's what you meant.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:85:
result->SetString(std::string("videoConstraints.mandatory.") +
On 2012/10/16 04:22:28, Aaron Boodman wrote:
> On 2012/10/16 04:19:27, Aaron Boodman wrote:
> > Question: why "constraints". Seem more like "details"?
> 
> I take this back. 'constraints' is fine.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:85:
result->SetString(std::string("videoConstraints.mandatory.") +
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> Question: why "constraints". Seem more like "details"?

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.h (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:36: typedef
std::map<std::string, TabCaptureRequest> DeviceCaptureRequestMap;
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> This one can be private.

Done.

http://codereview.chromium.org/11038021/diff/36001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/public/extensions/tabCapture.html
(right):

http://codereview.chromium.org/11038021/diff/36001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/public/extensions/tabCapture.html:1:
{{+partials.standard_extensions_api api:apis.tab_capture}}
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> I don't think it makes sense to doc this feature yet. I'd propose 'nodoc', but
I
> don't think it works for IDL.

Done.

http://codereview.chromium.org/11038021/diff/36001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set.cc (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set.cc:50: "tabCapture",
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> I don't think we should do this. I think it should have to be listed.

Done.

http://codereview.chromium.org/11038021/diff/36001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/tab_capture_custom_bindings.js
(right):

http://codereview.chromium.org/11038021/diff/36001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/tab_capture_custom_bindings.js:27:
navigator.webkitGetUserMedia(options, successFunc, errorFunc);
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> Silly question: Does the implementation of webKitGetUserMedia() check
> TabCaptureRegistry? Otherwise it seems like people could bypass the permission
> checks.

I think you're right. Unfortunately, I've thought about it I don't see a clean
way to fix this yet. There isn't any easy way to access the registry or figure
out the script that called it after the webkit call. I've added the a
command-line flag to protect this a bit. Seeing as this would eventually be
callable from any web page, I'm not sure if it's worth trying to figure out a
hack for this. What do you think?

http://codereview.chromium.org/11038021/diff/36001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/tab_capture/experimental/test.js
(right):

http://codereview.chromium.org/11038021/diff/36001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/tab_capture/experimental/test.js:5: var
tabCapture = chrome.tabCapture;
On 2012/10/16 04:19:27, Aaron Boodman wrote:
> It would be good to have a test somewhere that you can't call
> webkitGetUserMedia() without first going through chrome.tabCapture.

See previous comment.

[Aaron Boodman, 2012-10-17 20:28:13]

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:50: void
TabCaptureCaptureFunction::Work() {
On 2012/10/17 08:32:26, justinlin wrote:
> On 2012/10/16 04:19:27, Aaron Boodman wrote:
> > Wait a second... Why are you using AsyncApiFunction in the first place?
> There's
> > nothing in ::Work() that needs to be on the IO thread!
> 
> Converting process and tab id to process and routing id needs to be done on
the
> IO thread: see ExtensionTabIdMap::GetProcessAndRoutingId or any method in that
> class.

You can get both the routing and process ID for a tab on the UI thread.

To get the routing ID:
UIThreadExtensionFunction::render_view_host()->GetRoutingID();

To get the process ID:
UIThreadExtensionFunction::render_view_host()->GetProcess()->GetID();

This is actually the typical way to work with these values in Chrome. The
ExtensionTabIdMap thing is a hack for certain functions that must do other work
on the IO thread.

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:83:
base::DictionaryValue* result = new base::DictionaryValue();
On 2012/10/17 08:32:26, justinlin wrote:
> On 2012/10/16 04:19:27, Aaron Boodman wrote:
> > JSON Schema Compiler generates structs you can use for the result too,
instead
> > of doing this manual JSON stuff.
> 
> These are sort of intermediate results that we are creating here, which the
> tab_capture_custom_bindings.js will make use of. I don't think it makes sense
to
> put these structs in the IDL file, if that's what you meant.

I see.

http://codereview.chromium.org/11038021/diff/36001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/tab_capture_custom_bindings.js
(right):

http://codereview.chromium.org/11038021/diff/36001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/tab_capture_custom_bindings.js:27:
navigator.webkitGetUserMedia(options, successFunc, errorFunc);
On 2012/10/17 08:32:26, justinlin wrote:
> On 2012/10/16 04:19:27, Aaron Boodman wrote:
> > Silly question: Does the implementation of webKitGetUserMedia() check
> > TabCaptureRegistry? Otherwise it seems like people could bypass the
permission
> > checks.
> 
> I think you're right. Unfortunately, I've thought about it I don't see a clean
> way to fix this yet. There isn't any easy way to access the registry or figure
> out the script that called it after the webkit call. I've added the a
> command-line flag to protect this a bit. Seeing as this would eventually be
> callable from any web page, I'm not sure if it's worth trying to figure out a
> hack for this. What do you think?

If this is going to ever be exposed without the flag to users, then you'll have
to figure this out. I'm not totally clear on what the long-term plan is: are you
planning to eventually not have an extension API at all? If so, why do we need
it now again?

Assuming that the extension API is going to users, I think we should just figure
out how this should work now, rather than having the weird half-step with the
flag.

I see that there is a content::MediaObserver interface that Chrome uses. Can
this (or something similar) be extended to be used for permission checking? The
Chrome implementation would get the extension for the calling context, then make
sure that it has the tabCapture permission.

http://codereview.chromium.org/11038021/diff/46002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:4751: Video capture of your tabs and browsing
activity
No separate warning for this permission. It will use the generic 'access to your
private data on all websites warning.'

http://codereview.chromium.org/11038021/diff/46002/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:16: 
extra line here

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/f...
File chrome/common/extensions/feature_switch.cc (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/f...
chrome/common/extensions/feature_switch.cc:136: if
(command_line_->HasSwitch(switch_name_))
Should not be necessary (GetLegacyEnableFlag() should be doing the same thing).

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:226:
IDS_EXTENSION_PROMPT_WARNING_TAB_CAPTURE,
This should be below with the "full url access permissions", and should follow
the same pattern (no specific warning, kFlagImpliesFullURLAccess).

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

lgtm on the media_stream_manager.cc

[justinlin, 2012-10-17 23:41:48]

Thanks again. Please take another look. Resolved the using AsyncApiFunction
issue (thanks), and the fix for getUserMedia possibly bypassing the extension is
in the works (need to fix a bug with it):
http://codereview.chromium.org/11198044/

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

http://codereview.chromium.org/11038021/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:50: void
TabCaptureCaptureFunction::Work() {
On 2012/10/17 20:28:14, Aaron Boodman wrote:
> On 2012/10/17 08:32:26, justinlin wrote:
> > On 2012/10/16 04:19:27, Aaron Boodman wrote:
> > > Wait a second... Why are you using AsyncApiFunction in the first place?
> > There's
> > > nothing in ::Work() that needs to be on the IO thread!
> > 
> > Converting process and tab id to process and routing id needs to be done on
> the
> > IO thread: see ExtensionTabIdMap::GetProcessAndRoutingId or any method in
that
> > class.
> 
> You can get both the routing and process ID for a tab on the UI thread.
> 
> To get the routing ID:
> UIThreadExtensionFunction::render_view_host()->GetRoutingID();
> 
> To get the process ID:
> UIThreadExtensionFunction::render_view_host()->GetProcess()->GetID();
> 
> This is actually the typical way to work with these values in Chrome. The
> ExtensionTabIdMap thing is a hack for certain functions that must do other
work
> on the IO thread.

Done. OK, sorry for the contention surrounding AsyncApiFunction. I don't quite
want the current routing/render_process_id, but I want the one of the target
tab. Figured out how to do it. Looks better not as a SyncExtensionFunction.

http://codereview.chromium.org/11038021/diff/36001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/tab_capture_custom_bindings.js
(right):

http://codereview.chromium.org/11038021/diff/36001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/tab_capture_custom_bindings.js:27:
navigator.webkitGetUserMedia(options, successFunc, errorFunc);
On 2012/10/17 20:28:14, Aaron Boodman wrote:
> On 2012/10/17 08:32:26, justinlin wrote:
> > On 2012/10/16 04:19:27, Aaron Boodman wrote:
> > > Silly question: Does the implementation of webKitGetUserMedia() check
> > > TabCaptureRegistry? Otherwise it seems like people could bypass the
> permission
> > > checks.
> > 
> > I think you're right. Unfortunately, I've thought about it I don't see a
clean
> > way to fix this yet. There isn't any easy way to access the registry or
figure
> > out the script that called it after the webkit call. I've added the a
> > command-line flag to protect this a bit. Seeing as this would eventually be
> > callable from any web page, I'm not sure if it's worth trying to figure out
a
> > hack for this. What do you think?
> 
> If this is going to ever be exposed without the flag to users, then you'll
have
> to figure this out. I'm not totally clear on what the long-term plan is: are
you
> planning to eventually not have an extension API at all? If so, why do we need
> it now again?

My PM (markdavidscott@) might have a better answer for this, but putting it out
as an experimental extension API first is a good way to prototype and get
feedback about the feature before trying to propose it as a stardard WebKit API.
We also have a product that may make use of this feature.

> 
> Assuming that the extension API is going to users, I think we should just
figure
> out how this should work now, rather than having the weird half-step with the
> flag.
> 
> I see that there is a content::MediaObserver interface that Chrome uses. Can
> this (or something similar) be extended to be used for permission checking?
The
> Chrome implementation would get the extension for the calling context, then
make
> sure that it has the tabCapture permission.

Yea, I was brainstorming something similar, but it doesn't quiet feel right to
create another interface just for a permission check. Also, that's an observer
class, so feeding back data would be weird. I think I have a solution for this
that I'd like to submit separately. It's media-only changes:
http://codereview.chromium.org/11198044/
Basically fake a WebRTC permission check, and catch it when it's in the browser
to check the registry and allow/deny the request. There's still a problem with
the patch (for some reason, I'm getting the incorrect WebContents delegate when
the permission check comes from a background page), but I'll sort it out Today.
Minor note: it's actually hard for a user to use this anyway because they need
to figure out routing and render_process id's, which I don't think you can do
easily without being in chrome.

http://codereview.chromium.org/11038021/diff/46002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:4751: Video capture of your tabs and browsing
activity
On 2012/10/17 20:28:14, Aaron Boodman wrote:
> No separate warning for this permission. It will use the generic 'access to
your
> private data on all websites warning.'

Done.

http://codereview.chromium.org/11038021/diff/46002/chrome/browser/extensions/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/browser/extensions/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:16: 
On 2012/10/17 20:28:14, Aaron Boodman wrote:
> extra line here

Done.

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/f...
File chrome/common/extensions/feature_switch.cc (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/f...
chrome/common/extensions/feature_switch.cc:136: if
(command_line_->HasSwitch(switch_name_))
On 2012/10/17 20:28:14, Aaron Boodman wrote:
> Should not be necessary (GetLegacyEnableFlag() should be doing the same
thing).

The problem is the key is switches::kEnableTabCapture == "enable-tab-capture".
So, the legacy checks will check "enable-enable-tab-capture", but not the actual
key.

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:226:
IDS_EXTENSION_PROMPT_WARNING_TAB_CAPTURE,
On 2012/10/17 20:28:14, Aaron Boodman wrote:
> This should be below with the "full url access permissions", and should follow
> the same pattern (no specific warning, kFlagImpliesFullURLAccess).

Done.

[justinlin, 2012-10-18 20:09:31]

Adding sky@ for chrome/*.gypi changes
and avi@ for content/ changes. Please take a look, thanks!

[sky, 2012-10-18 21:27:22]

http://codereview.chromium.org/11038021/diff/47033/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

http://codereview.chromium.org/11038021/diff/47033/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:196:
'browser/extensions/api/tab_capture/tab_capture_api.cc',
sort

[Aaron Boodman, 2012-10-18 22:51:35]

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/f...
File chrome/common/extensions/feature_switch.cc (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/f...
chrome/common/extensions/feature_switch.cc:136: if
(command_line_->HasSwitch(switch_name_))
On 2012/10/17 23:41:48, justinlin wrote:
> On 2012/10/17 20:28:14, Aaron Boodman wrote:
> > Should not be necessary (GetLegacyEnableFlag() should be doing the same
> thing).
> 
> The problem is the key is switches::kEnableTabCapture == "enable-tab-capture".
> So, the legacy checks will check "enable-enable-tab-capture", but not the
actual
> key.

So change kEnableTabCapture to "tab-capture" (probably makes sense to change
constant name too, to just kTabCapture).

http://codereview.chromium.org/11038021/diff/51003/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

http://codereview.chromium.org/11038021/diff/51003/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:282:
kFlagImpliesFullURLAccess | kFlagCannotBeOptional },
I think flag can be optional.

http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:249: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
I think this check should be done in RequestMediaAccessPermission() on the
Chrome side.

[justinlin, 2012-10-19 00:03:33]

Thanks

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/f...
File chrome/common/extensions/feature_switch.cc (right):

http://codereview.chromium.org/11038021/diff/46002/chrome/common/extensions/f...
chrome/common/extensions/feature_switch.cc:136: if
(command_line_->HasSwitch(switch_name_))
On 2012/10/18 22:51:35, Aaron Boodman wrote:
> On 2012/10/17 23:41:48, justinlin wrote:
> > On 2012/10/17 20:28:14, Aaron Boodman wrote:
> > > Should not be necessary (GetLegacyEnableFlag() should be doing the same
> > thing).
> > 
> > The problem is the key is switches::kEnableTabCapture ==
"enable-tab-capture".
> > So, the legacy checks will check "enable-enable-tab-capture", but not the
> actual
> > key.
> 
> So change kEnableTabCapture to "tab-capture" (probably makes sense to change
> constant name too, to just kTabCapture).

Done.

http://codereview.chromium.org/11038021/diff/47033/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

http://codereview.chromium.org/11038021/diff/47033/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:196:
'browser/extensions/api/tab_capture/tab_capture_api.cc',
On 2012/10/18 21:27:22, sky wrote:
> sort

Done.

http://codereview.chromium.org/11038021/diff/51003/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

http://codereview.chromium.org/11038021/diff/51003/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:282:
kFlagImpliesFullURLAccess | kFlagCannotBeOptional },
On 2012/10/18 22:51:35, Aaron Boodman wrote:
> I think flag can be optional.

Done.

http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:249: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/10/18 22:51:35, Aaron Boodman wrote:
> I think this check should be done in RequestMediaAccessPermission() on the
> Chrome side.

While I agree that might be nicer, I think it's better to keep this flag check
in content/ for now for safety. In case something changes in the way user
authorization works, we don't want to allow tab media streams without the flag.

[Aaron Boodman, 2012-10-19 00:37:20]

http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:249: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/10/19 00:03:33, justinlin wrote:
> On 2012/10/18 22:51:35, Aaron Boodman wrote:
> > I think this check should be done in RequestMediaAccessPermission() on the
> > Chrome side.
> 
> While I agree that might be nicer, I think it's better to keep this flag check
> in content/ for now for safety. In case something changes in the way user
> authorization works, we don't want to allow tab media streams without the
flag.

The way that FeatureSwitch works, you can do --tab-capture=1 or --tab-capture=0.
--tab-capture doesn't mean anything on its own. So this logic is broken.

I don't want you to duplicate part of the logic of FeatureSwitch here, and you
can't access it from content/. I think the risk is ok given the other safeguards
we have.

[justinlin, 2012-10-19 00:47:25]

http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:249: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/10/19 00:37:20, Aaron Boodman wrote:
> On 2012/10/19 00:03:33, justinlin wrote:
> > On 2012/10/18 22:51:35, Aaron Boodman wrote:
> > > I think this check should be done in RequestMediaAccessPermission() on the
> > > Chrome side.
> > 
> > While I agree that might be nicer, I think it's better to keep this flag
check
> > in content/ for now for safety. In case something changes in the way user
> > authorization works, we don't want to allow tab media streams without the
> flag.
> 
> The way that FeatureSwitch works, you can do --tab-capture=1 or
--tab-capture=0.
> --tab-capture doesn't mean anything on its own. So this logic is broken.
> 
> I don't want you to duplicate part of the logic of FeatureSwitch here, and you
> can't access it from content/. I think the risk is ok given the other
safeguards
> we have.

Sorry should have explained. The way I set it up is there's really 2 flags:
"enable-tab-capture" and "tab-capture". "enable-tab-capture" is purely a
content/ flag and this is the one that is setup in about_flags.cc. The
"tab-capture" flag one is the one that is added to FeatureSwitch, and I rely on
the IsEnabled() method doing the legacy mode check (i.e. it prepends "enable-"
and checks HasSwitch).

Maybe I should just remove the FeatureSwitch and not check that flag in the
extension API. Then, we only have this content flag which prevent all tab
capture streams.

Would that be better?

[justinlin, 2012-10-20 00:03:15]

Nevermind. Done moving the flag from content to chrome.
Also, disabling this feature for Windows for now. There's some issue with the
FakeVideoDevice, but that may be fixed when we land the actual
WebContentsVideoDevice.

On 2012/10/19 00:47:25, justinlin wrote:
>
http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
> content/browser/renderer_host/media/media_stream_manager.cc:249: if
> (!CommandLine::ForCurrentProcess()->HasSwitch(
> On 2012/10/19 00:37:20, Aaron Boodman wrote:
> > On 2012/10/19 00:03:33, justinlin wrote:
> > > On 2012/10/18 22:51:35, Aaron Boodman wrote:
> > > > I think this check should be done in RequestMediaAccessPermission() on
the
> > > > Chrome side.
> > > 
> > > While I agree that might be nicer, I think it's better to keep this flag
> check
> > > in content/ for now for safety. In case something changes in the way user
> > > authorization works, we don't want to allow tab media streams without the
> > flag.
> > 
> > The way that FeatureSwitch works, you can do --tab-capture=1 or
> --tab-capture=0.
> > --tab-capture doesn't mean anything on its own. So this logic is broken.
> > 
> > I don't want you to duplicate part of the logic of FeatureSwitch here, and
you
> > can't access it from content/. I think the risk is ok given the other
> safeguards
> > we have.
> 
> Sorry should have explained. The way I set it up is there's really 2 flags:
> "enable-tab-capture" and "tab-capture". "enable-tab-capture" is purely a
> content/ flag and this is the one that is setup in about_flags.cc. The
> "tab-capture" flag one is the one that is added to FeatureSwitch, and I rely
on
> the IsEnabled() method doing the legacy mode check (i.e. it prepends "enable-"
> and checks HasSwitch).
> 
> Maybe I should just remove the FeatureSwitch and not check that flag in the
> extension API. Then, we only have this content flag which prevent all tab
> capture streams.
> 
> Would that be better?

[justinlin, 2012-10-20 00:03:34]

aa: Please take another look, thanks!

On 2012/10/20 00:03:15, justinlin wrote:
> Nevermind. Done moving the flag from content to chrome.
> Also, disabling this feature for Windows for now. There's some issue with the
> FakeVideoDevice, but that may be fixed when we land the actual
> WebContentsVideoDevice.
> 
> On 2012/10/19 00:47:25, justinlin wrote:
> >
>
http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
> > File content/browser/renderer_host/media/media_stream_manager.cc (right):
> > 
> >
>
http://codereview.chromium.org/11038021/diff/51003/content/browser/renderer_h...
> > content/browser/renderer_host/media/media_stream_manager.cc:249: if
> > (!CommandLine::ForCurrentProcess()->HasSwitch(
> > On 2012/10/19 00:37:20, Aaron Boodman wrote:
> > > On 2012/10/19 00:03:33, justinlin wrote:
> > > > On 2012/10/18 22:51:35, Aaron Boodman wrote:
> > > > > I think this check should be done in RequestMediaAccessPermission() on
> the
> > > > > Chrome side.
> > > > 
> > > > While I agree that might be nicer, I think it's better to keep this flag
> > check
> > > > in content/ for now for safety. In case something changes in the way
user
> > > > authorization works, we don't want to allow tab media streams without
the
> > > flag.
> > > 
> > > The way that FeatureSwitch works, you can do --tab-capture=1 or
> > --tab-capture=0.
> > > --tab-capture doesn't mean anything on its own. So this logic is broken.
> > > 
> > > I don't want you to duplicate part of the logic of FeatureSwitch here, and
> you
> > > can't access it from content/. I think the risk is ok given the other
> > safeguards
> > > we have.
> > 
> > Sorry should have explained. The way I set it up is there's really 2 flags:
> > "enable-tab-capture" and "tab-capture". "enable-tab-capture" is purely a
> > content/ flag and this is the one that is setup in about_flags.cc. The
> > "tab-capture" flag one is the one that is added to FeatureSwitch, and I rely
> on
> > the IsEnabled() method doing the legacy mode check (i.e. it prepends
"enable-"
> > and checks HasSwitch).
> > 
> > Maybe I should just remove the FeatureSwitch and not check that flag in the
> > extension API. Then, we only have this content flag which prevent all tab
> > capture streams.
> > 
> > Would that be better?

[Aaron Boodman, 2012-10-24 01:12:22]

Because of the rebase it's hard to tell what changed, but I don't see content/
in the file list anymore, so LGTM!

[commit-bot: I haz the power, 2012-10-24 19:49:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/11038021/78002

[commit-bot: I haz the power, 2012-10-24 21:10:52]

Change committed as 163902