Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12472032:
Add screen capture constraints, video tag resize and removed auto video resize (Closed)
|
Created:
7 years, 9 months ago by jansson Modified:
7 years, 8 months ago Reviewers:
kjellander_chromium CC:
chromium-reviews, phoglund_chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@testingscreen Visibility:
Public. |
DescriptionAdd screen capture constraints, video tag resize and removed auto video resize
-screen capture tickbox added with constraints
-added buttons for resizing the video tag
-added video tag & stream size printout
-removed video tag resize after getUserMedia and adding remote stream
-Fixed some comments
TEST=Tested calling with audio/video/screencapture. Ran gjslint and will create separate CL for errors due to high error count. Mostly comments missing.
BUG=None
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194916
Patch Set 1 #Patch Set 2 : removed line break #
Total comments: 42
Patch Set 3 : Fixing stream size bug, coding style, comments #Patch Set 4 : gjslint issues + fixed if statement for constraints #
Total comments: 31
Patch Set 5 : Fixed per reviewers comments, fixed debug print bug #Patch Set 6 : Removed redundant div + added video-area class to div instead #Patch Set 7 : removed semicolon #
Total comments: 12
Patch Set 8 : fixed video tag color, nit format, misc based on comments #
Total comments: 2
Patch Set 9 : change border color to #aaa #
Messages
Total messages: 11 (0 generated)
Please review at your convenience. As noted I will do a separate CL with lint fixes as there are lots on the jsep01 file for example.
cc: phoglund. Nice work to create a better solution for the handling of the sizes of the video tags. I have a few suggestions, not all of them needs to be implemented in this CL though. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... File chrome/test/data/webrtc/getusermedia.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:137: // videoTag.onloadedmetadata = updateVideoTagSize_('local-view'); Update this line (meant to be the code used when 110938 is fixed) to read: // videoTag.onloadedmetadata = displayVideoSize(videoTag); https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:139: // setTimeout(function() {updateVideoTagSize_('local-view')}, 500); Clean out the old code instead of commenting it. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:147: * @param {string} width The width of the video to update the video The implementation is not exactly as documented, as if one is set to 0 and not the other, both width and height of the video tag will be set to videoWidth/videoHeight. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:154: if (width && height > 0) { This is confusing since width is probably always true if specified. I suggest changing to (width > 0 && height > 0) https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:162: debug('Set video tag "' + videoTagId + '" width and height to ' + I prefer replacing "width and height" with just "size". https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:172: function displayVideoSize(videoTag) { private functions shall end with underscore. http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:175: videoTag.videoHeight + ' )'; Skip the white space at the end of the string, before the closing parenthesis. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/js... File chrome/test/data/webrtc/jsep01_call.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/js... chrome/test/data/webrtc/jsep01_call.js:225: // setTimeout(function() {updateVideoTagSize_('remote-view')}, 500); Remove the unused code and update the code to be put in when 110938 is fixed. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... File chrome/test/data/webrtc/manual/peerconnection.html (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:18: <input type="text" id="getusermedia-constraints" size="30"> I suggest making the text box wider on the page so it at least fits the majority of the constraint contents (if not all). I think it would be fine to put a <br/> before the "Use constraints.html..." text to make it go on the next line instead. Just make sure to change the text too, so it says paste into the box "above" instead of "left". https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:37: <h2>Local Preview</h2> Optional: I think it would be great if all the stuff that belongs to a video tag could be put in a div wrapping it and putting it with a slightly different background color (or possibly a gray border line). Just to make it more obvious which texts (and now buttons) that belongs to the video tag. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:39: </video><br/> This line shall actually be indented 4 spaces according to the style guide. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:41: <table> Please don't use tables. Divs+CSS should be enough (even if it's harder to get it right initially it pays of in the long run). https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:45: <td id="local-view-stream-size">(stream size: NA)</td> I prefer N/A https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:52: <td><button onclick="updateVideoTagSize_('local-view')"> To stream size Wrap properly (applies below too). https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:64: <h2>Settings</h2> I'd like to keep the blank line above this heading for readability. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:117: <table> Same here, try to work out a layout with divs and proper CSS instead of tables. It shouldn't be that hard. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:143: <h2>Debug</h2> I'd like to keep that blank line for improved readability. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.js:127: audio: false, Is audio not possible to have set to true? If that's the case a bug should be filed. Please leave it as: audio: $('audio').checked, since having the actual constraint being something different from the checkbox state will surely confuse users. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.js:128: video: {mandatory: {chromeMediaSource: 'screen'}} Is there no way to get both screen capture and a webcam video stream at the same time? It might be worth investigating and possibly file a bug if so. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.js:131: var constraints = { Depending on if it's a bug or not to not being able to have audio: true I think you can simplify the if-else construct here to make it smaller. Example (not tested): var constraints = { audio: $('audio').checked, video: $('video').checked, }; if $('screencapture').checked constraints['video'] = {mandatory: {chromeMediaSource: 'screen'}};
Fixed all the comments as well as the stream 0 size bug + some gjslint issues, not all but many. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... File chrome/test/data/webrtc/getusermedia.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:137: // videoTag.onloadedmetadata = updateVideoTagSize_('local-view'); On 2013/03/25 13:33:58, Henrik Kjellander wrote: > Update this line (meant to be the code used when 110938 is fixed) to read: > // videoTag.onloadedmetadata = displayVideoSize(videoTag); Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:139: // setTimeout(function() {updateVideoTagSize_('local-view')}, 500); On 2013/03/25 13:33:58, Henrik Kjellander wrote: > Clean out the old code instead of commenting it. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:147: * @param {string} width The width of the video to update the video On 2013/03/25 13:33:58, Henrik Kjellander wrote: > The implementation is not exactly as documented, as if one is set to 0 and not > the other, both width and height of the video tag will be set to > videoWidth/videoHeight. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:154: if (width && height > 0) { On 2013/03/25 13:33:58, Henrik Kjellander wrote: > This is confusing since width is probably always true if specified. > I suggest changing to (width > 0 && height > 0) Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:162: debug('Set video tag "' + videoTagId + '" width and height to ' + On 2013/03/25 13:33:58, Henrik Kjellander wrote: > I prefer replacing "width and height" with just "size". Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:172: function displayVideoSize(videoTag) { On 2013/03/25 13:33:58, Henrik Kjellander wrote: > private functions shall end with underscore. > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:175: videoTag.videoHeight + ' )'; On 2013/03/25 13:33:58, Henrik Kjellander wrote: > Skip the white space at the end of the string, before the closing parenthesis. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/js... File chrome/test/data/webrtc/jsep01_call.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/js... chrome/test/data/webrtc/jsep01_call.js:225: // setTimeout(function() {updateVideoTagSize_('remote-view')}, 500); On 2013/03/25 13:33:58, Henrik Kjellander wrote: > Remove the unused code and update the code to be put in when 110938 is fixed. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... File chrome/test/data/webrtc/manual/peerconnection.html (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:18: <input type="text" id="getusermedia-constraints" size="30"> On 2013/03/25 13:33:58, Henrik Kjellander wrote: > I suggest making the text box wider on the page so it at least fits the majority > of the constraint contents (if not all). > I think it would be fine to put a <br/> before the "Use constraints.html..." > text to make it go on the next line instead. Just make sure to change the text > too, so it says paste into the box "above" instead of "left". Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:39: </video><br/> On 2013/03/25 13:33:58, Henrik Kjellander wrote: > This line shall actually be indented 4 spaces according to the style guide. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:45: <td id="local-view-stream-size">(stream size: NA)</td> On 2013/03/25 13:33:58, Henrik Kjellander wrote: > I prefer N/A Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:64: <h2>Settings</h2> On 2013/03/25 13:33:58, Henrik Kjellander wrote: > I'd like to keep the blank line above this heading for readability. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:117: <table> On 2013/03/25 13:33:58, Henrik Kjellander wrote: > Same here, try to work out a layout with divs and proper CSS instead of tables. > It shouldn't be that hard. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.html:143: <h2>Debug</h2> On 2013/03/25 13:33:58, Henrik Kjellander wrote: > I'd like to keep that blank line for improved readability. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.js:127: audio: false, On 2013/03/25 13:33:58, Henrik Kjellander wrote: > Is audio not possible to have set to true? If that's the case a bug should be > filed. > > Please leave it as: > audio: $('audio').checked, > since having the actual constraint being something different from the checkbox > state will surely confuse users. Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.js:128: video: {mandatory: {chromeMediaSource: 'screen'}} On 2013/03/25 13:33:58, Henrik Kjellander wrote: > Is there no way to get both screen capture and a webcam video stream at the same > time? It might be worth investigating and possibly file a bug if so. It's not possible to send both with our test pages. I do not know if it's possible by just adding it a as a separate track? https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.js:131: var constraints = { On 2013/03/25 13:33:58, Henrik Kjellander wrote: > Depending on if it's a bug or not to not being able to have audio: true I think > you can simplify the if-else construct here to make it smaller. > > Example (not tested): > var constraints = { > audio: $('audio').checked, > video: $('video').checked, > }; > if $('screencapture').checked > constraints['video'] = {mandatory: {chromeMediaSource: 'screen'}}; Done.
Nice progress, but needs more work. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... File chrome/test/data/webrtc/getusermedia.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:137: // videoTag.onloadedmetadata = updateVideoTagSize_('local-view'); You must have missed this, it's still not updated. On 2013/04/05 10:29:42, jansson wrote: > On 2013/03/25 13:33:58, Henrik Kjellander wrote: > > Update this line (meant to be the code used when 110938 is fixed) to read: > > // videoTag.onloadedmetadata = displayVideoSize(videoTag); > > Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:172: function displayVideoSize(videoTag) { Move underscore to the end of the function name, not as a prefix. On 2013/04/05 10:29:42, jansson wrote: > On 2013/03/25 13:33:58, Henrik Kjellander wrote: > > private functions shall end with underscore. > > > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > > Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ma... chrome/test/data/webrtc/manual/peerconnection.js:128: video: {mandatory: {chromeMediaSource: 'screen'}} On 2013/04/05 10:29:42, jansson wrote: > On 2013/03/25 13:33:58, Henrik Kjellander wrote: > > Is there no way to get both screen capture and a webcam video stream at the > same > > time? It might be worth investigating and possibly file a bug if so. > > It's not possible to send both with our test pages. I do not know if it's > possible by just adding it a as a separate track? > I think it should be possible according to the spec, but this test page doesn't support it properly (yet). Please investigate if a bug should be filed if Chrome doesn't handle that combination of constraints properly. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/g... File chrome/test/data/webrtc/getusermedia.js (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/g... chrome/test/data/webrtc/getusermedia.js:62: * @return {string} Returns not-called-yet if we have More words can go on the same row as @return (i.e. the wrap is too early). https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/g... chrome/test/data/webrtc/getusermedia.js:168: debug('"' + videoTagId + '" video stream size is 0, cannot resize'); change 'cannot resize' -> 'skipping resize' (since it would be possible to resize to 0x0 even if it doesn't make sense) https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/g... chrome/test/data/webrtc/getusermedia.js:180: if (videoTag.videoWidth || videoTag.videoHeight > 0) { videoTag.videoWidth > 0 ? https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/j... File chrome/test/data/webrtc/jsep01_call.js (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/j... chrome/test/data/webrtc/jsep01_call.js:224: // videoTag.onloadedmetadata = updateVideoTagSize_('remote-view'); Change this to use the displayVideoSize_ function. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/peerconnection.html (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:37: <div id="local-view-area"> I suggest adding a CSS class for this div tag that wraps around the video tag and it's related controls. If you don't use the ID anywhere (I couldn't find any reference), please remove it to improve readability. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:40: </video><br/> Indent +2 spaces. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:41: <div id="size"> Do you use the ID for this and the 'resize' div tag further down? If not, please remove them. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:48: <button onclick="updateVideoTagSize_('local-view', '320', '240' );"> You should be fine passing integer numbers here. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:67: <br/> I don't see the need for this to be indented as it's a stand alone element. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:68: PeerConnection <a href="http://goo.gl/0TjfX">createAnswer MediaConstraints Please leave untouched instead of adding a space before the colon. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:108: <div id="right-half"> +2 indent (affects content below too) https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:116: <div id="remote-view-stream-size">(stream size: N/A)</div><br/> +2 indent. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.js:131: audio: $('audio').checked = false, I don't think we should alter the $('audio').checked attribute at all, even if it's not supported in Chrome (unless the spec says so). Please modify the error messages displayed when invalid constraints combinations are attempted (possibly try to help the user by stating which combinations are currently not supported). https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/stylesheet.css (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:54: #getusermedia-input { Add a class to be used for the div tags that surrount the stuff that relates to a video, with either a very light gray background color and/or a discrete border. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:62: #local-view-size { I suggest using a CSS class for this instead, like .inline-contents, if the inline property is all you need for the div elements that are using it.
Fixed per comments + fixed a bug regarding debug printout when re-sizing to stream size. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... File chrome/test/data/webrtc/getusermedia.js (right): https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:172: function displayVideoSize(videoTag) { On 2013/04/08 14:19:51, Henrik Kjellander wrote: > Move underscore to the end of the function name, not as a prefix. > > On 2013/04/05 10:29:42, jansson wrote: > > On 2013/03/25 13:33:58, Henrik Kjellander wrote: > > > private functions shall end with underscore. > > > > > > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > > > > Done. > Done. https://codereview.chromium.org/12472032/diff/2001/chrome/test/data/webrtc/ge... chrome/test/data/webrtc/getusermedia.js:172: function displayVideoSize(videoTag) { On 2013/04/08 14:19:51, Henrik Kjellander wrote: > Move underscore to the end of the function name, not as a prefix. > > On 2013/04/05 10:29:42, jansson wrote: > > On 2013/03/25 13:33:58, Henrik Kjellander wrote: > > > private functions shall end with underscore. > > > > > > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > > > > Done. > Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/g... File chrome/test/data/webrtc/getusermedia.js (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/g... chrome/test/data/webrtc/getusermedia.js:62: * @return {string} Returns not-called-yet if we have On 2013/04/08 14:19:52, Henrik Kjellander wrote: > More words can go on the same row as @return (i.e. the wrap is too early). Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/g... chrome/test/data/webrtc/getusermedia.js:168: debug('"' + videoTagId + '" video stream size is 0, cannot resize'); On 2013/04/08 14:19:52, Henrik Kjellander wrote: > change 'cannot resize' -> 'skipping resize' (since it would be possible to > resize to 0x0 even if it doesn't make sense) Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/g... chrome/test/data/webrtc/getusermedia.js:180: if (videoTag.videoWidth || videoTag.videoHeight > 0) { On 2013/04/08 14:19:52, Henrik Kjellander wrote: > videoTag.videoWidth > 0 ? Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/j... File chrome/test/data/webrtc/jsep01_call.js (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/j... chrome/test/data/webrtc/jsep01_call.js:224: // videoTag.onloadedmetadata = updateVideoTagSize_('remote-view'); On 2013/04/08 14:19:52, Henrik Kjellander wrote: > Change this to use the displayVideoSize_ function. Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/peerconnection.html (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:37: <div id="local-view-area"> On 2013/04/08 14:19:52, Henrik Kjellander wrote: > I suggest adding a CSS class for this div tag that wraps around the video tag > and it's related controls. > If you don't use the ID anywhere (I couldn't find any reference), please remove > it to improve readability. Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:40: </video><br/> On 2013/04/08 14:19:52, Henrik Kjellander wrote: > Indent +2 spaces. Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:41: <div id="size"> On 2013/04/08 14:19:52, Henrik Kjellander wrote: > Do you use the ID for this and the 'resize' div tag further down? If not, please > remove them. Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:48: <button onclick="updateVideoTagSize_('local-view', '320', '240' );"> On 2013/04/08 14:19:52, Henrik Kjellander wrote: > You should be fine passing integer numbers here. Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:67: <br/> On 2013/04/08 14:19:52, Henrik Kjellander wrote: > I don't see the need for this to be indented as it's a stand alone element. Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:68: PeerConnection <a href="http://goo.gl/0TjfX">createAnswer MediaConstraints On 2013/04/08 14:19:52, Henrik Kjellander wrote: > Please leave untouched instead of adding a space before the colon. Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:108: <div id="right-half"> On 2013/04/08 14:19:52, Henrik Kjellander wrote: > +2 indent (affects content below too) Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:116: <div id="remote-view-stream-size">(stream size: N/A)</div><br/> On 2013/04/08 14:19:52, Henrik Kjellander wrote: > +2 indent. Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.js:131: audio: $('audio').checked = false, On 2013/04/08 14:19:52, Henrik Kjellander wrote: > I don't think we should alter the $('audio').checked attribute at all, even if > it's not supported in Chrome (unless the spec says so). > > Please modify the error messages displayed when invalid constraints combinations > are attempted (possibly try to help the user by stating which combinations are > currently not supported). Done. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/stylesheet.css (right): https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:54: #getusermedia-input { On 2013/04/08 14:19:52, Henrik Kjellander wrote: > Add a class to be used for the div tags that surrount the stuff that relates to > a video, with either a very light gray background color and/or a discrete > border. I've added a class but I'm not happy with the overall layout still, it's not good for large resolutions (1900+) like when doing screen capture or full HD. But the use case for this page is not high res I guess. Please provide feedback if it's not good enough for this iteration. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:54: #getusermedia-input { On 2013/04/08 14:19:52, Henrik Kjellander wrote: > Add a class to be used for the div tags that surrount the stuff that relates to > a video, with either a very light gray background color and/or a discrete > border. I added a class for the video stuff, however it's not good for high res stuff and it's not pretty but does the job. Not sure if it's good enough. https://codereview.chromium.org/12472032/diff/13001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:62: #local-view-size { On 2013/04/08 14:19:52, Henrik Kjellander wrote: > I suggest using a CSS class for this instead, like .inline-contents, if the > inline property is all you need for the div elements that are using it. Done.
Almost there! https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/g... File chrome/test/data/webrtc/getusermedia.js (right): https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/g... chrome/test/data/webrtc/getusermedia.js:166: 'resize'); nit: Indent to match above parenthesis. https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/peerconnection.html (right): https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:37: <div class="video-area"> Looks almost as I wanted it. Can you see if it's possible to get this div to not automatically fill itself to the entire width of the surrounding div? It looks kind of bad with the gray background. https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:45: Size: <div id="local-view-size" class="inline-contents"></div> Check if it's enough to set the class for the surrounding div instead. That would be much cleaner. https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:120: <div> same here with the CSS class. https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/stylesheet.css (right): https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:50: background: #ccc; Ouch, please make this a very light gray so the contrast is not that high.
https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/g... File chrome/test/data/webrtc/getusermedia.js (right): https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/g... chrome/test/data/webrtc/getusermedia.js:166: 'resize'); On 2013/04/12 09:58:18, Henrik Kjellander wrote: > nit: Indent to match above parenthesis. Done. https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/peerconnection.html (right): https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:37: <div class="video-area"> On 2013/04/12 09:58:18, Henrik Kjellander wrote: > Looks almost as I wanted it. Can you see if it's possible to get this div to not > automatically fill itself to the entire width of the surrounding div? It looks > kind of bad with the gray background. The easiest way forward is just adding the video-area class to the video tag directly, however doing do means the size/resize buttons will not have a gray background. Is that okay? https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:45: Size: <div id="local-view-size" class="inline-contents"></div> On 2013/04/12 09:58:18, Henrik Kjellander wrote: > Check if it's enough to set the class for the surrounding div instead. That > would be much cleaner. It was not as simple as adding it to the surrounding div unfortunately. https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:120: <div> On 2013/04/12 09:58:18, Henrik Kjellander wrote: > same here with the CSS class. see previous question/answer https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/stylesheet.css (right): https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:50: background: #ccc; On 2013/04/12 09:58:18, Henrik Kjellander wrote: > Ouch, please make this a very light gray so the contrast is not that high. Done.
LGTM! Optionally do the final touch of the border color https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/peerconnection.html (right): https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:37: <div class="video-area"> On 2013/04/16 14:25:17, jansson wrote: > On 2013/04/12 09:58:18, Henrik Kjellander wrote: > > Looks almost as I wanted it. Can you see if it's possible to get this div to > not > > automatically fill itself to the entire width of the surrounding div? It looks > > kind of bad with the gray background. > The easiest way forward is just adding the video-area class to the video tag > directly, however doing do means the size/resize buttons will not have a gray > background. Is that okay? > Nevermind which way, I guess this is nicer than having a large gray bar across the whole screen. The important thing was a lighter gray color. https://codereview.chromium.org/12472032/diff/22006/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/peerconnection.html:45: Size: <div id="local-view-size" class="inline-contents"></div> OK, CSS is tricky sometimes. On 2013/04/16 14:25:17, jansson wrote: > On 2013/04/12 09:58:18, Henrik Kjellander wrote: > > Check if it's enough to set the class for the surrounding div instead. That > > would be much cleaner. > It was not as simple as adding it to the surrounding div unfortunately. https://codereview.chromium.org/12472032/diff/33001/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/stylesheet.css (right): https://codereview.chromium.org/12472032/diff/33001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:51: border:1px solid #000; nit: make the border #aaa or something like that, to lessen the contrast.
changed to #aaa, looks better. https://codereview.chromium.org/12472032/diff/33001/chrome/test/data/webrtc/m... File chrome/test/data/webrtc/manual/stylesheet.css (right): https://codereview.chromium.org/12472032/diff/33001/chrome/test/data/webrtc/m... chrome/test/data/webrtc/manual/stylesheet.css:51: border:1px solid #000; On 2013/04/18 13:29:47, Henrik Kjellander wrote: > nit: make the border #aaa or something like that, to lessen the contrast. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jansson@chromium.org/12472032/40001
Message was sent while issue was closed.
Change committed as 194916 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
