[Webkit-unassigned] [Bug 43395] [GTK/EFL] rendering was broken in naver.com

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43395





--- Comment #8 from Leandro Pereira <leandro at profusion.mobi>  2010-08-05 09:53:55 PST ---
(From update of attachment 63416)
I'm not a reviewer, so consider this as an informal review.

> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [GTK/EFL] rendering was broken in naver.com
> +        https://bugs.webkit.org/show_bug.cgi?id=43395
> +
> +        implement RenderThemeEfl::systemFont to fix broken rendering in www.naver.com
> +        FontDescription without size make cairo status as error while rendering
> +

Is this patch only a workaround a buggy website? Or is the current implementation buggy?

> +const String& RenderThemeEfl::defaultGUIFont()
>  {
> -    // If you remove this notImplemented(), replace it with an comment that explains why.
> -    notImplemented();
> +    DEFINE_STATIC_LOCAL(String, fontFace, ("Arial"));
> +    return fontFace;
> +}

Why "Arial" is inside parenthesis? What happens if the user doesn't have this font installed? Doesn't using Sans instead of Arial provides a better fallback?

> +
> +    static void setDefaultFontSize(int size);
> +

There isn't a way to obtain the font size; it might be useful to display it on a settings UI or something. Also, wouldn't it be useful to have getters/setters for the defaultFontFace also?

> +
> +        * ewk/ewk_view.cpp:
> +        (_ewk_view_priv_new):
> +        (ewk_view_setting_font_default_size_set):
> +

Similar comment applies: wouldn't it be useful to also have a getter/setter for the defaultFontSize and defaultFontFace properties?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.