Bug 89842 – [EFL] Move codes related to theme setting from Widget to RenderTheme

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

WebKit/Efl uses custom theme for Scrollbar, RenderTheme, Cursor.
However, Theme information itself is in WidgetEfl so it is accessed by calling recursive function.

Because theme can be managed by each page,
we'd better to move it to RenderThemeEfl which page contains.

[Ryuan Choi]

Created attachment 149222 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 149222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149222&action=review

> Source/WebCore/ChangeLog:11
> +        Because theme is managed by each page, this patch moves theme related codes

I don't understand below sentence.

*theme related codes*, Do you mean you move codes related to theme setting ?

> Source/WebKit/efl/ChangeLog:8
> +        Call RenderThemeEfl::setThemePath instaed of setting theme of FrameView.

s/instaed/instead/g

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:626
> +        m_canvas = ecore_evas_buffer_new(1, 1);

Are you sure that ecore_evas_buffer_new never returns null ?

[Ryuan Choi]

Created attachment 149892 [details]
Patch

[Ryuan Choi]

Created attachment 149893 [details]
Patch

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 149222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149222&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Because theme is managed by each page, this patch moves theme related codes
> 
> I don't understand below sentence.
> 
> *theme related codes*, Do you mean you move codes related to theme setting ?
> 
OK, I changed it like you mentioned.

> > Source/WebKit/efl/ChangeLog:8
> > +        Call RenderThemeEfl::setThemePath instaed of setting theme of FrameView.
> 
> s/instaed/instead/g
Fixed.

> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:626
> > +        m_canvas = ecore_evas_buffer_new(1, 1);
> 
> Are you sure that ecore_evas_buffer_new never returns null ?
Fixed.

[Chris Dumez]

Comment on attachment 149893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149893&action=review

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:628
> +            ASSERT(m_canvas);

What's the point of this ASSERT? You already know that m_canvas is NULL. Maybe you want ASSERT_NOT_REACHED(); ?

[Ryuan Choi]

Created attachment 149903 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> (From update of attachment 149893 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149893&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:628
> > +            ASSERT(m_canvas);
> 
> What's the point of this ASSERT? You already know that m_canvas is NULL. Maybe you want ASSERT_NOT_REACHED(); ?

Make sense, I changed.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 149903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149903&action=review

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:-460
> -    else {
> -        m_edje = edje_object_add(ecore_evas_get(m_canvas));
> -        if (!m_edje)
> -            EINA_LOG_ERR("Could not create base edje object.");
> -        else if (!edje_object_file_set(m_edje, theme.utf8().data(), "webkit/base")) {
> -            Edje_Load_Error err = edje_object_load_error_get(m_edje);
> -            const char* errmsg = edje_load_error_str(err);
> -            EINA_LOG_ERR("Could not load 'webkit/base' from theme %s: %s",
> -                         theme.utf8().data(), errmsg);
> -            evas_object_del(m_edje);
> -            m_edje = 0;
> -        } else {
> -#define CONNECT(cc, func)                                               \
> -            edje_object_signal_callback_add(m_edje, "color_class,set",  \
> -                                            "webkit/"cc, func, this)
> -
> -            CONNECT("selection/active",
> -                    renderThemeEflColorClassSelectionActive);
> -            CONNECT("selection/inactive",
> -                    renderThemeEflColorClassSelectionInactive);
> -            CONNECT("focus_ring", renderThemeEflColorClassFocusRing);

As much as getting rid of the useless else block makes sense, it's unrelated to the purpose of this patch.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:-664
> -    if (page && page->mainFrame() && page->mainFrame()->view())
> -        themeChanged();

Why is this not needed anymore?

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:101
> -    if (!object) {
> -        if (!view || !view->evas() || !view->evasObject())
> -            return;
> +    if (!view || !view->evas() || !view->evasObject())
> +        return;
>  
> +    if (!object) {

Ditto.

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:-112
> -    } else if (!view || !view->evas() || !view->evasObject()) {
> -        evas_object_hide(object);
> -        return;

Why is this not needed anymore?

> Source/WebKit/CMakeLists.txt:19
> +    "${WEBCORE_DIR}/html/shadow"

What's needed from here?

[Ryuan Choi]

Comment on attachment 149903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149903&action=review

>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:-460
>> -            CONNECT("focus_ring", renderThemeEflColorClassFocusRing);
> 
> As much as getting rid of the useless else block makes sense, it's unrelated to the purpose of this patch.

Yes, this is some refactoring and it's not related to this bug.

I will remain as it is.

>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:-664
>> -        themeChanged();
> 
> Why is this not needed anymore?

In previous implementation,
RenderThemeEfl does not have theme path so that it should try to get path from root widget of view.
After this change,
RenderThemeEfl itself contains theme path so that it can control when theme is really changed.

Indeed, we can not set theme path before creating RenderThemeEfl 
because ewk_view, Page and RenderTheme is created sequentially before setting theme.

>> Source/WebCore/platform/efl/ScrollbarEfl.cpp:101
>> +    if (!object) {
> 
> Ditto.

This is not related to this bug.
I will remain as it is.

>> Source/WebCore/platform/efl/ScrollbarEfl.cpp:-112
>> -        return;
> 
> Why is this not needed anymore?

Ditto.

>> Source/WebKit/CMakeLists.txt:19
>> +    "${WEBCORE_DIR}/html/shadow"
> 
> What's needed from here?

RenderThemeEfl requires this.

[Ryuan Choi]

Created attachment 150350 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150350&action=review

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:-437
> -    String theme = view ? view->edjeThemeRecursive() : "";

I wonder whether m_themePath has initial value. It seems that existing implementation has an init path by default.
For example,
 =>  /home/gyuyoung/webkit/WebKit/WebKitBuild/Release/WebKit/efl/DefaultTheme/default.edj

[Ryuan Choi]

(In reply to comment #12)
> (From update of attachment 150350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150350&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:-437
> > -    String theme = view ? view->edjeThemeRecursive() : "";
> 
> I wonder whether m_themePath has initial value. It seems that existing implementation has an init path by default.
> For example,
>  =>  /home/gyuyoung/webkit/WebKit/WebKitBuild/Release/WebKit/efl/DefaultTheme/default.edj

No, It does not have initial value.
It's same as current implementation.

If you tested EWebLauncher, EWebLauncher calle ewk_view_theme_set.
http://trac.webkit.org/browser/trunk/Tools/EWebLauncher/main.c#L675

[Gyuyoung Kim]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 150350 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=150350&action=review
> > 
> > > Source/WebCore/platform/efl/RenderThemeEfl.cpp:-437
> > > -    String theme = view ? view->edjeThemeRecursive() : "";
> > 
> > I wonder whether m_themePath has initial value. It seems that existing implementation has an init path by default.
> > For example,
> >  =>  /home/gyuyoung/webkit/WebKit/WebKitBuild/Release/WebKit/efl/DefaultTheme/default.edj
> 
> No, It does not have initial value.
> It's same as current implementation.
> 
> If you tested EWebLauncher, EWebLauncher calle ewk_view_theme_set.
> http://trac.webkit.org/browser/trunk/Tools/EWebLauncher/main.c#L675

If so, looks good to me.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 150350 [details]
Patch

Looks good to me as well, provided there are no pixel test regressions.

[Ryuan Choi]

(In reply to comment #15)
> (From update of attachment 150350 [details])
> Looks good to me as well, provided there are no pixel test regressions.

In my local, I tried pixel tests of fast/forms with and without patch.
They return same pass rate.

[Dominik Röttsches (drott)]

Nice, much better results for WTR
"29439 tests ran as expected, 833 didn't" (Details: http://pastebin.com/cE9nH6qA)
when combining this one with bug 90107.

[Ryuan Choi]

Created attachment 152701 [details]
Patch

[Ryuan Choi]

(In reply to comment #17)
> Nice, much better results for WTR
> "29439 tests ran as expected, 833 didn't" (Details: http://pastebin.com/cE9nH6qA)
> when combining this one with bug 90107.

I updated because bug 90107 was landed.

[WebKit Review Bot]

Comment on attachment 152701 [details]
Patch

Clearing flags on attachment: 152701

Committed r122817: <http://trac.webkit.org/changeset/122817>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.