Bug 188475 – [IntersectionObserver] Validate threshold values

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ali Juma]

Throw an exception if any of the thresholds are outside the range [0, 1].

[Ali Juma]

Created attachment 346912 [details]
Patch

[Darin Adler]

Comment on attachment 346912 [details]
Patch

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

> Source/WebCore/page/IntersectionObserver.cpp:94
> +    Vector<double> thresholds;
> +    if (WTF::holds_alternative<double>(init.threshold))
> +        thresholds.append(WTF::get<double>(init.threshold));
> +    else
> +        thresholds = WTF::get<Vector<double>>(WTFMove(init.threshold));

I realize this code is just being moved not newly written, but two thoughts:

1) This kind of thing typically reads better with the "switchOn" style that you can find elsewhere in WebKit code -- makes it clearer all cases are handled even when there are just two cases; it is a little more wordy, though.

2) It is probably slightly more efficient to make the Vector in a way that indicates it will have only one double in it, even though it will be deallocated soon after. One way is to use reserveInitialCapacity(1) and then uncheckedAppend rather than just append. Another way is to write something like "thresholds = Vector<double> { 1, init.threshold }", a third way is "thresholds = Vector<Double> { { threshold } };". All of these have the benefit that the vector capacity will be 1 rather than a larger size as part of optimizing growth.

> Source/WebCore/page/IntersectionObserver.cpp:97
> +        if (threshold < 0 || threshold > 1)

For floating point, it’s typically a good practice to take advantage of the property that "NaN" comparisons all return false, so we would instead write:

    if (!(threshold >= 0 && threshold <=1))

This would give us an exception when one of the values is a NaN. Perhaps that’s already handled at the bindings layer, though. I think we should make sure we have a test that covers NaN (and infinity and other interesting floating point values like denormalized ones), and then still consider the above syntax since it’s better when NaN is involved and I don’t see a lot of downside to writing it this way.

Another consideration is that this approach looks good if we decide put the "validity rule" into a small inline function:

    if (!isValidThreshold(threshold))

So it’s kind of logical to do the one that has a "not" in it.

> LayoutTests/imported/w3c/web-platform-tests/intersection-observer/observer-exceptions-expected.txt:4
>  PASS IntersectionObserver constructor witth { rootMargin: "1" } 

Typo in the test here.

[Ali Juma]

Created attachment 347034 [details]
Patch

[Ali Juma]

Comment on attachment 346912 [details]
Patch

Thanks for the review!

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

>> Source/WebCore/page/IntersectionObserver.cpp:94
>> +        thresholds = WTF::get<Vector<double>>(WTFMove(init.threshold));
> 
> I realize this code is just being moved not newly written, but two thoughts:
> 
> 1) This kind of thing typically reads better with the "switchOn" style that you can find elsewhere in WebKit code -- makes it clearer all cases are handled even when there are just two cases; it is a little more wordy, though.
> 
> 2) It is probably slightly more efficient to make the Vector in a way that indicates it will have only one double in it, even though it will be deallocated soon after. One way is to use reserveInitialCapacity(1) and then uncheckedAppend rather than just append. Another way is to write something like "thresholds = Vector<double> { 1, init.threshold }", a third way is "thresholds = Vector<Double> { { threshold } };". All of these have the benefit that the vector capacity will be 1 rather than a larger size as part of optimizing growth.

1) Changed to use the switchOn style.

2) Used the reserveInitialCapacity(1) approach.

>> Source/WebCore/page/IntersectionObserver.cpp:97
>> +        if (threshold < 0 || threshold > 1)
> 
> For floating point, it’s typically a good practice to take advantage of the property that "NaN" comparisons all return false, so we would instead write:
> 
>     if (!(threshold >= 0 && threshold <=1))
> 
> This would give us an exception when one of the values is a NaN. Perhaps that’s already handled at the bindings layer, though. I think we should make sure we have a test that covers NaN (and infinity and other interesting floating point values like denormalized ones), and then still consider the above syntax since it’s better when NaN is involved and I don’t see a lot of downside to writing it this way.
> 
> Another consideration is that this approach looks good if we decide put the "validity rule" into a small inline function:
> 
>     if (!isValidThreshold(threshold))
> 
> So it’s kind of logical to do the one that has a "not" in it.

Done. It turns out that Nan and infinity are indeed handled already at the bindings layer, but added test coverage nevertheless.

>> LayoutTests/imported/w3c/web-platform-tests/intersection-observer/observer-exceptions-expected.txt:4
>>  PASS IntersectionObserver constructor witth { rootMargin: "1" } 
> 
> Typo in the test here.

Fixed. Fixed it upstream as well.

[WebKit Commit Bot]

Comment on attachment 347034 [details]
Patch

Clearing flags on attachment: 347034

Committed r234818: <https://trac.webkit.org/changeset/234818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43254352>