Bug 89842

Summary: [EFL] Move codes related to theme setting from Widget to RenderTheme
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: cshu, d-r, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 90788    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2012-06-24 17:52:53 PDT
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.
Comment 1 Ryuan Choi 2012-06-24 18:45:12 PDT
Created attachment 149222 [details]
Patch
Comment 2 Gyuyoung Kim 2012-06-25 18:58:51 PDT
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 ?
Comment 3 Ryuan Choi 2012-06-28 00:10:48 PDT
Created attachment 149892 [details]
Patch
Comment 4 Ryuan Choi 2012-06-28 00:12:08 PDT
Created attachment 149893 [details]
Patch
Comment 5 Ryuan Choi 2012-06-28 00:14:01 PDT
(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.
Comment 6 Chris Dumez 2012-06-28 01:18:31 PDT
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(); ?
Comment 7 Ryuan Choi 2012-06-28 01:43:57 PDT
Created attachment 149903 [details]
Patch
Comment 8 Ryuan Choi 2012-06-28 01:44:33 PDT
(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.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-06-28 19:35:15 PDT
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?
Comment 10 Ryuan Choi 2012-07-01 18:02:16 PDT
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.
Comment 11 Ryuan Choi 2012-07-01 18:11:52 PDT
Created attachment 150350 [details]
Patch
Comment 12 Gyuyoung Kim 2012-07-04 23:26:29 PDT
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
Comment 13 Ryuan Choi 2012-07-05 00:24:30 PDT
(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
Comment 14 Gyuyoung Kim 2012-07-05 00:53:51 PDT
(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.
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-07-05 07:59:44 PDT
Comment on attachment 150350 [details]
Patch

Looks good to me as well, provided there are no pixel test regressions.
Comment 16 Ryuan Choi 2012-07-05 15:45:48 PDT
(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.
Comment 17 Dominik Röttsches (drott) 2012-07-06 03:55:25 PDT
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.
Comment 18 Ryuan Choi 2012-07-16 23:29:12 PDT
Created attachment 152701 [details]
Patch
Comment 19 Ryuan Choi 2012-07-16 23:30:30 PDT
(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.
Comment 20 WebKit Review Bot 2012-07-17 00:52:45 PDT
Comment on attachment 152701 [details]
Patch

Clearing flags on attachment: 152701

Committed r122817: <http://trac.webkit.org/changeset/122817>
Comment 21 WebKit Review Bot 2012-07-17 00:52:52 PDT
All reviewed patches have been landed.  Closing bug.