Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
110853006:
Combining all javascript files to one for the manual test page. (Closed)
|
Created:
7 years ago by jansson Modified:
6 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCombining all javascript files to one for the manual test page and perform re-factoring.
BUG=330823
TEST=Regression testing manually
NOTRY=TRUE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243839
Patch Set 1 #Patch Set 2 : improved DOM event registration #Patch Set 3 : fixing nits #
Total comments: 34
Patch Set 4 : Fixed according to comments #
Total comments: 10
Patch Set 5 : fixed nits #
Total comments: 3
Patch Set 6 : fixed nits #
Total comments: 2
Patch Set 7 : fixed nits + rebase #
Messages
Total messages: 13 (0 generated)
I've now added all the required functionality to peerconnection.js and started re-factoring all the redundant code. Also cleaned up peerconnection.html by removing some functional calls and added them to an event listener instead. More will be done in following CL's. There are more work to be done but want to land an initial CL.
On 2013/12/27 09:14:33, jansson wrote: > I've now added all the required functionality to peerconnection.js and started > re-factoring all the redundant code. > > Also cleaned up peerconnection.html by removing some functional calls and added > them to an event listener instead. More will be done in following CL's. > > There are more work to be done but want to land an initial CL. Also need to privatize more as most things will be called internally but will create a separate CL for that.
Seems to be some new code in here. Split into a patch that moves code and one that adds the new code. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:8: * See http://dev.w3.org/2011/webrtc/editor/getusermedia.html for more Also link to the peerconnection spec. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:47: * shared with browser tests or automated tests. Enumerates devices available Except they're not shared anymore, right? https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:66: if (!isDisconnected_()) Move this check into disconnect_() (just return if we're already disconnected) and remove the other isDisconnected calls before disconnect_(). https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:88: function getUserMediaFromHere() { You should be able to get rid of all of these (but perhaps in the next patch). https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:266: if (global.localStream == null) In the next patch, make these accessors on the globals object and make the accessor check for null instead. That way you won't have to duplicate this check on line 266 and 280, for instance. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:273: // Functions callable from other JavaScript modules. Remove this comment. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:358: These setters should be on the global object (perhaps next patch). https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:366: global.transformOutgoingSdp = function(sdp) { return sdp; }; This is wrong: should assign to transformFunction. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:420: /** TODO Fix param definitions Remove or fix this comment. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:432: // Public interface towards the other javascript files, such as Remove this comment, it no longer applies. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:491: error_('Creating DataChannel, but we already have one.'); Nit: remove {}. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:612: function toggleRemoteStream(selectAudioOrVideoTrack, typeToToggle) { I think these functions are probably over-generalized and could be simplified. Again, next patch. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:665: gAutoAddLocalToPeerConnectionStreamWhenCalled = false; You missed this global variable? https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:743: function registerEventBasedOnElementType_(element) { I don't understand these. Where did they come from? Did you write them now? If so, move to a separate patch.
https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:8: * See http://dev.w3.org/2011/webrtc/editor/getusermedia.html for more On 2014/01/03 16:07:55, phoglund wrote: > Also link to the peerconnection spec. This http://dev.w3.org/2011/webrtc/editor/webrtc.html or http://tools.ietf.org/html/draft-ietf-rtcweb-jsep-00 ? I've added the former. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:47: * shared with browser tests or automated tests. Enumerates devices available On 2014/01/03 16:07:55, phoglund wrote: > Except they're not shared anymore, right? Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:66: if (!isDisconnected_()) On 2014/01/03 16:07:55, phoglund wrote: > Move this check into disconnect_() (just return if we're already disconnected) > and remove the other isDisconnected calls before disconnect_(). Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:88: function getUserMediaFromHere() { On 2014/01/03 16:07:55, phoglund wrote: > You should be able to get rid of all of these (but perhaps in the next patch). I will create a separate patch where I assign appropriate events to the elements. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:266: if (global.localStream == null) On 2014/01/03 16:07:55, phoglund wrote: > In the next patch, make these accessors on the globals object and make the > accessor check for null instead. That way you won't have to duplicate this check > on line 266 and 280, for instance. Adding it to my notes. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:273: // Functions callable from other JavaScript modules. On 2014/01/03 16:07:55, phoglund wrote: > Remove this comment. Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:358: On 2014/01/03 16:07:55, phoglund wrote: > These setters should be on the global object (perhaps next patch). Adding to my todo list. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:366: global.transformOutgoingSdp = function(sdp) { return sdp; }; On 2014/01/03 16:07:55, phoglund wrote: > This is wrong: should assign to transformFunction. Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:420: /** TODO Fix param definitions On 2014/01/03 16:07:55, phoglund wrote: > Remove or fix this comment. Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:432: // Public interface towards the other javascript files, such as On 2014/01/03 16:07:55, phoglund wrote: > Remove this comment, it no longer applies. Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:491: error_('Creating DataChannel, but we already have one.'); On 2014/01/03 16:07:55, phoglund wrote: > Nit: remove {}. Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:612: function toggleRemoteStream(selectAudioOrVideoTrack, typeToToggle) { On 2014/01/03 16:07:55, phoglund wrote: > I think these functions are probably over-generalized and could be simplified. > Again, next patch. Adding to my TODO list. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:665: gAutoAddLocalToPeerConnectionStreamWhenCalled = false; On 2014/01/03 16:07:55, phoglund wrote: > You missed this global variable? Good catch, also It's not used anymore (used by auto tests) however I will add a checkbox for this later so the behavor can be toggled. I've removed it for now as there is nothing using it. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:743: function registerEventBasedOnElementType_(element) { On 2014/01/03 16:07:55, phoglund wrote: > I don't understand these. Where did they come from? Did you write them now? If > so, move to a separate patch. Sorry, my eagerness have added extra functionality. Removing it all and adding it as a separate patch.
Just some minor comments. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:420: /** TODO Fix param definitions I rather meant to fix the TODO. In this case, remove the @param descriptions since they are incorrect. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:835: function disconnect_() { You forgot to move the check if we're already disconnected into this function. So if our peer id is null, just return rather than reporting an error. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:1348: if (isDisconnected_()) Not sure why you removed this? https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:60: //registerElementForEvents_(); Remove the line rather than commenting it out. Or at the very least add a TODO describing why it is commented out
https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:420: /** TODO Fix param definitions On 2014/01/07 09:52:25, phoglund wrote: > I rather meant to fix the TODO. In this case, remove the @param descriptions > since they are incorrect. Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:835: function disconnect_() { On 2014/01/07 09:52:25, phoglund wrote: > You forgot to move the check if we're already disconnected into this function. > So if our peer id is null, just return rather than reporting an error. Done. https://codereview.chromium.org/110853006/diff/50001/chrome/test/data/webrtc/... chrome/test/data/webrtc/manual/peerconnection.js:1348: if (isDisconnected_()) Not sure, what I was thinking, added it back. https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:60: //registerElementForEvents_(); On 2014/01/07 09:52:25, phoglund wrote: > Remove the line rather than commenting it out. Or at the very least add a TODO > describing why it is commented out Done.
lgtm with nits. I suggest a follow up CL to cleanup some of the style violations and additional refactoring. https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (left): https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:210: function toggleHelp() { This was unused already, right? https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:14: /** TODO give it a better name TODO's should have an owner like this: TODO(jansson) https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:52: hookupDataChannelCallbacks_(); Wow! It's excellent that so much of the old stuff in window.onload could be removed. https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:859: function getSourcesFromField_(audio_select, video_select) { Please correct function parameter variable names so they follow the convention: variableNamesLikeThis http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... https://codereview.chromium.org/110853006/diff/200001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/200001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:865: function getSourcesFromField_(audio_select, video_select) { Please correct the function parameter variable names that have underscores to follow the convention: variableNamesLikeThis Or to that in a separate CL: your choice. http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... https://codereview.chromium.org/110853006/diff/200001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:1220: function parseOurPeerId_(responseText) { I don't know why this is called ourPeerId and not localPeerId. Can you fix in a future CL?
https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (left): https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:210: function toggleHelp() { On 2014/01/07 10:34:17, Henrik Kjellander wrote: > This was unused already, right? Yes therefore I removed it. For the future I've added a TODO to my list. I want to create a general help module which contains all help info and use the hover event on elements to display info. Also make this toggable via a checkbox. https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:14: /** TODO give it a better name On 2014/01/07 10:34:17, Henrik Kjellander wrote: > TODO's should have an owner like this: TODO(jansson) Done. https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:52: hookupDataChannelCallbacks_(); On 2014/01/07 10:34:17, Henrik Kjellander wrote: > Wow! It's excellent that so much of the old stuff in window.onload could be > removed. Indeed, there might be more after thorough re-factoring. https://codereview.chromium.org/110853006/diff/110001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:859: function getSourcesFromField_(audio_select, video_select) { On 2014/01/07 10:34:17, Henrik Kjellander wrote: > Please correct function parameter variable names so they follow the convention: > variableNamesLikeThis > > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... Done.
Forgot to answer a comment. https://codereview.chromium.org/110853006/diff/200001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/200001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:1220: function parseOurPeerId_(responseText) { On 2014/01/07 10:34:17, Henrik Kjellander wrote: > I don't know why this is called ourPeerId and not localPeerId. Can you fix in a > future CL? Adding to my todo!
lgtm https://codereview.chromium.org/110853006/diff/260001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/260001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:859: * as HTMLPptionsCollection children. Nit: HTMLOptions
https://codereview.chromium.org/110853006/diff/260001/chrome/test/data/webrtc... File chrome/test/data/webrtc/manual/peerconnection.js (right): https://codereview.chromium.org/110853006/diff/260001/chrome/test/data/webrtc... chrome/test/data/webrtc/manual/peerconnection.js:859: * as HTMLPptionsCollection children. On 2014/01/09 10:01:51, phoglund wrote: > Nit: HTMLOptions Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jansson@chromium.org/110853006/330001
Message was sent while issue was closed.
Change committed as 243839 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
