Bug 100633 - Text Autosizing: Add Text Autosizing APIs for WK2
Summary: Text Autosizing: Add Text Autosizing APIs for WK2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: FontBoosting 99074
  Show dependency treegraph
 
Reported: 2012-10-29 00:02 PDT by Jaehun Lim
Modified: 2012-11-26 03:17 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.13 KB, patch)
2012-10-29 00:05 PDT, Jaehun Lim
buildbot: commit-queue-
Details | Formatted Diff | Diff
Fixed patch (7.26 KB, patch)
2012-10-29 00:30 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Fixed patch (6.88 KB, patch)
2012-10-30 16:48 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Patch (5.09 KB, patch)
2012-10-31 20:13 PDT, Jaehun Lim
sam: review-
Details | Formatted Diff | Diff
Add testcase (6.37 KB, patch)
2012-11-12 00:07 PST, Jaehun Lim
sam: review-
sam: commit-queue-
Details | Formatted Diff | Diff
patch (5.25 KB, patch)
2012-11-25 20:04 PST, Jaehun Lim
no flags 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-10-29 00:02:43 PDT
Add basic Text Autosizing APIs for WK2.

- WKPreferences{Set, Get}TextAutosizingEnabled()
- WKPreferences{Set, Get}TextAutosizingFontScaleFactor()
Comment 1 Jaehun Lim 2012-10-29 00:05:17 PDT
Created attachment 171164 [details]
Patch
Comment 2 Build Bot 2012-10-29 00:11:27 PDT
Comment on attachment 171164 [details]
Patch

Attachment 171164 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14626053
Comment 3 Jaehun Lim 2012-10-29 00:30:31 PDT
Created attachment 171168 [details]
Fixed patch
Comment 4 John Mellor 2012-10-29 04:13:16 PDT
Thanks for the cc. We may as well mark this as blocking the Text Autosizing master bug.
Comment 5 Jaehun Lim 2012-10-29 16:59:15 PDT
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.
Comment 6 Jaehun Lim 2012-10-29 20:11:54 PDT
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 7 John Mellor 2012-10-30 03:35:08 PDT
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.
Comment 8 Jaehun Lim 2012-10-30 16:40:41 PDT
(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.
Comment 9 Jaehun Lim 2012-10-30 16:48:45 PDT
Created attachment 171548 [details]
Fixed patch
Comment 10 Sam Weinig 2012-10-30 23:00:32 PDT
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.
Comment 11 Jaehun Lim 2012-10-30 23:56:40 PDT
(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 ?
Comment 12 Sam Weinig 2012-10-31 00:11:29 PDT
(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.
Comment 13 Jaehun Lim 2012-10-31 20:13:29 PDT
Created attachment 171762 [details]
Patch
Comment 14 John Mellor 2012-11-01 08:16:50 PDT
(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 15 John Mellor 2012-11-01 08:19:49 PDT
Comment on attachment 171762 [details]
Patch

The latest patch looks good to me too.
Comment 16 Jaehun Lim 2012-11-07 01:00:46 PST
Sam, please review the fixed patch.
Comment 17 Sam Weinig 2012-11-10 14:39:36 PST
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.
Comment 18 Jaehun Lim 2012-11-12 00:07:57 PST
Created attachment 173576 [details]
Add testcase
Comment 19 Sam Weinig 2012-11-15 15:04:47 PST
Comment on attachment 173576 [details]
Add testcase

r-, as this still doesn't seem to have a use case mentioned.
Comment 20 Jaehun Lim 2012-11-15 15:52:30 PST
(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.
Comment 21 Jaehun Lim 2012-11-19 21:54:03 PST
Sam, I added one use case in comment #19.
Please, review again.
Comment 22 Sam Weinig 2012-11-23 16:17:26 PST
I suppose having a pref to turn the feature on or off is acceptable, but I don't think the custom scale makes sense.
Comment 23 Jaehun Lim 2012-11-25 20:04:31 PST
Created attachment 175916 [details]
patch

I agree with sam.
APIs for scale factor are removed.
Comment 24 WebKit Review Bot 2012-11-25 21:53:27 PST
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
Comment 25 John Mellor 2012-11-26 02:55:12 PST
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 26 Peter Beverloo 2012-11-26 02:59:53 PST
Comment on attachment 175916 [details]
patch

Unused APIs are unlikely to cause SVG tests to fail. Setting cq+ again on request.
Comment 27 WebKit Review Bot 2012-11-26 03:16:56 PST
Comment on attachment 175916 [details]
patch

Clearing flags on attachment: 175916

Committed r135702: <http://trac.webkit.org/changeset/135702>
Comment 28 WebKit Review Bot 2012-11-26 03:17:01 PST
All reviewed patches have been landed.  Closing bug.