Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9960063:
CNS seek tests for <video>. (Closed)
|
Created:
8 years, 8 months ago by shadi Modified:
8 years, 8 months ago Reviewers:
DaleCurtis CC:
chromium-reviews, dennis_jeffrey, feature-media-reviews_chromium.org, anantha, dyu1, Nirnimesh Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCNS seek tests for <video>.
We record seek performance using the product of:
- Video formats: webm and ogv.
- Network constraints: cable, wifi, and no constraints.
- Seek cases: long, short, and buffered seeks.
- Video location: cached and un-cached videos.
BUG=122749
TEST=manual run
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133585
Patch Set 1 : #
Total comments: 22
Patch Set 2 : Re-write. #
Total comments: 32
Patch Set 3 : New tests with cached videos. #
Total comments: 30
Patch Set 4 : Re comments. #
Total comments: 5
Patch Set 5 : Running version. #Patch Set 6 : #Patch Set 7 : Full video test list. #
Total comments: 4
Patch Set 8 : Value vs index #
Messages
Total messages: 15 (0 generated)
Dale, Can you PTAL? The set of test files is not final yet.
Needs quite a bit of refactoring to isolate common code from media_constrained_network_perf. In general the concept looks sound though. https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... File chrome/test/data/media/html/benchmark.js (right): https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:1: Header w/ details on where this is from? Will this code reusable by other tests? If not can we keep it inside the HTML? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:4: return src; Dead code? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:12: new Date().getTime(); Why? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:21: this.times_.push((new Date().getTime()) - this.start_); Extra parens seem unnecessary? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:22: // console.log(this.times_[this.times_.length - 1]); Comment? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:27: for (var i in this.times_) { In w/ arrays can get messy. http://stackoverflow.com/questions/500504/javascript-for-in-with-arrays https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:43: this.timer_ = new Timer; Timer() ? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:66: this.error = function() { done_cb(false, "Error loading media"); } s/"/'/ https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/benchmark.js:97: testLongSeek: function(iterations, done_cb) { Both these functions seem do exactly the same thing minus the difference in Seek time. Extract them out into a common method parameterized with seek time. https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... File chrome/test/data/media/html/media_seek.html (right): https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/media_seek.html:1: <!-- Used by media_constrained_network_perf to record perf metrics. --> Name? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/media_seek.html:11: <div id="l"></div> Choose a better ID here. https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/media_seek.html:16: var l = document.getElementById("l"); Does .querySelector('div') not work? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/media_seek.html:34: log('Starting seeking tests on ' + src); Benchmark seems to already log this? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/media_seek.html:36: log('Short seek (cached): ' + result); Why are these labeled cached/un-cached? I don't see you setting any headers or anything for this. https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/data/med... chrome/test/data/media/html/media_seek.html:56: console.log(text); if (console && console.log) ? https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/function... File chrome/test/functional/media/media_seek_perf.py (right): https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/function... chrome/test/functional/media/media_seek_perf.py:26: # Number of threads to use during testing. Lots of duplicated defines here. Should refactor against media_constrained_network_perf. https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/function... chrome/test/functional/media/media_seek_perf.py:47: class TestWorker(threading.Thread): This worker code should probably be refactored/abstracted out a bit if multiple tests are going to use it. Even more awesome would be building a general framework in PyAuto for threaded tab testing. https://chromiumcodereview.appspot.com/9960063/diff/2001/chrome/test/function... chrome/test/functional/media/media_seek_perf.py:175: # Spin up worker threads. No dummy test necessary anymore?
I re-wrote the seek tests based on the cns_base and re-wrote media_seek.html as well. Added cached_after_seek to tests. Preliminary results are interesting. PTAL http://codereview.chromium.org/9960063/diff/2001/chrome/test/data/media/html/... File chrome/test/data/media/html/media_seek.html (right): http://codereview.chromium.org/9960063/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:1: <!-- Used by media_constrained_network_perf to record perf metrics. --> On 2012/04/10 23:00:06, DaleCurtis wrote: > Name? Done. http://codereview.chromium.org/9960063/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:11: <div id="l"></div> On 2012/04/10 23:00:06, DaleCurtis wrote: > Choose a better ID here. Done. http://codereview.chromium.org/9960063/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:16: var l = document.getElementById("l"); On 2012/04/10 23:00:06, DaleCurtis wrote: > Does .querySelector('div') not work? It does since this is the only div. http://codereview.chromium.org/9960063/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:34: log('Starting seeking tests on ' + src); On 2012/04/10 23:00:06, DaleCurtis wrote: > Benchmark seems to already log this? Done. http://codereview.chromium.org/9960063/diff/8001/media/tools/constrained_netw... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/9960063/diff/8001/media/tools/constrained_netw... media/tools/constrained_network_server/cns.py:210: I do not use this yet since <video> caches anyway (supposedly).
Looking good! Should write a more descriptive commit message too. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... File chrome/test/data/media/html/media_seek.html (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:10: <video height=350 controls></video> Why set height? http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:11: <div id="log"></div> Don't need the id anymore since you're using querySelector. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:44: if (iteration < 3) { Set iterations in startTest or in a const at the top. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:46: seek_turn = 0; seek_turn is a bit opaque, might be clearer as seek_test_state or something similar with a javascript "enum"... var SeekTestState = { kShortSeek: 0, kLongSeek: 1, kCachedSeek: 2, ... } switch(seek_test_state) { case SeekTestState.kShortSeek: ... case ... } http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:50: else { Else goes on previous line. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:52: log (cached_seeks); s/log (/log(/ here and elsewhere. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:54: log (cached_after_seek); Instead of having cached, uncached, cached_after_seek. Maybe a nested dict indexed by SeekTestState? seeks[SeekTestState.kShortSeek] = [] ? ... http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:73: seek_turn++; Set seek_turn prior to timer.start() here and below. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:32: this.start_ = 0; Can this just be this.reset()? http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:54: function generateSrc(src) { Better name? GenerateUniqueURL? http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:30: os.path.join('crowd', name) for name in Not sure about this indenting, but if pylint doesn't complain okay. http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:69: retry_sleep=1, timeout=500, debug=False) Big timeout? http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:86: graph_name = series_name +'_' + os.path.basename(file_name) s/+'/+ '/ Gpylint :) http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:103: Two lines.
I still need to select the ogg/ogv files for the tests. Can you please review? http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... File chrome/test/data/media/html/media_seek.html (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:10: <video height=350 controls></video> On 2012/04/18 18:57:53, DaleCurtis wrote: > Why set height? I had it while running the tests manually. Took it off now. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:11: <div id="log"></div> On 2012/04/18 18:57:53, DaleCurtis wrote: > Don't need the id anymore since you're using querySelector. Done. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:44: if (iteration < 3) { On 2012/04/18 18:57:53, DaleCurtis wrote: > Set iterations in startTest or in a const at the top. Done. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:46: seek_turn = 0; On 2012/04/18 18:57:53, DaleCurtis wrote: > seek_turn is a bit opaque, might be clearer as seek_test_state or something > similar with a javascript "enum"... > > var SeekTestState = { > kShortSeek: 0, > kLongSeek: 1, > kCachedSeek: 2, > ... > } > > switch(seek_test_state) { > case SeekTestState.kShortSeek: > ... > case ... > } Done. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:50: else { On 2012/04/18 18:57:53, DaleCurtis wrote: > Else goes on previous line. Done. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:52: log (cached_seeks); On 2012/04/18 18:57:53, DaleCurtis wrote: > s/log (/log(/ here and elsewhere. Done. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:54: log (cached_after_seek); On 2012/04/18 18:57:53, DaleCurtis wrote: > Instead of having cached, uncached, cached_after_seek. Maybe a nested dict > indexed by SeekTestState? > > seeks[SeekTestState.kShortSeek] = [] ? > ... Done. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/media_seek.html:73: seek_turn++; On 2012/04/18 18:57:53, DaleCurtis wrote: > Set seek_turn prior to timer.start() here and below. Done. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:32: this.start_ = 0; On 2012/04/18 18:57:53, DaleCurtis wrote: > Can this just be this.reset()? No, since reset is not defined at this point. http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:54: function generateSrc(src) { On 2012/04/18 18:57:53, DaleCurtis wrote: > Better name? GenerateUniqueURL? Done. http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:30: os.path.join('crowd', name) for name in On 2012/04/18 18:57:53, DaleCurtis wrote: > Not sure about this indenting, but if pylint doesn't complain okay. gpylint does not complain. :) http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:69: retry_sleep=1, timeout=500, debug=False) On 2012/04/18 18:57:53, DaleCurtis wrote: > Big timeout? On slow connections, I got seeks in > 25 secs. So counting all iterations they can add up but not to 500sec :). I decreased it to 3 minutes. http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:86: graph_name = series_name +'_' + os.path.basename(file_name) On 2012/04/18 18:57:53, DaleCurtis wrote: > s/+'/+ '/ > > Gpylint :) I do run gpylint, but it is not catching this, nor the 2 lines comment below. http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:103: On 2012/04/18 18:57:53, DaleCurtis wrote: > Two lines. Done.
http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:32: this.start_ = 0; On 2012/04/19 04:16:59, shadi wrote: > On 2012/04/18 18:57:53, DaleCurtis wrote: > > Can this just be this.reset()? > No, since reset is not defined at this point. Said no, but code shows you changed it? http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/functional/media... chrome/test/functional/media/media_seek_perf.py:69: retry_sleep=1, timeout=500, debug=False) On 2012/04/19 04:16:59, shadi wrote: > On 2012/04/18 18:57:53, DaleCurtis wrote: > > Big timeout? > On slow connections, I got seeks in > 25 secs. So counting all iterations they > can add up but not to 500sec :). I decreased it to 3 minutes. Add a comment about the timeout then. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... File chrome/test/data/media/html/media_seek.html (right): http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:10: <div></div> Will having the div above the video cause it to move down as the log messages come in? Should be below or set a fixed height + scroll. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:26: UN_CACHED: 0, s/_// http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:35: seekRecords = [2]; Why [2]? Wouldn't the whole function be simpler/more generic as: seekRecords = [] for (cache_key in CachedState) seekRecords[cache_key] = [] for (seek_key in SeekTestCase) seekRecords[cache_key][seek_key] = [] http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:53: cacheState = CachedState.UN_CACHED; Should bundle this with the log message below so if they are changed they are changed together. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:68: log('Test iteration ' + iteration); s/'/"/g http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:73: log("Starting seek tests with browser caching:"); Indent? http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:84: return GenerateUniqueURL(originalSrc); Indent. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:100: // first short seek Remove comment or elaborate, SeekTestCase.SHORT_SEEK is pretty clear :) http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:108: // Seek to almost end of file (uncached) s/(uncached)/./ http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:116: // Second seek to a buffered aread. Remove or elaborate. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:119: log("cached seek seek in " + delta + "ms.") s/cached/buffered/ ? http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/utils.js:52: //this.start_ = 0; Comments? http://codereview.chromium.org/9960063/diff/12001/chrome/test/functional/medi... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/12001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:43: Seek_Test_Case = { No_Naming_Please. :) SEEK_TEST_CASES and CACHED_STATE. Ideally you should just pull these from the JavaScript, but that might be tricky. http://codereview.chromium.org/9960063/diff/12001/media/tools/constrained_net... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/9960063/diff/12001/media/tools/constrained_net... media/tools/constrained_network_server/cns.py:256: # Build constrained URL. Only pass on the file parameter. Update comment.
Dale can you please take a look? http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9960063/diff/8001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:32: this.start_ = 0; On 2012/04/19 19:42:10, DaleCurtis wrote: > On 2012/04/19 04:16:59, shadi wrote: > > On 2012/04/18 18:57:53, DaleCurtis wrote: > > > Can this just be this.reset()? > > No, since reset is not defined at this point. > > Said no, but code shows you changed it? Actually I don't know how this got uploaded. This will not work. I reverted the changes. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... File chrome/test/data/media/html/media_seek.html (right): http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:10: <div></div> On 2012/04/19 19:42:10, DaleCurtis wrote: > Will having the div above the video cause it to move down as the log messages > come in? Should be below or set a fixed height + scroll. Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:26: UN_CACHED: 0, On 2012/04/19 19:42:10, DaleCurtis wrote: > s/_// Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:35: seekRecords = [2]; On 2012/04/19 19:42:10, DaleCurtis wrote: > Why [2]? Wouldn't the whole function be simpler/more generic as: > > seekRecords = [] > for (cache_key in CachedState) > seekRecords[cache_key] = [] > for (seek_key in SeekTestCase) > seekRecords[cache_key][seek_key] = [] Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:53: cacheState = CachedState.UN_CACHED; On 2012/04/19 19:42:10, DaleCurtis wrote: > Should bundle this with the log message below so if they are changed they are > changed together. Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:68: log('Test iteration ' + iteration); On 2012/04/19 19:42:10, DaleCurtis wrote: > s/'/"/g Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:73: log("Starting seek tests with browser caching:"); On 2012/04/19 19:42:10, DaleCurtis wrote: > Indent? Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:84: return GenerateUniqueURL(originalSrc); On 2012/04/19 19:42:10, DaleCurtis wrote: > Indent. Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:100: // first short seek On 2012/04/19 19:42:10, DaleCurtis wrote: > Remove comment or elaborate, SeekTestCase.SHORT_SEEK is pretty clear :) Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:108: // Seek to almost end of file (uncached) On 2012/04/19 19:42:10, DaleCurtis wrote: > s/(uncached)/./ Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:116: // Second seek to a buffered aread. On 2012/04/19 19:42:10, DaleCurtis wrote: > Remove or elaborate. Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/media_seek.html:119: log("cached seek seek in " + delta + "ms.") On 2012/04/19 19:42:10, DaleCurtis wrote: > s/cached/buffered/ ? Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9960063/diff/12001/chrome/test/data/media/html... chrome/test/data/media/html/utils.js:52: //this.start_ = 0; On 2012/04/19 19:42:10, DaleCurtis wrote: > Comments? Done. http://codereview.chromium.org/9960063/diff/12001/chrome/test/functional/medi... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/12001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:43: Seek_Test_Case = { On 2012/04/19 19:42:10, DaleCurtis wrote: > No_Naming_Please. :) SEEK_TEST_CASES and CACHED_STATE. Ideally you should just > pull these from the JavaScript, but that might be tricky. It is tricky! Plus the names here are used for graph names. So, it is good to have them here in case we want to change it without affecting the test file. http://codereview.chromium.org/9960063/diff/12001/media/tools/constrained_net... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/9960063/diff/12001/media/tools/constrained_net... media/tools/constrained_network_server/cns.py:256: # Build constrained URL. Only pass on the file parameter. On 2012/04/19 19:42:10, DaleCurtis wrote: > Update comment. Done.
http://codereview.chromium.org/9960063/diff/12001/chrome/test/functional/medi... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/12001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:43: Seek_Test_Case = { On 2012/04/20 00:17:47, shadi wrote: > On 2012/04/19 19:42:10, DaleCurtis wrote: > > No_Naming_Please. :) SEEK_TEST_CASES and CACHED_STATE. Ideally you should just > > pull these from the JavaScript, but that might be tricky. > It is tricky! Plus the names here are used for graph names. So, it is good to > have them here in case we want to change it without affecting the test file. How about instead of constants you have the following: seek_test_cases = self.GetDOMValue("keys(SeekTestCase).join(',')").split(',') cached_states = <same> The rest of the code would stay the same. This fixes your keys/values issue in the javascript as well. http://codereview.chromium.org/9960063/diff/24001/chrome/test/functional/medi... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/24001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:33: os.path.join('dartmoor', 'dartmoor.ogg')] ? http://codereview.chromium.org/9960063/diff/24001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:73: # Time out is dependent on (seeking time * iterations). For 3 iterations per Style in the file is 2 spaces after a period sadly :( http://codereview.chromium.org/9960063/diff/24001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:75: # cached seeks to be fast. Thus an average of 10secs per seek is chosen. "Thus..." doesn't really follow from what you've said, "Through experimentation an average of 10 secs per seek was found to be adequate."
http://codereview.chromium.org/9960063/diff/12001/chrome/test/functional/medi... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/12001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:43: Seek_Test_Case = { On 2012/04/20 22:37:06, DaleCurtis wrote: > On 2012/04/20 00:17:47, shadi wrote: > > On 2012/04/19 19:42:10, DaleCurtis wrote: > > > No_Naming_Please. :) SEEK_TEST_CASES and CACHED_STATE. Ideally you should > just > > > pull these from the JavaScript, but that might be tricky. > > It is tricky! Plus the names here are used for graph names. So, it is good to > > have them here in case we want to change it without affecting the test file. > > How about instead of constants you have the following: > > seek_test_cases = self.GetDOMValue("keys(SeekTestCase).join(',')").split(',') > > cached_states = <same> > > The rest of the code would stay the same. This fixes your keys/values issue in > the javascript as well. I think I got what you are saying, but how can the rest of the code stay the same? http://codereview.chromium.org/9960063/diff/24001/chrome/test/functional/medi... File chrome/test/functional/media/media_seek_perf.py (right): http://codereview.chromium.org/9960063/diff/24001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:33: os.path.join('dartmoor', 'dartmoor.ogg')] On 2012/04/20 22:37:06, DaleCurtis wrote: > ? Done. http://codereview.chromium.org/9960063/diff/24001/chrome/test/functional/medi... chrome/test/functional/media/media_seek_perf.py:75: # cached seeks to be fast. Thus an average of 10secs per seek is chosen. On 2012/04/20 22:37:06, DaleCurtis wrote: > "Thus..." doesn't really follow from what you've said, > > "Through experimentation an average of 10 secs per seek was found to be > adequate." Done.
I added additional ogv files to the test list. Can you please take a look?
LGTM % some strange naming. https://chromiumcodereview.appspot.com/9960063/diff/35001/chrome/test/data/me... File chrome/test/data/media/html/media_seek.html (right): https://chromiumcodereview.appspot.com/9960063/diff/35001/chrome/test/data/me... chrome/test/data/media/html/media_seek.html:36: for (cache_value in Object.keys(CachedState)) { Why the change to cache_value? Aren't we reading the keys? https://chromiumcodereview.appspot.com/9960063/diff/35001/chrome/test/data/me... chrome/test/data/media/html/media_seek.html:38: for (seek_value in Object.keys(SeekTestCase)) { Ditto.
Any comments? https://chromiumcodereview.appspot.com/9960063/diff/35001/chrome/test/data/me... File chrome/test/data/media/html/media_seek.html (right): https://chromiumcodereview.appspot.com/9960063/diff/35001/chrome/test/data/me... chrome/test/data/media/html/media_seek.html:36: for (cache_value in Object.keys(CachedState)) { On 2012/04/23 22:56:20, DaleCurtis wrote: > Why the change to cache_value? Aren't we reading the keys? Well, Object.keys(CachedState) is ["UNCACHED", "CACHED"] and cache_value in for (cache_value in Object.keys(CachedState)) is [0, 1]. I changed the name to *_index to make it clearer. https://chromiumcodereview.appspot.com/9960063/diff/35001/chrome/test/data/me... chrome/test/data/media/html/media_seek.html:38: for (seek_value in Object.keys(SeekTestCase)) { On 2012/04/23 22:56:20, DaleCurtis wrote: > Ditto. Done.
Nope, fine with me.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shadi@chromium.org/9960063/21004
Change committed as 133585 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
