Add spaceport benchmark to Chrome Remote Control.

Add spaceport benchmark to Chrome Remote Control.

BUG=124811
TEST=Manual on linux

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=166223

[nduca, 2012-11-04 19:53:29]

My big thing here is why not switch to a wpr recording of spaceport? That seems
the correct thing to do here.

My second question, not opinion yet, is the following: is spaceport a benchmark,
or a "run_benchmark" interaction on a page with a special
"get_results_expression"?

https://codereview.chromium.org/11314012/diff/3001/chrome/test/data/third_par...
File chrome/test/data/third_party/spaceport/js/tests/sprites.js (left):

https://codereview.chromium.org/11314012/diff/3001/chrome/test/data/third_par...
chrome/test/data/third_party/spaceport/js/tests/sprites.js:171: var
estimatedMaxObjects = Math.floor(1/(targetFramerate * timePerObjectEstimate));
should we be editing the README.chromium to notate this edit?

https://codereview.chromium.org/11314012/diff/3001/chrome/test/data/third_par...
chrome/test/data/third_party/spaceport/js/tests/sprites.js:172: 
shouldn't we switch from having a real checkout of spaceport to using a wpr
recording? I have this feeling we're doing it wrong if not...

https://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_contro...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(left):

https://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:88:
trace = measurement + (trace_tag or '')
what is this '.'? Can we make this less hacky feeling? Or is there something
obvious I'm not getting?

https://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_contro...
File tools/chrome_remote_control/chrome_remote_control/page.py (left):

https://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/page.py:9: parsed_url =
urlparse.urlparse(url)
Test coverage

https://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_contro...
File tools/chrome_remote_control/chrome_remote_control/page_set.py (left):

https://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/page_set.py:33: url =
page_attributes.pop('url')
These changes feel significant. They should have unit test coverage for whatever
motivated them.

https://codereview.chromium.org/11314012/diff/3001/tools/perf/perf_tools/spac...
File tools/perf/perf_tools/spaceport.py (right):

https://codereview.chromium.org/11314012/diff/3001/tools/perf/perf_tools/spac...
tools/perf/perf_tools/spaceport.py:8: class
Spaceport(multi_page_benchmark.MultiPageBenchmark):
SpaceportBenchmark

https://codereview.chromium.org/11314012/diff/3001/tools/perf/perf_tools/spac...
tools/perf/perf_tools/spaceport.py:22: 
how are we protecting ourselves from running this against google.com? The test
should fail in that case.

https://codereview.chromium.org/11314012/diff/3001/tools/perf/perf_tools/spac...
tools/perf/perf_tools/spaceport.py:23: js_get_results =
'JSON.stringify(window.__results)'
why put the js here rather than just inside the evaluate call where we use this
var?

[dtu, 2012-11-05 19:48:53]

https://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_contro...
File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right):

https://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/page_runner.py:37: def
_ContainsOnlyFileURLs(page_set):
Put into class PageSet.

[tonyg, 2012-11-05 23:02:01]

http://codereview.chromium.org/11314012/diff/3001/chrome/test/data/third_part...
File chrome/test/data/third_party/spaceport/js/tests/sprites.js (left):

http://codereview.chromium.org/11314012/diff/3001/chrome/test/data/third_part...
chrome/test/data/third_party/spaceport/js/tests/sprites.js:171: var
estimatedMaxObjects = Math.floor(1/(targetFramerate * timePerObjectEstimate));
On 2012/11/04 19:53:30, nduca wrote:
> should we be editing the README.chromium to notate this edit?

Done.

http://codereview.chromium.org/11314012/diff/3001/chrome/test/data/third_part...
chrome/test/data/third_party/spaceport/js/tests/sprites.js:172: 
On 2012/11/04 19:53:30, nduca wrote:
> shouldn't we switch from having a real checkout of spaceport to using a wpr
> recording? I have this feeling we're doing it wrong if not...

There are a few reasons it is checked in:
1. Spaceport isn't hosted anywhere online to record from.
2. There are a couple of local modifications which are easier to track this way
than in a recording.
3. Currently android is using this checked in version downstream (but that can
change).

http://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(left):

http://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:88:
trace = measurement + (trace_tag or '')
On 2012/11/04 19:53:30, nduca wrote:
> what is this '.'? Can we make this less hacky feeling? Or is there something
> obvious I'm not getting?
The perfbots support a chart and a trace as the "key" for each result. So I
cleaned this up by adding an optional, explicit chart_name to Add().

http://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/page.py (left):

http://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/page.py:9: parsed_url =
urlparse.urlparse(url)
On 2012/11/04 19:53:30, nduca wrote:
> Test coverage

Done.

http://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right):

http://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/page_runner.py:37: def
_ContainsOnlyFileURLs(page_set):
On 2012/11/05 19:48:53, Dave Tu wrote:
> Put into class PageSet.

Done.

http://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/page_set.py (left):

http://codereview.chromium.org/11314012/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/page_set.py:33: url =
page_attributes.pop('url')
On 2012/11/04 19:53:30, nduca wrote:
> These changes feel significant. They should have unit test coverage for
whatever
> motivated them.

Done.

http://codereview.chromium.org/11314012/diff/3001/tools/perf/perf_tools/space...
File tools/perf/perf_tools/spaceport.py (right):

http://codereview.chromium.org/11314012/diff/3001/tools/perf/perf_tools/space...
tools/perf/perf_tools/spaceport.py:8: class
Spaceport(multi_page_benchmark.MultiPageBenchmark):
On 2012/11/04 19:53:30, nduca wrote:
> SpaceportBenchmark

Done.

http://codereview.chromium.org/11314012/diff/3001/tools/perf/perf_tools/space...
tools/perf/perf_tools/spaceport.py:22: 
On 2012/11/04 19:53:30, nduca wrote:
> how are we protecting ourselves from running this against google.com? The test
> should fail in that case.
Rather than start the test with ?auto, I added a button click. So if the
'#start-performance-tests' button doesn't exist on the page, it will fail.

http://codereview.chromium.org/11314012/diff/3001/tools/perf/perf_tools/space...
tools/perf/perf_tools/spaceport.py:23: js_get_results =
'JSON.stringify(window.__results)'
On 2012/11/04 19:53:30, nduca wrote:
> why put the js here rather than just inside the evaluate call where we use
this
> var?
Because it is used twice.

[nduca, 2012-11-06 08:30:26]

LGTM, thanks for all the clarifications! Can we file a bug to the effect of
"make spaceport benchmark work without the svn-internal?" Maybe we can get a
partner to do the work for us...

[tonyg, 2012-11-06 16:52:43]

On Tue, Nov 6, 2012 at 12:30 AM, <nduca@chromium.org> wrote:

> LGTM, thanks for all the clarifications! Can we file a bug to the effect of
> "make spaceport benchmark work without the svn-internal?" Maybe we can get
> a
> partner to do the work for us...
>

It does work without svn-internal. Since the benchmark is open source, the
static files are checked into src, not src-internal.


>
>
http://codereview.chromium.**org/11314012/<http://codereview.chromium.org/113...
>

[nduca, 2012-11-06 16:54:41]

Ohhhh Im retarded. I'm sorry!!!!

[dtu, 2012-11-06 18:28:03]

lgtm