Bug 43395 – [EFL] rendering was broken when missing plugin.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryuan Choi]

in latest build, I found wrongly rendered naver.com

in my poor tracing,
I think that it was related of RenderEmbeddedObject::getReplacementTextGeometry

I reproduced this in GTK and EFL port

[Ryuan Choi]

Created attachment 63285 [details]
error screen

[Ryuan Choi]

Created attachment 63293 [details]
Patch

[Ryuan Choi]

I create EFL patch first

[Rafael Antognolli]

(In reply to comment #2)
> Created an attachment (id=63293) [details]
> Patch

I'm not a reviewer, but I have some comments:

@@ -605,6 +606,8 @@ void RenderThemeEfl::themeChanged()
     applyPartDescriptions();
 }
 
+float RenderThemeEfl::defaultFontSize = 16.0f;
+

I know this is the defaultFontSize, but maybe we should get this from settings, as Chromium does. For this you would need to create a RenderThemeEfl::setDefaultFontSize(), and call it from ewk_view_setting_font_default_size_set and _ewk_view_priv_new (setting to 16 on the last one).

But I'm not sure if this is the best approach though.

+        // Why 2 points smaller? Because that's what Gecko does. Note that we
+        // are assuming a 96dpi screen, which is the default that we use on
+        // Windows.
+        static const float pointsPerInch = 72.0f;
+        static const float pixelsPerInch = 96.0f;
+        fontSize -= (2.0f / pointsPerInch) * pixelsPerInch;
+        break;
+    }

Well, at least you should remove the "Windows" part of this comment, no? :P
And I don't know exactly what pointsPerInch is for, but it's possible to get the dpi from Ecore_X. Maybe this helps...

[Ryuan Choi]

(In reply to comment #4)
> (In reply to comment #2)
> > Created an attachment (id=63293) [details] [details]
> > Patch
> 
> I'm not a reviewer, but I have some comments:
> 
> @@ -605,6 +606,8 @@ void RenderThemeEfl::themeChanged()
>      applyPartDescriptions();
>  }
> 
> +float RenderThemeEfl::defaultFontSize = 16.0f;
> +
> 
> I know this is the defaultFontSize, but maybe we should get this from settings, as Chromium does. For this you would need to create a RenderThemeEfl::setDefaultFontSize(), and call it from ewk_view_setting_font_default_size_set and _ewk_view_priv_new (setting to 16 on the last one).
> 
I agree, I'll prepare

> But I'm not sure if this is the best approach though.
> 
> +        // Why 2 points smaller? Because that's what Gecko does. Note that we
> +        // are assuming a 96dpi screen, which is the default that we use on
> +        // Windows.
> +        static const float pointsPerInch = 72.0f;
> +        static const float pixelsPerInch = 96.0f;
> +        fontSize -= (2.0f / pointsPerInch) * pixelsPerInch;
> +        break;
> +    }
> 
> Well, at least you should remove the "Windows" part of this comment, no? :P
> And I don't know exactly what pointsPerInch is for, but it's possible to get the dpi from Ecore_X. Maybe this helps...

yes, I just copied :)
I think we have more discussion about dpi, but we can remove this now.

[Ryuan Choi]

Created attachment 63416 [details]
Patch

[Rafael Antognolli]

Adding the two main EFL reviewers to CC.

[Leandro Pereira]

Comment on attachment 63416 [details]
Patch

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?

[Rafael Antognolli]

(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.

[Ryuan Choi]

(In reply to comment #8)
> (From update of attachment 63416 [details])
> 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?
> 
I found this bug in that site, but I believe that this is not for that site.
EWK can't render text "Missing Plugin" because of SystemFont called by RenderEmbeddedObject::getReplacementTextGeometry.

I'll try to add more comment.

> > +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'll check Sans Instead of Arial, I think that It's not problem

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

I agree with Rafael. Do I need to add getDefaultFontSize if we have ewk_view_setting_font_default_size_get ?

[Ryuan Choi]

Created attachment 63690 [details]
Patch

[Ryuan Choi]

Created attachment 64441 [details]
error case

this is simple test case to reproduce easily.

GtkLauncher and EWeblauncher can not render some contents because of missing plugin

missing plugin make cairo instance as error while rendering

[Ryuan Choi]

Created attachment 64495 [details]
Patch

[Antonio Gomes]

Comment on attachment 64495 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 65425)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Ryuan Choi  <[e-mail]>
> +
> +        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 @@
> [phone]  Ryuan Choi  <[e-mail]>
> +
> +        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);

[Ryuan Choi]

(In reply to comment #14)
> (From update of attachment 64495 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 65425)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,19 @@
> > [phone]  Ryuan Choi  <[e-mail]>
> > +
> > +        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.

At first, Thank you for reviewing.

I thought that this is cairo error so I add some segmemtation fault in _cairo_set_error.

below is stack trace
#0  _cairo_set_error (cr=0x81af208, status=CAIRO_STATUS_INVALID_MATRIX) at cairo.c:115
#1  0x00b8f4c6 in WebCore::Font::drawGlyphs (this=0xbfffdb90, context=0xbfffe988, font=0x84f1278, glyphBuffer=..., from=0, numGlyphs=15, point=...) at WebCore/platform/graphics/cairo/FontCairo.cpp:51
#2  0x00803eda in WebCore::Font::drawGlyphBuffer (this=0xbfffdb90, context=0xbfffe988, glyphBuffer=..., point=...) at WebCore/platform/graphics/FontFastPath.cpp:241
#3  0x00803d11 in WebCore::Font::drawSimpleText (this=0xbfffdb90, context=0xbfffe988, run=..., point=..., from=0, to=15) at WebCore/platform/graphics/FontFastPath.cpp:214
#4  0x007f5d59 in WebCore::Font::drawText (this=0xbfffdb90, context=0xbfffe988, run=..., point=..., from=0, to=15) at WebCore/platform/graphics/Font.cpp:154
#5  0x0080f564 in WebCore::GraphicsContext::drawBidiText (this=0xbfffe988, font=..., run=..., point=...) at WebCore/platform/graphics/GraphicsContext.cpp:366
#6  0x008e039b in WebCore::RenderEmbeddedObject::paintReplaced (this=0x84ba63c, paintInfo=..., tx=8, ty=27) at WebCore/rendering/RenderEmbeddedObject.cpp:420
#7  0x0092b0fe in WebCore::RenderReplaced::paint (this=0x84ba63c, paintInfo=..., tx=8, ty=27) at WebCore/rendering/RenderReplaced.cpp:145
#8  0x008dff87 in WebCore::RenderEmbeddedObject::paint (this=0x84ba63c, paintInfo=..., tx=8, ty=8) at WebCore/rendering/RenderEmbeddedObject.cpp:380

especially in frame 6,
webkit pass font infomation to drawBidiText after getting it in getReplacementTextGeometry.
but EFL doesn't implement systemFont() called by getReplacementTextGeometry.
so when webkit called cairo_set_scaled_font in Font::drawGlyphs, cairo was failed.


> 
> Also you are not fixing the Gtk bug, but only EFL. Please make it explicit in the patch/ChangeLog.
> 
Ok, I'll remove GTK letter in 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.
I just copied from chromium.
I think that I can remove this function into systemFont()

> 
> >  {
> > -    // If you remove this notImplemented(), replace it with an comment that explains why.
> 
> Please respect the request above :-)
> 
Ok I'll add some comment in systemFont()

> > -    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 @@
> > [phone]  Ryuan Choi  <[e-mail]>
> > +
> > +        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".
> 
I'm sorry. I'll do that.

> >  #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);

hmm. I'll check how defaultFont of settings can use.

[Ryuan Choi]

Created attachment 64809 [details]
Patch

[Antonio Gomes]

Comment on attachment 64809 [details]
Patch

r=me

[Ryuan Choi]

Created attachment 65069 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 65069 [details]
Patch

Clearing flags on attachment: 65069

Committed r65813: <http://trac.webkit.org/changeset/65813>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.