Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20804002:
Added the next series of files that go into media-internals. (Closed)
|
Created:
7 years, 5 months ago by Ty Overby Modified:
7 years, 4 months ago Reviewers:
scherkus (not reviewing) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded the next series of files that go into media-internals.
BUG=260005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214624
Patch Set 1 #Patch Set 2 : #
Total comments: 27
Patch Set 3 : Fixed style issues, slightly modified how the media singleton is tested" #
Total comments: 13
Patch Set 4 : new way to initialize media #Patch Set 5 : #Patch Set 6 : #
Total comments: 19
Patch Set 7 : #
Total comments: 11
Patch Set 8 : #Patch Set 9 : Wraped util in a module #
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... File content/browser/resources/media/new/goog.js (right): https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/goog.js:7: * @fileoverview A placeholder for google closure library functions. what are the plans for this placeholder w.r.t. google closure library functions? https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... File content/browser/resources/media/new/main.js (right): https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:20: new RenderManager : mockRenderManager); this is a very subtle way of injecting a mock that relies on *not* including RenderManager code prior to this code OOC, if this is used for the integration test should we be mocking out more functionality? what's stopping us from using the real RenderManager? https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:23: * Call to modify or add a system property periods on comments here + below https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:63: // Everything beyond this point is for backwards try to use more chars per-line (up to 80) if you can https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:88: 'playing', event.playing); indentation for any continued line gets either: 1) A 4-space indent if there's no preceeding parameter OR 2) Aligned to the opening ( if there are preceeding parameter check out style guide for some detailed examples: http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Code_fo... but basically, the following are all acceptable forms foo.onBar(arg1, arg2, arg3); foo.onBar(arg1, arg2, arg3); foo.onBar(arg1, arg2, arg3); foo.onBar( /* NOTE: no arg here! */ arg1, arg2, arg3); foo.onBar( /* NOTE: no arg here! */ arg1, arg2, arg3); foo.onBar( /* NOTE: no arg here! */ arg1, arg2, arg3); https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:108: event.renderer); ditto https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:110: event.player); ditto https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:116: key === 'buffer_current' || key === 'is_downloading_data') { same style rules apply here - this should be aligned at ( https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:127: event.type); ditto https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... File content/browser/resources/media/new/media_internals_integration_test.html (right): https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/media_internals_integration_test.html:1: <!DOCTYPE html> copyright https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/media_internals_integration_test.html:12: window.setUp = function () { remove spaces between function and ( here + below https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/media_internals_integration_test.html:28: name = event.renderer + ':' + event.player; just use a separate var statement here https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/media_internals_integration_test.html:47: window.media.onRendererTerminated(12); instead of documenting the magic number 12 (and 4), how about defining constants? example: var TEST_RENDERER = 12; var TEST_PLAYER = 4; https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... File content/browser/resources/media/new/player_manager.js (right): https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/player_manager.js:54: removeAllForDebug: function() { s/ForDebug/ForTest/ also ... remove all of what? your other functions names mention players -- I'd add it here as well (i.e., removeAllPlayersForTest) alternatively, since reusing objects between tests is frowned upon (leads to flaky / broken / hard to maintain tests) you may want to consider adding hooks to manually recreate the PlayerManager instance
Fixed the things that you mentioned, removed the ForTest method in PlayerManager and instead makes a new PlayerManager from inside |media|. New Patch Set that has these changes is ready for review. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... File content/browser/resources/media/new/goog.js (right): https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/goog.js:7: * @fileoverview A placeholder for google closure library functions. On 2013/07/29 20:35:54, scherkus wrote: > what are the plans for this placeholder w.r.t. google closure library functions? It's basically impossible to use the google closure library, so I'll probably rename the 'goog' namespace to 'util' or something. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... File content/browser/resources/media/new/main.js (right): https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:20: new RenderManager : mockRenderManager); On 2013/07/29 20:35:54, scherkus wrote: > what's stopping us from using the real RenderManager? Because RenderManager depends on every "yet to be added" file in the project, so I'd either have to remove 80% of the code in the file or mock it. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:23: * Call to modify or add a system property On 2013/07/29 20:35:54, scherkus wrote: > periods on comments here + below Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:63: // Everything beyond this point is for backwards On 2013/07/29 20:35:54, scherkus wrote: > try to use more chars per-line (up to 80) if you can Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:108: event.renderer); On 2013/07/29 20:35:54, scherkus wrote: > ditto Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:110: event.player); On 2013/07/29 20:35:54, scherkus wrote: > ditto Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:116: key === 'buffer_current' || key === 'is_downloading_data') { On 2013/07/29 20:35:54, scherkus wrote: > same style rules apply here - this should be aligned at ( Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/main.js:127: event.type); On 2013/07/29 20:35:54, scherkus wrote: > ditto Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... File content/browser/resources/media/new/media_internals_integration_test.html (right): https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/media_internals_integration_test.html:1: <!DOCTYPE html> On 2013/07/29 20:35:54, scherkus wrote: > copyright Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/media_internals_integration_test.html:12: window.setUp = function () { On 2013/07/29 20:35:54, scherkus wrote: > remove spaces between function and ( > > here + below Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/media_internals_integration_test.html:28: name = event.renderer + ':' + event.player; On 2013/07/29 20:35:54, scherkus wrote: > just use a separate var statement here Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/media_internals_integration_test.html:47: window.media.onRendererTerminated(12); On 2013/07/29 20:35:54, scherkus wrote: > instead of documenting the magic number 12 (and 4), how about defining > constants? > > example: > > var TEST_RENDERER = 12; > var TEST_PLAYER = 4; Done. https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... File content/browser/resources/media/new/player_manager.js (right): https://codereview.chromium.org/20804002/diff/3001/content/browser/resources/... content/browser/resources/media/new/player_manager.js:54: removeAllForDebug: function() { On 2013/07/29 20:35:54, scherkus wrote: > s/ForDebug/ForTest/ > > also ... remove all of what? your other functions names mention players -- I'd > add it here as well (i.e., removeAllPlayersForTest) > > > alternatively, since reusing objects between tests is frowned upon (leads to > flaky / broken / hard to maintain tests) you may want to consider adding hooks > to manually recreate the PlayerManager instance I like the idea of modifying the media singleton to make a new PlayerManager.
fewwwwwwwww more nits / suggestions https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... File content/browser/resources/media/new/goog.js (right): https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/goog.js:7: * @fileoverview A placeholder for google closure library functions. want to rename this file util.js and introduce helper functions within the media module / it's own module? side note: I suspect we'll have to switch to the cr.define(...) helper from cr.js at some point (can do that as a separate CL) https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/goog.js:23: goog.time.millisToString = function(timeMillis) { is this function used anywhere yet? if not, please remove until you have production code that uses it https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... File content/browser/resources/media/new/integration_test.html (right): https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/integration_test.html:19: window.pm = media.getManager(); s/pm/somethingWayMoreDescriptive/ for example ... playerManager https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/integration_test.html:22: // The renderer and player ids are completely arbitrarily periods! https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/integration_test.html:30: ticksMillis: 132, this block should be de-indented by two spaces the four space thing is for calling functions, if statements, etc... but declaring objects 'n arrays is still two spaces: http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Code_fo... https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... File content/browser/resources/media/new/main.js (right): https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/main.js:20: new RenderManager : mockRenderManager); OK ... what if we exposed something like an Initialize() / SetPlayerManager() method here, then: - Production code can call Initialize(new PlayerManager(new RenderManager)) - Test code inside of setUp() can call Initialize(new PlayerManager(mockRenderManager)) That way we have the mock/testing code where we need it (inside of integration_test.html) and replace the subtleness with explicitness. We can also remove getManager() as the test code will be the one creating the manager and it can keep its own reference to it. WDYT? https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/main.js:137: event.type); style not fixed here
https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... File content/browser/resources/media/new/goog.js (right): https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/goog.js:7: * @fileoverview A placeholder for google closure library functions. On 2013/07/29 22:16:28, scherkus wrote: > want to rename this file util.js and introduce helper functions within the media > module / it's own module? > > side note: I suspect we'll have to switch to the cr.define(...) helper from > cr.js at some point (can do that as a separate CL) Done. https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/goog.js:23: goog.time.millisToString = function(timeMillis) { On 2013/07/29 22:16:28, scherkus wrote: > is this function used anywhere yet? > > if not, please remove until you have production code that uses it Done. https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... File content/browser/resources/media/new/integration_test.html (right): https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/integration_test.html:19: window.pm = media.getManager(); On 2013/07/29 22:16:28, scherkus wrote: > s/pm/somethingWayMoreDescriptive/ > > for example ... playerManager Done. https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/integration_test.html:22: // The renderer and player ids are completely arbitrarily On 2013/07/29 22:16:28, scherkus wrote: > periods! Done. https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/integration_test.html:30: ticksMillis: 132, On 2013/07/29 22:16:28, scherkus wrote: > this block should be de-indented by two spaces > > the four space thing is for calling functions, if statements, etc... but > declaring objects 'n arrays is still two spaces: > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Code_fo... Done. https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... File content/browser/resources/media/new/main.js (right): https://codereview.chromium.org/20804002/diff/11001/content/browser/resources... content/browser/resources/media/new/main.js:20: new RenderManager : mockRenderManager); On 2013/07/29 22:16:28, scherkus wrote: > OK ... what if we exposed something like an Initialize() / SetPlayerManager() > method here, then: > - Production code can call Initialize(new PlayerManager(new RenderManager)) > - Test code inside of setUp() can call Initialize(new > PlayerManager(mockRenderManager)) > > That way we have the mock/testing code where we need it (inside of > integration_test.html) and replace the subtleness with explicitness. We can also > remove getManager() as the test code will be the one creating the manager and it > can keep its own reference to it. > > WDYT? Done.
https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... File content/browser/resources/media/new/integration_test.html (right): https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/integration_test.html:18: var doNothing = function (){}; no space before ( add a space after ) var doNothing = function() {}; https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/integration_test.html:79: assertEquals(event.playing, window.playerManager.players_[event.id].properties['playing']) fix 80 chars https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... File content/browser/resources/media/new/main.js (right): https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/main.js:11: var doNothing = function() { remove this -- not used https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/main.js:14: // Use must call media.initialize(); this comment would be better served as jsdoc on initialize() function e.g., "Clients must call initialize() prior to calling other methods blah blah blah" https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/main.js:20: remove extra blank line (1 blank line is always enough in terms of vertical whitespace) https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/main.js:63: getManager: function() { this doesn't appear to be used anymore -- remove https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... File content/browser/resources/media/new/util.js (right): https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:5: remove extra blank line https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:12: util.object = {}; nit: I'd drop the .object sub-module namespace https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:13: util.object.forEach = function(obj, f, optObj) { docs on functions https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:22: remove extra blank line
https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... File content/browser/resources/media/new/main.js (right): https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/main.js:11: var doNothing = function() { On 2013/07/29 23:09:23, scherkus wrote: > remove this -- not used Done. https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/main.js:14: // Use must call media.initialize(); On 2013/07/29 23:09:23, scherkus wrote: > this comment would be better served as jsdoc on initialize() function > > e.g., "Clients must call initialize() prior to calling other methods blah blah > blah" Done. https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/main.js:20: On 2013/07/29 23:09:23, scherkus wrote: > remove extra blank line (1 blank line is always enough in terms of vertical > whitespace) Done. https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/main.js:63: getManager: function() { On 2013/07/29 23:09:23, scherkus wrote: > this doesn't appear to be used anymore -- remove Done. https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... File content/browser/resources/media/new/util.js (right): https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:5: On 2013/07/29 23:09:23, scherkus wrote: > remove extra blank line Done. https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:12: util.object = {}; On 2013/07/29 23:09:23, scherkus wrote: > nit: I'd drop the .object sub-module namespace I disagree. Usually forEach is defined on arrays/lists, but this makes it explicitly obvious that it's being done on an object. Otherwise, people might think that arr.forEach(fn); and util.forEach(obj, fn); do the same thing if the variable names weren't so obvious. https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:13: util.object.forEach = function(obj, f, optObj) { On 2013/07/29 23:09:23, scherkus wrote: > docs on functions Done. https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:22: On 2013/07/29 23:09:23, scherkus wrote: > remove extra blank line Done.
https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... File content/browser/resources/media/new/util.js (right): https://codereview.chromium.org/20804002/diff/22001/content/browser/resources... content/browser/resources/media/new/util.js:12: util.object = {}; On 2013/07/29 23:26:56, Ty Overby wrote: > On 2013/07/29 23:09:23, scherkus wrote: > > nit: I'd drop the .object sub-module namespace > > I disagree. Usually forEach is defined on arrays/lists, but this makes it > explicitly obvious that it's being done on an object. Otherwise, people might > think that > > arr.forEach(fn); > and > util.forEach(obj, fn); > > do the same thing if the variable names weren't so obvious. SGTM https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... File content/browser/resources/media/new/integration_test.html (right): https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/integration_test.html:18: var doNothing = function(){}; still missing a space between ) and { https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/integration_test.html:78: var player = window.playerManager.players_[event.id]; if you're going to define a local variable to fix 80 chars, then you should be consistent with your usage of that variable for example, why aren't you using it on line 80? alternatively, ditch the local variable and drop the 2nd function of the assertEquals() to next line and indent it to match the indentation of event.playing https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... File content/browser/resources/media/new/main.js (right): https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/main.js:15: */ jsdoc for man argument? also man isn't terribly great name and falls under the category of ambiguous abbreviations http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... how about playerManager? https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... File content/browser/resources/media/new/util.js (right): https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/util.js:10: var util = {}; OOC is there a reason why this file doesn't use the module format found in main.js? e.g., var util = (function() { // ... }); we should try to be consistent https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/util.js:20: */ this comment is copied verbatim from Apache-licensed closure: http://docs.closure-library.googlecode.com/git/closure_goog_object_object.js.... please don't do that -- let's rewrite this comment with your own words
https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... File content/browser/resources/media/new/integration_test.html (right): https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/integration_test.html:18: var doNothing = function(){}; On 2013/07/29 23:55:18, scherkus wrote: > still missing a space between ) and { Done. https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/integration_test.html:78: var player = window.playerManager.players_[event.id]; On 2013/07/29 23:55:18, scherkus wrote: > if you're going to define a local variable to fix 80 chars, then you should be > consistent with your usage of that variable > > for example, why aren't you using it on line 80? > > > alternatively, ditch the local variable and drop the 2nd function of the > assertEquals() to next line and indent it to match the indentation of > event.playing Done. https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... File content/browser/resources/media/new/main.js (right): https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/main.js:15: */ On 2013/07/29 23:55:18, scherkus wrote: > jsdoc for man argument? > > also man isn't terribly great name and falls under the category of ambiguous > abbreviations > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... > > how about playerManager? Done. https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... File content/browser/resources/media/new/util.js (right): https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/util.js:10: var util = {}; On 2013/07/29 23:55:18, scherkus wrote: > OOC is there a reason why this file doesn't use the module format found in > main.js? > > e.g., > > var util = (function() { > // ... > }); > > we should try to be consistent Because it doesn't need to? The only reason that the module pattern is used is to hide local variables that would otherwise be global. If we had another 'var something = ....' that we didn't want to be defined on |window|, then it would make sense to use the module pattern. https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/util.js:20: */ On 2013/07/29 23:55:18, scherkus wrote: > this comment is copied verbatim from Apache-licensed closure: > http://docs.closure-library.googlecode.com/git/closure_goog_object_object.js.... > > please don't do that -- let's rewrite this comment with your own words The *code* was copied verbatim from the closure-closure library. That's the reason the object |util| used to be called |goog|. I just rewrote it a bit to fit with best practices to keep my IDE from showering me with warnings. That said, ok, I updated it.
lgtm! https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... File content/browser/resources/media/new/util.js (right): https://codereview.chromium.org/20804002/diff/30001/content/browser/resources... content/browser/resources/media/new/util.js:10: var util = {}; On 2013/07/30 00:13:34, Ty Overby wrote: > On 2013/07/29 23:55:18, scherkus wrote: > > OOC is there a reason why this file doesn't use the module format found in > > main.js? > > > > e.g., > > > > var util = (function() { > > // ... > > }); > > > > we should try to be consistent > > Because it doesn't need to? The only reason that the module pattern is used is > to hide local variables that would otherwise be global. > > If we had another 'var something = ....' that we didn't want to be defined on > |window|, then it would make sense to use the module pattern. Doesn't need to ... today ;) (I'm fine leaving as is, although my preference tips towards a consistent codebase so the next person who touches this code doesn't have to ask the same question [or worse, mistakenly introduce a global])
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/20804002/36001
Message was sent while issue was closed.
Change committed as 214624 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
