Bug 96501 - [EFL] Avoid manual memory management in RenderThemeEfl
Summary: [EFL] Avoid manual memory management in RenderThemeEfl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on: 96540
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-12 05:30 PDT by Kenneth Rohde Christiansen
Modified: 2012-09-12 12:26 PDT (History)
7 users (show)

See Also:


Attachments
Patch (15.86 KB, patch)
2012-09-12 05:33 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (15.85 KB, patch)
2012-09-12 06:09 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2012-09-12 05:30:42 PDT
Use OwnPtr as it works for Evas_Object and Evas_Ecore objects.
Comment 1 Kenneth Rohde Christiansen 2012-09-12 05:33:03 PDT
Created attachment 163602 [details]
Patch
Comment 2 Ryuan Choi 2012-09-12 05:38:35 PDT
Comment on attachment 163602 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:494
> -#ifndef NDEBUG
> -    if (m_edje) {
> +#if !NDEBUG

Can I know why you revert this?
Comment 3 Kenneth Rohde Christiansen 2012-09-12 05:43:31 PDT
Comment on attachment 163602 [details]
Patch

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

>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:494
>> +#if !NDEBUG
> 
> Can I know why you revert this?

Rebase mistake! Thanks for catching!
Comment 4 Gyuyoung Kim 2012-09-12 05:54:22 PDT
Comment on attachment 163602 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:606
> +    OwnPtr<Evas_Object> temp = adoptPtr(edje_object_add(ecore_evas_get(canvas())));

In this patch, can't change variable name together? Doesn't *temp* variable name looks unclear? Maybe, I think tempView looks better.
Comment 5 Kenneth Rohde Christiansen 2012-09-12 06:07:33 PDT
> In this patch, can't change variable name together? Doesn't *temp* variable name looks unclear? Maybe, I think tempView looks better.

That is incorrect. It is a temporary edje object for loading different groups that we need temporarily in order to get the part descriptions. I think temp is OK.
Comment 6 Kenneth Rohde Christiansen 2012-09-12 06:09:03 PDT
Created attachment 163611 [details]
Patch
Comment 7 Ryuan Choi 2012-09-12 06:19:56 PDT
Comment on attachment 163611 [details]
Patch

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

Looks good to me.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:225
> +    ecore_evas_alpha_set(entry->canvas(), EINA_TRUE);

As a nit, I think that we use true/false .
Comment 8 Kenneth Rohde Christiansen 2012-09-12 06:45:52 PDT
(In reply to comment #7)
> (From update of attachment 163611 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163611&action=review
> 
> Looks good to me.
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:225
> > +    ecore_evas_alpha_set(entry->canvas(), EINA_TRUE);
> 
> As a nit, I think that we use true/false .

That is a separate change then which should be done all over. Also I need to know that what you are thinking is actually true :-)
Comment 9 Ryuan Choi 2012-09-12 06:55:25 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 163611 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163611&action=review
> > 
> > Looks good to me.
> > 
> > > Source/WebCore/platform/efl/RenderThemeEfl.cpp:225
> > > +    ecore_evas_alpha_set(entry->canvas(), EINA_TRUE);
> > 
> > As a nit, I think that we use true/false .
> 
> That is a separate change then which should be done all over. Also I need to know that what you are thinking is actually true :-)

I just mean that s/EINA_TRUE/true/.

It's not important. never mind.
Comment 10 Gyuyoung Kim 2012-09-12 07:45:23 PDT
Comment on attachment 163611 [details]
Patch

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

Looks nice.

>>>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:225
>>>> +    ecore_evas_alpha_set(entry->canvas(), EINA_TRUE);
>>> 
>>> As a nit, I think that we use true/false .
>> 
>> That is a separate change then which should be done all over. Also I need to know that what you are thinking is actually true :-)
> 
> I just mean that s/EINA_TRUE/true/.
> 
> It's not important. never mind.

I'm not sure whether you can remember this. When EFL port has efl specific coding style before, boolean type was one of confusing coding style in reviewers. So, you recommended to use standard boolean type instead of EINA_BOOL. Then, EFL port has used true/false except for public APIs so far. I was told that WebKit culturally doesn't like to only fix coding style. So, IMO, it would be good to fix this one together.
Comment 11 WebKit Review Bot 2012-09-12 08:07:46 PDT
Comment on attachment 163611 [details]
Patch

Clearing flags on attachment: 163611

Committed r128311: <http://trac.webkit.org/changeset/128311>
Comment 12 WebKit Review Bot 2012-09-12 08:07:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Chris Dumez 2012-09-12 12:26:25 PDT
Broke the debug build. Addressing issue in Bug 96540.