| Summary: | Text Autosizing: Change m_textAutosizingFontScaleFactor to double type | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jaehun Lim <ljaehun.lim> | ||||||
| Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED WONTFIX | ||||||||
| Severity: | Normal | CC: | gustavo, jchaffraix, johnme, kenneth, philn, sam, webkit.review.bot, xan.lopez | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Jaehun Lim
2012-11-01 18:44:06 PDT
Created attachment 171968 [details]
Patch
John Mellor agreed with this. https://bugs.webkit.org/show_bug.cgi?id=100633#c15 Comment on attachment 171968 [details] Patch Attachment 171968 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14705004 Comment on attachment 171968 [details] Patch Attachment 171968 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14680101 Comment on attachment 171968 [details] Patch Attachment 171968 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14685823 Comment on attachment 171968 [details] Patch Attachment 171968 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14663710 Created attachment 171973 [details]
Fixed Patch
Looks good to me. jchaffraix, can you r+? Comment on attachment 171973 [details]
Fixed Patch
John said he was fine with this, so r=me
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. 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! (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. 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, 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.
|