Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7273089:
Display active audio streams on chrome://media-internals. (Closed)
|
Created:
9 years, 5 months ago by Scott Franklin Modified:
9 years, 5 months ago Reviewers:
commit-bot: I haz the power, arv (Not doing code reviews), Evan Stade, scherkus (not reviewing) CC:
chromium-reviews, arv (Not doing code reviews) Visibility:
Public. |
DescriptionDisplay active audio streams on chrome://media-internals.
BUG=none
TEST=navigate to chrome://media-internals and open an audio stream
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92873
Patch Set 1 #
Total comments: 10
Patch Set 2 : Restructuring. #
Total comments: 5
Patch Set 3 : Removing print hooks and inheritance. #
Total comments: 19
Patch Set 4 : Namespacing, jsdoc, etc. #Patch Set 5 : Fixing bug and removing double quotes. #
Total comments: 10
Patch Set 6 : Nitpicking, reordering methods. #
Total comments: 12
Patch Set 7 : Removing now extraneous members of namespace interface #Patch Set 8 : Final(?) nits. #
Total comments: 2
Patch Set 9 : Removing silliness. #
Messages
Total messages: 17 (0 generated)
I _think_ this follows js and html style...
nits I'm not a javascript app or anything so I'm not sure of the best way to structure programs something I've done before is to set up something MVC-ish where in this case: - Your model is a javascript object that handles the chrome.send() stuff and lives in its own .js file - Your view is HTML+CSS, naturally - Your controller are one-line statements hooking up onclick/input events to the model + passing in callbacks to the model in general I get wary of .js files referring to DOM nodes and stuff since it has a tendency ending up as spaghetti code, but again I'm only a webapp hobbyist :) anyway just something to think about when adding more code http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... File chrome/browser/resources/media_internals.js (right): http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:18: // Returns an English description of the status of |stream|. nit: s/an English/a text/ ? http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:21: ' at ' + Math.round(stream.volume*100) + '% volume.'; nit: spacing on * http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:26: if(!stream) { nit: spacing http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:27: stream=document.createElement('li'); nit: spacing on = http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:50: if(stream) nit: spacing
Probably a good idea to add some structure to it. I think this should extend well to encapsulate lots of different types of data, but I can't even claim the title of webapp hobbyist. http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... File chrome/browser/resources/media_internals.js (right): http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:18: // Returns an English description of the status of |stream|. On 2011/07/01 17:47:42, scherkus wrote: > nit: s/an English/a text/ ? Done. http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:21: ' at ' + Math.round(stream.volume*100) + '% volume.'; On 2011/07/01 17:47:42, scherkus wrote: > nit: spacing on * Done. http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:26: if(!stream) { On 2011/07/01 17:47:42, scherkus wrote: > nit: spacing Line removed. http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:27: stream=document.createElement('li'); On 2011/07/01 17:47:42, scherkus wrote: > nit: spacing on = Line removed. http://codereview.chromium.org/7273089/diff/1/chrome/browser/resources/media_... chrome/browser/resources/media_internals.js:50: if(stream) On 2011/07/01 17:47:42, scherkus wrote: > nit: spacing Line removed. http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... File chrome/browser/resources/media_internals/item_store.js (right): http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:71: AudioStreamStore.prototype = new ItemStore(); I'm tempted to ditch the inheritance and do something along the lines of: var audioStreams = new ItemStore(); audioStreams.printItem = function(id) { ... }; audioStreams.print = function() { ... }; to get the single instance I need, but I have no idea what the official stance is on such practice.
nits also update this CL to include a reviewer other than arv if you want some extra JS advice http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... File chrome/browser/resources/media_internals/item_store.js (right): http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:71: AudioStreamStore.prototype = new ItemStore(); On 2011/07/02 00:57:20, Scott Franklin wrote: > I'm tempted to ditch the inheritance and do something along the lines of: > var audioStreams = new ItemStore(); > audioStreams.printItem = function(id) { ... }; > audioStreams.print = function() { ... }; > > to get the single instance I need, but I have no idea what the official stance > is on such practice. from what I've experienced/read JS + inheritance is somewhat of a sham and not worth pursuing (i.e., over time it ends in tears) I think I like your suggestion here a bit better, namely because if you think about this application there will be many ItemStores, but only one AudioStreamStore, so do you really need to bless AudioStreamStore w/ its own class 'n stuff? ALSO... couldn't we have standalone generateAudioHTML() functions inside media_internals.js that accept an ItemStore? I don't think ItemStore really needs to both itself w/ printing "hooks" then code would look like: var audioStreams = new ItemStore(); // setup event callbacks function generateAudioHTML() { // blah blah w/ audioStreams object } http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:26: case "audio_streams" : nit: remove extra space
+estade for JS expertise. Background: I'm looking to display data from incoming dictionaries (for now, as a simple list). This should easily extend to handle several sources of data that will be rendered in different ways. http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... File chrome/browser/resources/media_internals/item_store.js (right): http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:71: AudioStreamStore.prototype = new ItemStore(); On 2011/07/07 20:57:30, scherkus wrote: > On 2011/07/02 00:57:20, Scott Franklin wrote: > > I'm tempted to ditch the inheritance and do something along the lines of: > > var audioStreams = new ItemStore(); > > audioStreams.printItem = function(id) { ... }; > > audioStreams.print = function() { ... }; > > > > to get the single instance I need, but I have no idea what the official stance > > is on such practice. > > from what I've experienced/read JS + inheritance is somewhat of a sham and not > worth pursuing (i.e., over time it ends in tears) > > I think I like your suggestion here a bit better, namely because if you think > about this application there will be many ItemStores, but only one > AudioStreamStore, so do you really need to bless AudioStreamStore w/ its own > class 'n stuff? > > ALSO... couldn't we have standalone generateAudioHTML() functions inside > media_internals.js that accept an ItemStore? > > I don't think ItemStore really needs to both itself w/ printing "hooks" > > then code would look like: > > var audioStreams = new ItemStore(); > // setup event callbacks > > function generateAudioHTML() { > // blah blah w/ audioStreams object > } Yeah, I think I'll ditch the inheritance altogether. It doesn't appear to be too spaghetti-like this way. http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/5001/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:26: case "audio_streams" : On 2011/07/07 20:57:30, scherkus wrote: > nit: remove extra space Done.
http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals.html (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals.html:12: <script type="text/javascript" src="media_internals/media_internals.js"></script> 80 http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals/item_store.js (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:4: namespace the stuff in this file via cr.define http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:12: ItemStore.prototype.ids = function() { ItemStore.prototype = { ids: function() { }, addItem: function(item) { }, ... } http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:36: ItemStore.prototype.map = function(mapper) { jsdoc style documentation for functions and their params http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:4: namespace this file as well http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:13: var out = '<li id=' + stream.id; easier to read if you do this all via js out = document.createElement('li'); out.id = stream.id; out.className = ... etc. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:14: out += ' class="audio-stream-' + stream.status + '">'; I think it makes more sense to set the status as an attribute, you can still apply CSS with .audio-stream[status='error']
http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals.html (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals.html:12: <script type="text/javascript" src="media_internals/media_internals.js"></script> On 2011/07/11 21:31:52, Evan Stade wrote: > 80 Unfortunately, grit's resource flattening regexen are very fragile, so breaking the line prevents the js from being inlined properly. It looks like everyone else circumvents this by ditching the type= attribute, which works, I guess. Done. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals/item_store.js (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:4: On 2011/07/11 21:31:52, Evan Stade wrote: > namespace the stuff in this file via cr.define Somehow I missed that everything else in here was doing that... Done. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:12: ItemStore.prototype.ids = function() { On 2011/07/11 21:31:52, Evan Stade wrote: > ItemStore.prototype = { > ids: function() { > > }, > > addItem: function(item) { > > }, > > ... > > } Oh, handy. Done. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/item_store.js:36: ItemStore.prototype.map = function(mapper) { On 2011/07/11 21:31:52, Evan Stade wrote: > jsdoc style documentation for functions and their params Done. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:4: On 2011/07/11 21:31:52, Evan Stade wrote: > namespace this file as well Done. Should the on* functions called directly by the WebUI go in here as well? If so, I'll prefix the 'media.' to them in the C++. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:13: var out = '<li id=' + stream.id; On 2011/07/11 21:31:52, Evan Stade wrote: > easier to read if you do this all via js > > out = document.createElement('li'); > out.id = stream.id; > out.className = ... > > etc. Unfortunately this required more fiddling with the DOM later on (unless I want to serialize each one to html and back). But you're right, it is more readable. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:14: out += ' class="audio-stream-' + stream.status + '">'; On 2011/07/11 21:31:52, Evan Stade wrote: > I think it makes more sense to set the status as an attribute, you can still > apply CSS with > > .audio-stream[status='error'] Good idea. Done.
I don't have anything else to add here, but that attribute suggestion is neat! http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:14: out += ' class="audio-stream-' + stream.status + '">'; On 2011/07/12 22:35:13, Scott Franklin wrote: > On 2011/07/11 21:31:52, Evan Stade wrote: > > I think it makes more sense to set the status as an attribute, you can still > > apply CSS with > > > > .audio-stream[status='error'] > > Good idea. Done. Cool!
Any comments, estade? Also, removed some double quotes and put member initialization back in the ctor, since putting it in the prototype shares it across all instances.
mostly nits http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:4: On 2011/07/12 22:35:13, Scott Franklin wrote: > On 2011/07/11 21:31:52, Evan Stade wrote: > > namespace this file as well > > Done. > Should the on* functions called directly by the WebUI go in here as well? If so, > I'll prefix the 'media.' to them in the C++. yes, that would be choice http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:13: var out = '<li id=' + stream.id; On 2011/07/12 22:35:13, Scott Franklin wrote: > On 2011/07/11 21:31:52, Evan Stade wrote: > > easier to read if you do this all via js > > > > out = document.createElement('li'); > > out.id = stream.id; > > out.className = ... > > > > etc. > > Unfortunately this required more fiddling with the DOM later on (unless I want > to serialize each one to html and back). But you're right, it is more readable. sorry, what do you mean? http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... File chrome/browser/resources/media_internals/item_store.js (right): http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/item_store.js:42: * don't need blank line between description and params (do we do that somewhere else?) http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/item_store.js:64: * @return {Array} An array of mapped items. extra space http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/item_store.js:76: don't need this blank line http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... File chrome/browser/resources/media_internals/media_internals.css (right): http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.css:1: /* /* Copyright (c) 2011 The Chromium Authors. All rights reserved. * Use of this source code is governed by a BSD-style license that can be * found in the LICENSE file. */ http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.css:7: .audio-stream[status = 'created'] { no spaces around = (I don't know why)
Looks like it now needs approval from scherkus@ for chrome/browser/media as well. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:4: On 2011/07/15 22:31:05, Evan Stade wrote: > On 2011/07/12 22:35:13, Scott Franklin wrote: > > On 2011/07/11 21:31:52, Evan Stade wrote: > > > namespace this file as well > > > > Done. > > Should the on* functions called directly by the WebUI go in here as well? If > so, > > I'll prefix the 'media.' to them in the C++. > > yes, that would be choice Done. http://codereview.chromium.org/7273089/diff/9002/chrome/browser/resources/med... chrome/browser/resources/media_internals/media_internals.js:13: var out = '<li id=' + stream.id; On 2011/07/15 22:31:05, Evan Stade wrote: > On 2011/07/12 22:35:13, Scott Franklin wrote: > > On 2011/07/11 21:31:52, Evan Stade wrote: > > > easier to read if you do this all via js > > > > > > out = document.createElement('li'); > > > out.id = stream.id; > > > out.className = ... > > > > > > etc. > > > > Unfortunately this required more fiddling with the DOM later on (unless I want > > to serialize each one to html and back). But you're right, it is more > readable. > > sorry, what do you mean? Just that the following lines also had to turn into DOM manipulation (add/remove children) instead of strings; which is fine, just an interesting side effect. http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... File chrome/browser/resources/media_internals/item_store.js (right): http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/item_store.js:42: * On 2011/07/15 22:31:05, Evan Stade wrote: > don't need blank line between description and params (do we do that somewhere > else?) On a few with really long descriptions. It just felt more readable with the spacing. Done. http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/item_store.js:64: * @return {Array} An array of mapped items. On 2011/07/15 22:31:05, Evan Stade wrote: > extra space Done. http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/item_store.js:76: On 2011/07/15 22:31:05, Evan Stade wrote: > don't need this blank line Done. http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... File chrome/browser/resources/media_internals/media_internals.css (right): http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.css:1: /* On 2011/07/15 22:31:05, Evan Stade wrote: > /* Copyright (c) 2011 The Chromium Authors. All rights reserved. > * Use of this source code is governed by a BSD-style license that can be > * found in the LICENSE file. > */ Done. http://codereview.chromium.org/7273089/diff/21001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.css:7: .audio-stream[status = 'created'] { On 2011/07/15 22:31:05, Evan Stade wrote: > no spaces around = (I don't know why) Done.
LGTM
lgtm http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:8: var audioStreamDiv; document these vars http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:42: audioStreams.map(printStream).forEach(function(s) { out.appendChild(s) }); I believe the proper spacing is audioStreams.map(printStream).forEach(function(s) { out.appendChild(s); }); http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:44: if (audioStreamDiv.firstChild) audioStreamDiv.textContent = ''; http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:54: onAudioUpdate = function(stream) { I think addAudioStream is a more appropriate name for this function. http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:61: * Add it all to the appropriate stores and refresh. I don't understand 'refresh' in this context. Do you really reload the page? I suspect it's an extraneous word. http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:62: * remove
http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:8: var audioStreamDiv; On 2011/07/15 23:38:58, Evan Stade wrote: > document these vars Done. http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:42: audioStreams.map(printStream).forEach(function(s) { out.appendChild(s) }); On 2011/07/15 23:38:58, Evan Stade wrote: > I believe the proper spacing is > > audioStreams.map(printStream).forEach(function(s) { > out.appendChild(s); > }); Done. http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:44: if (audioStreamDiv.firstChild) On 2011/07/15 23:38:58, Evan Stade wrote: > audioStreamDiv.textContent = ''; Done, both in the if and below. http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:54: onAudioUpdate = function(stream) { On 2011/07/15 23:38:58, Evan Stade wrote: > I think addAudioStream is a more appropriate name for this function. Done. http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:61: * Add it all to the appropriate stores and refresh. On 2011/07/15 23:38:58, Evan Stade wrote: > I don't understand 'refresh' in this context. Do you really reload the page? I > suspect it's an extraneous word. It was an attempt to describe the operation of writing the updated audio stream data to the page. http://codereview.chromium.org/7273089/diff/26001/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:62: * On 2011/07/15 23:38:58, Evan Stade wrote: > remove Done.
http://codereview.chromium.org/7273089/diff/28007/chrome/browser/resources/me... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/28007/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:49: audioStreamDiv.textContent = ''; the if is not necessary
Thanks for that. I'm not entirely sure what I was thinking there. I think I'll take at least one more long look through this before trying to commit. http://codereview.chromium.org/7273089/diff/28007/chrome/browser/resources/me... File chrome/browser/resources/media_internals/media_internals.js (right): http://codereview.chromium.org/7273089/diff/28007/chrome/browser/resources/me... chrome/browser/resources/media_internals/media_internals.js:49: audioStreamDiv.textContent = ''; On 2011/07/16 00:34:43, Evan Stade wrote: > the if is not necessary Huh... Did not think that one through very clearly...
Change committed as 92873 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
