Bug 77074 - runner.js in performance tests should define a class
Summary: runner.js in performance tests should define a class
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
Depends on:
Blocks: 77037 77401 77469
  Show dependency treegraph
Reported: 2012-01-26 00:41 PST by Ryosuke Niwa
Modified: 2012-01-31 13:09 PST (History)
7 users (show)

See Also:

Wrap functions in PerfTestRunner (23.75 KB, patch)
2012-01-31 00:24 PST, Ryosuke Niwa
eric: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>