Create CanvasCaptureMediaStreamTrack in a regular MediaStream

Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call.

BUG=524218
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0] https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t78/edit#heading=h.gee451fatrq7
TBR=esprehn@chromium.org on third_party/WebKit/public/platform/Platform.h

Committed: https://crrev.com/9bdf6b77b4c685aeb895e7b7a0ea1ae400e61bb3
Cr-Commit-Position: refs/heads/master@{#364602}

[emircan, 2015-12-08 00:25:36]

Description was changed from

==========
BUG=
==========

to

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call. 

BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

[emircan, 2015-12-08 00:25:45]

Patchset #1 (id:1) has been deleted

[emircan, 2015-12-08 00:29:08]

emircan@chromium.org changed reviewers:
+ hta@chromium.org, perkj@chromium.org

[emircan, 2015-12-08 00:29:11]

I applied the spec change that we discussed offline. PTAL.

[emircan, 2015-12-08 00:33:44]

Patchset #1 (id:20001) has been deleted

[emircan, 2015-12-08 03:45:41]

Patchset #2 (id:60001) has been deleted

[perkj_chrome, 2015-12-08 09:05:06]

I mostly looked at the content/renderer changes again. 
Will there be a CanvasCaptureMediaStreamTrack in JS according to the spec?

https://codereview.chromium.org/1505013002/diff/80001/content/public/renderer...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/1505013002/diff/80001/content/public/renderer...
content/public/renderer/media_stream_api.cc:133: bool
AddVideoCapturerSourceToVideoTrack(
This feel like a special case where we create a source after the track. So I
would just put this in CanvasCaptureHandler.

https://codereview.chromium.org/1505013002/diff/80001/content/public/renderer...
content/public/renderer/media_stream_api.cc:139: blink::WebMediaStreamSource
webkit_source;
please don't call anything webkit. Maybe just web_source.

[emircan, 2015-12-08 18:09:32]

Description was changed from

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call. 

BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

to

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call. 

BUG=524218
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

[emircan, 2015-12-08 19:28:01]

https://codereview.chromium.org/1505013002/diff/80001/content/public/renderer...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/1505013002/diff/80001/content/public/renderer...
content/public/renderer/media_stream_api.cc:133: bool
AddVideoCapturerSourceToVideoTrack(
On 2015/12/08 09:05:06, perkj wrote:
> This feel like a special case where we create a source after the track. So I
> would just put this in CanvasCaptureHandler.

Done.

https://codereview.chromium.org/1505013002/diff/80001/content/public/renderer...
content/public/renderer/media_stream_api.cc:139: blink::WebMediaStreamSource
webkit_source;
On 2015/12/08 09:05:06, perkj wrote:
> please don't call anything webkit. Maybe just web_source.

Done. I was following the naming used in Add*TrackToMediaStream() funcs.

[emircan, 2015-12-08 21:49:21]

Patchset #3 (id:100001) has been deleted

[hta - Chromium, 2015-12-09 07:54:24]

lgtm

Worry:

Where are the layout tests that call the newly created interfaces? I would
expect at least one that verifies that the canvas returned in the canvas
attribute is the same as the canvas from which the track was created, and one
that verifies that requestFrame() does the right thing for notImplemented
(throws exception?)

https://codereview.chromium.org/1505013002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.idl
(right):

https://codereview.chromium.org/1505013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.idl:6:
// https://www.w3.org/TR/mediacapture-fromelement/#the-canvascapturemediastream
The change landed today in http://w3c.github.io/mediacapture-fromelement/

[emircan, 2015-12-10 00:59:26]

Thanks. I have another CL below that is adding requestFrame() behavior and
testing with a layout test. I split up the CLs to have easier and faster
reviews.
https://codereview.chromium.org/1505753005/

https://codereview.chromium.org/1505013002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.idl
(right):

https://codereview.chromium.org/1505013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.idl:6:
// https://www.w3.org/TR/mediacapture-fromelement/#the-canvascapturemediastream
On 2015/12/09 07:54:24, hta - Chromium wrote:
> The change landed today in http://w3c.github.io/mediacapture-fromelement/

Thanks, removing the comment.

[emircan, 2015-12-10 01:05:44]

emircan@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org, sievers@chromium.org

[emircan, 2015-12-10 01:05:44]

sievers@chromium.org: Please OWNERS review changes in
content/renderer/renderer_blink_platform_impl.*

haraken@chromium.org: Please OWNERS review changes in
third_party/WebKit/Source/modules/mediastream/*

[haraken, 2015-12-10 01:25:01]

modules/ LGTM

https://codereview.chromium.org/1505013002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp
(right):

https://codereview.chromium.org/1505013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp:13:


Unnecessary empty line.

https://codereview.chromium.org/1505013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp:42:
suspendIfNeeded();

Oh, this was missing before this CL. Nice catch.

[no sievers, 2015-12-10 01:26:10]

On 2015/12/10 01:05:44, emircan wrote:
> mailto:sievers@chromium.org: Please OWNERS review changes in
> content/renderer/renderer_blink_platform_impl.*
> 


lgtm

[perkj_chrome, 2015-12-10 15:48:57]

lgtm with nits

Can you add a layout test that clone a CanvasCaptureMediaStreamTrack ? 
Also, what happens if you clone CanvasCaptureMediaStreamTrack and call stop on
the original CanvasCaptureMediaStreamTrack? 
I think the cloned track should continue to work.

https://codereview.chromium.org/1505013002/diff/140001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1505013002/diff/140001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:28: // is owned by
CanvasCaptureMediaStreamTrack.
owned by a Can

https://codereview.chromium.org/1505013002/diff/140001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:31: // used to send back frames
on IO thread.
s on the IO thread

https://codereview.chromium.org/1505013002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/1505013002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:471:
interface CanvasCaptureMediaStreamTrack : MediaStreamTrack
What happens if you clone a CanvasCaptureMediaStreamTrack ?

[emircan, 2015-12-11 01:50:56]

Thanks. I addressed the nits below.

perkj@, thanks for pointing out the clone behavior. I realized that it wouldn't
work as expected right now, and worked on a seperate patch addressing it -added
the said layout test there too-. PTAL
https://codereview.chromium.org/1514113005/

https://codereview.chromium.org/1505013002/diff/140001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1505013002/diff/140001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:28: // is owned by
CanvasCaptureMediaStreamTrack.
On 2015/12/10 15:48:57, perkj wrote:
> owned by a Can

Done.

https://codereview.chromium.org/1505013002/diff/140001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:31: // used to send back frames
on IO thread.
On 2015/12/10 15:48:57, perkj wrote:
> s on the IO thread

Done.

https://codereview.chromium.org/1505013002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp
(right):

https://codereview.chromium.org/1505013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp:13:

On 2015/12/10 01:25:01, haraken wrote:
> 
> Unnecessary empty line.

Done.

[emircan, 2015-12-11 01:51:26]

Description was changed from

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call. 

BUG=524218
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

to

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call. 

BUG=524218
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
TBR=esprehn@chromium.org on third_party/WebKit/public/platform/Platform.h
==========

[emircan, 2015-12-11 01:51:38]

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

[commit-bot: I haz the power, 2015-12-11 01:53:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505013002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505013002/160001

[commit-bot: I haz the power, 2015-12-11 03:05:49]

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

[commit-bot: I haz the power, 2015-12-11 03:05:50]

Dry run: This issue passed the CQ dry run.

[emircan, 2015-12-11 03:06:59]

The CQ bit was checked by emircan@chromium.org

[emircan, 2015-12-11 03:06:59]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org,
perkj@chromium.org, haraken@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1505013002/#ps160001
(title: "Addressed nits.")

[commit-bot: I haz the power, 2015-12-11 03:07:53]

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

[commit-bot: I haz the power, 2015-12-11 03:14:08]

Description was changed from

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call. 

BUG=524218
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
TBR=esprehn@chromium.org on third_party/WebKit/public/platform/Platform.h
==========

to

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call. 

BUG=524218
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
TBR=esprehn@chromium.org on third_party/WebKit/public/platform/Platform.h
==========

[commit-bot: I haz the power, 2015-12-11 03:14:09]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-12-11 03:15:09]

Description was changed from

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call. 

BUG=524218
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
TBR=esprehn@chromium.org on third_party/WebKit/public/platform/Platform.h
==========

to

==========
Create CanvasCaptureMediaStreamTrack in a regular MediaStream

This CL handles the recent spec change where we decided to
return a specialized CanvasCaptureMediaStreamTrack object
instead of a CanvasCaptureMediaStream from a
canvas::captureStream() call.

BUG=524218
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
TBR=esprehn@chromium.org on third_party/WebKit/public/platform/Platform.h

Committed: https://crrev.com/9bdf6b77b4c685aeb895e7b7a0ea1ae400e61bb3
Cr-Commit-Position: refs/heads/master@{#364602}
==========

[commit-bot: I haz the power, 2015-12-11 03:15:10]

Patchset 5 (id:??) landed as
https://crrev.com/9bdf6b77b4c685aeb895e7b7a0ea1ae400e61bb3
Cr-Commit-Position: refs/heads/master@{#364602}

[esprehn, 2015-12-11 03:33:30]

lgtm

https://codereview.chromium.org/1505013002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.idl
(right):

https://codereview.chromium.org/1505013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.idl:9:
[RaisesException] MediaStream captureStream ();
no space before ()