-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[geometry-1] Define minimum and maximum as preferring NaN #395
Conversation
@svgeesus Heya Chris, Timothy is an undergrad, can you hook them up with a non-participant licensing agreement? (Or maybe @astearns or @atanassov know how to do this.) |
It's a team-only thing. @xfq should be able to kick it off, too. |
Done. |
@TimothyGu fwiw, I am not actively working on web platform stuff. If you were just aiming to cc someone from Mozilla, probably someone else is a good idea... |
This seems good to me. Tests should include at least this case #222 (comment) Thanks! |
@emilio Would you happen to have any thoughts on this? |
This looks pretty reasonable to me off-hand. @saschanaz has done some geometry stuff in the past and may have stronger opinions. |
LGTM too. |
The code looks fine. This also needs an accompanying WPT test, could you add one? |
@chrishtr Yes. I'm adding one as a part of https://chromium-review.googlesource.com/c/chromium/src/+/2129889, which will be sent out for review as soon as dry-running finishes. |
Great! |
Currently in some geometry classes we are using std::min() and std::max() over numbers specified in IDL as "unrestricted double", meaning they could take the special value NaN. These STL helpers internally use the < operator, as in std::min(a, b) = a < b ? a : b. However, the IEEE 794 < operator always returns false when _either_ operand is NaN, so the result of min(0, NaN) and min(NaN, 0) could, confusingly, be different. This is difference is in fact visible through JavaScript. For instance, new DOMQuad({ x: NaN }, { x: 0 }, { x: 0 }, { x: 0 }).getBounds().x gives NaN, but new DOMQuad({ x: 0 }, { x: 0 }, { x: 0 }, { x: NaN }).getBounds().x gives 0. A similar problem is present for DOMRect/DOMRectReadOnly as well. This CL implements the rough consensus in [1][2], which is to adopt semantics similar to JavaScript's Math.min() and Math.max(), which propagates NaN from either operand. This also aligns our behavior with WebKit. [1]: w3c/fxtf-drafts#222 [2]: w3c/fxtf-drafts#395 Fixed: 1066499 Change-Id: Id9a4282fa00dafcfe9c5616643efbe2eaace411e
@zcorpan The tests are in web-platform-tests/wpt#22585 / https://chromium-review.googlesource.com/c/chromium/src/+/2129889. Feel free to review it anywhere. |
Currently in some geometry classes we are using std::min() and std::max() over numbers specified in IDL as "unrestricted double", meaning they could take the special value NaN. These STL helpers internally use the < operator, as in std::min(a, b) = a < b ? a : b. However, the IEEE 794 < operator always returns false when _either_ operand is NaN, so the result of min(0, NaN) and min(NaN, 0) could, confusingly, be different. This is difference is in fact visible through JavaScript. For instance, new DOMQuad({ x: NaN }, { x: 0 }, { x: 0 }, { x: 0 }).getBounds().x gives NaN, but new DOMQuad({ x: 0 }, { x: 0 }, { x: 0 }, { x: NaN }).getBounds().x gives 0. A similar problem is present for DOMRect/DOMRectReadOnly as well. This CL implements [1], which is to adopt semantics similar to JavaScript's Math.min() and Math.max(), which propagates NaN from either operand. This also aligns our behavior with WebKit. [1]: w3c/fxtf-drafts#395 Fixed: 1066499 Change-Id: Id9a4282fa00dafcfe9c5616643efbe2eaace411e
Currently in some geometry classes we are using std::min() and std::max() over numbers specified in IDL as "unrestricted double", meaning they could take the special value NaN. These STL helpers internally use the < operator, as in std::min(a, b) = a < b ? a : b. However, the IEEE 794 < operator always returns false when _either_ operand is NaN, so the result of min(0, NaN) and min(NaN, 0) could, confusingly, be different. This is difference is in fact visible through JavaScript. For instance, new DOMQuad({ x: NaN }, { x: 0 }, { x: 0 }, { x: 0 }).getBounds().x gives NaN, but new DOMQuad({ x: 0 }, { x: 0 }, { x: 0 }, { x: NaN }).getBounds().x gives 0. A similar problem is present for DOMRect/DOMRectReadOnly as well. This CL implements [1], which is to adopt semantics similar to JavaScript's Math.min() and Math.max(), which propagates NaN from either operand. This also aligns our behavior with WebKit. [1]: w3c/fxtf-drafts#395 Fixed: 1066499 Change-Id: Id9a4282fa00dafcfe9c5616643efbe2eaace411e
Currently in some geometry classes we are using std::min() and std::max() over numbers specified in IDL as "unrestricted double", meaning they could take the special value NaN. These STL helpers internally use the < operator, as in std::min(a, b) = a < b ? a : b. However, the IEEE 794 < operator always returns false when _either_ operand is NaN, so the result of min(0, NaN) and min(NaN, 0) could, confusingly, be different. This is difference is in fact visible through JavaScript. For instance, new DOMQuad({ x: NaN }, { x: 0 }, { x: 0 }, { x: 0 }).getBounds().x gives NaN, but new DOMQuad({ x: 0 }, { x: 0 }, { x: 0 }, { x: NaN }).getBounds().x gives 0. A similar problem is present for DOMRect/DOMRectReadOnly as well. This CL implements [1], which is to adopt semantics similar to JavaScript's Math.min() and Math.max(), which propagates NaN from either operand. This also aligns our behavior with WebKit. [1]: w3c/fxtf-drafts#395 Fixed: 1066499 Change-Id: Id9a4282fa00dafcfe9c5616643efbe2eaace411e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2129889 Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Jinho Bang <jinho.bang@samsung.com> Cr-Commit-Position: refs/heads/master@{#755373}
…tions, a=testonly Automatic update from web-platform-tests geometry: Propagate NaN in min/max operations (#22585) Currently in some geometry classes we are using std::min() and std::max() over numbers specified in IDL as "unrestricted double", meaning they could take the special value NaN. These STL helpers internally use the < operator, as in std::min(a, b) = a < b ? a : b. However, the IEEE 794 < operator always returns false when _either_ operand is NaN, so the result of min(0, NaN) and min(NaN, 0) could, confusingly, be different. This is difference is in fact visible through JavaScript. For instance, new DOMQuad({ x: NaN }, { x: 0 }, { x: 0 }, { x: 0 }).getBounds().x gives NaN, but new DOMQuad({ x: 0 }, { x: 0 }, { x: 0 }, { x: NaN }).getBounds().x gives 0. A similar problem is present for DOMRect/DOMRectReadOnly as well. This CL implements [1], which is to adopt semantics similar to JavaScript's Math.min() and Math.max(), which propagates NaN from either operand. This also aligns our behavior with WebKit. [1]: w3c/fxtf-drafts#395 Fixed: 1066499 Change-Id: Id9a4282fa00dafcfe9c5616643efbe2eaace411e -- wpt-commits: 452d7c5e1a0b683ccfbfaf6fcac833b3e96bb32e wpt-pr: 22585
…tions, a=testonly Automatic update from web-platform-tests geometry: Propagate NaN in min/max operations (#22585) Currently in some geometry classes we are using std::min() and std::max() over numbers specified in IDL as "unrestricted double", meaning they could take the special value NaN. These STL helpers internally use the < operator, as in std::min(a, b) = a < b ? a : b. However, the IEEE 794 < operator always returns false when _either_ operand is NaN, so the result of min(0, NaN) and min(NaN, 0) could, confusingly, be different. This is difference is in fact visible through JavaScript. For instance, new DOMQuad({ x: NaN }, { x: 0 }, { x: 0 }, { x: 0 }).getBounds().x gives NaN, but new DOMQuad({ x: 0 }, { x: 0 }, { x: 0 }, { x: NaN }).getBounds().x gives 0. A similar problem is present for DOMRect/DOMRectReadOnly as well. This CL implements [1], which is to adopt semantics similar to JavaScript's Math.min() and Math.max(), which propagates NaN from either operand. This also aligns our behavior with WebKit. [1]: w3c/fxtf-drafts#395 Fixed: 1066499 Change-Id: Id9a4282fa00dafcfe9c5616643efbe2eaace411e -- wpt-commits: 452d7c5e1a0b683ccfbfaf6fcac833b3e96bb32e wpt-pr: 22585
This follows the rough consensus reached in #222, as well as what WebKit
implements today.
Fixes #222.
Tests: ...
I'm be happy to implement this in Chrome. If this is deemed acceptable, I could just write the WPTs as part of the Chromium change.