Early out if the scroll position passed in by js is NaN

Early out if the scroll position passed in by js is NaN

This is interesting bug from slashdot.org. When the page is first loaded,
it has a js scroll call but a NaN scroll position is passed in.

We used to have IntPoint representing scrollPosition, and the scroll position
goes as:
double(NaN) -> static_cast<int> -> int(LONG/INT_MIN) -> adjusted to 0
and it happens to work that page is not scrolled with NaN scroll position.

Now that we change to use DoublePoint to represent scroll position, the NaN
value eventually gets adjusted to be the maximal scroll position and the page
will scroll to the bottom when loaded.

Instead of depending on the nasty conversion rule of NaN, lets make it specific
that if NaN is passed in, just early out in the scrolling path.

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

[Yufeng Shen (Slow to review), 2014-10-21 20:09:06]

miletus@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2014-10-21 20:44:15]

Interesting, can you please add the link to the actual stackOverflow discussion?

Is there an identical issue with Infinity and -Infinity?

If we're going to do this for scrollTo then for consistency we should probably
do it for all the scrollTop/scrollLeft setters to, right?

Please add a test.

+simonp to comment on whether the CSS OM View spec should say something about
this.

https://codereview.chromium.org/670833002/diff/1/Source/core/frame/LocalDOMWi...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/670833002/diff/1/Source/core/frame/LocalDOMWi...
Source/core/frame/LocalDOMWindow.cpp:1407: if (x != x)
use std::isnan (then you can also remove the comment which will be redundant
with the code).

[Rick Byers, 2014-10-21 20:45:00]

On 2014/10/21 20:44:15, Rick Byers wrote:
> Interesting, can you please add the link to the actual stackOverflow
discussion?

Sorry, ignore this - just saw there are details in the bug (and it's the site
itself, not some discussion there).

[miletus1, 2014-10-21 22:11:14]

On 2014/10/21 20:44:15, Rick Byers wrote:
> Interesting, can you please add the link to the actual stackOverflow
discussion?
> 
> Is there an identical issue with Infinity and -Infinity?

maybe, we can do it separately if there is a real bug filed.

> 
> If we're going to do this for scrollTo then for consistency we should probably
> do it for all the scrollTop/scrollLeft setters to, right?
>

Yeah, I am adding the guard also to Element.setScrollTop/Left. (and it is
already
partially guarding against the NaN).
 
> Please add a test.
> 

done.

> +simonp to comment on whether the CSS OM View spec should say something about
> this.
> 
>
https://codereview.chromium.org/670833002/diff/1/Source/core/frame/LocalDOMWi...
> File Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/670833002/diff/1/Source/core/frame/LocalDOMWi...
> Source/core/frame/LocalDOMWindow.cpp:1407: if (x != x)
> use std::isnan (then you can also remove the comment which will be redundant
> with the code).

[Rick Byers, 2014-10-21 22:22:33]

Looking good, just a couple more little things

https://codereview.chromium.org/670833002/diff/20001/LayoutTests/fast/scrolli...
File LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html (right):

https://codereview.chromium.org/670833002/diff/20001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:26: {
you should start by scrolling to some specific position (eg. 1,1) so the test
can detect the difference betweeen converting NaN to 0 and actually scrolling
back to 0.

https://codereview.chromium.org/670833002/diff/20001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:27: //
window.nan is undefined so NaN.
technically NaN and undefined are different in JS.  If you want to actually use
NaN then use window.NaN (note case).  If you want undefined then use just
'undefined'.  Testing either (or some combination of both) is probably fine
since the bindings (I assume) convert both to NaN.

https://codereview.chromium.org/670833002/diff/20001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:36:
document.body.scrollTop = window.nan;
Once you scroll the document these will probably start failing (body isn't
technically scrollable in the standard behavior which we have enabled by default
for tests).  May want to just exclude the body case.

https://codereview.chromium.org/670833002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/670833002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1384: return;
You said the previous behavior was to scroll to 0, is that right?  Or did it
actually ignore the scroll?  Either is probably OK - just want to understand
your thinking here.

[Yufeng Shen (Slow to review), 2014-10-21 22:49:31]

https://codereview.chromium.org/670833002/diff/20001/LayoutTests/fast/scrolli...
File LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html (right):

https://codereview.chromium.org/670833002/diff/20001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:26: {
On 2014/10/21 22:22:33, Rick Byers wrote:
> you should start by scrolling to some specific position (eg. 1,1) so the test
> can detect the difference betweeen converting NaN to 0 and actually scrolling
> back to 0.

Done.

https://codereview.chromium.org/670833002/diff/20001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:27: //
window.nan is undefined so NaN.
On 2014/10/21 22:22:33, Rick Byers wrote:
> technically NaN and undefined are different in JS.  If you want to actually
use
> NaN then use window.NaN (note case).  If you want undefined then use just
> 'undefined'.  Testing either (or some combination of both) is probably fine
> since the bindings (I assume) convert both to NaN.

window.NaN is used.

https://codereview.chromium.org/670833002/diff/20001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:36:
document.body.scrollTop = window.nan;
On 2014/10/21 22:22:33, Rick Byers wrote:
> Once you scroll the document these will probably start failing (body isn't
> technically scrollable in the standard behavior which we have enabled by
default
> for tests).  May want to just exclude the body case.

Done.

https://codereview.chromium.org/670833002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/670833002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1384: return;
On 2014/10/21 22:22:33, Rick Byers wrote:
> You said the previous behavior was to scroll to 0, is that right?  Or did it
> actually ignore the scroll?  Either is probably OK - just want to understand
> your thinking here.

Yeah, I changed my mind and felt that ignore the nan scroll position is more
reasonable. It is also consistent with how Element.setScrollLeft/Top handles
nan.

[Rick Byers, 2014-10-22 13:55:34]

LGTM with a couple minor nits

https://codereview.chromium.org/670833002/diff/40001/LayoutTests/fast/scrolli...
File LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html (right):

https://codereview.chromium.org/670833002/diff/40001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:15: <body
style="width:30000px;height:30000px" onload="runTest()">
nit: omit body tag (put style in CSS and see below)

https://codereview.chromium.org/670833002/diff/40001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:50: function
runTest()
to avoid needing <body onload=runTest> use just:
onload = function() {

}

https://codereview.chromium.org/670833002/diff/40001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:57: </html>
nit: omit </html> (you have no <html> tag)

https://codereview.chromium.org/670833002/diff/40001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/670833002/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:664: void Element::setScrollLeft(double newLeft)
Sorry, should have mentioned this earlier: setScrollLeft/setScrollTop are
overridden by HTMLBodyElement - for consistency you should have the same logic
there (hopefully we can eventually simplify all this when
RuntimeEnabledFeatures::scrollTopLeftInterop is the default).

[Yufeng Shen (Slow to review), 2014-10-22 17:38:46]

https://codereview.chromium.org/670833002/diff/40001/LayoutTests/fast/scrolli...
File LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html (right):

https://codereview.chromium.org/670833002/diff/40001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:15: <body
style="width:30000px;height:30000px" onload="runTest()">
On 2014/10/22 13:55:33, Rick Byers wrote:
> nit: omit body tag (put style in CSS and see below)

Done.

https://codereview.chromium.org/670833002/diff/40001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:50: function
runTest()
On 2014/10/22 13:55:33, Rick Byers wrote:
> to avoid needing <body onload=runTest> use just:
> onload = function() {
> 
> }

Done.

https://codereview.chromium.org/670833002/diff/40001/LayoutTests/fast/scrolli...
LayoutTests/fast/scrolling/scrolling-apis-nan-scroll-position.html:57: </html>
On 2014/10/22 13:55:33, Rick Byers wrote:
> nit: omit </html> (you have no <html> tag)

Done.

https://codereview.chromium.org/670833002/diff/40001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/670833002/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:664: void Element::setScrollLeft(double newLeft)
On 2014/10/22 13:55:33, Rick Byers wrote:
> Sorry, should have mentioned this earlier: setScrollLeft/setScrollTop are
> overridden by HTMLBodyElement - for consistency you should have the same logic
> there (hopefully we can eventually simplify all this when
> RuntimeEnabledFeatures::scrollTopLeftInterop is the default).

Done.

[Yufeng Shen (Slow to review), 2014-10-22 17:38:52]

The CQ bit was checked by miletus@chromium.org

[commit-bot: I haz the power, 2014-10-22 17:39:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/670833002/60001

[commit-bot: I haz the power, 2014-10-22 18:44:35]

Committed patchset #4 (id:60001) as 184210