[webkit-reviews] review granted: [Bug 170513] Update to Speedometer 2.0 w/updated frameworks + new workloads : [Attachment 310057] Patch

Mon May 15 16:09:03 PDT 2017

Ryosuke Niwa <rniwa at webkit.org> has granted Addy Osmani <addyo at chromium.org>'s
request for review:
Bug 170513: Update to Speedometer 2.0 w/updated frameworks + new workloads
https://bugs.webkit.org/show_bug.cgi?id=170513

Attachment 310057: Patch

https://bugs.webkit.org/attachment.cgi?id=310057&action=review

--- Comment #33 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 310057
  --> https://bugs.webkit.org/attachment.cgi?id=310057
Patch

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

> PerformanceTests/ChangeLog:8
> +	   Speedometer: refresh test runner and fix apps to work with it

I don't think "Speedometer: " prefix is needed. Also please end the sentence
with a period.

> PerformanceTests/ChangeLog:11
> +	   (parseQueryString): Adds support for startAutomatically param

Nit: query parameter* and missing a period.

> PerformanceTests/ChangeLog:18
> +	   (base): enable Ember TodoMVC to be run from any directory/level

Nit: Capitalize "enable" and add a period as well.

> PerformanceTests/ChangeLog:31
> +	   (director): Add missing director.js dependency for jQuery TodoMVC
implementation

(~) is used for a function. Here, you're talking about adding a new directory
so your comment should appear next to the first file being added.
Since it's a long time, you can wrap the line after the first sentence
and align your second line of comment to "* " at the beginning of each file as
in:
	* Speedometer/~/: This is some comment.
	More comment here.

> PerformanceTests/ChangeLog:33
> +	   (path): Fix path to built Elm TodoMVC scripts (build -> dist)

You're not fixing a function named path so this should appear after : in the
previous line.
You can wrap the line at a reasonable length, of course.

> PerformanceTests/Speedometer/InteractiveRunner.html:90
> +    if (suiteName === undefined || suiteName === '')
> +	   return;

Why are we checking this twice in its call site and here?

> PerformanceTests/Speedometer/resources/tests.js:56
> +		   newTodo.value = 'Something to do' + i;

I think we should have a space after "do" otherwise, the text would look like
"Something to do5".

> PerformanceTests/Speedometer/resources/tests.js:86
> +		   newTodo.value = 'Something to do' + i;