Improve performance of time-multiple-fields-stepup-stepdown-from-renderer-hour.html

Improve performance of time-multiple-fields-stepup-stepdown-from-renderer-hour.html

This CL improves the performance of the test by converting it to testharness.js.

With macOS debug: 7.3 sec -> 2.4 sec

BUG=658318
Review-Url: https://codereview.chromium.org/2631483002
Cr-Commit-Position: refs/heads/master@{#443522}
Committed: https://chromium.googlesource.com/chromium/src/+/2b57920cbb778f03295ae1c7e0cee380667b6d5a

[tkent, 2017-01-13 01:49:41]

Description was changed from

==========
in-progress

BUG=
==========

to

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

BUG=658318
==========

[tkent, 2017-01-13 01:51:45]

Description was changed from

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

BUG=658318
==========

to

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

The test had flaky timeout due to the slowness.
This CL improve the performance by converting to testharness.js.

With macOS debug: 7.3 sec -> 2.4 sec

BUG=658318
==========

[tkent, 2017-01-13 01:54:13]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[tkent, 2017-01-13 01:54:57]

Description was changed from

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

The test had flaky timeout due to the slowness.
This CL improve the performance by converting to testharness.js.

With macOS debug: 7.3 sec -> 2.4 sec

BUG=658318
==========

to

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

This CL improve the performance of the test by converting it to testharness.js.

With macOS debug: 7.3 sec -> 2.4 sec

BUG=658318
==========

[commit-bot: I haz the power, 2017-01-13 01:55:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-01-13 01:55:33]

tkent@chromium.org changed reviewers:
+ yosin@chromium.org

[tkent, 2017-01-13 01:55:33]

yosin@, would you review this please?

[tkent, 2017-01-13 02:26:33]

Description was changed from

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

This CL improve the performance of the test by converting it to testharness.js.

With macOS debug: 7.3 sec -> 2.4 sec

BUG=658318
==========

to

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

This CL improves the performance of the test by converting it to testharness.js.

With macOS debug: 7.3 sec -> 2.4 sec

BUG=658318
==========

[yosin_UTC9, 2017-01-13 02:33:54]

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html
(right):

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:10:
console.assert(!!window.internals, 'This test requires window.internals.');
We found good pattern for this:

test(() =>
  if (!window.internals) {
    assert_not_reached('This test requires window.internals');
    return;
  }
  ... code depends on window.internals ...
});

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:51:
input.type = 'time';
It is better to move this code fragment in to |test()|

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:222:
var keySequence = typeof item.operation == 'string' ? [item.operation] :
item.operation;
It is better to use sequence of function rather than data driven, e.g.

assert_input_type('<input type="time" value="17:00" min="17:00" max="18:00"
step="7200">', 'ArrowDown', '17:00');
assert_input_type('<input type="time" value="17:00" min="17:00" max="18:00"
step="7200">', ['ArrowDown', 'ArrowUp'], '17:00');

You can generate this by modifying this code fragment to print out them.

|assert_input_type()| can be

function assert_input_type(html, operation, expectedValue) {
  const div = document.createElement'div');
  div.innerHTML = html;
  document.body.appendChild(div);
  ... run operations ...
}

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:226:
}, label + ': ' + item.operation + ' to INPUT with initial-value=' +
item.initial +
It is better to use template, .e.g

`${label}: ${item.operation} to INPUT with initial-value=${item.initial}
step=${item.step} ...`

[commit-bot: I haz the power, 2017-01-13 03:55:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 03:55:42]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-01-13 04:20:28]

On 2017/01/13 at 02:33:54, Yosi_UTC9 wrote:
>
https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html
(right):
> 
>
https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:10:
console.assert(!!window.internals, 'This test requires window.internals.');
> We found good pattern for this:
> 
> test(() =>
>   if (!window.internals) {
>     assert_not_reached('This test requires window.internals');
>     return;
>   }
>   ... code depends on window.internals ...
> });
> 
>
https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:51:
input.type = 'time';
> It is better to move this code fragment in to |test()|
> 
>
https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:222:
var keySequence = typeof item.operation == 'string' ? [item.operation] :
item.operation;
> It is better to use sequence of function rather than data driven, e.g.
> 
> assert_input_type('<input type="time" value="17:00" min="17:00" max="18:00"
step="7200">', 'ArrowDown', '17:00');
> assert_input_type('<input type="time" value="17:00" min="17:00" max="18:00"
step="7200">', ['ArrowDown', 'ArrowUp'], '17:00');
> 
> You can generate this by modifying this code fragment to print out them.
> 
> |assert_input_type()| can be
> 
> function assert_input_type(html, operation, expectedValue) {
>   const div = document.createElement'div');
>   div.innerHTML = html;
>   document.body.appendChild(div);
>   ... run operations ...
> }
> 
>
https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:226:
}, label + ': ' + item.operation + ' to INPUT with initial-value=' +
item.initial +
> It is better to use template, .e.g
> 
> `${label}: ${item.operation} to INPUT with initial-value=${item.initial}
step=${item.step} ...`

Here is another example to avoid control-flow in test:
https://codereview.chromium.org/2453313005

It bloat number of lines but it is more predictable and easy to fix on test
failure since test harness print line number of failure case.

[tkent, 2017-01-13 04:45:52]

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html
(right):

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:10:
console.assert(!!window.internals, 'This test requires window.internals.');
On 2017/01/13 at 02:33:54, Yosi_UTC9 wrote:
> We found good pattern for this:
> 
> test(() =>
>   if (!window.internals) {
>     assert_not_reached('This test requires window.internals');
>     return;
>   }
>   ... code depends on window.internals ...
> });

Done.

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:222:
var keySequence = typeof item.operation == 'string' ? [item.operation] :
item.operation;
On 2017/01/13 at 02:33:54, Yosi_UTC9 wrote:
> It is better to use sequence of function rather than data driven, e.g.
> 
> assert_input_type('<input type="time" value="17:00" min="17:00" max="18:00"
step="7200">', 'ArrowDown', '17:00');
> assert_input_type('<input type="time" value="17:00" min="17:00" max="18:00"
step="7200">', ['ArrowDown', 'ArrowUp'], '17:00');
> 
> You can generate this by modifying this code fragment to print out them.
> 
> |assert_input_type()| can be
> 
> function assert_input_type(html, operation, expectedValue) {
>   const div = document.createElement'div');
>   div.innerHTML = html;
>   document.body.appendChild(div);
>   ... run operations ...
> }

Unfortunately, we can't do it due to a performance reason.  Creating INPUT
element on every test is twice slower than Patch Set 1, and it isn't acceptable.

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:226:
}, label + ': ' + item.operation + ' to INPUT with initial-value=' +
item.initial +
On 2017/01/13 at 02:33:54, Yosi_UTC9 wrote:
> It is better to use template, .e.g
> 
> `${label}: ${item.operation} to INPUT with initial-value=${item.initial}
step=${item.step} ...`

Done.

[yosin_UTC9, 2017-01-13 05:31:13]

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html
(right):

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:222:
var keySequence = typeof item.operation == 'string' ? [item.operation] :
item.operation;
On 2017/01/13 at 04:45:52, tkent wrote:
> On 2017/01/13 at 02:33:54, Yosi_UTC9 wrote:
> > It is better to use sequence of function rather than data driven, e.g.
> > 
> > assert_input_type('<input type="time" value="17:00" min="17:00" max="18:00"
step="7200">', 'ArrowDown', '17:00');
> > assert_input_type('<input type="time" value="17:00" min="17:00" max="18:00"
step="7200">', ['ArrowDown', 'ArrowUp'], '17:00');
> > 
> > You can generate this by modifying this code fragment to print out them.
> > 
> > |assert_input_type()| can be
> > 
> > function assert_input_type(html, operation, expectedValue) {
> >   const div = document.createElement'div');
> >   div.innerHTML = html;
> >   document.body.appendChild(div);
> >   ... run operations ...
> > }
> 
> Unfortunately, we can't do it due to a performance reason.  Creating INPUT
element on every test is twice slower than Patch Set 1, and it isn't acceptable.

My main point is "avoiding data drive" instead of using html fragment.
Let's use function abstraction to have number of functions instead of
control-flow.

assert_up('07:00', 1, '08:00');
assert_perations('06:00', '7200, ['Delete', 'ArrowDown' '22:00'},
assert_up_minmax('17:00', 7200, '1700', '17:00', '17:00');

Function names are just examples, it may be better naming and better
abstractions.

[tkent, 2017-01-13 06:35:49]

Patchset #3 (id:40001) has been deleted

[tkent, 2017-01-13 06:35:55]

Patchset #2 (id:20001) has been deleted

[tkent, 2017-01-13 06:35:59]

Patchset #1 (id:1) has been deleted

[tkent, 2017-01-13 06:56:57]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[tkent, 2017-01-13 06:57:05]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-01-13 06:57:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-01-13 06:57:44]

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html
(right):

https://codereview.chromium.org/2631483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:222:
var keySequence = typeof item.operation == 'string' ? [item.operation] :
item.operation;
> My main point is "avoiding data drive" instead of using html fragment.
> Let's use function abstraction to have number of functions instead of
control-flow.

Ah, I see.  The latest Patch Set follows it.

[yosin_UTC9, 2017-01-13 07:13:52]

lgtm

Thanks!

https://codereview.chromium.org/2631483002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html
(right):

https://codereview.chromium.org/2631483002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:23:
if (input.getAttribute(name) != value)
nit: s/!=/!==/

https://codereview.chromium.org/2631483002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:37:
for (var i = 0; i < keySequence.length; ++i)
nit: s/var/let/

https://codereview.chromium.org/2631483002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:71:
assert_exists(window, 'internals');
You found new pattern. It is better than before.

[commit-bot: I haz the power, 2017-01-13 07:52:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 07:52:05]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-01-13 08:39:02]

https://codereview.chromium.org/2631483002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html
(right):

https://codereview.chromium.org/2631483002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:23:
if (input.getAttribute(name) != value)
On 2017/01/13 at 07:13:52, Yosi_UTC9 wrote:
> nit: s/!=/!==/

Done

https://codereview.chromium.org/2631483002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer-hour.html:37:
for (var i = 0; i < keySequence.length; ++i)
On 2017/01/13 at 07:13:52, Yosi_UTC9 wrote:
> nit: s/var/let/

I made this |forEach|.

[tkent, 2017-01-13 08:39:12]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-01-13 08:39:13]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2631483002/#ps140001
(title: "!==, forEach")

[commit-bot: I haz the power, 2017-01-13 08:39:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 09:52:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484296752701650,
"parent_rev": "27032ce5253ef29711946ce32a68f3ce0ab6298f", "commit_rev":
"2b57920cbb778f03295ae1c7e0cee380667b6d5a"}

[commit-bot: I haz the power, 2017-01-13 09:52:30]

Description was changed from

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

This CL improves the performance of the test by converting it to testharness.js.

With macOS debug: 7.3 sec -> 2.4 sec

BUG=658318
==========

to

==========
Improve performance of
time-multiple-fields-stepup-stepdown-from-renderer-hour.html

This CL improves the performance of the test by converting it to testharness.js.

With macOS debug: 7.3 sec -> 2.4 sec

BUG=658318

Review-Url: https://codereview.chromium.org/2631483002
Cr-Commit-Position: refs/heads/master@{#443522}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b57920cbb778f03295ae1c7e0ce...
==========

[commit-bot: I haz the power, 2017-01-13 09:52:32]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2b57920cbb778f03295ae1c7e0ce...