Summary: | [EFL][WK2] Remove some C'ism from EwkView | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, mikhail.pozdnyakov, rakuco, tonikitoo, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 100273 | ||||||||||
Attachments: |
|
Description
Chris Dumez
2012-10-25 05:00:23 PDT
Created attachment 170623 [details]
Patch
Comment on attachment 170623 [details]
Patch
looks good
Comment on attachment 170623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170623&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:112 > +PassOwnPtr<Ecore_IMF_Context> EwkViewImpl::createIMFContext(Ewk_View_Smart_Data* sd) Why not using smartData instead of sd ? (In reply to comment #3) > (From update of attachment 170623 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170623&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:112 > > +PassOwnPtr<Ecore_IMF_Context> EwkViewImpl::createIMFContext(Ewk_View_Smart_Data* sd) > > Why not using smartData instead of sd ? Because it conflicts with the method name smartData() in the same class. Created attachment 170636 [details]
Patch
Requesting r? again because I had to move the mouse / touch callbacks to EwkViewImpl to address an API tests regression.
Comment on attachment 170636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170636&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:225 > + data = edje_object_data_get(m_cursorObject.get(), "hot.y"); This is getting quite hot :-) > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:559 > +bool EwkViewImpl::focused() const why not isFocused() that is like webkit style > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:576 > +Ecore_IMF_Context* EwkViewImpl::imfContext() i really dislike the imf abbrivation Comment on attachment 170636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170636&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:106 > + free(buffer); Unrelated? > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:588 > - _ewk_view_imf_context_destroy(imfContext); > - > void* item; > EINA_LIST_FREE(popupMenuItems, item) > delete static_cast<Ewk_Popup_Menu_Item*>(item); > + if (m_theme != theme) { Do you really want to compare the memory addresses here? Comment on attachment 170636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170636&action=review >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:106 >> + free(buffer); > > Unrelated? Hmm. Yes, this is a memory leak fix. Do you really want me to move it to a separate patch? >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:559 >> +bool EwkViewImpl::focused() const > > why not isFocused() that is like webkit style Ok. >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:588 >> + if (m_theme != theme) { > > Do you really want to compare the memory addresses here? I am not :) m_theme is a WKEinaSharedString. Therefore it is safe. Comment on attachment 170636 [details]
Patch
Will take feedback into consideration and reupload.
Created attachment 170657 [details] Patch Take feedback from Kenneth and Antonio into consideration: - Renamed visible() / focused() to isVisible() / isFocused() - Moved memory leak fix to another patch (Bug 100380) - renamed "imf" to "inputMethod" - Rebase on master since some AC fixes landed Comment on attachment 170657 [details]
Patch
Could someone please cq+ ?
Comment on attachment 170657 [details] Patch Clearing flags on attachment: 170657 Committed r132498: <http://trac.webkit.org/changeset/132498> All reviewed patches have been landed. Closing bug. |