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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied 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 308636: Patch

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




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

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

> PerformanceTests/Speedometer/InteractiveRunner.html:78
> +// Parse query string parameters in case a specific 'suite' is specified
> +// Part of enabling InteractiveRunner.html?suite=React-TodoMVC support
> +// so that an individual test can be automatically run via a URL

In WebKit, we avoid adding comments these "what" comments.

> PerformanceTests/Speedometer/InteractiveRunner.html:79
> +var queryString = (function(a) {

Instad, we use a descriptive function name like "function parseQueryString()".

Also, please don't use a single letter variable like "a":
https://webkit.org/code-style-guidelines/#names-full-words
How about pairList?

Also, for any anonymous function, there should be a space between function and
().
See https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide

> PerformanceTests/Speedometer/InteractiveRunner.html:80
> +    if (a == '') return {};

How can this ever be true? I think we can remove this code.
If we need to keep this code, return should be on a separate line with an
indention as in:
if (~)
    return {};
https://webkit.org/code-style-guidelines/#linebreaking-multiple-statements

> PerformanceTests/Speedometer/InteractiveRunner.html:83
> +    for (var i = 0; i < a.length; ++i)
> +    {

Please place { after for followed by exactly one space as in: for (~) {.

> PerformanceTests/Speedometer/InteractiveRunner.html:84
> +	   var p=a[i].split('=', 2);

There should be a space around each assignment and comparison operator such as
=:
https://webkit.org/code-style-guidelines/#spacing-binary-ternary-op

So pairList[i].split('=', 2);
I'd call this variable pair or keyValue.

> PerformanceTests/Speedometer/InteractiveRunner.html:95
> +    if (suiteName !== undefined && suiteName !== '') {

We prefer early exits over wrapping in if so:
if (~)
  return;
Suites.forEach...

> PerformanceTests/Speedometer/InteractiveRunner.html:99
> +	       if (element.name !== suiteName) {
> +		   element.disabled = true;
> +	       }

No curly braces around a single statement:
https://webkit.org/code-style-guidelines/#braces-one-line

> PerformanceTests/Speedometer/InteractiveRunner.html:105
> +    var queryParam = queryString['suite'];

Why don't we call parseQueryString here and store the parsed query string in a
local variable instead of a global variable?

> PerformanceTests/Speedometer/InteractiveRunner.html:107
> +	   // Reset all selected options except selection

This comment repeats the code below. Please remove, and remove the curly braces
since it would be a single line statement.

> PerformanceTests/Speedometer/InteractiveRunner.html:154
> +    if (queryParam !== undefined) {
> +	   // Individually run the specified test automatically
> +	   document.getElementById('runSuites').click();
> +    }

I think this should be a separate query string option like autoStart,
startOnLoad, startAutomatically.
Again, the comment repeats what the code below says. Please remove along with
the curly braces.

> PerformanceTests/Speedometer/resources/tests.js:4
> +function triggerKeyboardEvent(el, keyCode) {

Please don't use abbreviations such as el:
https://webkit.org/code-style-guidelines/#names-full-words

> PerformanceTests/Speedometer/resources/tests.js:6
> +   var eventObj = document.createEventObject ?
> +	  document.createEventObject() : document.createEvent("Events");

Do we really need a separate branch for IE?
I tried to stay way from having IE-specific code for the sake of running the
same code on all browsers.

> PerformanceTests/Speedometer/resources/tests.js:11
> +	 eventObj.initEvent("keydown", true, true);

Nit: indentation here is three spaces. Also, no braces around a single line
statement.

> PerformanceTests/Speedometer/resources/tests.js:17
> +   el.dispatchEvent ? el.dispatchEvent(eventObj) : el.fireEvent("onkeydown",
eventObj);

Do we really need to have a branch for fireEvent?

> PerformanceTests/Speedometer/resources/tests.js:33
> +		   triggerKeyboardEvent(newTodo, 13);

Instead of repeating the magic key code of 13 everywhere, can we define a
variable with a descriptive name somewhere?
e.g. var ENTER_KEY_CODE = 13 in the global code.

> PerformanceTests/Speedometer/resources/tests.js:67
> +	       for (var i = 0; i < checkboxes.length; i++)

Given this is a ES2015 test, we might as well as use for (const box of
checkboxes) here.

> PerformanceTests/Speedometer/resources/tests.js:72
> +	       for (var i = 0; i < deleteButtons.length; i++)

Ditto.

> PerformanceTests/Speedometer/resources/tests.js:126
> +		   var keydownEvent = document.createEvent('Event');
> +		   keydownEvent.initEvent('keydown', true, true);
> +		   keydownEvent.which = 13; // VK_ENTER
> +		   newTodo.dispatchEvent(keydownEvent);

We can't use triggerKeyboardEvent here?

> PerformanceTests/Speedometer/resources/tests.js:132
> +	       for (var i = 0; i < checkboxes.length; i++)
> +		   checkboxes[i].click();

Does this click result in React doing some work?
If it's just dirtying React tests, we might be just testing how fast click
executes, which isn't really all that interesting.
This is why my Ember test case had manually called emberRun to force flushing
of the DOM state updates
since what we're really interested in measuring how long framework code takes
to run after user interaction
not so much how many user interactions it can handle per their event loop
(since a normal user would never click 25 elements in a single task).

> PerformanceTests/Speedometer/resources/tests.js:318
> +		   var keydownEvent = document.createEvent('Event');
> +		   keydownEvent.initEvent('keydown', true, true);
> +		   keydownEvent.which = 13;
> +		   newTodo.dispatchEvent(keydownEvent);

Can't use triggerKeyboardEvent here either?

> PerformanceTests/Speedometer/resources/tests.js:359
> -	       for (var i = 0; i < checkboxes.length; i++)
> +	       for (var i = 0; i < checkboxes.length; i++) {
>		   itemIndexToId[i] =
$(checkboxes[i]).closest('li').data('id');
> +	       } 

Nit: This code change is unnecessary, and in fact makes the code violate
WebKit's code style guideline:
https://webkit.org/code-style-guidelines/#braces-one-line

> PerformanceTests/Speedometer/resources/tests.js:381
> +	       var clear = contentDocument.querySelector('#clear-completed');
> +	       if (clear !== null) {
> +		   clear.click();
> +	       }

Huh, it's kind of interesting that we only clear items in the jQuery test case.
It's probably an oversight when I first wrote it.
But why do we need to add this if now? It's bad for benchmarks to sometimes
call clear.
Either we should get rid this call to clear() altogether or make the test case
deterministic.
r- because we can't have this non-determinism / ambiguity.

> PerformanceTests/Speedometer/resources/tests.js:395
> -    name: 'AngularJS-TodoMVC',
> -    url: 'todomvc/architecture-examples/angularjs/index.html',
> +    name: 'Preact-TodoMVC',
> +    url: 'todomvc/architecture-examples/preact/build/index.html',

I would have preferred if you didn't change the ordering so that the diff
doesn't look so crazy but perhaps you're ordering them lexicologically?
If so, we could just do that after all suites are added.

> PerformanceTests/Speedometer/resources/tests.js:424
> +    url: 'todomvc/vanilla-examples/es2015-babel-webpack/dist/index.html',

I think you meant to refer to todomvc/architecture-examples/inferno/index.html
?