Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1065763002:
Add data reduction proxy integration tests for video compression. (Closed)
|
Created:
5 years, 8 months ago by Tom Bergan Modified:
5 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd data reduction proxy integration tests for video compression.
BUG=474397
Committed: https://crrev.com/43ec6aed0e8e9b44c451657f80c9cc7002a5e3fd
Cr-Commit-Position: refs/heads/master@{#330856}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Fixes for reviewer comments. #
Total comments: 30
Patch Set 3 : Fix reviewer comments, round #2 #
Total comments: 16
Patch Set 4 : Merge with HEAD and address reviewer comments. #
Total comments: 2
Patch Set 5 : Reviewer comments, plus fix a compile error #
Total comments: 1
Patch Set 6 : Reviewer comment: removed unused class ChromeProxyLatency #
Messages
Total messages: 19 (4 generated)
tombergan@chromium.org changed reviewers: + bengr@chromium.org, bolian@chromium.org
bengr@chromium.org changed reviewers: + sclittle@chromium.org
https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:621: if found: Can we not have restriction of only one video at this level? There can be a expected number of video results to verify against, but we should not restrict tests to have only one video all the time. https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/videowrapper.js (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:57: this.start_ = 0; delete this line? https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:67: // Return delta time since start in millisecs. this is in seconds but with precision to ms? https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:73: if (window.performance) For chrome only, we don't this complexity?
https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:621: if found: On 2015/04/10 21:00:03, bolian wrote: > Can we not have restriction of only one video at this level? > There can be a expected number of video results to verify against, but we should > not restrict tests to have only one video all the time. This is a good feature for Dan to add :-) https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/04/10 21:00:03, bolian wrote: > 2015 Done. https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/videowrapper.js (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/04/10 21:00:03, bolian wrote: > 2015 Done. https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:57: this.start_ = 0; On 2015/04/10 21:00:03, bolian wrote: > delete this line? I think uninitialized fields default to null? See line 68 https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:67: // Return delta time since start in millisecs. On 2015/04/10 21:00:03, bolian wrote: > this is in seconds but with precision to ms? window.performance.now returns a double in ms. This math rounds to the nearest .001ms. Copied this from media.js, happy to change it if you like. https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:73: if (window.performance) On 2015/04/10 21:00:03, bolian wrote: > For chrome only, we don't this complexity? I don't know why all this code is here. I copied from media.js (see comments at the top). I think all the Chromes that we care about support window.performance.now, but I don't know. Scott, do you know?
lgtm https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/videowrapper.js (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:57: this.start_ = 0; I mean this.start() will initialize it, so we don't need this. https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:67: // Return delta time since start in millisecs. ok. lg as is. https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:73: if (window.performance) Probably they copy from general js library to support multiple browsers. I think we should just use preformance.now.
https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/videowrapper.js (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:73: if (window.performance) On 2015/04/10 21:46:29, bolian wrote: > Probably they copy from general js library to support multiple browsers. I think > we should just use preformance.now. Either way seems fine to me, but I'd recommend keeping it as-is; it's more consistent with media.js from which it was copied. Chrome has supported window.performance.now to some degree since M24 or so, so we shouldn't have any problems there. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:10: from telemetry.core.backends.chrome import cros_browser_finder alphabetize https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:260: class ChromeProxyVideoCompare(benchmark.Benchmark): What is this benchmark for? Please add a classdoc here https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:36: class DirectFetchLatency(ChromeProxyLatency): Maybe name this ChromeProxyLatencyDirect to be consistent with the benchmark and the other measurements in this file. Same with DirectFetchDataSaving below. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:413: """Video correctness measurements.""" Please add more detailed description. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:419: self._allMetrics = {} Please add a comment describing what _allMetrics is https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:434: # Verify this page. nit: the code in this method looks dense, could you add blank lines where appropriate to make it easier to read? https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:447: def _CompareProxiedAndDirectMetrics(self, url, pm, dm): What is this method for? Please add a comment https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:456: for x in ('video_height', 'video_width', 'video_duration', Add a comment describing the purpose of this for loop; i.e. is it to verify that all the stats are present? https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:564: 2 blank lines https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:566: """A Chrome proxy video metric.""" Could you add a more detailed description of the class here? https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:581: # Now wait for the video to stop playing. nit: this code seems kinda dense, could you add some blank lines to separate major blocks? https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:599: return self._AddResultsShared('proxied', tab, results) This 'proxied' string is used in a bunch of places, could it be a constant? Same with 'direct'. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:623: found = True nit: again, code seems kinda dense; adding some blank lines where appropriate would make this easier to read. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:648: results.AddValue(scalar.ScalarValue(results.current_page, k, "", v)) Could you just call results.AddValue(...) inline above instead of building up a dict of results? https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py:38: for url in urls_list: Could you just have VideoDirectPageSet, etc. call the appropriate AddUserStory, instead of having this for loop, and remove 'mode'?
https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/videowrapper.js (right): https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:57: this.start_ = 0; On 2015/04/10 21:46:29, bolian wrote: > I mean this.start() will initialize it, so we don't need this. Done. https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:67: // Return delta time since start in millisecs. On 2015/04/10 21:46:29, bolian wrote: > ok. lg as is. Acknowledged. https://codereview.chromium.org/1065763002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/videowrapper.js:73: if (window.performance) On 2015/04/10 23:01:49, sclittle wrote: > On 2015/04/10 21:46:29, bolian wrote: > > Probably they copy from general js library to support multiple browsers. I > think > > we should just use preformance.now. > > Either way seems fine to me, but I'd recommend keeping it as-is; it's more > consistent with media.js from which it was copied. Chrome has supported > window.performance.now to some degree since M24 or so, so we shouldn't have any > problems there. Acknowledged. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:10: from telemetry.core.backends.chrome import cros_browser_finder On 2015/04/10 23:01:49, sclittle wrote: > alphabetize Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:260: class ChromeProxyVideoCompare(benchmark.Benchmark): On 2015/04/10 23:01:49, sclittle wrote: > What is this benchmark for? Please add a classdoc here Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:36: class DirectFetchLatency(ChromeProxyLatency): On 2015/04/10 23:01:49, sclittle wrote: > Maybe name this ChromeProxyLatencyDirect to be consistent with the benchmark and > the other measurements in this file. > > Same with DirectFetchDataSaving below. Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:413: """Video correctness measurements.""" On 2015/04/10 23:01:49, sclittle wrote: > Please add more detailed description. Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:419: self._allMetrics = {} On 2015/04/10 23:01:49, sclittle wrote: > Please add a comment describing what _allMetrics is Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:434: # Verify this page. On 2015/04/10 23:01:49, sclittle wrote: > nit: the code in this method looks dense, could you add blank lines where > appropriate to make it easier to read? Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:447: def _CompareProxiedAndDirectMetrics(self, url, pm, dm): On 2015/04/10 23:01:49, sclittle wrote: > What is this method for? Please add a comment Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:456: for x in ('video_height', 'video_width', 'video_duration', On 2015/04/10 23:01:49, sclittle wrote: > Add a comment describing the purpose of this for loop; i.e. is it to verify that > all the stats are present? Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:564: On 2015/04/10 23:01:49, sclittle wrote: > 2 blank lines Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:581: # Now wait for the video to stop playing. On 2015/04/10 23:01:50, sclittle wrote: > nit: this code seems kinda dense, could you add some blank lines to separate > major blocks? Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:599: return self._AddResultsShared('proxied', tab, results) On 2015/04/10 23:01:49, sclittle wrote: > This 'proxied' string is used in a bunch of places, could it be a constant? Same > with 'direct'. Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py:38: for url in urls_list: On 2015/04/10 23:01:50, sclittle wrote: > Could you just have VideoDirectPageSet, etc. call the appropriate AddUserStory, > instead of having this for loop, and remove 'mode'? Done.
https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:261: """ Runs the ChromeProxyVideoDirect and ChromeProxyVideoProxied benchmarks and See style guide for python classdoc style: https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme... https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:412: PROXIED = 'proxied' I think you can just call metrics.PROXIED to use the constant from metrics.py. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:416: """Measures pages using metrics.ChromeProxyVideoMetric, then compares See style guide for python classdoc style: https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme... https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:458: """ Compares video metrics computed by videowrapper.js for PROXIED and See style guide for method doc style: https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme... https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:570: """Wraps the video metrics produced by videowrapper.js. Also checks a few See style guide: https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme... https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:658: for (k,v) in self.videoMetrics.iteritems(): Could you just call results.AddValue(...) inline above instead of building up a dict of results? https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py (right): https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py:14: Otherwise, fetches are sent directly to the origin. nit: Indent this line 2 more spaces. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py:23: """ Base class for Chrome proxy video tests. """ nit: Remove spaces between the """s and the text in the middle. Same below.
Before I submit, I need to move the test page from aws1.mdw.la to check.googlezip.net. I'll ping when that's done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:566: """A Chrome proxy video metric.""" On 2015/04/10 23:01:50, sclittle wrote: > Could you add a more detailed description of the class here? Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:623: found = True On 2015/04/10 23:01:50, sclittle wrote: > nit: again, code seems kinda dense; adding some blank lines where appropriate > would make this easier to read. Done. https://codereview.chromium.org/1065763002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:648: results.AddValue(scalar.ScalarValue(results.current_page, k, "", v)) On 2015/04/10 23:01:49, sclittle wrote: > Could you just call results.AddValue(...) inline above instead of building up a > dict of results? This dict is used by ChromeProxyVideoValidation (in chrome_proxy_measurements.py) when comparing proxied and direct fetches. I could just call results.AddValue instead of making this dict, but then it becomes harder to do the comparison in ChromeProxyVideoValidation because it's more cumbersome to extract values from results. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:261: """ Runs the ChromeProxyVideoDirect and ChromeProxyVideoProxied benchmarks and On 2015/04/11 00:13:02, sclittle wrote: > See style guide for python classdoc style: > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme... Done. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:412: PROXIED = 'proxied' On 2015/04/11 00:13:02, sclittle wrote: > I think you can just call metrics.PROXIED to use the constant from metrics.py. Done. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:416: """Measures pages using metrics.ChromeProxyVideoMetric, then compares On 2015/04/11 00:13:02, sclittle wrote: > See style guide for python classdoc style: > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme... Done. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:458: """ Compares video metrics computed by videowrapper.js for PROXIED and On 2015/04/11 00:13:02, sclittle wrote: > See style guide for method doc style: > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme... Done. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:570: """Wraps the video metrics produced by videowrapper.js. Also checks a few On 2015/04/11 00:13:02, sclittle wrote: > See style guide: > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme... Done. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:658: for (k,v) in self.videoMetrics.iteritems(): On 2015/04/11 00:13:02, sclittle wrote: > Could you just call results.AddValue(...) inline above instead of building up a > dict of results? This dict is used by ChromeProxyVideoValidation (in chrome_proxy_measurements.py) when comparing proxied and direct fetches. I could just call results.AddValue instead of making this dict, but then it becomes harder to do the comparison in ChromeProxyVideoValidation because it's more cumbersome to extract values from results. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py (right): https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py:14: Otherwise, fetches are sent directly to the origin. On 2015/04/11 00:13:02, sclittle wrote: > nit: Indent this line 2 more spaces. Done. https://codereview.chromium.org/1065763002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/video.py:23: """ Base class for Chrome proxy video tests. """ On 2015/04/11 00:13:02, sclittle wrote: > nit: Remove spaces between the """s and the text in the middle. Same below. Done.
Looks good modulo nits, but you should rebase on HEAD first, since Richard restructured a bunch of the tests in https://codereview.chromium.org/1098253004/. https://codereview.chromium.org/1065763002/diff/60001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:593: Wraps the video metrics produced by videowrapper.js. Also checks a few Which metrics? You don't necessarily need to list them all here, just please provide some examples.
https://codereview.chromium.org/1065763002/diff/60001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/1065763002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:593: Wraps the video metrics produced by videowrapper.js. Also checks a few On 2015/05/12 18:05:40, sclittle wrote: > Which metrics? You don't necessarily need to list them all here, just please > provide some examples. Done.
LGTM % nit Thanks for doing this! https://codereview.chromium.org/1065763002/diff/80001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/1065763002/diff/80001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:36: class ChromeProxyLatencyDirect(ChromeProxyLatency): I don't think this measurement is used by any benchmarks, could you remove it?
re-LGTM
The CQ bit was checked by tombergan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bolian@chromium.org Link to the patchset: https://codereview.chromium.org/1065763002/#ps100001 (title: "Reviewer comment: removed unused class ChromeProxyLatency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1065763002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/43ec6aed0e8e9b44c451657f80c9cc7002a5e3fd Cr-Commit-Position: refs/heads/master@{#330856} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
