Bug 138558 – Use std::unique_ptr<>|std::make_unique_ptr in RenderThemeEfl::ThemePartCacheEntry::create()

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 241279 [details]
Patch

[Filip Pizlo]

Comment on attachment 241279 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:246
> -        ThemePartCacheEntry* entry = ThemePartCacheEntry::create(themePath(), type, size).leakPtr();
> +        ThemePartCacheEntry* entry = ThemePartCacheEntry::create(themePath(), type, size).get();

get() isn't really the equivalent of get().  In particular, it seems that this will destroy ThemePartCacheEntry after this statement, leaving entry to be a dangling pointer.

Can you look into this some more?

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 241279 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241279&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:246
> > -        ThemePartCacheEntry* entry = ThemePartCacheEntry::create(themePath(), type, size).leakPtr();
> > +        ThemePartCacheEntry* entry = ThemePartCacheEntry::create(themePath(), type, size).get();
> 
> get() isn't really the equivalent of get(). 

Lol, meant to say: get() isn't really the equivalent of leakPtr().

> In particular, it seems that
> this will destroy ThemePartCacheEntry after this statement, leaving entry to
> be a dangling pointer.
> 
> Can you look into this some more?

[Gyuyoung Kim]

(In reply to comment #2)
> Comment on attachment 241279 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241279&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:246
> > -        ThemePartCacheEntry* entry = ThemePartCacheEntry::create(themePath(), type, size).leakPtr();
> > +        ThemePartCacheEntry* entry = ThemePartCacheEntry::create(themePath(), type, size).get();
> 
> get() isn't really the equivalent of get().  In particular, it seems that
> this will destroy ThemePartCacheEntry after this statement, leaving entry to
> be a dangling pointer.
> 
> Can you look into this some more?

Ah, right. thank you for pointing it out. Let me fix it soon.

[Gyuyoung Kim]

Created attachment 241287 [details]
WIP

[Gyuyoung Kim]

Created attachment 241337 [details]
Patch

[Gyuyoung Kim]

Filip, latest patch fixes the leakPtr() removal. I removes original Eina_List with Vector<std::unique_ptr> because the Eina_List can't support std::unique_ptr. I wonder whether you can take a look this patch again.

[Anders Carlsson]

Comment on attachment 241337 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:270
> +    size_t cacheSize = m_partCache.size();
> +    for (size_t i = 0; i < cacheSize; ++i)
> +        m_partCache[i].reset();

I think you should do something like

for (auto& part : m_partCache)
    part = nullptr;

here instead.

[Gyuyoung Kim]

Created attachment 241355 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 241355 [details]
Patch for landing

Clearing flags on attachment: 241355

Committed r175953: <http://trac.webkit.org/changeset/175953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.