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

Description Ahmad Saleem 2022-09-19 00:59:29 PDT

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!!

Comment 1 Nikolas Zimmermann 2022-09-21 03:53:30 PDT

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.

Comment 2 Ahmad Saleem 2022-09-21 12:45:39 PDT

Let me give it a spin via PR and see what EWS show.

Comment 3 Ahmad Saleem 2022-09-21 12:57:38 PDT

Let's try - https://github.com/WebKit/WebKit/pull/4580

Comment 4 Radar WebKit Bug Importer 2022-09-26 01:00:20 PDT

<rdar://problem/100401071>

Comment 5 Ahmad Saleem 2022-10-21 14:25:54 PDT

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".