Issue 1515006: Playback benchmark scripts.

Issue 1515006: Playback benchmark scripts. (Closed)

Created:
10 years, 8 months ago by podivilov

Modified:
9 years, 3 months ago

Reviewers:
Vitaly Repeshko

CC:
chromium-reviews

Base URL:
http://src.chromium.org/svn/trunk/src/

Visibility:
Public.

Patch Set 6 : '' #

Created: 10 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+932 lines, -0 lines)			Patch
A	tools/playback_benchmark/common.js	View	1 2 3 4 5	1 chunk	+318 lines, -0 lines	0 comments	Download
A	tools/playback_benchmark/playback.js	View	1 2 3 4 5	1 chunk	+256 lines, -0 lines	0 comments	Download
A	tools/playback_benchmark/playback_driver.py	View	1 2 3 4 5	1 chunk	+196 lines, -0 lines	0 comments	Download
A	tools/playback_benchmark/proxy_handler.py	View	1 2 3 4 5	1 chunk	+123 lines, -0 lines	0 comments	Download
A	tools/playback_benchmark/run.py	View	1 2 3 4	1 chunk	+39 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Vitaly Repeshko

I only looked at the python parts so far. http://codereview.chromium.org/1515006/diff/14001/15004 File tools/playback_benchmark/playback_driver.py (right): http://codereview.chromium.org/1515006/diff/14001/15004#newcode11 tools/playback_benchmark/playback_driver.py:11: ...

10 years, 8 months ago (2010-04-10 02:24:38 UTC) #2

podivilov

Thanks for review, Vitaly! http://codereview.chromium.org/1515006/diff/14001/15004 File tools/playback_benchmark/playback_driver.py (right): http://codereview.chromium.org/1515006/diff/14001/15004#newcode11 tools/playback_benchmark/playback_driver.py:11: from sys import stdout, stderr ...

10 years, 8 months ago (2010-04-12 11:44:24 UTC) #3

Thanks for review, Vitaly!

http://codereview.chromium.org/1515006/diff/14001/15004
File tools/playback_benchmark/playback_driver.py (right):

http://codereview.chromium.org/1515006/diff/14001/15004#newcode11
tools/playback_benchmark/playback_driver.py:11: from sys import stdout, stderr
On 2010/04/10 02:24:38, Vitaly wrote:
> Style rule: Use from x import y only when x is a package and y is a module.

Done.

http://codereview.chromium.org/1515006/diff/14001/15004#newcode16
tools/playback_benchmark/playback_driver.py:16: window.open("%s", "", "width=%i,
height=%i");
On 2010/04/10 02:24:38, Vitaly wrote:
> Hint: you can use %(url)s, %(width)i, %(height)i to avoid dependency on
> placeholder order.

Done.

http://codereview.chromium.org/1515006/diff/14001/15004#newcode21
tools/playback_benchmark/playback_driver.py:21: class PlaybackDriver(object):
Yes, it's actually not a driver, it's just a handler. Renamed to
PlaybackRequestHandler.
On 2010/04/10 02:24:38, Vitaly wrote:
> I'm not sure "Driver" is the best name for this class because drivers usually
> control the process and objects of this class only respond to requests. Maybe
> it's "PlaybackHandler"?

http://codereview.chromium.org/1515006/diff/14001/15004#newcode25
tools/playback_benchmark/playback_driver.py:25: "Loads test cache."
On 2010/04/10 02:24:38, Vitaly wrote:
> Document "test_callback".

Done.

http://codereview.chromium.org/1515006/diff/14001/15004#newcode28
tools/playback_benchmark/playback_driver.py:28: f = open(os.path.join(test_dir,
'cache.json'), 'r')
On 2010/04/10 02:24:38, Vitaly wrote:
> Consider using a helper ReadFile function instead of repeated open, read,
close
> calls.

Done.

http://codereview.chromium.org/1515006/diff/14001/15004#newcode37
tools/playback_benchmark/playback_driver.py:37: self.requests = {}
On 2010/04/10 02:24:38, Vitaly wrote:
> Probably this field should be named "cache" and the variable above
"cache_data".

Done.

http://codereview.chromium.org/1515006/diff/14001/15004#newcode44
tools/playback_benchmark/playback_driver.py:44: key = (request['command'],
request['path'])
The source of confusion was BaseHTTPServer module, where 'command' stands for
'method'...
On 2010/04/10 02:24:38, Vitaly wrote:
> What does "command" stand for? HTTP method? If yes, why not name it "method"?

http://codereview.chromium.org/1515006/diff/14001/15004#newcode50
tools/playback_benchmark/playback_driver.py:50:
self.benchmark_resources['data.js'] = 'Benchmark.data = %s' % f.read()
On 2010/04/10 02:24:38, Vitaly wrote:
> Extract data.js template for consistency with START_PAGE. 

Done.

http://codereview.chromium.org/1515006/diff/14001/15004#newcode53
tools/playback_benchmark/playback_driver.py:53: f = open(resource)
On 2010/04/10 02:24:38, Vitaly wrote:
> This requires these scripts to be in the current directory. Maybe add a flag
> ("script-dir") with the default set to cwd?

Done.

http://codereview.chromium.org/1515006/diff/14001/15003
File tools/playback_benchmark/proxy_handler.py (right):

http://codereview.chromium.org/1515006/diff/14001/15003#newcode16
tools/playback_benchmark/proxy_handler.py:16: from BaseHTTPServer import
BaseHTTPRequestHandler, HTTPServer
On 2010/04/10 02:24:38, Vitaly wrote:
> See style rule on imports above.

Done.

http://codereview.chromium.org/1515006/diff/14001/15003#newcode22
tools/playback_benchmark/proxy_handler.py:22: class
RequestHandler(BaseHTTPRequestHandler):
I'd prefer to parse HTTPS requests here in ProxyHandler, to make HTTPS requests
handling similar to plain HTTP requests handling (from driver's point of view).
Otherwise, we'll have to parse it in recording driver as well.
On 2010/04/10 02:24:38, Vitaly wrote:
> Merge this with the driver class?

http://codereview.chromium.org/1515006/diff/14001/15003#newcode77
tools/playback_benchmark/proxy_handler.py:77: # Create SLL context.
On 2010/04/10 02:24:38, Vitaly wrote:
> Typo: SLL -> SSL.

Done.

http://codereview.chromium.org/1515006/diff/14001/15005
File tools/playback_benchmark/run.py (right):

http://codereview.chromium.org/1515006/diff/14001/15005#newcode12
tools/playback_benchmark/run.py:12: playback.py -c <path_to_cache>
On 2010/04/10 02:24:38, Vitaly wrote:
> This needs to be updated.

Done.

Vitaly Repeshko

LGTM http://codereview.chromium.org/1515006/diff/20001/21002 File tools/playback_benchmark/common.js (right): http://codereview.chromium.org/1515006/diff/20001/21002#newcode23 tools/playback_benchmark/common.js:23: window.setTimeout = function(callback, timeout) { Instead of having ...

10 years, 7 months ago (2010-05-12 13:41:50 UTC) #4

podivilov

10 years, 7 months ago (2010-05-19 13:31:08 UTC) #5

http://codereview.chromium.org/1515006/diff/20001/21002
File tools/playback_benchmark/common.js (right):

http://codereview.chromium.org/1515006/diff/20001/21002#newcode23
tools/playback_benchmark/common.js:23: window.setTimeout = function(callback,
timeout) {
On 2010/05/12 13:41:50, Vitaly wrote:
> Instead of having all overrides in top-level code consider moving them to a
> function installOverrides(window). This function can also be used when setting
> up frames.

Done.

http://codereview.chromium.org/1515006/diff/20001/21002#newcode41
tools/playback_benchmark/common.js:41: 
On 2010/05/12 13:41:50, Vitaly wrote:
> At least, log something here.

Done.

http://codereview.chromium.org/1515006/diff/20001/21002#newcode48
tools/playback_benchmark/common.js:48: window.addEventListener = function(type,
listener, useCapture) {
On 2010/05/12 13:41:50, Vitaly wrote:
> I think we need remove listeners methods as well. Otherwise, we might keep
> reporting events to listeners that the app thinks it has removed.

Done.

http://codereview.chromium.org/1515006/diff/20001/21002#newcode49
tools/playback_benchmark/common.js:49: if (type == 'mousemove' || type ==
'mouseover' || type == 'mouseout') return;
On 2010/05/12 13:41:50, Vitaly wrote:
> Extract the list of ignored events.

Done.

http://codereview.chromium.org/1515006/diff/20001/21002#newcode112
tools/playback_benchmark/common.js:112: window.__defineSetter__('onload',
function() {});
On 2010/05/12 13:41:50, Vitaly wrote:
> Should this setter function log a warning?

Done.

http://codereview.chromium.org/1515006/diff/20001/21002#newcode115
tools/playback_benchmark/common.js:115:
Benchmark.originals.addEventListenerToWindow.call(window, 'DOMContentLoaded',
Good point! I've added dynamic frames as well.

http://codereview.chromium.org/1515006/diff/20001/21002#newcode256
tools/playback_benchmark/common.js:256: try { (0)() } catch(ex) {
console.error(ex.stack); }
Unfortunately it does not work in chrome, so let's keep it for now.

http://codereview.chromium.org/1515006/diff/20001/21001
File tools/playback_benchmark/playback.js (right):

http://codereview.chromium.org/1515006/diff/20001/21001#newcode125
tools/playback_benchmark/playback.js:125:
Benchmark.Agent.prototype.readyToExecuteScript = function(scriptId, doc,
inlined, src) {
On 2010/05/12 13:41:50, Vitaly wrote:
> nit: long line.

Done.

http://codereview.chromium.org/1515006/diff/20001/21001#newcode248
tools/playback_benchmark/playback.js:248:
Benchmark.originals.addEventListenerToWindow.call(window, 'message',
function(event) {
On 2010/05/12 13:41:50, Vitaly wrote:
> nit: long line.

Done.

http://codereview.chromium.org/1515006/diff/20001/21004
File tools/playback_benchmark/playback_driver.py (right):

http://codereview.chromium.org/1515006/diff/20001/21004#newcode35
tools/playback_benchmark/playback_driver.py:35: } {
On 2010/05/12 13:41:50, Vitaly wrote:
> Huh? Missing else?

Done.

Expand Messages | Collapse Messages