Bug 100370

Summary: [EFL][WK2] Remove some C'ism from EwkView
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: 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 Flags
Patch
gyuyoung.kim: review+
Patch
none
Patch none

Chris Dumez
Reported 2012-10-25 05:00:23 PDT
EwkView still contains quite a bit of C'ism. We should gradually get rid of it.
Attachments
Patch (32.92 KB, patch)
2012-10-25 06:18 PDT, Chris Dumez 		gyuyoung.kim: review+
DetailsFormatted DiffDiff
Patch (44.65 KB, patch)
2012-10-25 06:59 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (45.27 KB, patch)
2012-10-25 08:33 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2012-10-25 06:18:11 PDT
Created attachment 170623 [details] Patch
Mikhail Pozdnyakov
Comment 2 2012-10-25 06:21:21 PDT
Comment on attachment 170623 [details] Patch looks good
Gyuyoung Kim
Comment 3 2012-10-25 06:27:56 PDT
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 ?
Chris Dumez
Comment 4 2012-10-25 06:52:01 PDT
(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.
Chris Dumez
Comment 5 2012-10-25 06:59:47 PDT
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.
Kenneth Rohde Christiansen
Comment 6 2012-10-25 07:05:22 PDT
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
Antonio Gomes
Comment 7 2012-10-25 07:11:31 PDT
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?
Chris Dumez
Comment 8 2012-10-25 07:27:15 PDT
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.
Chris Dumez
Comment 9 2012-10-25 07:43:06 PDT
Comment on attachment 170636 [details] Patch Will take feedback into consideration and reupload.
Chris Dumez
Comment 10 2012-10-25 08:33:25 PDT
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
Chris Dumez
Comment 11 2012-10-25 08:45:13 PDT
Comment on attachment 170657 [details] Patch Could someone please cq+ ?
WebKit Review Bot
Comment 12 2012-10-25 09:55:18 PDT
Comment on attachment 170657 [details] Patch Clearing flags on attachment: 170657 Committed r132498: <http://trac.webkit.org/changeset/132498>
WebKit Review Bot
Comment 13 2012-10-25 09:55:23 PDT
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.