Bug 101006 – Text Autosizing: Change m_textAutosizingFontScaleFactor to double type

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Text

[Jaehun Lim]

WKPreferences in WK2 prefers to use double than float.
Change m_textAutosizingFontScaleFactor to double type for the consistency with WKPreferences.

[Jaehun Lim]

Created attachment 171968 [details]
Patch

[Jaehun Lim]

John Mellor agreed with this.
https://bugs.webkit.org/show_bug.cgi?id=100633#c15

[Early Warning System Bot]

Comment on attachment 171968 [details]
Patch

Attachment 171968 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14705004

[Early Warning System Bot]

Comment on attachment 171968 [details]
Patch

Attachment 171968 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14680101

[EFL EWS Bot]

Comment on attachment 171968 [details]
Patch

Attachment 171968 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14685823

[Build Bot]

Comment on attachment 171968 [details]
Patch

Attachment 171968 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14663710

[Jaehun Lim]

Created attachment 171973 [details]
Fixed Patch

[John Mellor]

Looks good to me. jchaffraix, can you r+?

[Kenneth Rohde Christiansen]

Comment on attachment 171973 [details]
Fixed Patch

John said he was fine with this, so r=me

[Julien Chaffraix]

Comment on attachment 171973 [details]
Fixed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171973&action=review

Please make this bug block the WK2 implementation (bug 100633).

There is one issue with the change as-is: the WebCore math is still float based which is not great. Because we manipulate fonts, it doesn't make sense to switch to 'double'. We may as well do the conversion at the Settings level (as we are doing now) and let the API take the value they want. 

IMHO this change doesn't add much but as other people may have different views so I won't block the change.

> Source/WebCore/ChangeLog:8
> +        WKPreferences in WK2 prefers to use double than float.

s/than/instead of/? (Note: I am not a native speaker but the use of 'than' here sounds wrong).

> Source/WebCore/ChangeLog:9
> +        Change m_textAutosizingFontScaleFactor to double type for the consistency with WKPreferences.

s/the consistency/consistency/.

> Source/WebCore/ChangeLog:11
> +        No need to test.

Usually you want to explain 'why' you don't add a test. Here is some examples:
* Refactoring, covered by existing tests.
* No change in behavior expected so no test.
* Type change only, no expected change in behavior.

[John Mellor]

Julien makes a good point, that the existing APIs in Settings.h are all double-based, but that's because they are all timeouts/durations. When dealing with font sizes WebCore tends to deal exclusively with floats (see RenderStyle.h, FontDescription.h, etc), so it makes sense for Settings::fontScaleFactor to be a float as well.

So it's probably fine to leave things as they stand before this change, i.e. WK2 API takes a double since they prefer that, but it gets converted to a float when calling the WebCore Settings API. Sorry we didn't think of this earlier!

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> Julien makes a good point, that the existing APIs in Settings.h are all double-based, but that's because they are all timeouts/durations. When dealing with font sizes WebCore tends to deal exclusively with floats (see RenderStyle.h, FontDescription.h, etc), so it makes sense for Settings::fontScaleFactor to be a float as well.
> 
> So it's probably fine to leave things as they stand before this change, i.e. WK2 API takes a double since they prefer that, but it gets converted to a float when calling the WebCore Settings API. Sorry we didn't think of this earlier!

I am OK with that.

[Jaehun Lim]

Thank you for all of your comments. I understood well.
I agree that m_textAutosizingFontScaleFactor remains unchanged.

Julien, Would let me know the example of "We may as well do the conversion at the Settings level (as we are doing now) and let the API take the value they want." ?
It will be helpful for me to make WK2 API.

[Julien Chaffraix]

> Julien, Would let me know the example of "We may as well do the conversion at the Settings level (as we are doing now) and let the API take the value they want." ?

I don't have a specific example in mind, most API convert the type at the WebKit level to match what WebCore expected (qfloat -> float for Qt, ...). Here we couldn't make the 2 types agree which is why we should just decouple the 2 and do the conversion when crossing the WebKit / WebCore border (which is what I meant by the "conversion at the Settings level").

There seem to be some agreement that the existing code is fine, closing the bug.