Bug 138558

Summary: Use std::unique_ptr<>|std::make_unique_ptr in RenderThemeEfl::ThemePartCacheEntry::create()
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, lucas.de.marchi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 128007, 138652    
Attachments:
Description Flags
Patch
none
WIP
none
Patch
none
Patch for landing none

Gyuyoung Kim
Reported 2014-11-09 23:24:00 PST
SSIA
Attachments
Patch (4.09 KB, patch)
2014-11-09 23:25 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
WIP (7.30 KB, patch)
2014-11-10 05:55 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (7.88 KB, patch)
2014-11-10 23:03 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.83 KB, patch)
2014-11-11 09:03 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Gyuyoung Kim
Comment 1 2014-11-09 23:25:08 PST
Created attachment 241279 [details] Patch
Filip Pizlo
Comment 2 2014-11-09 23:55:15 PST
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
Comment 3 2014-11-09 23:55:40 PST
(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
Comment 4 2014-11-09 23:57:22 PST
(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
Comment 5 2014-11-10 05:55:54 PST
Created attachment 241287 [details] WIP
Gyuyoung Kim
Comment 6 2014-11-10 23:03:29 PST
Created attachment 241337 [details] Patch
Gyuyoung Kim
Comment 7 2014-11-10 23:07:09 PST
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 8 2014-11-11 06:21:42 PST
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
Comment 9 2014-11-11 09:03:30 PST
Created attachment 241355 [details] Patch for landing
WebKit Commit Bot
Comment 10 2014-11-11 09:53:34 PST
Comment on attachment 241355 [details] Patch for landing Clearing flags on attachment: 241355 Committed r175953: <http://trac.webkit.org/changeset/175953>
WebKit Commit Bot
Comment 11 2014-11-11 09:53:38 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.