Add basic Text Autosizing APIs for WK2. - WKPreferences{Set, Get}TextAutosizingEnabled() - WKPreferences{Set, Get}TextAutosizingFontScaleFactor()
Created attachment 171164 [details] Patch
Comment on attachment 171164 [details] Patch Attachment 171164 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14626053
Created attachment 171168 [details] Fixed patch
Thanks for the cc. We may as well mark this as blocking the Text Autosizing master bug.
setFloatValueIfInUserDefaults() has nothing to do with Text Autosizing. I added these functions to fix build breaks. I think it's better to make another bug for them. I'll upload new patches soon.
I tried to split this patch (Text Autosizing + setFloatValueIfInUserDefaults()). But I couldn't because of "unused function error" in MAC port. I'm sorry to annoy you. Please review my current patch.
Comment on attachment 171168 [details] Fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=171168&action=review Looks fine to me. I'm not a WebKit reviewer though, nor am I familiar with WebKit2, so best to find a reviewer there. Kenneth, could you suggest a reviewer for this perhaps? > Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:949 > +#if ENABLE(TEXT_AUTOSIZING) I can't see any other preferences in this file which are guarded by feature defines. For example WKPreferencesSetWebArchiveDebugModeEnabled isn't guarded, even though setWebArchiveDebugModeEnabled is behind ENABLE(WEB_ARCHIVE) in WebCore/page/Settings.h. Are you sure WebKit2 style isn't to have these preferences always gettable/settable, even if their feature is disabled at compile time? (though I agree that this way makes sense) > Source/WebKit2/UIProcess/cf/WebPreferencesCF.cpp:96 > +static void setFloatValueIfInUserDefaults(const String& identifier, const String& baseKey, WebPreferencesStore& store) Are the setFloatValueIfInUserDefaults functions needed because FOR_EACH_WEBKIT_FLOAT_PREFERENCE is no longer empty? If so that seems reasonable.
(In reply to comment #7) > (From update of attachment 171168 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171168&action=review > > Looks fine to me. I'm not a WebKit reviewer though, nor am I familiar with WebKit2, so best to find a reviewer there. > > Kenneth, could you suggest a reviewer for this perhaps? > > > Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:949 > > +#if ENABLE(TEXT_AUTOSIZING) > > I can't see any other preferences in this file which are guarded by feature defines. For example WKPreferencesSetWebArchiveDebugModeEnabled isn't guarded, even though setWebArchiveDebugModeEnabled is behind ENABLE(WEB_ARCHIVE) in WebCore/page/Settings.h. Are you sure WebKit2 style isn't to have these preferences always gettable/settable, even if their feature is disabled at compile time? (though I agree that this way makes sense) It seems like no problem to remove guards. I'll upload new patch soon. > > > Source/WebKit2/UIProcess/cf/WebPreferencesCF.cpp:96 > > +static void setFloatValueIfInUserDefaults(const String& identifier, const String& baseKey, WebPreferencesStore& store) > > Are the setFloatValueIfInUserDefaults functions needed because FOR_EACH_WEBKIT_FLOAT_PREFERENCE is no longer empty? If so that seems reasonable. Yes, you are right. I want to split them from this patch, but I can't as I said my previous comments.
Created attachment 171548 [details] Fixed patch
Comment on attachment 171548 [details] Fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=171548&action=review > Source/WebKit2/UIProcess/API/C/WKPreferences.h:229 > +// Defaults to 1.0 > +WK_EXPORT void WKPreferencesSetTextAutosizingFontScaleFactor(WKPreferencesRef preferences, float fontScaleFactor); > +WK_EXPORT float WKPreferencesGetTextAutosizingFontScaleFactor(WKPreferencesRef preferences); All the other preferences use double, why use float here? I'd prefer we keep it consistent and use double.
(In reply to comment #10) > (From update of attachment 171548 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171548&action=review > > > Source/WebKit2/UIProcess/API/C/WKPreferences.h:229 > > +// Defaults to 1.0 > > +WK_EXPORT void WKPreferencesSetTextAutosizingFontScaleFactor(WKPreferencesRef preferences, float fontScaleFactor); > > +WK_EXPORT float WKPreferencesGetTextAutosizingFontScaleFactor(WKPreferencesRef preferences); > > All the other preferences use double, why use float here? I'd prefer we keep it consistent and use double. "Text Autosizing" uses float variable for font scale factor. So I just conform to its implementation. Using double instead of float in my patch is trivial. But I think it's strange to use different types between WebCore::Settings and WebKit::Preferences. If using double is a concrete rule. I'll follow. John, how about using double instead of float in Text Autosizing ? Text Autosizing is only feature using float in Settings. :( If you agree, I will fix. Sam, this is just my curiosity, is there any technical reasons to use double instead of float ?
(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 171548 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171548&action=review > > > > > Source/WebKit2/UIProcess/API/C/WKPreferences.h:229 > > > +// Defaults to 1.0 > > > +WK_EXPORT void WKPreferencesSetTextAutosizingFontScaleFactor(WKPreferencesRef preferences, float fontScaleFactor); > > > +WK_EXPORT float WKPreferencesGetTextAutosizingFontScaleFactor(WKPreferencesRef preferences); > > > > All the other preferences use double, why use float here? I'd prefer we keep it consistent and use double. > > "Text Autosizing" uses float variable for font scale factor. So I just conform to its implementation. > > Using double instead of float in my patch is trivial. But I think it's strange to use different types between WebCore::Settings and WebKit::Preferences. > > If using double is a concrete rule. I'll follow. > > John, how about using double instead of float in Text Autosizing ? > Text Autosizing is only feature using float in Settings. :( > If you agree, I will fix. > > Sam, this is just my curiosity, is there any technical reasons to use double instead of float ? The is no technical reason, but I would like the API to remain consistent unless there is a good reason to deviate, and I don't think this qualifies. Please do change it to a double.
Created attachment 171762 [details] Patch
(In reply to comment #11) > John, how about using double instead of float in Text Autosizing ? > Text Autosizing is only feature using float in Settings. :( > If you agree, I will fix. I'm fine with switching the textAutosizingFontScaleFactor to being a double in Settings.h/cpp. There was no strong reason for using a float. Thanks!
Comment on attachment 171762 [details] Patch The latest patch looks good to me too.
Sam, please review the fixed patch.
Comment on attachment 171762 [details] Patch Please add an API test for this new functionality. Also, please add what the use case for being able to control text autosizing at the preference level is.
Created attachment 173576 [details] Add testcase
Comment on attachment 173576 [details] Add testcase r-, as this still doesn't seem to have a use case mentioned.
(In reply to comment #19) > (From update of attachment 173576 [details]) > r-, as this still doesn't seem to have a use case mentioned. Sam, this is an optional, mobile specific feature like frame flattening. Frame flattening APIs are implemented in WKPreferences already. Text Autosizing adjusts the original CSS properties to enhance usability. But not all applications need it. I explain one scenario. When I see the news site using WebKit-based browser in mobile phone, text autosizing is useful. But when I go back home, and connect my phone to TV via cable or dock (I heard Motorola made this), text autosizing is useless at this time, because TV screen is big enough. And font scale factor should be adjustable according to the screen size or user preference, too.
Sam, I added one use case in comment #19. Please, review again.
I suppose having a pref to turn the feature on or off is acceptable, but I don't think the custom scale makes sense.
Created attachment 175916 [details] patch I agree with sam. APIs for scale factor are removed.
Comment on attachment 175916 [details] patch Attachment 175916 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14980596 New failing tests: svg/W3C-SVG-1.1/animate-elem-78-t.svg
The fontScaleFactor is indeed optional. It is intended to be used for accessibility purposes, controlled by either browser or OS text size settings; but that's something you can hook up later, according to your OS's accessibility conventions. For reference: Increasing the fontScaleFactor causes text that is already autosized to be proportionately larger (so when that block is fit to screen width, the legibility increases proportionately). As a consequence of this, it also reduces the minimum cluster width that gets autosized (with fontScaleFactor=1 and device-width=320 UI pixels, no cluster narrower than 320 CSS px gets autosized; but with fontScaleFactor=2, no cluster narrower than 160 CSS px gets autosized, and a 320 CSS px cluster would have a multiplier of 2x). Since the fontScaleFactor doesn't affect non-autosized text, it is intended to be integrated into double-tap zoom, such that double-tapping on non-autosized text zooms such that there are fontScaleFactor UI pixels per 1 CSS px. Thus the fontScaleFactor would directly scale up/down the effective text size that is seen after double-tapping to zoom in on text (whether or not that text is autosized).
Comment on attachment 175916 [details] patch Unused APIs are unlikely to cause SVG tests to fail. Setting cq+ again on request.
Comment on attachment 175916 [details] patch Clearing flags on attachment: 175916 Committed r135702: <http://trac.webkit.org/changeset/135702>
All reviewed patches have been landed. Closing bug.