Speed up moving cursor/selection up or down past non-renderered elements.

Speed up moving cursor/selection up or down past non-renderered elements.

Skips calling firstPositionInOrBeforeNode when a node doesn't have a
renderer, since there couldn't possibly be a visible position there.
This was particularly wasteful when trying to move the cursor up when at
the beginning of a document, as it did a O(n^2) scan through the document head.

See bug for easy repro steps.

BUG=263499
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154977

[leviw_travelin_and_unemployed, 2013-07-24 18:13:56]

The code change lgtm.

https://codereview.chromium.org/20049007/diff/1/LayoutTests/editing/execComma...
File
LayoutTests/editing/execCommand/move-up-down-should-skip-hidden-elements.html
(right):

https://codereview.chromium.org/20049007/diff/1/LayoutTests/editing/execComma...
LayoutTests/editing/execCommand/move-up-down-should-skip-hidden-elements.html:16:
description("Test moving up and down through non-renderered elements. This test
should execute quickly if it's working correctly; a timeout is a failure.");
Could this be a Magnitude test instead? See LayoutTests/perf

[dmazzoni, 2013-07-24 18:22:11]

https://codereview.chromium.org/20049007/diff/1/LayoutTests/editing/execComma...
File
LayoutTests/editing/execCommand/move-up-down-should-skip-hidden-elements.html
(right):

https://codereview.chromium.org/20049007/diff/1/LayoutTests/editing/execComma...
LayoutTests/editing/execCommand/move-up-down-should-skip-hidden-elements.html:16:
description("Test moving up and down through non-renderered elements. This test
should execute quickly if it's working correctly; a timeout is a failure.");
On 2013/07/24 18:13:56, Levi wrote:
> Could this be a Magnitude test instead? See LayoutTests/perf

Happy to use Magnitude where it makes sense.

In this case, a Magnitude test would be quite slow, as it'd have to create a lot
of nodes to accurately measure the time.

The way I wrote this test, it runs *very* fast (constant time) when the bug is
fixed, but times out, even in Release mode on a z620, when the bug is not fixed.

Sound reasonable, or do you think a Magnitude test would still be better?

[leviw_travelin_and_unemployed, 2013-07-26 04:40:11]

Sorry, I finished this review awhile ago but never sent out my feedback! Sorry
for the delay!

https://codereview.chromium.org/20049007/diff/1/LayoutTests/editing/execComma...
File
LayoutTests/editing/execCommand/move-up-down-should-skip-hidden-elements.html
(right):

https://codereview.chromium.org/20049007/diff/1/LayoutTests/editing/execComma...
LayoutTests/editing/execCommand/move-up-down-should-skip-hidden-elements.html:16:
description("Test moving up and down through non-renderered elements. This test
should execute quickly if it's working correctly; a timeout is a failure.");
On 2013/07/24 18:22:11, Dominic Mazzoni wrote:
> On 2013/07/24 18:13:56, Levi wrote:
> > Could this be a Magnitude test instead? See LayoutTests/perf
> 
> Happy to use Magnitude where it makes sense.
> 
> In this case, a Magnitude test would be quite slow, as it'd have to create a
lot
> of nodes to accurately measure the time.
> 
> The way I wrote this test, it runs *very* fast (constant time) when the bug is
> fixed, but times out, even in Release mode on a z620, when the bug is not
fixed.
> 
> Sound reasonable, or do you think a Magnitude test would still be better?

Sounds reasonable. LGTM :)

[commit-bot: I haz the power, 2013-07-26 05:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/20049007/1

[commit-bot: I haz the power, 2013-07-26 06:13:29]

Change committed as 154977