Bug 95832 – [EFL] Fuzzy load the Edje theme for HTML forms

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

Created attachment 162193 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 162193 [details]
Patch

Looks fine. Thanks.

[Ryuan Choi]

This is what I really want.

Thank you.

[Kenneth Rohde Christiansen]

Landed in 127573

[WebKit Review Bot]

Re-opened since this is blocked by 95844

[Kenneth Rohde Christiansen]

Created attachment 162234 [details]
Patch

[Thiago Marcos P. Santos]

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.

[Ryuan Choi]

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.

[Kenneth Rohde Christiansen]

Created attachment 162255 [details]
Patch

[Gyuyoung Kim]

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.

[Kenneth Rohde Christiansen]

(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

[Kenneth Rohde Christiansen]

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

[Ryuan Choi]

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

[Kenneth Rohde Christiansen]

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

[Ryuan Choi]

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

[Kenneth Rohde Christiansen]

Created attachment 162468 [details]
Patch

[Ryuan Choi]

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

Looks good to me.

[Kenneth Rohde Christiansen]

Created attachment 162477 [details]
Patch

[Chris Dumez]

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?

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

Created attachment 162497 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 162497 [details]
Patch

Wrong patch

[Kenneth Rohde Christiansen]

Created attachment 162498 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 162498 [details]
Patch

Clearing flags on attachment: 162498

Committed r127786: <http://trac.webkit.org/changeset/127786>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.