Bug 101006 - Text Autosizing: Change m_textAutosizingFontScaleFactor to double type
Summary: Text Autosizing: Change m_textAutosizingFontScaleFactor to double type
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-01 18:44 PDT by Jaehun Lim
Modified: 2012-11-07 10:53 PST (History)
8 users (show)

See Also:


Attachments
Patch (4.26 KB, patch)
2012-11-01 18:45 PDT, Jaehun Lim
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Fixed Patch (4.74 KB, patch)
2012-11-01 19:24 PDT, Jaehun Lim
kenneth: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaehun Lim 2012-11-01 18:44:06 PDT
WKPreferences in WK2 prefers to use double than float.
Change m_textAutosizingFontScaleFactor to double type for the consistency with WKPreferences.
Comment 1 Jaehun Lim 2012-11-01 18:45:59 PDT
Created attachment 171968 [details]
Patch
Comment 2 Jaehun Lim 2012-11-01 18:48:11 PDT
John Mellor agreed with this.
https://bugs.webkit.org/show_bug.cgi?id=100633#c15
Comment 3 Early Warning System Bot 2012-11-01 18:55:16 PDT
Comment on attachment 171968 [details]
Patch

Attachment 171968 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14705004
Comment 4 Early Warning System Bot 2012-11-01 18:55:57 PDT
Comment on attachment 171968 [details]
Patch

Attachment 171968 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14680101
Comment 5 EFL EWS Bot 2012-11-01 19:09:42 PDT
Comment on attachment 171968 [details]
Patch

Attachment 171968 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14685823
Comment 6 Build Bot 2012-11-01 19:21:38 PDT
Comment on attachment 171968 [details]
Patch

Attachment 171968 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14663710
Comment 7 Jaehun Lim 2012-11-01 19:24:42 PDT
Created attachment 171973 [details]
Fixed Patch
Comment 8 John Mellor 2012-11-05 04:31:50 PST
Looks good to me. jchaffraix, can you r+?
Comment 9 Kenneth Rohde Christiansen 2012-11-05 05:09:46 PST
Comment on attachment 171973 [details]
Fixed Patch

John said he was fine with this, so r=me
Comment 10 Julien Chaffraix 2012-11-05 05:24:13 PST
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.
Comment 11 John Mellor 2012-11-05 05:30:33 PST
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!
Comment 12 Kenneth Rohde Christiansen 2012-11-05 05:34:26 PST
(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.
Comment 13 Jaehun Lim 2012-11-05 15:38:05 PST
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.
Comment 14 Julien Chaffraix 2012-11-07 10:53:45 PST
> 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.