Bug 100633 – Text Autosizing: Add Text Autosizing APIs for WK2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jaehun Lim]

Add basic Text Autosizing APIs for WK2.

- WKPreferences{Set, Get}TextAutosizingEnabled()
- WKPreferences{Set, Get}TextAutosizingFontScaleFactor()

[Jaehun Lim]

Created attachment 171164 [details]
Patch

[Build Bot]

Comment on attachment 171164 [details]
Patch

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

[Jaehun Lim]

Created attachment 171168 [details]
Fixed patch

[John Mellor]

Thanks for the cc. We may as well mark this as blocking the Text Autosizing master bug.

[Jaehun Lim]

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.

[Jaehun Lim]

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.

[John Mellor]

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.

[Jaehun Lim]

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

[Jaehun Lim]

Created attachment 171548 [details]
Fixed patch

[Sam Weinig]

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.

[Jaehun Lim]

(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 ?

[Sam Weinig]

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

[Jaehun Lim]

Created attachment 171762 [details]
Patch

[John Mellor]

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

[John Mellor]

Comment on attachment 171762 [details]
Patch

The latest patch looks good to me too.

[Jaehun Lim]

Sam, please review the fixed patch.

[Sam Weinig]

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.

[Jaehun Lim]

Created attachment 173576 [details]
Add testcase

[Sam Weinig]

Comment on attachment 173576 [details]
Add testcase

r-, as this still doesn't seem to have a use case mentioned.

[Jaehun Lim]

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

[Jaehun Lim]

Sam, I added one use case in comment #19.
Please, review again.

[Sam Weinig]

I suppose having a pref to turn the feature on or off is acceptable, but I don't think the custom scale makes sense.

[Jaehun Lim]

Created attachment 175916 [details]
patch

I agree with sam.
APIs for scale factor are removed.

[WebKit Review Bot]

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

[John Mellor]

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

[Peter Beverloo]

Comment on attachment 175916 [details]
patch

Unused APIs are unlikely to cause SVG tests to fail. Setting cq+ again on request.

[WebKit Review Bot]

Comment on attachment 175916 [details]
patch

Clearing flags on attachment: 175916

Committed r135702: <http://trac.webkit.org/changeset/135702>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.