[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 #9 from Rafael Antognolli <antognolli at profusion.mobi>  2010-08-05 10:09:37 PST ---
(In reply to comment #8)
> (From update of attachment 63416 [details])
> I'm not a reviewer, so consider this as an informal review.
> 
> > +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?

I think the reason is in the comment from chromium port:

"We aim to match IE here.
-IE uses a font based on the encoding as the default font for form controls.
-Gecko uses MS Shell Dlg (actually calls GetStockObject(DEFAULT_GUI_FONT),
which returns MS Shell Dlg)
-Safari uses Lucida Grande.

FIXME: The only case where we know we don't match IE is for ANSI encodings.
IE uses MS Shell Dlg there, which we render incorrectly at certain pixel
sizes (e.g. 15px). So, for now we just use Arial."

Maybe just using Sans is enough, but I would have to test and understand what is the correct layout for this site.

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

Actually, the way that the browser should get the font size would be through ewk_view_setting_font_default_size_get, which is already provided. His patch is adding this method so that he could call it from ewk_view_setting_font_default_size_set. Do you see any use for a getDefaultFontSize method?

> > +
> > +        * 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?

There's already some methods for this. Just check the ewk_view.h.

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