remoting/webapp/client_session.js - Issue 386853002: Add a Record button to the web-app if the host supports video recording. - Code Review

Chromium Code Reviews

chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out

(750)

My Issues | Starred Open | Closed | All

Side by Side Diff: remoting/webapp/client_session.js

Issue 386853002: Add a Record button to the web-app if the host supports video recording. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Rebase Created 6 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

« remoting/webapp/client_screen.js ('K') | « remoting/webapp/client_screen.js ('k') | remoting/webapp/html/toolbar.html » ('j') | remoting/webapp/html/toolbar.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 /**	5 /**

6 * @fileoverview	6 * @fileoverview

7 * Class handling creation and teardown of a remoting client session.	7 * Class handling creation and teardown of a remoting client session.

8 *	8 *

9 * The ClientSession class controls lifetime of the client plugin	9 * The ClientSession class controls lifetime of the client plugin

10 * object and provides the plugin with the functionality it needs to	10 * object and provides the plugin with the functionality it needs to

(...skipping 149 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
160	160

161 if (this.mode_ == remoting.ClientSession.Mode.IT2ME) {	161 if (this.mode_ == remoting.ClientSession.Mode.IT2ME) {

162 // Resize-to-client is not supported for IT2Me hosts.	162 // Resize-to-client is not supported for IT2Me hosts.

163 this.resizeToClientButton_.hidden = true;	163 this.resizeToClientButton_.hidden = true;

164 } else {	164 } else {

165 this.resizeToClientButton_.hidden = false;	165 this.resizeToClientButton_.hidden = false;

166 }	166 }

167	167

168 this.fullScreenButton_.addEventListener(	168 this.fullScreenButton_.addEventListener(

169 'click', this.callToggleFullScreen_, false);	169 'click', this.callToggleFullScreen_, false);

	170

	171 this.isVideoRecording_ = false;

	172 this.videoRecordingFileWriter_ = null;
	Jamie 2014/08/01 01:28:36 Have you run this through jscompile? I'm surprised Have you run this through jscompile? I'm surprised these don't need type annotations. Wez 2014/08/19 00:15:52 I am doing now, yes; compile fails because of pre- Show quoted text 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().
	173 var toggleRecordingButton = document.getElementById('start-stop-recording');

	174 toggleRecordingButton.addEventListener(

	175 'click', this.startStopRecording_.bind(this), false);

	176 var toggleRecordingMenu = document.querySelector('.menu-start-stop-recording') ;
	Jamie 2014/08/01 01:28:36 You're using a mix of getElementById and querySele 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. 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. Jamie 2014/08/01 01:28:37 Nit: line-length. Nit: line-length. Wez 2014/08/19 00:15:53 I just cribbed from the relevant toolbar and menu Show quoted text 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. ;) Show quoted text > 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.
	177 toggleRecordingMenu.addEventListener(

	178 'click', this.startStopRecording_.bind(this), false);

	179

	180 console.log("WIBBLE");
	Jamie 2014/08/01 01:28:37 I think we can probably live without this line :) I think we can probably live without this line :) Wez 2014/08/19 00:15:53 Done. Show quoted text On 2014/08/01 01:28:37, Jamie wrote: > I think we can probably live without this line :) Done.
	181

170 this.defineEvents(Object.keys(remoting.ClientSession.Events));	182 this.defineEvents(Object.keys(remoting.ClientSession.Events));

171 };	183 };

172	184

173 base.extend(remoting.ClientSession, base.EventSource);	185 base.extend(remoting.ClientSession, base.EventSource);

174	186

175 /** @enum {string} */	187 /** @enum {string} */

176 remoting.ClientSession.Events = {	188 remoting.ClientSession.Events = {

177 stateChanged: 'stateChanged',	189 stateChanged: 'stateChanged',

178 videoChannelStateChanged: 'videoChannelStateChanged'	190 videoChannelStateChanged: 'videoChannelStateChanged'

179 };	191 };

(...skipping 883 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1063 }	1075 }

1064	1076

1065 this.capabilities_ = capabilities;	1077 this.capabilities_ = capabilities;

1066 if (this.hasCapability_(	1078 if (this.hasCapability_(

1067 remoting.ClientSession.Capability.SEND_INITIAL_RESOLUTION)) {	1079 remoting.ClientSession.Capability.SEND_INITIAL_RESOLUTION)) {

1068 var clientArea = this.getClientArea_();	1080 var clientArea = this.getClientArea_();

1069 this.plugin_.notifyClientResolution(clientArea.width,	1081 this.plugin_.notifyClientResolution(clientArea.width,

1070 clientArea.height,	1082 clientArea.height,

1071 window.devicePixelRatio);	1083 window.devicePixelRatio);

1072 }	1084 }

	1085 if (this.hasCapability_(

	1086 remoting.ClientSession.Capability.VIDEO_RECORDER)) {
	Jamie 2014/08/01 01:28:37 There are a couple of ways of handling this withou 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...) Wez 2014/08/19 00:15:53 OptionsMenu now sets this based on ClientSession.c Show quoted text 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.
	1087 var recordingButton = document.getElementById('start-stop-recording');

	1088 recordingButton.hidden = false;

	1089 var recordingMenu = document.querySelector('.menu-start-stop-recording');

	1090 recordingMenu.hidden = false;

	1091 }

1073 };	1092 };

1074	1093

1075 /**	1094 /**

1076 * @private	1095 * @private

1077 * @param {remoting.ClientSession.State} newState The new state for the session.	1096 * @param {remoting.ClientSession.State} newState The new state for the session.

1078 * @return {void} Nothing.	1097 * @return {void} Nothing.

1079 */	1098 */

1080 remoting.ClientSession.prototype.setState_ = function(newState) {	1099 remoting.ClientSession.prototype.setState_ = function(newState) {

1081 var oldState = this.state_;	1100 var oldState = this.state_;

1082 this.state_ = newState;	1101 this.state_ = newState;

(...skipping 461 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1544 * @param {number} hotspotY	1563 * @param {number} hotspotY

1545 */	1564 */

1546 remoting.ClientSession.prototype.updateMouseCursorImage_ =	1565 remoting.ClientSession.prototype.updateMouseCursorImage_ =

1547 function(url, hotspotX, hotspotY) {	1566 function(url, hotspotX, hotspotY) {

1548 this.mouseCursorOverlay_.hidden = !url;	1567 this.mouseCursorOverlay_.hidden = !url;

1549 if (url) {	1568 if (url) {

1550 this.mouseCursorOverlay_.style.marginLeft = '-' + hotspotX + 'px';	1569 this.mouseCursorOverlay_.style.marginLeft = '-' + hotspotX + 'px';

1551 this.mouseCursorOverlay_.style.marginTop = '-' + hotspotY + 'px';	1570 this.mouseCursorOverlay_.style.marginTop = '-' + hotspotY + 'px';

1552 this.mouseCursorOverlay_.src = url;	1571 this.mouseCursorOverlay_.src = url;

1553 }	1572 }

1554 };	1573 };

	1574

	1575 remoting.ClientSession.prototype.startStopRecording_ = function() {
	Jamie 2014/08/01 01:28:36 I think there is enough code here that it's worth I think there is enough code here that it's worth putting it into a separate class. Wez 2014/08/19 00:15:52 Agreed; already done. Show quoted text 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.
	1576 var data = {};

	1577 if (this.isVideoRecording_) {

	1578 data = { type: 'stop' }

	1579 this.isVideoRecording_ = false;

	1580

	1581 console.log("CLICKY!");
	Jamie 2014/08/01 01:28:36 I don't think so... I don't think so... Wez 2014/08/19 00:15:53 Totally. Show quoted text On 2014/08/01 01:28:36, Jamie wrote: > I don't think so... Totally.
	1582

	1583 chrome.fileSystem.chooseEntry(

	1584 {type: 'saveFile', suggestedName: "videoRecording.pb"},

	1585 this.onVideoRecordingFileChosen_.bind(this));

	1586 } else {

	1587 data = { type: 'start' }

	1588 this.isVideoRecording_ = true;

	1589 }

	1590 this.plugin_.sendClientMessage('video-recorder', JSON.stringify(data));

	1591 }

	1592

	1593 remoting.ClientSession.prototype.onVideoRecordingFileChosen_ = function(entry) {

	1594 if (!entry) {

	1595 console.log("Cancelled save of video frames.");

	1596 } else {

	1597 chrome.fileSystem.getDisplayPath(entry, function(path) {

	1598 console.log("Saving video frames to:" + path);

	1599 });

	1600 entry.createWriter(this.onVideoRecordingFileWriter_.bind(this));

	1601 }

	1602 }

	1603

	1604 remoting.ClientSession.prototype.onVideoRecordingFileWriter_ =

	1605 function(writer) {

	1606 console.log("Obtained FileWriter for video frame write");

	1607 writer.onwriteend = this.onVideoRecordingWriteComplete_.bind(this);

	1608 this.videoRecordingFileWriter_ = writer;

	1609 this.fetchNextVideoRecordingFrame_();

	1610 }

	1611

	1612 remoting.ClientSession.prototype.onVideoRecordingWriteComplete_ = function(e) {

	1613 console.log("Video frame write complete");

	1614 this.fetchNextVideoRecordingFrame_();

	1615 }

	1616

	1617 remoting.ClientSession.prototype.fetchNextVideoRecordingFrame_ = function() {

	1618 console.log("Request next video frame");

	1619 var data = { type: 'next-frame' }

	1620 this.plugin_.sendClientMessage('video-recorder', JSON.stringify(data));

	1621 }

	1622

	1623 remoting.ClientSession.prototype.handleVideoRecorderMessage =

	1624 function(type, data) {

	1625 if (type != 'video-recorder') {

	1626 return false;

	1627 }

	1628

	1629 var message = getJsonObjectFromString(data);

	1630

	1631 if (message.type == 'next-frame-reply') {

	1632 if (!this.videoRecordingFileWriter_) {

	1633 console.log("Received frame but have no writer");

	1634 return true;

	1635 }

	1636 if (!message.data) {

	1637 console.log("Finished receiving frames");

	1638 this.videoRecordingFileWriter_ = null;

	1639 return true;

	1640 }

	1641

	1642 console.log("Received frame");

	1643 var videoPacketString = atob(message.data);
	Wez 2014/08/19 00:15:52 This line angers jscompile, but there's no obvious This line angers jscompile, but there's no obvious place to add the necessary annotation - what would you suggest? Jamie 2014/08/19 00:48:56 I would add it to dom_proto.js. It's not DOM-relat Show quoted text On 2014/08/19 00:15:52, Wez wrote: > This line angers jscompile, but there's no obvious place to add the necessary > annotation - what would you suggest? I would add it to dom_proto.js. It's not DOM-related, but there's already some non-DOM stuff in there so I think that's okay. If we end up needing those proto files for much long, we can pull out the non-DOM stuff into something like base_proto.js, but I'm hopeful we'll switch to a better compile in the not-too-distant future. Wez 2014/08/19 21:19:00 Done. Show quoted text On 2014/08/19 00:48:56, Jamie wrote: > On 2014/08/19 00:15:52, Wez wrote: > > This line angers jscompile, but there's no obvious place to add the necessary > > annotation - what would you suggest? > > I would add it to dom_proto.js. It's not DOM-related, but there's already some > non-DOM stuff in there so I think that's okay. If we end up needing those proto > files for much long, we can pull out the non-DOM stuff into something like > base_proto.js, but I'm hopeful we'll switch to a better compile in the > not-too-distant future. Done.
	1644

	1645 console.log("Converted from Base64 - length:" + videoPacketString.length);

	1646 var byteArrays = [];

	1647

	1648 for (var offset = 0; offset < videoPacketString.length; offset += 512) {

	1649 var slice = videoPacketString.slice(offset, offset + 512);

	1650 var byteNumbers = new Array(slice.length);

	1651 for (var i = 0; i < slice.length; i++) {

	1652 byteNumbers[i] = slice.charCodeAt(i);

	1653 }

	1654 var byteArray = new Uint8Array(byteNumbers);

	1655 byteArrays.push(byteArray);

	1656 }

	1657

	1658 console.log("Writing frame");

	1659 videoPacketString = null;

	1660 var videoPacketBlob = new Blob(byteArrays);

	1661 byteArrays = null;

	1662

	1663 this.videoRecordingFileWriter_.write(videoPacketBlob);

	1664

	1665 return true;

	1666 }

	1667

	1668 console.log("Unrecognized message: " + message.type);

	1669 return true;

	1670 }

OLD	NEW

« remoting/webapp/client_screen.js ('K') | « remoting/webapp/client_screen.js ('k') | remoting/webapp/html/toolbar.html » ('j') | remoting/webapp/html/toolbar.html » ('J')

Powered by Google App Engine

This is Rietveld 408576698