Bug 96501 – [EFL] Avoid manual memory management in RenderThemeEfl

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

[Kenneth Rohde Christiansen]

Use OwnPtr as it works for Evas_Object and Evas_Ecore objects.

[Kenneth Rohde Christiansen]

Created attachment 163602 [details]
Patch

[Ryuan Choi]

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?

[Kenneth Rohde Christiansen]

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!

[Gyuyoung Kim]

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.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

Created attachment 163611 [details]
Patch

[Ryuan Choi]

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 .

[Kenneth Rohde Christiansen]

(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 :-)

[Ryuan Choi]

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

[Gyuyoung Kim]

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.

[WebKit Review Bot]

Comment on attachment 163611 [details]
Patch

Clearing flags on attachment: 163611

Committed r128311: <http://trac.webkit.org/changeset/128311>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Broke the debug build. Addressing issue in Bug 96540.