The Edje theme is being loaded twice when launching MiniBrowser. First the default theme until the MiniBrowser sets its own theme. Most web sites don't even expose native forms anymore, so for many cases (hybrid apps etc) there is no need in loading the theme at all. Patch coming...
Created attachment 162193 [details] Patch
Comment on attachment 162193 [details] Patch Looks fine. Thanks.
This is what I really want. Thank you.
Landed in 127573
Re-opened since this is blocked by 95844
Created attachment 162234 [details] Patch
Comment on attachment 162234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162234&action=review > Source/WebCore/platform/efl/RenderThemeEfl.cpp:468 > +Evas_Object* RenderThemeEfl::edje() > { What about making this RenderThemeEfl::edje() an small inline like: ALWAYS_INLINE Evas_Object* RenderThemeEfl::edje() { return m_edje ? m_edje : createEdje(); } The rationale here is this function will be called very often and we might get some performance improvement.
Comment on attachment 162234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162234&action=review I think that it looks almost fine. But I want to test one more thing. (to check activeSelectionColor) I am not sure whether we should call edje() in platformActiveSelectionForegroundColor() and similar functions. > Source/WebCore/platform/efl/RenderThemeEfl.cpp:89 > + ASSERT(m_partDescs); As a nit, m_partDescs is array so this ASSERT is always fine.
Created attachment 162255 [details] Patch
Comment on attachment 162255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162255&action=review > Source/WebCore/platform/efl/RenderThemeEfl.cpp:460 > + Evas_Object* o = edje_object_add(ecore_evas_get(m_canvas)); Please use meaningful parameter name. EFL port follows webkit coding style except for public API. > Source/WebCore/platform/efl/RenderThemeEfl.cpp:502 > +void RenderThemeEfl::applyEdjeColorsFrom(Evas_Object* o) ditto.
(In reply to comment #10) > (From update of attachment 162255 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162255&action=review > > > Source/WebCore/platform/efl/RenderThemeEfl.cpp:460 > > + Evas_Object* o = edje_object_add(ecore_evas_get(m_canvas)); > > Please use meaningful parameter name. EFL port follows webkit coding style except for public API. > > > Source/WebCore/platform/efl/RenderThemeEfl.cpp:502 > > +void RenderThemeEfl::applyEdjeColorsFrom(Evas_Object* o) > > ditto. o is used all over the layouting code in WebKit and in this file as well. It is better for consistency
> > Please use meaningful parameter name. EFL port follows webkit coding style except for public API. object itself is a very meaning less word but sometimes it makes sense. o is use all over the Rendering code (see WebCore/rendering) and I think it is fine, unless a better replacement than "object" can be found. But that actually requires someone to go thru all the objects in that file and figure out what exactly they represent.
(In reply to comment #8) > (From update of attachment 162234 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162234&action=review > > I think that it looks almost fine. > > But I want to test one more thing. (to check activeSelectionColor) > I am not sure whether we should call edje() in platformActiveSelectionForegroundColor() and similar functions. > Unfortunately, I can not get correct output without adding edje() in platformActiveSelectionBackgroundColor and similar functions. Simple test scenario is below. 1) change color of webkit/selection/active in Source/WebKit/efl/DefaultTheme/default.edc for exameple, "color: 255 0 0 255; /* foreground */" 2) load webkit.org and drag some text to select. forground text should be red.(default is white)
> Unfortunately, I can not get correct output without adding edje() in platformActiveSelectionBackgroundColor and similar functions. > > Simple test scenario is below. > 1) change color of webkit/selection/active in Source/WebKit/efl/DefaultTheme/default.edc > for exameple, "color: 255 0 0 255; /* foreground */" > 2) load webkit.org and drag some text to select. > > forground text should be red.(default is white) OK, I will look into that tomorrow. Thanks for the feedback. With that change all your tests are working?
(In reply to comment #14) > > Unfortunately, I can not get correct output without adding edje() in platformActiveSelectionBackgroundColor and similar functions. > > > > Simple test scenario is below. > > 1) change color of webkit/selection/active in Source/WebKit/efl/DefaultTheme/default.edc > > for exameple, "color: 255 0 0 255; /* foreground */" > > 2) load webkit.org and drag some text to select. > > > > forground text should be red.(default is white) > > OK, I will look into that tomorrow. Thanks for the feedback. With that change all your tests are working? Hopefully yes, All tests which I can guess are passed with that.
Created attachment 162468 [details] Patch
(In reply to comment #16) > Created an attachment (id=162468) [details] > Patch Looks good to me.
Created attachment 162477 [details] Patch
Comment on attachment 162477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162477&action=review Looks sane but this is not really my area. > Source/WebCore/platform/efl/RenderThemeEfl.h:214 > + return !!m_edje || const_cast<RenderThemeEfl*>(this)->loadTheme(); Do we really need the !! here? We are converting it to a bool anyway, right?
Attachment 162477 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/efl/RenderThemeEfl.cpp:83: More than one command on the same line [whitespace/newline] [4] Source/WebCore/platform/efl/RenderThemeEfl.cpp:83: More than one command on the same line in if [whitespace/parens] [4] Source/WebCore/platform/efl/RenderThemeEfl.cpp:85: More than one command on the same line [whitespace/newline] [4] Source/WebCore/platform/efl/RenderThemeEfl.cpp:85: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 162497 [details] Patch
Comment on attachment 162497 [details] Patch Wrong patch
Created attachment 162498 [details] Patch
Attachment 162498 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/efl/RenderThemeEfl.cpp:83: More than one command on the same line [whitespace/newline] [4] Source/WebCore/platform/efl/RenderThemeEfl.cpp:83: More than one command on the same line in if [whitespace/parens] [4] Source/WebCore/platform/efl/RenderThemeEfl.cpp:85: More than one command on the same line [whitespace/newline] [4] Source/WebCore/platform/efl/RenderThemeEfl.cpp:85: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 162498 [details] Patch Clearing flags on attachment: 162498 Committed r127786: <http://trac.webkit.org/changeset/127786>
All reviewed patches have been landed. Closing bug.