Bug 77074

Summary: runner.js in performance tests should define a class
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Tools / TestsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adamk, arv, eric, morrita, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77037, 77401, 77469    
Attachments:
Description Flags
Wrap functions in PerfTestRunner eric: review+, webkit.review.bot: commit-queue-

Description Ryosuke Niwa 2012-01-26 00:41:54 PST
Right now, runner.js defines a bunch of global functions in the global (window) scope. We should be defining a class instead.
Comment 1 Adam Barth 2012-01-26 00:50:27 PST
Yeah, definitely.  Go to town!
Comment 2 Ryosuke Niwa 2012-01-31 00:24:27 PST
Created attachment 124677 [details]
Wrap functions in PerfTestRunner
Comment 3 Eric Seidel (no email) 2012-01-31 01:55:50 PST
Comment on attachment 124677 [details]
Wrap functions in PerfTestRunner

My mindz!  They are the blownz.
Comment 4 WebKit Review Bot 2012-01-31 02:59:02 PST
Comment on attachment 124677 [details]
Wrap functions in PerfTestRunner

Rejecting attachment 124677 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ching file PerformanceTests/Parser/tiny-innerHTML.html
patching file PerformanceTests/Parser/url-parser.html
patching file PerformanceTests/Parser/xml-parser.html
patching file PerformanceTests/resources/runner.js
Hunk #1 FAILED at 1.
Hunk #2 succeeded at 99 (offset 11 lines).
1 out of 2 hunks FAILED -- saving rejects to file PerformanceTests/resources/runner.js.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1

Full output: http://queues.webkit.org/results/11365944
Comment 5 Ojan Vafai 2012-01-31 10:39:15 PST
Comment on attachment 124677 [details]
Wrap functions in PerfTestRunner

View in context: https://bugs.webkit.org/attachment.cgi?id=124677&action=review

Some random nits as I was looking through the code...

> PerformanceTests/resources/runner.js:2
> +var PerfTestRunner = { };

Nit: typical JS style is to not put a space between the curly braces here.

> PerformanceTests/resources/runner.js:91
> +    var start = new Date();

Nit: here and below I'd use Date.now(). Constructing a Date object is surprisingly expensive. Date.now() avoids the constructor and just gives the the current time.
Comment 6 Ryosuke Niwa 2012-01-31 12:04:58 PST
Thanks for the review, Eric and thanks for the suggestions, Ojan. Landing it now.
Comment 7 Ryosuke Niwa 2012-01-31 12:05:42 PST
Committed r106379: <http://trac.webkit.org/changeset/106379>