Bug 245356
| Summary: | Make narrowPrecisionToFloat() use clampTo<float>() instead of static_cast | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Ahmad Saleem <ahmad.saleem792> |
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED INVALID | ||
| Severity: | Normal | CC: | ap, cdumez, karlcow, rniwa, sabouhallawa, webkit-bug-importer, zalan, zimmermann |
| Priority: | P2 | Keywords: | BrowserCompat, InRadar |
| Version: | Safari Technology Preview | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
Ahmad Saleem
Following Blink patch changed:
https://src.chromium.org/viewvc/blink?view=revision&revision=182563
below code:
Webkit GitHub - https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/WebCore/platform/FloatConversion.h#L44
It addresses following issue:
For numbers within the range of valid floats, this has no effect. For numbers
outside that range, the old code was technically undefined behavior (and in
practice would result in bad things like converting to infinity). Now such
numbers will be deterministically clamped to +/-FLOAT_MAX
This issue in large zoom-in for SVG etc.
I think it would be good to merge this patch unless if people advise, it shouldn't be case.
Thanks!!
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Nikolas Zimmermann
Seems like a valid change. Thanks to SFINAE clampTo<> generates fast code - boils down to a static_cast for doubles within "float" range, plus some additional branching/testing overhead -- hopefully small enough. I think it's fine -- did you try running full layout tests to see if we have differences anywhere due to this?
Do you have a testcase which gets fixed after your patch? (Blink mentions some svg/text/ test + large zoom factor). That would be ideal.
Ahmad Saleem
Let me give it a spin via PR and see what EWS show.
Ahmad Saleem
Let's try - https://github.com/WebKit/WebKit/pull/4580
Radar WebKit Bug Importer
<rdar://problem/100401071>
Ahmad Saleem
Valid explanation from Darin on why this change is not right approach:
It’s fine to change this, but I don’t like the tactical choice.
If we want to use clampTo<float> we should change the call sites, not change the meaning of this function. The reason we invented narrowPrecisionToFloat was to give a way to convert to float that would not warn and would not be type-unsafe, since static_cast<float> can be used on other types, not just double. It’s possible that we don’t need that, and it would be better to clamp. If so, I’d like to be explicit about the clamping. Unless perhaps we want a type-safe version of clampTo<float> that compiles only for double and gives a compiler error if you use it on other types. In that case I think a new function would be OK, but I would not name it the same thing.
_____
If we need to do this, we either have another function "narrowPrecisionToFloat" for type-unsafe use case or type-safe use cases and brute forcing it for all is not ideal choice. Closing this as "RESOLVED INVALID".