Bug 89842

Created attachment 149222 [details] Patch

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 ?

Created attachment 149892 [details] Patch

Created attachment 149893 [details] Patch

(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 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(); ?

Created attachment 149903 [details] Patch

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

Created attachment 150350 [details] Patch

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

(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

(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 on attachment 150350 [details] Patch Looks good to me as well, provided there are no pixel test regressions.

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

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.

Created attachment 152701 [details] Patch

(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 on attachment 152701 [details] Patch Clearing flags on attachment: 152701 Committed r122817: <http://trac.webkit.org/changeset/122817>

All reviewed patches have been landed. Closing bug.