Prevent floating point overflow when using calc() with large values

Prevent floating point overflow when using calc() with large values

BUG=676286
Committed: https://crrev.com/66a95ed79bb18584d8ea1b045e43a96ec4e80dfc
Cr-Commit-Position: refs/heads/master@{#441339}

[alancutter (OOO until 2018), 2016-12-22 05:41:44]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[alancutter (OOO until 2018), 2016-12-22 05:41:53]

alancutter@chromium.org changed reviewers:
+ sashab@chromium.org

[commit-bot: I haz the power, 2016-12-22 05:41:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 09:17:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 09:17:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sashab, 2017-01-02 23:42:57]

Well done, LGTM!! How do we build in this clampTo<> functionality without having
to put it all over the place?

[alancutter (OOO until 2018), 2017-01-02 23:55:30]

On 2017/01/02 at 23:42:57, sashab wrote:
> Well done, LGTM!! How do we build in this clampTo<> functionality without
having to put it all over the place?

The issue is with the math operators (+-*/) having overflow behaviour. To
automatically clampTo<> the results you could create a wrapper class that
redefines the +-*/ operators as desired.

[alancutter (OOO until 2018), 2017-01-02 23:55:35]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2017-01-02 23:55:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 00:01:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 00:02:00]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alancutter (OOO until 2018), 2017-01-03 00:42:32]

alancutter@chromium.org changed reviewers:
+ eae@chromium.org

[alancutter (OOO until 2018), 2017-01-03 00:42:32]

+eae for platform/LengthFunctions.cpp.

[eae, 2017-01-03 04:37:30]

LGTM w/caveat

https://codereview.chromium.org/2597103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/LengthFunctions.cpp (right):

https://codereview.chromium.org/2597103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/LengthFunctions.cpp:41: return
clampTo<float>(maximumValue * length.percent() / 100.0f);
clampTo is significantly more expensive than a static_cast. You might wan to
consider using saturation arithmetic instead. At the very least please be aware
of the overhead and monitor our perf tests and numbers carefully.

[alancutter (OOO until 2018), 2017-01-03 05:17:02]

https://codereview.chromium.org/2597103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/LengthFunctions.cpp (right):

https://codereview.chromium.org/2597103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/LengthFunctions.cpp:41: return
clampTo<float>(maximumValue * length.percent() / 100.0f);
On 2017/01/03 at 04:37:30, eae wrote:
> clampTo is significantly more expensive than a static_cast. You might wan to
consider using saturation arithmetic instead. At the very least please be aware
of the overhead and monitor our perf tests and numbers carefully.

The performance impact of this is a bit scary actually. The undefined behaviour
fuzzer never highlighted this file so I might just leave it alone and stick with
just fixing calc() code instead.

[alancutter (OOO until 2018), 2017-01-03 05:48:47]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2017-01-03 05:48:47]

The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2597103002/#ps40001
(title: "Revert changes to LengthFunctions.cpp")

[commit-bot: I haz the power, 2017-01-03 05:49:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 08:33:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 08:33:33]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alancutter (OOO until 2018), 2017-01-04 04:23:30]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2017-01-04 04:23:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2597103002/#ps60001
(title: "Remove layout affected test case")

[commit-bot: I haz the power, 2017-01-04 04:23:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 07:04:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 07:04:53]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alancutter (OOO until 2018), 2017-01-04 07:05:57]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2017-01-04 07:06:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 08:09:05]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483513557440810,
"parent_rev": "bbaa5bd7c5957318f1441e4c1cf4cb0ccb27f1e9", "commit_rev":
"5bc70fa14d5a408437469e1b874ce5894f57e58d"}

[commit-bot: I haz the power, 2017-01-04 08:09:34]

Description was changed from

==========
Prevent floating point overflow when using calc() with large values

BUG=676286
==========

to

==========
Prevent floating point overflow when using calc() with large values

BUG=676286

Review-Url: https://codereview.chromium.org/2597103002
==========

[commit-bot: I haz the power, 2017-01-04 08:09:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-04 08:11:31]

Description was changed from

==========
Prevent floating point overflow when using calc() with large values

BUG=676286

Review-Url: https://codereview.chromium.org/2597103002
==========

to

==========
Prevent floating point overflow when using calc() with large values

BUG=676286

Committed: https://crrev.com/66a95ed79bb18584d8ea1b045e43a96ec4e80dfc
Cr-Commit-Position: refs/heads/master@{#441339}
==========

[commit-bot: I haz the power, 2017-01-04 08:11:32]

Patchset 4 (id:??) landed as
https://crrev.com/66a95ed79bb18584d8ea1b045e43a96ec4e80dfc
Cr-Commit-Position: refs/heads/master@{#441339}