Issue 386853002: Add a Record button to the web-app if the host supports video recording.

Issue 386853002: Add a Record button to the web-app if the host supports video recording. (Closed)

Created:
6 years, 5 months ago by Wez

Modified:
6 years, 4 months ago

Reviewers:
Sergey Ulanov, Jamie, kelvinp

CC:
chromium-reviews, chromoting-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add a Record button to the web-app if the host supports video recording. Video recording allows sequences of frames to be recorded and delivered to the client for performance-evaluation purposes. The client can save recorded sequences out to files on disk. Since live playback is not a requirement, performance of the implementation is not a priority. See crrev.com/372943002 for the host side of this. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290721

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 24

Patch Set 3 : Move implementation out to remoting.VideoFrameRecorder. #

Total comments: 2

Patch Set 4 : Move code out into OptionsMenu and VideoFrameRecorder #

Total comments: 7

Patch Set 5 : Add missing isRecording and some more comments #

Patch Set 6 : Fix almost all type annotations #

Total comments: 1

Patch Set 7 : Suppress type error for new Blob() #

Patch Set 8 : function -> var #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+257 lines, -4 lines)			Patch
M	remoting/remoting_webapp_files.gypi	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	remoting/resources/remoting_strings.grd	View	1 2 3 4	1 chunk	+6 lines, -0 lines	0 comments	Download
M	remoting/webapp/client_screen.js	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	remoting/webapp/client_session.js	View	1 2 3 4	4 chunks	+49 lines, -0 lines	0 comments	Download
M	remoting/webapp/html/window_frame.html	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	remoting/webapp/js_proto/chrome_proto.js	View	1 2 3 4 5	2 chunks	+14 lines, -1 line	0 comments	Download
M	remoting/webapp/manifest.json.jinja2	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	remoting/webapp/options_menu.js	View	1 2 3 4 5	4 chunks	+24 lines, -1 line	0 comments	Download
M	remoting/webapp/toolbar.js	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download
A	remoting/webapp/video_frame_recorder.js	View	1 2 3 4 5 6 7	1 chunk	+152 lines, -0 lines	0 comments	Download
M	remoting/webapp/window_frame.js	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

Wez

This is a quick hack of a recording button into the Chromoting client for the ...

6 years, 4 months ago (2014-08-01 00:03:24 UTC) #1

Jamie

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_screen.js File remoting/webapp/client_screen.js (right): https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_screen.js#newcode364 remoting/webapp/client_screen.js:364: remoting.clientSession.handleVideoRecorderMessage(type, data)) { Since handleVideoRecorderMessage has side-effects, I think ...

6 years, 4 months ago (2014-08-01 01:28:37 UTC) #2

kelvinp

https://codereview.chromium.org/386853002/diff/40001/remoting/webapp/client_session.js File remoting/webapp/client_session.js (right): https://codereview.chromium.org/386853002/diff/40001/remoting/webapp/client_session.js#newcode1091 remoting/webapp/client_session.js:1091: this.videoFrameRecorder_ = new remoting.VideoFrameRecorder(); This change looks pretty cool. ...

6 years, 4 months ago (2014-08-15 23:37:31 UTC) #3

Sergey Ulanov

6 years, 4 months ago (2014-08-16 00:16:58 UTC) #4

Wez

6 years, 4 months ago (2014-08-19 00:15:53 UTC) #5

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
File remoting/webapp/client_screen.js (right):

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_screen.js:364:
remoting.clientSession.handleVideoRecorderMessage(type, data)) {
On 2014/08/01 01:28:36, Jamie wrote:
> Since handleVideoRecorderMessage has side-effects, I think this would be
clearer
> as:
> 
> if (remoting.clientSession) {
>   return remoting.clientSession.handleVideoRecorderMessage(type, data);
> }

Done.

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_session.js:172: this.videoRecordingFileWriter_ = null;
On 2014/08/01 01:28:36, Jamie wrote:
> Have you run this through jscompile? I'm surprised these don't need type
> annotations.

I am doing now, yes; compile fails because of pre-existing failures in
background.js, etc, though.

It doesn't complain about the fileWriter type not being defined, curiously; the
only thing it complains about is atob().

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_session.js:176: var toggleRecordingMenu =
document.querySelector('.menu-start-stop-recording');
On 2014/08/01 01:28:36, Jamie wrote:
> You're using a mix of getElementById and querySelector. The convention I've
> tried to follow (though this class is old enough that it doesn't follow it
100%)
> is that classes should not use getElementById; rather, they should be passed a
> suitable parent node (or nodes) in the ctor and call querySelector to find the
> child elements it needs access to.

I just cribbed from the relevant toolbar and menu code, and assumed there was
some good reason for using one versus the other. ;)

> In this case, since you're adding a button to both the tool-bar and the
options
> menu, I think the OptionsMenu class is the place to add these event handlers.
It
> gets instantiated either by the Toolbar or the WindowFrame classes for v1 or
v2,
> respectively.

In the latest patch I've moved it into OptionsMenu, though that adds a lot of
boilerplate - that could be reduced by having ClientSession expose the
VideoFrameRecorder directly for the OptionsMenu to access, of course.

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_session.js:180: console.log("WIBBLE");
On 2014/08/01 01:28:37, Jamie wrote:
> I think we can probably live without this line :)

Done.

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_session.js:1086:
remoting.ClientSession.Capability.VIDEO_RECORDER)) {
On 2014/08/01 01:28:37, Jamie wrote:
> There are a couple of ways of handling this without this class needing to know
> the id/class of the elements. You could add a setRecordingSupported method to
> OptionsMenu, or you could set a recording-supported class on the <body> and
add
> a CSS rule similar to the one for hiding v1/v2 elements
>
(https://code.google.com/p/chromium/codesearch#chromium/src/remoting/webapp/ma...)

OptionsMenu now sets this based on ClientSession.canRecordVideo, which in turn
depends upon whether the VideoFrameRecorder was instantiated.

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_session.js:1575:
remoting.ClientSession.prototype.startStopRecording_ = function() {
On 2014/08/01 01:28:36, Jamie wrote:
> I think there is enough code here that it's worth putting it into a separate
> class.

Agreed; already done.

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_session.js:1581: console.log("CLICKY!");
On 2014/08/01 01:28:36, Jamie wrote:
> I don't think so...

Totally.

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_session.js:1643: var videoPacketString =
atob(message.data);
This line angers jscompile, but there's no obvious place to add the necessary
annotation - what would you suggest?

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/html/too...
File remoting/webapp/html/toolbar.html (right):

https://codereview.chromium.org/386853002/diff/20001/remoting/webapp/html/too...
remoting/webapp/html/toolbar.html:33: </button>
On 2014/08/01 01:28:37, Jamie wrote:
> I'm not quite sure what you mean about only implementing this for v2--it looks
> like you've implemented it for both.

Yes, I started with the v1 app, but we really want it in the v2 app removing
this.

> If you are intending to keep this in the
> tool-bar, I think you should move it to the Screen Options menu, since there
> isn't much horizontal space, and this takes up quite a lot.

It'll only be visible for hosts that were specifically run with this debug
option, so I don't think that's an issue.

Jamie

LGTM once it compiles, but if you want to me take another look at that ...

6 years, 4 months ago (2014-08-19 00:48:57 UTC) #6

Wez

https://codereview.chromium.org/386853002/diff/20001/remoting/resources/remoting_strings.grd File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/386853002/diff/20001/remoting/resources/remoting_strings.grd#newcode710 remoting/resources/remoting_strings.grd:710: <message desc="Button for starting and stopping video frame recording, ...

6 years, 4 months ago (2014-08-19 21:19:00 UTC) #7