Summary: | [EFL][WK2] Add ModuleEfl.cpp | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | YoungTaeck Song <youngtaeck.song> | ||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, kling, lucas.de.marchi, rakuco, ryuan.choi, t.morawski, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Other | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | 76255 | ||||||||||||
Bug Blocks: | 61838 | ||||||||||||
Attachments: |
|
Description
YoungTaeck Song
2011-06-28 22:33:33 PDT
(In reply to comment #0) > Add First version of ModuleEfl.cpp for WebKit2 I seems that you have not added your patch. Could you please add it. Created attachment 120927 [details]
patch for ModuleEfl
Looks OK. Comment on attachment 120927 [details]
patch for ModuleEfl
LGTM too.
Comment on attachment 120927 [details] patch for ModuleEfl View in context: https://bugs.webkit.org/attachment.cgi?id=120927&action=review > Source/WebKit2/Platform/efl/ModuleEfl.cpp:51 > + void* symbolPointer = 0; > + > + if (m_module) > + symbolPointer = eina_module_symbol_get(m_module, functionName); > + > + return symbolPointer; Almost looks good to me. As small thing, below is clear for me. if (m_module) return eina_module_symbol_get(m_module, functionName); return 0; (In reply to comment #5) > (From update of attachment 120927 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120927&action=review > > > Source/WebKit2/Platform/efl/ModuleEfl.cpp:51 > > + void* symbolPointer = 0; > > + > > + if (m_module) > > + symbolPointer = eina_module_symbol_get(m_module, functionName); > > + > > + return symbolPointer; > > Almost looks good to me. > > As small thing, below is clear for me. > if (m_module) > return eina_module_symbol_get(m_module, functionName); > > return 0; Sorry. I don't agree with you. because platformFunctionPointer relies on the eina_module_symbol_get, My code is more clear. Created attachment 121985 [details]
patch for ModuleEfl
(In reply to comment #7) > Created an attachment (id=121985) [details] > patch for ModuleEfl LGTM, too. Comment on attachment 121985 [details] patch for ModuleEfl View in context: https://bugs.webkit.org/attachment.cgi?id=121985&action=review > Source/WebKit2/Platform/efl/ModuleEfl.cpp:33 > + m_module = eina_module_new(m_path.utf8().data()); > + if (m_module && eina_module_load(m_module)) > + return true; > + > + return false; If eina_module_load() fails, don't you need to call eina_module_free() before returning false? > Source/WebKit2/Platform/efl/ModuleEfl.cpp:41 > + if (m_module) { > + eina_module_unload(m_module); > + m_module = 0; > + } Is it safe to call eina_module_unload() here if m_module is non-null, but eina_module_load() failed in Module::load()? > Source/WebKit2/Platform/efl/ModuleEfl.cpp:47 > + if (m_module) > + return eina_module_symbol_get(m_module, functionName); Is it safe to call eina_module_symbol_get() here if m_module is non-null, but eina_module_load() failed in Module::load()? (In reply to comment #9) > (From update of attachment 121985 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121985&action=review > > > Source/WebKit2/Platform/efl/ModuleEfl.cpp:33 > > + m_module = eina_module_new(m_path.utf8().data()); > > + if (m_module && eina_module_load(m_module)) > > + return true; > > + > > + return false; > > If eina_module_load() fails, don't you need to call eina_module_free() before returning false? > You are right. I checked eina_module, so your point looks correct. And also, nobody calls eina_module_free. > > Source/WebKit2/Platform/efl/ModuleEfl.cpp:41 > > + if (m_module) { > > + eina_module_unload(m_module); > > + m_module = 0; > > + } > > Is it safe to call eina_module_unload() here if m_module is non-null, but eina_module_load() failed in Module::load()? > > > Source/WebKit2/Platform/efl/ModuleEfl.cpp:47 > > + if (m_module) > > + return eina_module_symbol_get(m_module, functionName); > > Is it safe to call eina_module_symbol_get() here if m_module is non-null, but eina_module_load() failed in Module::load()? It is safe because eina_module_symbol_get take care of it. However, we'd better to make it clear in this patch. After thinking more, It looks good to use OwnPtr<Eina_Module>. youngtaeck, could you make a bug to introduce OwnPtr<Eina_Module> ? (In reply to comment #11) > After thinking more, > It looks good to use OwnPtr<Eina_Module>. > > youngtaeck, > could you make a bug to introduce OwnPtr<Eina_Module> ? Thank. I make a bug for OwnPtr<Eina_Module>, 76255. and I attached new patch using OwnPtr. Created attachment 122401 [details]
patch for ModuleEfl
(In reply to comment #13) > Created an attachment (id=122401) [details] > patch for ModuleEfl Added kling in CCs. Could you take a look at this bug once more ? Comment on attachment 122401 [details] patch for ModuleEfl View in context: https://bugs.webkit.org/attachment.cgi?id=122401&action=review r=me, with two suggestions: > Source/WebKit2/Platform/efl/ModuleEfl.cpp:32 > + if (!m_module.get() || !eina_module_load(m_module.get())) { You don't need the .get() when null-checking an OwnPtr as it overrides operator!, this will work just as well: if (!m_module || !eina_module_load(m_module.get())) { > Source/WebKit2/Platform/efl/ModuleEfl.cpp:47 > + if (m_module.get()) Same here: if (m_module) Created attachment 125802 [details]
Patch
Comment on attachment 125802 [details]
Patch
Thank you kling.
Comment on attachment 125802 [details] Patch Clearing flags on attachment: 125802 Committed r106921: <http://trac.webkit.org/changeset/106921> All reviewed patches have been landed. Closing bug. |