Bug 96016

Summary: [EFL] Clean up the RenderTheme Edje caching
Product: WebKit Reporter: Kenneth Rohde Christiansen <kenneth>
Component: WebKit EFLAssignee: Kenneth Rohde Christiansen <kenneth>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, lucas.de.marchi, rakuco, ryuan.choi, tmpsantos, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Kenneth Rohde Christiansen 2012-09-06 13:22:18 PDT
SSIA
Comment 1 Kenneth Rohde Christiansen 2012-09-06 13:23:40 PDT
Created attachment 162566 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-09-06 13:27:50 PDT
This patch unfortunately got mixed with my former one.
Comment 3 Kenneth Rohde Christiansen 2012-09-06 13:39:30 PDT
Created attachment 162572 [details]
Patch
Comment 4 Kenneth Rohde Christiansen 2012-09-06 13:42:09 PDT
Comment on attachment 162572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162572&action=review

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:604
> +    const char* path;
> +    edje_object_file_get(o, &path, 0);
> +    _ASSERT_ON_RELEASE_RETURN(path, "Could not load Edje theme file.");

use themePath()

> Source/WebCore/platform/efl/RenderThemeEfl.h:256
> +        static ThemePartCacheEntry* create(const IntSize&, FormType, const String& themePath);
> +        void reuse(const String& themePath, FormType, const IntSize& size = IntSize());

Reorder arguments
Comment 5 Kenneth Rohde Christiansen 2012-09-10 04:03:08 PDT
Created attachment 163084 [details]
Patch
Comment 6 WebKit Review Bot 2012-09-10 04:08:04 PDT
Attachment 163084 [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.h:257:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/efl/RenderThemeEfl.cpp:281:  Declaration has space between type name and * in ThemePartCacheEntry *entry  [whitespace/declaration] [3]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Gyuyoung Kim 2012-09-10 04:09:33 PDT
Comment on attachment 163084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163084&action=review

> Source/WebCore/ChangeLog:58
> +        [EFL] Fuzzy load the Edje theme for HTML forms

Can we add multiple changelog ?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:121
> +    ASSERT((size_t)type < sizeof(groups) / sizeof(groups[0])); // out of sync?

our -> Out ?
Comment 8 Kenneth Rohde Christiansen 2012-09-10 04:10:33 PDT
Created attachment 163085 [details]
Patch
Comment 9 Kenneth Rohde Christiansen 2012-09-10 04:11:58 PDT
> Can we add multiple changelog ?

I don't know what when wrong... My git shows me that I am removing a change log entry now.

> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:121
> > +    ASSERT((size_t)type < sizeof(groups) / sizeof(groups[0])); // out of sync?
> 
> our -> Out ?

It says "out" not "our". I didn't add this comment btw
Comment 10 WebKit Review Bot 2012-09-10 04:15:37 PDT
Attachment 163085 [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.h:257:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Ryuan Choi 2012-09-10 05:12:48 PDT
Comment on attachment 163085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163085&action=review

Logical side, looks good to me.
This is really good.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:280
> +    for (unsigned i = 0; it != end; i++, it++) {

As a nit, size_t looks better.
Comment 12 Gyuyoung Kim 2012-09-10 05:20:56 PDT
Comment on attachment 163085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163085&action=review

Looks fine as well except for my question.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:185
> +    unsigned char* buffer = (unsigned char*)(ecore_evas_buffer_pixels_get(ee));

Is static_cast<> better in .cpp file ?
Comment 13 Kenneth Rohde Christiansen 2012-09-11 09:16:19 PDT
Created attachment 163381 [details]
Patch
Comment 14 WebKit Review Bot 2012-09-11 09:20:40 PDT
Attachment 163381 [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.h:264:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Kenneth Rohde Christiansen 2012-09-11 09:37:08 PDT
Created attachment 163386 [details]
Patch
Comment 16 Kenneth Rohde Christiansen 2012-09-11 10:34:48 PDT
Additional patch idea (avoid manual memory handling):

https://gist.github.com/4db97bc0568dfd4f5c2f
Comment 17 Ryuan Choi 2012-09-11 15:49:04 PDT
(In reply to comment #16)
> Additional patch idea (avoid manual memory handling):
> 
> https://gist.github.com/4db97bc0568dfd4f5c2f

Yes!
I had a plan to use OwnPtr after this bug lands.
But, your changes are quite better than mine.

I like it.
Comment 18 Kenneth Rohde Christiansen 2012-09-12 00:50:55 PDT
Comment on attachment 163386 [details]
Patch

Now we cross our fingers that this doesn't break anything :-) Otherwise we can roll out.

Ryuan has also tried the patch locally.
Comment 19 WebKit Review Bot 2012-09-12 00:55:25 PDT
Comment on attachment 163386 [details]
Patch

Clearing flags on attachment: 163386

Committed r128274: <http://trac.webkit.org/changeset/128274>
Comment 20 WebKit Review Bot 2012-09-12 00:55:29 PDT
All reviewed patches have been landed.  Closing bug.