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.
Created attachment 166845 [details] Work in progress patch
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?
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?
(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.
Created attachment 167046 [details] Patch It's in the Ewk_Settings's constructor now.
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"
Hmm. It seems other ports are initializing this in Source/WebKit2/Shared/WebPreferencesStore.h
Created attachment 167089 [details] Work in progress
Comment on attachment 167089 [details] Work in progress Why WIP? Please add changelog.
Created attachment 167274 [details] patch
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.
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
Created attachment 168640 [details] patch I've made some api for the font settings in Ewk_Settings and moved the setting into MiniBrowser.
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!
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.
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.