Bug 95832

Summary: [EFL] Fuzzy load the Edje theme for HTML forms
Product: WebKit Reporter: Kenneth Rohde Christiansen <kenneth>
Component: WebKit EFLAssignee: Kenneth Rohde Christiansen <kenneth>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, lucas.de.marchi, rakuco, ryuan.choi, tmpsantos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 95844    
Bug Blocks: 94670    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Kenneth Rohde Christiansen 2012-09-05 02:12:27 PDT
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...
Comment 1 Kenneth Rohde Christiansen 2012-09-05 02:13:51 PDT
Created attachment 162193 [details]
Patch
Comment 2 Gyuyoung Kim 2012-09-05 03:31:57 PDT
Comment on attachment 162193 [details]
Patch

Looks fine. Thanks.
Comment 3 Ryuan Choi 2012-09-05 03:48:01 PDT
This is what I really want.

Thank you.
Comment 4 Kenneth Rohde Christiansen 2012-09-05 04:02:42 PDT
Landed in 127573
Comment 5 WebKit Review Bot 2012-09-05 05:29:32 PDT
Re-opened since this is blocked by 95844
Comment 6 Kenneth Rohde Christiansen 2012-09-05 06:27:21 PDT
Created attachment 162234 [details]
Patch
Comment 7 Thiago Marcos P. Santos 2012-09-05 06:48:45 PDT
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 8 Ryuan Choi 2012-09-05 07:01:00 PDT
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.
Comment 9 Kenneth Rohde Christiansen 2012-09-05 08:16:52 PDT
Created attachment 162255 [details]
Patch
Comment 10 Gyuyoung Kim 2012-09-05 08:24:01 PDT
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.
Comment 11 Kenneth Rohde Christiansen 2012-09-05 08:32:04 PDT
(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
Comment 12 Kenneth Rohde Christiansen 2012-09-05 08:40:46 PDT
> > 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.
Comment 13 Ryuan Choi 2012-09-05 15:48:18 PDT
(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)
Comment 14 Kenneth Rohde Christiansen 2012-09-05 16:06:02 PDT
> 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?
Comment 15 Ryuan Choi 2012-09-05 16:18:48 PDT
(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.
Comment 16 Kenneth Rohde Christiansen 2012-09-06 03:38:59 PDT
Created attachment 162468 [details]
Patch
Comment 17 Ryuan Choi 2012-09-06 03:53:27 PDT
(In reply to comment #16)
> Created an attachment (id=162468) [details]
> Patch

Looks good to me.
Comment 18 Kenneth Rohde Christiansen 2012-09-06 04:51:25 PDT
Created attachment 162477 [details]
Patch
Comment 19 Chris Dumez 2012-09-06 05:01:58 PDT
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?
Comment 20 WebKit Review Bot 2012-09-06 05:05:06 PDT
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.
Comment 21 Kenneth Rohde Christiansen 2012-09-06 06:38:26 PDT
Created attachment 162497 [details]
Patch
Comment 22 Kenneth Rohde Christiansen 2012-09-06 06:39:12 PDT
Comment on attachment 162497 [details]
Patch

Wrong patch
Comment 23 Kenneth Rohde Christiansen 2012-09-06 06:41:56 PDT
Created attachment 162498 [details]
Patch
Comment 24 WebKit Review Bot 2012-09-06 06:43:38 PDT
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 25 WebKit Review Bot 2012-09-06 14:41:10 PDT
Comment on attachment 162498 [details]
Patch

Clearing flags on attachment: 162498

Committed r127786: <http://trac.webkit.org/changeset/127786>
Comment 26 WebKit Review Bot 2012-09-06 14:41:16 PDT
All reviewed patches have been landed.  Closing bug.