Bug 96016 – [EFL] Clean up the RenderTheme Edje caching

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

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 162566 [details]
Patch

[Kenneth Rohde Christiansen]

This patch unfortunately got mixed with my former one.

[Kenneth Rohde Christiansen]

Created attachment 162572 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

Created attachment 163084 [details]
Patch

[WebKit Review Bot]

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.

[Gyuyoung Kim]

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 ?

[Kenneth Rohde Christiansen]

Created attachment 163085 [details]
Patch

[Kenneth Rohde Christiansen]

> 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

[WebKit Review Bot]

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.

[Ryuan Choi]

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.

[Gyuyoung Kim]

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 ?

[Kenneth Rohde Christiansen]

Created attachment 163381 [details]
Patch

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

Created attachment 163386 [details]
Patch

[Kenneth Rohde Christiansen]

Additional patch idea (avoid manual memory handling):

https://gist.github.com/4db97bc0568dfd4f5c2f

[Ryuan Choi]

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

[Kenneth Rohde Christiansen]

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.

[WebKit Review Bot]

Comment on attachment 163386 [details]
Patch

Clearing flags on attachment: 163386

Committed r128274: <http://trac.webkit.org/changeset/128274>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.