[webkit-reviews] review denied: [Bug 43395] [GTK/EFL] rendering was broken in naver.com : [Attachment 64495] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 17 11:03:04 PDT 2010


Antonio Gomes <tonikitoo at webkit.org> has denied Ryuan Choi
<ryuan.choi at samsung.com>'s request for review:
Bug 43395: [GTK/EFL] rendering was broken in naver.com
https://bugs.webkit.org/show_bug.cgi?id=43395

Attachment 64495: Patch
https://bugs.webkit.org/attachment.cgi?id=64495&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 65425)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,19 @@
> +2010-08-16  Ryuan Choi  <ryuan.choi at gmail.com>
> +
> +	   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 render "Missing plugin" when
we
> +	   don't have proper plugin.
> +
> +	   * platform/efl/RenderThemeEfl.cpp:
> +	   (WebCore::RenderThemeEfl::setDefaultFontSize):
> +	   (WebCore::RenderThemeEfl::defaultGUIFont):
> +	   (WebCore::RenderThemeEfl::systemFont):
> +	   * platform/efl/RenderThemeEfl.h:

Could you please explain why RenderThemeEfl::systemFont fixes it? Maybe
mentioning the code path.

Also you are not fixing the Gtk bug, but only EFL. Please make it explicit in
the patch/ChangeLog.

> --- WebCore/platform/efl/RenderThemeEfl.cpp	(revision 65424)
> +++ WebCore/platform/efl/RenderThemeEfl.cpp	(working copy)
> @@ -26,6 +26,7 @@
>  #include "config.h"
>  #include "RenderThemeEfl.h"
>  
> +#include "CSSValueKeywords.h"
>  #include "FileSystem.h"
>  #include "Frame.h"
>  #include "FrameView.h"
> @@ -631,6 +632,8 @@ void RenderThemeEfl::themeChanged()
>      applyPartDescriptions();
>  }
>  
> +float RenderThemeEfl::defaultFontSize = 16.0f;
> +
>  RenderThemeEfl::RenderThemeEfl(Page* page)
>      : RenderTheme()
>      , m_page(page)
> @@ -985,10 +988,28 @@ bool RenderThemeEfl::paintSearchField(Re
>      return paintThemePart(o, SearchField, i, rect);
>  }
>  
> -void RenderThemeEfl::systemFont(int, FontDescription&) const
> +void RenderThemeEfl::setDefaultFontSize(int size)
> +{
> +    defaultFontSize = size;
> +}
> +
> +const String& RenderThemeEfl::defaultGUIFont()

I personally do not like defaultGUIFont() naming much.

>  {
> -    // If you remove this notImplemented(), replace it with an comment that
explains why.

Please respect the request above :-)

> -    notImplemented();
> +    DEFINE_STATIC_LOCAL(String, fontFace, ("Sans"));
> +    return fontFace;
> +}
> +
> +void RenderThemeEfl::systemFont(int propId, FontDescription&
fontDescription) const
> +{
> +    // copied from RenderThemeChromiumSkia
> +    float fontSize = defaultFontSize;
> +
> +    fontDescription.firstFamily().setFamily(defaultGUIFont());
> +    fontDescription.setSpecifiedSize(fontSize);
> +    fontDescription.setIsAbsoluteSize(true);
> +    fontDescription.setGenericFamily(FontDescription::NoFamily);
> +    fontDescription.setWeight(FontWeightNormal);
> +    fontDescription.setItalic(false);
>  }
>  
>  }
> Index: WebCore/platform/efl/RenderThemeEfl.h
> ===================================================================
> --- WebCore/platform/efl/RenderThemeEfl.h	(revision 65424)
> +++ WebCore/platform/efl/RenderThemeEfl.h	(working copy)
> @@ -142,6 +142,14 @@ public:
>  
>      virtual void adjustSliderThumbStyle(CSSStyleSelector*, RenderStyle*,
Element*) const;
>      virtual bool paintSliderThumb(RenderObject*, const PaintInfo&, const
IntRect&);
> +
> +    static void setDefaultFontSize(int size);
> +
> +protected:
> +    static const String& defaultGUIFont();
> +
> +    static float defaultFontSize;
> +
>  private:
>      void createCanvas();
>      void createEdje();
> Index: WebKit/efl/ChangeLog
> ===================================================================
> --- WebKit/efl/ChangeLog	(revision 65425)
> +++ WebKit/efl/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> +2010-08-16  Ryuan Choi  <ryuan.choi at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [GTK/EFL] rendering was broken in naver.com
> +	   https://bugs.webkit.org/show_bug.cgi?id=43395
> +
> +	   call RenderThemeEfl::setDefaultFontSize when initialzed and user
want to
> +	   change.

Start the phrase with capital, and correct the spelling of "initialzed".

>  #include <Ecore.h>
> @@ -552,6 +553,7 @@ static Ewk_View_Private_Data* _ewk_view_
>      priv->page_settings->setLoadsImagesAutomatically(true);
>      priv->page_settings->setDefaultFixedFontSize(12);
>      priv->page_settings->setDefaultFontSize(16);
> +    WebCore::RenderThemeEfl::setDefaultFontSize(16);

I wonder the difference between page_settings->setDefaultFontSize(16) and
RenderThemeEfl::setDefaultFontSize(16);


More information about the webkit-reviews mailing list