Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2590433002:
Stricter float-to-int conversion in SVGIntegerOptionalInteger (Closed)
DescriptionStricter float-to-int conversion in SVGIntegerOptionalInteger
SVGIntegerOptionalInteger parses values as floats but stores them as
integers. Add helpers to avoid issues with overflow and to make this
conversion the same way in all places it's needed.
The "normal" parsing code would truncate the float value while the parsing
code for animation values would round. Make them both use truncation (and the
avoid duplicating the code.)
BUG=675130
Committed: https://crrev.com/c81442affec64a78e310df6c7dfd5a05ff520cf4
Cr-Commit-Position: refs/heads/master@{#439533}
Patch Set 1 #Patch Set 2 : Truncate, don't round #Patch Set 3 : ...and back to doing things differently. #
Total comments: 4
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Stricter float-to-int conversion in SVGIntegerOptionalInteger SVGIntegerOptionalInteger parses values as floats but stores them as integers. Add helpers to avoid issues with overflow and to make this conversion the same way in all places it's needed. BUG=675130 ========== to ========== Stricter float-to-int conversion in SVGIntegerOptionalInteger SVGIntegerOptionalInteger parses values as floats but stores them as integers. Add helpers to avoid issues with overflow and to make this conversion the same way in all places it's needed. The parsing code would truncate the float value while the interpolation code would round. Make them both use truncation. BUG=675130 ==========
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Stricter float-to-int conversion in SVGIntegerOptionalInteger SVGIntegerOptionalInteger parses values as floats but stores them as integers. Add helpers to avoid issues with overflow and to make this conversion the same way in all places it's needed. The parsing code would truncate the float value while the interpolation code would round. Make them both use truncation. BUG=675130 ========== to ========== Stricter float-to-int conversion in SVGIntegerOptionalInteger SVGIntegerOptionalInteger parses values as floats but stores them as integers. Add helpers to avoid issues with overflow and to make this conversion the same way in all places it's needed. The "normal" parsing code would truncate the float value while the parsing code for animation values would round. Make them both use truncation (and the avoid duplicating the code.) BUG=675130 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fs@opera.com changed reviewers: + pdr@chromium.org, schenney@chromium.org
LGTM https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html (right): https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html:1: <!DOCTYPE html> Please don't rewrite this, but for future patches like this, can you try creating unittests instead of layouttests?
https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html (right): https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html:1: <!DOCTYPE html> On 2016/12/19 at 19:42:54, pdr (OOO Dec 23 to Jan 2) wrote: > Please don't rewrite this, but for future patches like this, can you try creating unittests instead of layouttests? The reason for adding a layout test here is because it's web exposed behavior. I find that unit tests are not as well suitable for those types of tests. (As an example, Gecko passes the first of tests but fails the second - likely because they store a [clamped] float value. I've considered doing that as well.)
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html (right): https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html:1: <!DOCTYPE html> On 2016/12/19 at 19:54:54, fs wrote: > On 2016/12/19 at 19:42:54, pdr (OOO Dec 23 to Jan 2) wrote: > > Please don't rewrite this, but for future patches like this, can you try creating unittests instead of layouttests? > > The reason for adding a layout test here is because it's web exposed behavior. I find that unit tests are not as well suitable for those types of tests. (As an example, Gecko passes the first of tests but fails the second - likely because they store a [clamped] float value. I've considered doing that as well.) In terms of web exposed behavior, unittests and layouttests should be equivalent with a possible exception of running javascript. Do you mean that it's just easier to test against firefox if we use an html file?
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482177654929270,
"parent_rev": "db5194fed186c81dcd10b664c0f60f297b16f4c4", "commit_rev":
"30b8804fb69c982be42bb872ed0e4b8f9ffec9d7"}
Message was sent while issue was closed.
Description was changed from ========== Stricter float-to-int conversion in SVGIntegerOptionalInteger SVGIntegerOptionalInteger parses values as floats but stores them as integers. Add helpers to avoid issues with overflow and to make this conversion the same way in all places it's needed. The "normal" parsing code would truncate the float value while the parsing code for animation values would round. Make them both use truncation (and the avoid duplicating the code.) BUG=675130 ========== to ========== Stricter float-to-int conversion in SVGIntegerOptionalInteger SVGIntegerOptionalInteger parses values as floats but stores them as integers. Add helpers to avoid issues with overflow and to make this conversion the same way in all places it's needed. The "normal" parsing code would truncate the float value while the parsing code for animation values would round. Make them both use truncation (and the avoid duplicating the code.) BUG=675130 Review-Url: https://codereview.chromium.org/2590433002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Stricter float-to-int conversion in SVGIntegerOptionalInteger SVGIntegerOptionalInteger parses values as floats but stores them as integers. Add helpers to avoid issues with overflow and to make this conversion the same way in all places it's needed. The "normal" parsing code would truncate the float value while the parsing code for animation values would round. Make them both use truncation (and the avoid duplicating the code.) BUG=675130 Review-Url: https://codereview.chromium.org/2590433002 ========== to ========== Stricter float-to-int conversion in SVGIntegerOptionalInteger SVGIntegerOptionalInteger parses values as floats but stores them as integers. Add helpers to avoid issues with overflow and to make this conversion the same way in all places it's needed. The "normal" parsing code would truncate the float value while the parsing code for animation values would round. Make them both use truncation (and the avoid duplicating the code.) BUG=675130 Committed: https://crrev.com/c81442affec64a78e310df6c7dfd5a05ff520cf4 Cr-Commit-Position: refs/heads/master@{#439533} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c81442affec64a78e310df6c7dfd5a05ff520cf4 Cr-Commit-Position: refs/heads/master@{#439533}
Message was sent while issue was closed.
https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html (right): https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html:1: <!DOCTYPE html> On 2016/12/19 at 20:01:51, pdr (OOO Dec 23 to Jan 2) wrote: > On 2016/12/19 at 19:54:54, fs wrote: > > On 2016/12/19 at 19:42:54, pdr (OOO Dec 23 to Jan 2) wrote: > > > Please don't rewrite this, but for future patches like this, can you try creating unittests instead of layouttests? > > > > The reason for adding a layout test here is because it's web exposed behavior. I find that unit tests are not as well suitable for those types of tests. (As an example, Gecko passes the first of tests but fails the second - likely because they store a [clamped] float value. I've considered doing that as well.) > > In terms of web exposed behavior, unittests and layouttests should be equivalent with a possible exception of running javascript. Do you mean that it's just easier to test against firefox if we use an html file? Yes, that's a large part of it. It also makes sure we don't miss to consider any WebIDL behavior (easily forgotten in a unit test.)
Message was sent while issue was closed.
On 2016/12/19 at 20:16:23, fs wrote: > https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html (right): > > https://codereview.chromium.org/2590433002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/svg/dom/integer-optional-integer-value-range.html:1: <!DOCTYPE html> > On 2016/12/19 at 20:01:51, pdr (OOO Dec 23 to Jan 2) wrote: > > On 2016/12/19 at 19:54:54, fs wrote: > > > On 2016/12/19 at 19:42:54, pdr (OOO Dec 23 to Jan 2) wrote: > > > > Please don't rewrite this, but for future patches like this, can you try creating unittests instead of layouttests? > > > > > > The reason for adding a layout test here is because it's web exposed behavior. I find that unit tests are not as well suitable for those types of tests. (As an example, Gecko passes the first of tests but fails the second - likely because they store a [clamped] float value. I've considered doing that as well.) > > > > In terms of web exposed behavior, unittests and layouttests should be equivalent with a possible exception of running javascript. Do you mean that it's just easier to test against firefox if we use an html file? > > Yes, that's a large part of it. It also makes sure we don't miss to consider any WebIDL behavior (easily forgotten in a unit test.) cool, sgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
