New tabCapture.captureOffscreenTab API, initially for Presentation API 1UA mode

New tabCapture.captureOffscreenTab API, initially for Presentation API 1UA mode

This is the second in a 2-part change to support the one user agent
(1UA) mode for the new W3C Presentation API.  This part adds a new
chrome.tabCapture.captureOffscreenTab API that spawns off-screen tabs
and captures their content into MediaStreams for transport to a remote
display.

This change includes some refactoring of code common to this new API and
the existing tabCapture.capture API.  In addition, the concept of
"anonymous" tab capture requests has been added to TabCaptureRegistry,
to explicitly make the off-screen tabs invisible to other extensions APIs
(this is by design).

At this time, the new API is only available on the Canary and Dev
channels, and to certain whitelisted extensions.

BUG=490890, 537732
Committed: https://crrev.com/b7e17a44a8bc21255a2f4993a549bccf8cca93d4
Cr-Commit-Position: refs/heads/master@{#352144}

[miu, 2015-06-26 20:11:30]

miu@chromium.org changed reviewers:
+ kmarshall@chromium.org

[miu, 2015-06-26 20:12:24]

Kevin, PTAL.

[Kevin M, 2015-06-29 22:17:02]

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:88: if
(options->audio.get() && *options->audio.get()) {
.get() isn't needed here or elsewhere in the function.

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:134: gfx::Size
DeterminePresentationSize(const TabCapture::CaptureOptions& options) {
Could this be unit tested to ensure that all the corner cases are correct? There
are a lot of conditionals here.

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:47: bool RunSync()
final;
// ExtensionFunction

https://codereview.chromium.org/1221483002/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right):

https://codereview.chromium.org/1221483002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/tab_capture/api_tests.js:138:
chrome.test.assertEq(captured_tabs_without_pres.length,
Consider comparing the stringified arrays instead, or at least factor the array
comparison logic into a function to share with
offscreenTabsDoNotShowUpInTabsQuery.

[miu, 2015-06-30 04:09:59]

PTAL.  Addressed comments:

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:88: if
(options->audio.get() && *options->audio.get()) {
On 2015/06/29 22:17:02, Kevin M wrote:
> .get() isn't needed here or elsewhere in the function.

Done.  In some places, it was still required.

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:134: gfx::Size
DeterminePresentationSize(const TabCapture::CaptureOptions& options) {
On 2015/06/29 22:17:02, Kevin M wrote:
> Could this be unit tested to ensure that all the corner cases are correct?
There
> are a lot of conditionals here.

Done.  Made this function a static class member to externalize it.

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.h (right):

https://codereview.chromium.org/1221483002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/tab_capture_api.h:47: bool RunSync()
final;
On 2015/06/29 22:17:02, Kevin M wrote:
> // ExtensionFunction

Done.

https://codereview.chromium.org/1221483002/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right):

https://codereview.chromium.org/1221483002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/tab_capture/api_tests.js:138:
chrome.test.assertEq(captured_tabs_without_pres.length,
On 2015/06/29 22:17:02, Kevin M wrote:
> Consider comparing the stringified arrays instead, or at least factor the
array
> comparison logic into a function to share with
> offscreenTabsDoNotShowUpInTabsQuery.

Done.

[Kevin M, 2015-06-30 16:58:25]

lgtm

[miu, 2015-06-30 20:01:34]

miu@chromium.org changed reviewers:
+ jyasskin@chromium.org, mpearson@chromium.org

[miu, 2015-06-30 20:01:35]

jyasskin: OWNERS approval for chrome/common/extensions/api/tab_capture.idl

mpearson: OWNERS approval for *histogram* (last 2 files)

[miu, 2015-06-30 20:02:47]

miu@chromium.org changed reviewers:
+ isherman@chromium.org
- mpearson@chromium.org

[miu, 2015-06-30 20:02:47]

isherman (instead of mpearson): OWNERS approval for *histogram* (last 2 files)

[Mark P, 2015-06-30 23:00:19]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-06-30 23:00:19]

histograms.xml lgtm

[miu, 2015-06-30 23:10:19]

miu@chromium.org changed reviewers:
- isherman@chromium.org

[miu, 2015-06-30 23:10:19]

(Removing isherman@ since mpearson@ approved.)

[Jeffrey Yasskin, 2015-07-06 20:19:28]

As a new function in a Chrome Apps/Extensions API, this needs to go through API
review. Can you follow the instructions at
http://www.chromium.org/developers/design-documents/extensions/proposed-chang...

[miu, 2015-09-25 00:10:11]

I've revived this, aiming to land in time for M47 branch.  I am switching the
set of reviewers from kmarshall+jyasskin to imcheng+kalman due to availability
and familiarity with the current API proposal.

[miu, 2015-09-25 00:11:07]

miu@chromium.org changed reviewers:
+ imcheng@chromium.org, kalman@chromium.org
- jyasskin@chromium.org, kmarshall@chromium.org

[miu, 2015-09-25 00:14:43]

imcheng: PTAL.

kalman: OWNERS review, please.

This change adds the API publicly, but only whitelisted extensions are able to
use it for now.  The plan is to finish our on-going group discussions and
iterate the code some more before opening this up for general dev-channel (and,
later, stable-channel) use.

[not at google - send to devlin, 2015-09-25 16:56:03]

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
Where does the id come from?

[miu, 2015-09-25 17:35:32]

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/25 16:56:03, kalman wrote:
> Where does the id come from?

This is explained a few sentences later in this paragraph:  ;-)

"|id| can be any client-provided string.  It is used only to disambiguate
off-screen tabs with the same |startUrl|."

Or, am I misunderstanding your question?

[not at google - send to devlin, 2015-09-25 17:42:05]

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/25 17:35:32, miu wrote:
> On 2015/09/25 16:56:03, kalman wrote:
> > Where does the id come from?
> 
> This is explained a few sentences later in this paragraph:  ;-)
> 
> "|id| can be any client-provided string.  It is used only to disambiguate
> off-screen tabs with the same |startUrl|."
> 
> Or, am I misunderstanding your question?

Yeah I didn't read on, but my follow-up was, it doesn't appear to be used
anywhere.

[miu, 2015-09-25 18:47:54]

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/25 17:42:04, kalman wrote:
> Yeah I didn't read on, but my follow-up was, it doesn't appear to be used
> anywhere.

See line 268 in tab_capture_api.cc.  It is passed to the
OffscreenPresentationsOwner to search for (or create) an off-screen tab.  You
can think of startUrl+id as the "key" for finding an off-screen tab.

(Side note: I put in a TODO comment to rename the OffscreenPresentation* classes
to OffscreenTab*)

[not at google - send to devlin, 2015-09-25 19:13:25]

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/25 18:47:54, miu wrote:
> On 2015/09/25 17:42:04, kalman wrote:
> > Yeah I didn't read on, but my follow-up was, it doesn't appear to be used
> > anywhere.
> 
> See line 268 in tab_capture_api.cc.  It is passed to the
> OffscreenPresentationsOwner to search for (or create) an off-screen tab.  You
> can think of startUrl+id as the "key" for finding an off-screen tab.
> 
> (Side note: I put in a TODO comment to rename the OffscreenPresentation*
classes
> to OffscreenTab*)

ok, 3rd question: can the API generate one itself? why leave it to the
extension?

[miu, 2015-09-25 21:59:18]

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/25 19:13:24, kalman wrote:
> ok, 3rd question: can the API generate one itself? why leave it to the
> extension?

As I was answering your question, I'm now thinking we should get rid of the |id|
field.  If we change this API to always create a new off-screen tab (i.e., not
attempt to find an existing one), then there's no need for it.  WDYT?

Otherwise, to answer your question...

I don't think generating these IDs should be the responsibility of this API.  It
does nothing with this |id| field except use it to disambiguate between two
off-screen tabs with the same |startUrl|.  Meaning, if the extension creating
off-screen tabs would never create two tabs with the same |startUrl|, it could
just provide an empty string for the |id| field.

The reason we need this field is that, in the Media Router extension use case,
it can create multiple off-screen tabs with the same |startUrl|, but for
different presentation sessions.  In this case, the presentation session ID
would be used to disambiguate.

[not at google - send to devlin, 2015-09-25 22:29:55]

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/25 21:59:17, miu wrote:
> On 2015/09/25 19:13:24, kalman wrote:
> > ok, 3rd question: can the API generate one itself? why leave it to the
> > extension?
> 
> As I was answering your question, I'm now thinking we should get rid of the
|id|
> field.  If we change this API to always create a new off-screen tab (i.e., not
> attempt to find an existing one), then there's no need for it.  WDYT?
> 
> Otherwise, to answer your question...
> 
> I don't think generating these IDs should be the responsibility of this API. 
It
> does nothing with this |id| field except use it to disambiguate between two
> off-screen tabs with the same |startUrl|.  Meaning, if the extension creating
> off-screen tabs would never create two tabs with the same |startUrl|, it could
> just provide an empty string for the |id| field.
> 
> The reason we need this field is that, in the Media Router extension use case,
> it can create multiple off-screen tabs with the same |startUrl|, but for
> different presentation sessions.  In this case, the presentation session ID
> would be used to disambiguate.

Yes I'd like to avoid having an ID here. If you want to have reusable tabs then
I'd have this API return a tab ID on creation and make it part of the capture
options if an extension wants to start capturing old tabs. Basically the same as
the foreground tab API.

However in the case of these offscreen tabs, they're closed when the reference
to the stream has been dropped, right? Insofar as I presume once there is no
reference to a stream, it gets closed?

In which case I don't see being able to fetch an existing tab really makes much
sense?

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:88: // provided its own dedicated,
non-shared off-the-record profile.  Also, it
Can we say "sandboxed" instead of "off-the-record"? is that correct? I think
that "off-the-record" may be confused with it runs in an existing off-the-record
profile, or possible the offscreen tabs share a profile.

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:93: // Note: This is a new API,
currently only available via whitelisting, and
whitelisting? This is dev channel now right? The documentation server should
know how to generate that information already.

[miu, 2015-09-26 20:40:14]

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/25 22:29:55, kalman wrote:
> Yes I'd like to avoid having an ID here. If you want to have reusable tabs
then

Done.

However, I forgot to mention there is a second reason we needed the ID field. 
To support the W3C Presentation API use case (via the internal Media Router
extension), we need a way to "register" the off-screen tab's WebContents as a
participant in the presentation session.  After considering several design
alternatives, I decided the cleanest solution is to add this back as an
undocumented |presentationId| field to the CaptureOptions.

Why we need this:
https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/extensio...
 (line 231)

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:88: // provided its own dedicated,
non-shared off-the-record profile.  Also, it
On 2015/09/25 22:29:55, kalman wrote:
> Can we say "sandboxed" instead of "off-the-record"? is that correct? I think
> that "off-the-record" may be confused with it runs in an existing
off-the-record
> profile, or possible the offscreen tabs share a profile.

Done.

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:93: // Note: This is a new API,
currently only available via whitelisting, and
On 2015/09/25 22:29:55, kalman wrote:
> whitelisting? This is dev channel now right? The documentation server should
> know how to generate that information already.

I wasn't sure we were ready for dev-channel yet, given the security discussion
that is still going on about use cases where the MediaStream can go to a
destination not directly under the user's control.  I had been planning on
making another CL to turn that on later, once we made more progress.

So, I went ahead and made this change.  However, if you feel it's not yet safe
for dev-channel, I can revert.

[miu, 2015-09-28 22:38:09]

miu@chromium.org changed reviewers:
+ jyasskin@chromium.org

[miu, 2015-09-28 22:38:09]

jyasskin: Can you take over the OWNER part of this review (kalman@ is away).  We
were hoping to land this in time for M47 branch.

Background: Have API approval for dev-channel.  However, I had a concern around
an open security issue, and wasn't sure this API should be enabled for
dev-channel yet.  So, for now, this API would only be available to whitelisted
extensions (for the Chrome Media Router).  When kalman@ is back, we can iterate
with follow-up CLs and proceed the with dev- and stable-channel launches.

[imcheng, 2015-09-29 01:28:56]

lgtm

https://codereview.chromium.org/1221483002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/1221483002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:176: return;
Did you mean continue; here?

https://codereview.chromium.org/1221483002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/tab_capture/max_offscreen_tabs.js
(right):

https://codereview.chromium.org/1221483002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/tab_capture/max_offscreen_tabs.js:26: if
(!stream2) return;
You'd have to stop stream1 before returning, right?

Also, I wonder if there's a cleaner way to write this test. It will be hard to
modify if we changed the limit. Maybe keep a counter in the outer scope, and
have the callback passed into captureOffscreenTab increment the counter, check
the current counter vs. result, increment counter and call captureOffscreenTab
again (passing itself as the callback again)?

[not at google - send to devlin, 2015-09-30 01:32:10]

kalman@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- jyasskin@chromium.org

[not at google - send to devlin, 2015-09-30 01:32:11]

-jyasskin
+devlin (who is taking over my reviews)

though I will try to finish this myself.

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/26 20:40:14, miu wrote:
> On 2015/09/25 22:29:55, kalman wrote:
> > Yes I'd like to avoid having an ID here. If you want to have reusable tabs
> then
> 
> Done.
> 
> However, I forgot to mention there is a second reason we needed the ID field. 
> To support the W3C Presentation API use case (via the internal Media Router
> extension), we need a way to "register" the off-screen tab's WebContents as a
> participant in the presentation session.  After considering several design
> alternatives, I decided the cleanest solution is to add this back as an
> undocumented |presentationId| field to the CaptureOptions.
> 
> Why we need this:
>
https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/extensio...
>  (line 231)

I'll reply there.

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:93: // Note: This is a new API,
currently only available via whitelisting, and
On 2015/09/26 20:40:14, miu wrote:
> On 2015/09/25 22:29:55, kalman wrote:
> > whitelisting? This is dev channel now right? The documentation server should
> > know how to generate that information already.
> 
> I wasn't sure we were ready for dev-channel yet, given the security discussion
> that is still going on about use cases where the MediaStream can go to a
> destination not directly under the user's control.  I had been planning on
> making another CL to turn that on later, once we made more progress.
> 
> So, I went ahead and made this change.  However, if you feel it's not yet safe
> for dev-channel, I can revert.

Makes sense. Just double checking.

https://codereview.chromium.org/1221483002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:239: const bool
is_whitelisted_extension =
A better way to whitelist is by adding a rule for tabCapture.captureOffscreenTab
to _api_features.json.

[miu, 2015-09-30 19:46:39]

kalman: Addressed your comments, PTAL.

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/tab_capture.idl (right):

https://codereview.chromium.org/1221483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/tab_capture.idl:71: // |startUrl| and |id|.  When
created, the off-screen tab is automatically
On 2015/09/30 01:32:11, kalman is away wrote:
> I'll reply there.

Didn't see a comment there.  Does that mean this is okay?

https://codereview.chromium.org/1221483002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:239: const bool
is_whitelisted_extension =
On 2015/09/30 01:32:11, kalman is away wrote:
> A better way to whitelist is by adding a rule for
tabCapture.captureOffscreenTab
> to _api_features.json.

Added a TODO for this.  The reason is that there is outside code (e.g., a CrOS
system tray UI feature) that references the extension ID arrays defined in this
file.  In other words, switching to using _api_features.json is a bit more work
than I'd feel comfortable doing in this CL.

https://codereview.chromium.org/1221483002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/1221483002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:176: return;
On 2015/09/29 01:28:55, imcheng1 wrote:
> Did you mean continue; here? 

Good catch.  Done.

https://codereview.chromium.org/1221483002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/tab_capture/max_offscreen_tabs.js
(right):

https://codereview.chromium.org/1221483002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/tab_capture/max_offscreen_tabs.js:26: if
(!stream2) return;
On 2015/09/29 01:28:56, imcheng1 wrote:
> You'd have to stop stream1 before returning, right?

Not really, but that *would* be cleaner.

I rewrote this test to be more iterative, and gracefully tear-down on failure. 
LGTY?

[not at google - send to devlin, 2015-10-01 00:09:14]

https://chromiumcodereview.appspot.com/1221483002/diff/100001/chrome/browser/...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://chromiumcodereview.appspot.com/1221483002/diff/100001/chrome/browser/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:241: //
http://crbug.com/537732
I don't understand your comment. This is new code? Why does it require changing
some other code?

https://chromiumcodereview.appspot.com/1221483002/diff/100001/chrome/browser/...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:276:
*params->options.presentation_id : std::string(),
Huh, guess I didn't save the comment.

It was:
- What does it mean if this isn't specified?
- Either way, rather than allowing the developer to specify a value, can you
generate one as part of the API?

[not at google - send to devlin, 2015-10-01 00:11:26]

On 2015/10/01 00:09:14, kalman is away wrote:
>
https://chromiumcodereview.appspot.com/1221483002/diff/100001/chrome/browser/...
> File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1221483002/diff/100001/chrome/browser/...
> chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:241: //
> http://crbug.com/537732
> I don't understand your comment. This is new code? Why does it require
changing
> some other code?
> 
>
https://chromiumcodereview.appspot.com/1221483002/diff/100001/chrome/browser/...
> chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:276:
> *params->options.presentation_id : std::string(),
> Huh, guess I didn't save the comment.

I made the comment on https://chromiumcodereview.appspot.com/1314413005/
actually, must have had it open on a different screen. I'm confused how these
CLs relate to each other.

[miu, 2015-10-01 22:34:46]

kalman: Responded to your comments, and Patch Set 7 is an alternative way to
register off-screen tabs as presentations.  Explained below:

https://codereview.chromium.org/1221483002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:241: //
http://crbug.com/537732
On 2015/10/01 00:09:14, kalman is away wrote:
> I don't understand your comment. This is new code? Why does it require
changing
> some other code?

The whitelist arrays themselves (kChromecastExtensionsIds and
kMediaRouterExtensionIds) are NOT new code.  They are currently referenced by
code outside this module.  Basically, to switch over to using
_api_features.json, I'd need to change a bunch of files that are completely
unrelated to adding an offscreen tab API.  So, I'm putting it down as a TODO for
a separate follow-up change.

https://codereview.chromium.org/1221483002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:276:
*params->options.presentation_id : std::string(),
On 2015/10/01 00:09:14, kalman is away wrote:
> Huh, guess I didn't save the comment.
> 
> It was:
> - What does it mean if this isn't specified?

Two ways to answer this:

1. It means the off-screen tab is not being used for the W3C Presentation API
use case (discussed in the API proposal's "Use cases" section:
https://docs.google.com/document/d/1oSsCaWRjkDBVYfLN_wQVghPlPwgIsgzp8jJZdxAwcLc).

2. If a non-empty presentationId string is given, the off-screen tab will
register itself with a "manager" in the Media Router framework that "connects"
the off-screen tab as a receiver in a presentation session.

> - Either way, rather than allowing the developer to specify a value, can you
> generate one as part of the API?

Yes.  As an experiment, I've done that and uploaded it as Patch Set 7.  I use
the custom bindings to add an 'offscreenTabId' property to the MediaStream
object returned by captureOffscreenTab() call.  Then, I added a new
tabCapturePrivate.register() API to allow the whitelisted Chrome Media Router
extension to register the off-screen tab as a presentation page.  In short, the
addition to api_tests.js is a simple usage example.

Which do you prefer: Patch Set 6 or 7?

[not at google - send to devlin, 2015-10-02 01:17:26]

Ok... got it. So it's important to be able to reconnect back to an existing
offscreen tab?

I didn't realise that, I was expecting extensions to just be kept alive while a
tab was being streamed, and therefore they can hold onto the media streams as JS
objects. No ID is necessary then.

But, that has problems of its own, like how to detect that an extension is no
longer streaming, and avoiding memory leaks.

I'll let you figure those out later.

For now I agree the simpler approach is to have these things take IDs. If it's
useful for some use case, and there aren't security/privacy issues (which there
aren't here) then might as well make it public. However, I could imagine this
changing to the never-shut-down-event-page model. Requiring extensions to
generate unique IDs themselves, or be given them and need to pass them back into
the API, can be awkward.

I think this more or less lgtm (a variation on the previous patch set, sorry)
but please ping back if there is an issue with the features stuff I've been
insisting on.

https://codereview.chromium.org/1221483002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:241: //
http://crbug.com/537732
On 2015/10/01 22:34:46, miu wrote:
> On 2015/10/01 00:09:14, kalman is away wrote:
> > I don't understand your comment. This is new code? Why does it require
> changing
> > some other code?
> 
> The whitelist arrays themselves (kChromecastExtensionsIds and
> kMediaRouterExtensionIds) are NOT new code.  They are currently referenced by
> code outside this module.  Basically, to switch over to using
> _api_features.json, I'd need to change a bunch of files that are completely
> unrelated to adding an offscreen tab API.  So, I'm putting it down as a TODO
for
> a separate follow-up change.

I mean to just duplicate the IDs in the feature file. Those IDs are already
duplicated in a few places in the codebase, including the feature file[1]. You
don't need to fix all of the duplication right now but it would be a simpler,
and idiomatic, change to put this all in the feature files.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc
(right):

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:175: while
(true) {
Curious: what is the advantage in generating a random ID vs just using something
like base::StaticAtomicSequenceNumber (increments)? It would be much simpler. Is
it important that these aren't guessable or something?

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:356: namespace
TabCapturePrivate = extensions::api::tab_capture_private;
already in extensions namespace, no extensions::

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:377:
content::WebContents* const extension_web_contents = GetSenderWebContents();
This API can only be called from extension pages, so there should be a web
contents. Consider making this just
EXTENSION_FUNCTION_VALIDATE(GetSenderWebContents());

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:393: LOG(ERROR) <<
"NOT IMPLEMENTED: Register with PresentationRouter, id="
NOT_IMPLEMENTED()?

[miu, 2015-10-02 18:58:21]

Thanks for the review!  :)

Responses to last round:

https://codereview.chromium.org/1221483002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:241: //
http://crbug.com/537732
On 2015/10/02 01:17:25, kalman is away wrote:
> On 2015/10/01 22:34:46, miu wrote:
> > The whitelist arrays themselves (kChromecastExtensionsIds and
> 
> I mean to just duplicate the IDs in the feature file. Those IDs are already
> duplicated in a few places in the codebase, including the feature file[1]. 

Oh!  I didn't realize they had been duplicated there.  ;-)

> don't need to fix all of the duplication right now but it would be a simpler,
> and idiomatic, change to put this all in the feature files.

Agree.  I tried to do this, but there are a few weird blockers that make this
rather complex to do right now:

1. The existing functions in the tabCapture namespace (capture() and
getCapturedTabs()) are available on stable channel, while this new function is
not.  So, there's some partitioning I'd have to do, and this risks breaking the
existing stable public functions.

2. The existing tabCapture.capture() function seems to have this weird
combination of permissions where all public uses of the API can only capture the
currently-active tab, but whitelisted extensions can capture any tab.  I'm not
sure if this functionality is being used, but again, I don't want to risk
breaking anything until I can take the time to research and understand it all.

So, I left things alone here for now.  I'll get right on it with an immediate
follow-up CL next week.

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc
(right):

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:175: while
(true) {
On 2015/10/02 01:17:26, kalman is away wrote:
> Curious: what is the advantage in generating a random ID vs just using
something
> like base::StaticAtomicSequenceNumber (increments)? It would be much simpler.
Is
> it important that these aren't guessable or something?

No longer applicable.

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:356: namespace
TabCapturePrivate = extensions::api::tab_capture_private;
On 2015/10/02 01:17:26, kalman is away wrote:
> already in extensions namespace, no extensions::

N/A, since reverting mostly back to PS6.

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:377:
content::WebContents* const extension_web_contents = GetSenderWebContents();
On 2015/10/02 01:17:26, kalman is away wrote:
> This API can only be called from extension pages, so there should be a web
> contents. Consider making this just
> EXTENSION_FUNCTION_VALIDATE(GetSenderWebContents());

Done in TabCaptureCaptureOffscreenTabFunction::RunSync(), since that's the only
place I did this in PS6.

https://codereview.chromium.org/1221483002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:393: LOG(ERROR) <<
"NOT IMPLEMENTED: Register with PresentationRouter, id="
On 2015/10/02 01:17:26, kalman is away wrote:
> NOT_IMPLEMENTED()?

Done.  (I had it as LOG(ERROR) to confirm the plumbing of the string ID field
while I was testing on my desktop.)

[miu, 2015-10-02 18:59:44]

The CQ bit was checked by miu@chromium.org

[miu, 2015-10-02 18:59:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, kmarshall@chromium.org, imcheng@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1221483002/#ps140001
(title: "Mostly revert to Patch Set 6, plus minor tweaks.  [and REBASE]")

[commit-bot: I haz the power, 2015-10-02 19:01:37]

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

[commit-bot: I haz the power, 2015-10-02 21:52:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-02 21:53:31]

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