Bug 98251 – [EFL][WK2] Setting the standard font familiy to "sans"

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Zoltan Nyul]

Setting  the standard font family to "sans" like it's done in wk1 efl makes some translate-transformation test pass because the transformation relies on the font's xHeight.

[Zoltan Nyul]

Created attachment 166845 [details]
Work in progress patch

[Chris Dumez]

Comment on attachment 166845 [details]
Work in progress patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166845&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:787
> +    WKPreferencesSetStandardFontFamily(pref, wkFontSans.get());

Could we maybe move this to the Ewk_Settings constructor?

[Chris Dumez]

Comment on attachment 166845 [details]
Work in progress patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166845&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:787
>> +    WKPreferencesSetStandardFontFamily(pref, wkFontSans.get());
> 
> Could we maybe move this to the Ewk_Settings constructor?

WebKit1 also sets SansSerifFontFamily to "sans", why not add it in WK2 as well?

[Zoltan Nyul]

(In reply to comment #3)
> (From update of attachment 166845 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166845&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:787
> >> +    WKPreferencesSetStandardFontFamily(pref, wkFontSans.get());
> > 
> > Could we maybe move this to the Ewk_Settings constructor?
> 
> WebKit1 also sets SansSerifFontFamily to "sans", why not add it in WK2 as well?

Indeed, i added the Serif and Fixed font settings too as in the WebKit1.

[Zoltan Nyul]

Created attachment 167046 [details]
Patch

It's in the Ewk_Settings's constructor now.

[Chris Dumez]

Comment on attachment 167046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167046&action=review

LGTM with a few nits.

> Source/WebKit2/ChangeLog:8
> +        Setting  the standard font family to "sans" like it's done in wk1 efl

"Setting  the" <- extra space.
"WK1 EFL"

[Chris Dumez]

Hmm. It seems other ports are initializing this in Source/WebKit2/Shared/WebPreferencesStore.h

[Zoltan Nyul]

Created attachment 167089 [details]
Work in progress

[Chris Dumez]

Comment on attachment 167089 [details]
Work in progress

Why WIP? Please add changelog.

[Zoltan Nyul]

Created attachment 167274 [details]
patch

[Jussi Kukkonen (jku)]

Comment on attachment 167274 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167274&action=review

> Source/WebKit2/ChangeLog:10
> +        transformation relies on the font's xHeight.

If this change alone makes tests pass, you should remove them from TestExpectations. If not, the comment here could probably be reworded.

[Kenneth Rohde Christiansen]

Comment on attachment 167274 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167274&action=review

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Setting the standard font familiy to "sans"

Spelling issue. Anyway this is weird, default fonts are supposed to be non-sans, ie something like times.

> Source/WebKit2/Shared/WebPreferencesStore.h:214
> +#elif PLATFORM(EFL)
> +
> +#define FOR_EACH_WEBKIT_FONT_FAMILY_PREFERENCE(macro) \
> +    macro(StandardFontFamily, standardFontFamily, String, String, "Sans") \
> +    macro(CursiveFontFamily, cursiveFontFamily, String, String, "Comic Sans MS") \
> +    macro(FantasyFontFamily, fantasyFontFamily, String, String, "Impact") \
> +    macro(FixedFontFamily, fixedFontFamily, String, String, "Courier New") \
> +    macro(SansSerifFontFamily, sansSerifFontFamily, String, String, "Helvetica") \
> +    macro(SerifFontFamily, serifFontFamily, String, String, "Times") \
> +    macro(PictographFontFamily, pictographFontFamily, String, String, "Times") \
> +    \
> +
> +
>  #endif

I dont really think it is nice deverging from other ports, especially this way

[Zoltan Nyul]

Created attachment 168640 [details]
patch

I've made some api for the font settings in Ewk_Settings and moved the setting into MiniBrowser.

[Kenneth Rohde Christiansen]

Comment on attachment 168640 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168640&action=review

> Tools/ChangeLog:9
> +
> +        Set the standard font to "sans" in the MiniBrowser.
> +

Missing reasoning!

[Gyuyoung Kim]

Comment on attachment 168640 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168640&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:158
> +    WKRetainPtr<WKStringRef> wkFont =  adoptWK(WKStringCreateWithUTF8CString(font));

It seems WK2 EFL port prefers to use below style for this.

WKRetainPtr<WKStringRef> wkFont(AdoptWK, WKStringCreateWithUTF8CString(font));

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:234
> +EAPI Eina_Bool ewk_settings_standard_font_family_set(Ewk_Settings* settings, const char* font);

Wrong '*' place.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:257
> +EAPI Eina_Bool ewk_settings_fixed_font_family_set(Ewk_Settings* settings, const char* font);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:280
> +EAPI Eina_Bool ewk_settings_serif_font_family_set(Ewk_Settings* settings, const char* font);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:297
> + * @param font Font family to set.

Font -> font ?

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:303
> +EAPI Eina_Bool ewk_settings_sans_serif_font_family_set(Ewk_Settings* settings, const char* font);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:326
> +EAPI Eina_Bool ewk_settings_cursive_font_family_set(Ewk_Settings* settings, const char* font);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:343
> + * @param font Font family to set.

We don't use . @param.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:349
> +EAPI Eina_Bool ewk_settings_fantasy_font_family_set(Ewk_Settings* settings, const char* font);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:358
> +EAPI const char* ewk_settings_fantasy_font_family_get(const Ewk_Settings *settings);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:366
> + * @param font Font family to set.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:372
> +EAPI Eina_Bool ewk_settings_pictograph_font_family_set(Ewk_Settings* settings, const char* font);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:381
> +EAPI const char* ewk_settings_pictograph_font_family_get(const Ewk_Settings *settings);

ditto.

[Zoltan Nyul]

I was wrong, it turned out that this issue was caused by cairo, the cairo_scaled_font_text_extents calucated wrong height for some fonts like Times, but it was ok with sans and some others. The cairo update has solved it.