Summary: | [EFL] Clean up the RenderTheme Edje caching | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Rohde Christiansen <kenneth> | ||||||||||||||
Component: | WebKit EFL | Assignee: | 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
Kenneth Rohde Christiansen
2012-09-06 13:22:18 PDT
Created attachment 162566 [details]
Patch
This patch unfortunately got mixed with my former one. Created attachment 162572 [details]
Patch
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 Created attachment 163084 [details]
Patch
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 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 ? Created attachment 163085 [details]
Patch
> 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 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 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 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 ? Created attachment 163381 [details]
Patch
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.
Created attachment 163386 [details]
Patch
Additional patch idea (avoid manual memory handling): https://gist.github.com/4db97bc0568dfd4f5c2f (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 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 on attachment 163386 [details] Patch Clearing flags on attachment: 163386 Committed r128274: <http://trac.webkit.org/changeset/128274> All reviewed patches have been landed. Closing bug. |