Bug 96465

*** Bug 96792 has been marked as a duplicate of this bug. ***

Created attachment 165074 [details] Patch

Comment on attachment 165074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165074&action=review Thank you. > Tools/ChangeLog:3 > + [EFL][WK2] TestRunner needs touch events support. We can update testExpectations in LayoutTests. Am I right? > Tools/WebKitTestRunner/EventSenderProxy.h:37 > +#if ENABLE(TOUCH_EVENTS) I think that it looks not needed > Tools/WebKitTestRunner/EventSenderProxy.h:137 > + Vector<Ewk_Touch_Point> m_touchPoints; Isn't it usefull using Eina_List instead of Vector? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:433 > + for (unsigned i = 0; i < m_touchPoints.size(); i++) { s/unsigned/size_t and ++i is preferred. > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:522 > + Ewk_Touch_Point* touchPoint = &m_touchPoints[index]; > + touchPoint->state = EVAS_TOUCH_POINT_UP; I think that touchPoint is not needed.

Comment on attachment 165074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165074&action=review > Tools/WebKitTestRunner/EventSenderProxy.h:52 > +typedef Evas_Object* PlatformWKView; This is commonly used file. You can use this in EventSenderProxyEfl.cpp > Tools/WebKitTestRunner/EventSenderProxy.h:107 > + PlatformWKView getPlatformView() const; We don't need this. > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:451 > + for (unsigned i = 0; i < m_touchPoints.size(); i++) { ++i

Created attachment 165253 [details] Patch

(In reply to comment #3) > (From update of attachment 165074 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165074&action=review > > Thank you. > > > Tools/ChangeLog:3 > > + [EFL][WK2] TestRunner needs touch events support. > > We can update testExpectations in LayoutTests. Am I right? Yes, I have updated it in new patch. Thank you. > > Tools/WebKitTestRunner/EventSenderProxy.h:37 > > +#if ENABLE(TOUCH_EVENTS) > > I think that it looks not needed > > > Tools/WebKitTestRunner/EventSenderProxy.h:137 > > + Vector<Ewk_Touch_Point> m_touchPoints; > > Isn't it usefull using Eina_List instead of Vector? > > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:433 > > + for (unsigned i = 0; i < m_touchPoints.size(); i++) { > > s/unsigned/size_t and ++i is preferred. > > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:522 > > + Ewk_Touch_Point* touchPoint = &m_touchPoints[index]; > > + touchPoint->state = EVAS_TOUCH_POINT_UP; > > I think that touchPoint is not needed. I have modified the code to use Eina_List instead of Vector. Please review.

(In reply to comment #4) > (From update of attachment 165074 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165074&action=review > > > Tools/WebKitTestRunner/EventSenderProxy.h:52 > > +typedef Evas_Object* PlatformWKView; > > This is commonly used file. > You can use this in EventSenderProxyEfl.cpp > > > Tools/WebKitTestRunner/EventSenderProxy.h:107 > > + PlatformWKView getPlatformView() const; > > We don't need this. > I added getPlatformView() to improve readability. I have now removed it from #if ENABLE(TOUCH_EVENTS) so that other apis can also use it. I think we can add getPlatformWindow() also to refactor code to improve readability (maybe in another patch). Please let me know what you think. > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:451 > > + for (unsigned i = 0; i < m_touchPoints.size(); i++) { > > ++i I have made the changes in new patch. Please review. Thank you.

Comment on attachment 165253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165253&action=review > Tools/WebKitTestRunner/EventSenderProxy.h:49 > +typedef Evas_Object* PlatformWKView; Why do we need this typedef if we use this type only if #if PLATFORM(EFL) protected code? We could just use Evas_Object* directly. It is clearer IMHO. > Tools/WebKitTestRunner/EventSenderProxy.h:104 > + void sendTouchEvent(const Ewk_Touch_Event_Type&); Isn't Ewk_Touch_Event_Type just an enumeration? We probably don't need to use a const reference here right? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:297 > + , m_touchPoints(0) We should probably free the list and its items in the destructor. > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:445 > + ASSERT(eina_list_count(m_touchPoints)); If you just want to make sure the list is not empty, why not so ASSERT(m_touchPoints) ? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:447 > + ewk_view_feed_touch_event(getPlatformView(), eventType, m_touchPoints, evas_key_modifier_get(evas_object_evas_get(getPlatformView()))); Maybe we should cache the view to avoid calling getPlatformView() twice? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:449 > + Eina_List* list = 0; Can be defined inside the for loop condition > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:456 > + m_touchPoints = eina_list_remove_list(m_touchPoints, tempList); You not freeing memory for the item being removed, you're probably leaking memory here. > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:465 > + if (eina_list_count(m_touchPoints)) { if (m_touchPoints) ? Calling eina_list_count() is probably costly for checking if a list is empty. > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:466 > + Ewk_Touch_Point* touchPoint = static_cast<Ewk_Touch_Point*>(eina_list_nth(m_touchPoints, eina_list_count(m_touchPoints)-1)); eina_list_last() ? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:470 > + Ewk_Touch_Point* touchPoint = new Ewk_Touch_Point; Can't we pass the initialisation members to Ewk_Touch_Point constructor (Adding one if necessary) ? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:491 > + static const char* modifierNames[] = { "Shift", "Control", "Alt", "Meta" }; Isn't this duplicated? I'm pretty sure we have the same code for Keyboard modifier. We can probably refactor to share code here? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:525 > + eina_list_free(m_touchPoints); You're clearing the list but not its items -> leak.

Created attachment 165261 [details] Patch

(In reply to comment #8) > (From update of attachment 165253 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165253&action=review > > We should probably free the list and its items in the destructor. > Thanks for reviewing. I have the modified the code as per comments. Please review. > > Can't we pass the initialisation members to Ewk_Touch_Point constructor (Adding one if necessary) ? > Currently it is not possible. Shall I add constructor as part of this patch? > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:491 > > + static const char* modifierNames[] = { "Shift", "Control", "Alt", "Meta" }; > > Isn't this duplicated? I'm pretty sure we have the same code for Keyboard modifier. We can probably refactor to share code here? > I've modified setEvasModifiers(). Please check.

Comment on attachment 165261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165261&action=review > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:134 > +static void setEvasModifiers(Evas* evas, WKEventModifiers wkModifiers, bool state = true, bool isTouchModifier = false) This becomes a bit complicated. Since the behavior is slightly different, maybe we should share only the modifier to const char* converting? e.g. static inline const char* toEvasModifier(unsigned modifier); What do you think? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:465 > + if (!touchPoint) How is this possible? Shouldn't it simply be an assertion? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:482 > + if (!touchPoint) Ditto. > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:488 > + Ewk_Touch_Point* touchPoint = new Ewk_Touch_Point; I would add the needed constructor. It looks weird to assign the members afterwards. > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:505 > + if (!touchPoint) assert? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:542 > + if (data) { Why is this needed? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:554 > + if (!touchPoint) assert? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:565 > + if (!touchPoint) assert?

Comment on attachment 165261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165261&action=review > Tools/WebKitTestRunner/EventSenderProxy.h:102 > + Evas_Object* getPlatformView() const; no point in having 'const' here since it returns pointer which is not const > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:489 > + if (!touchPoint) Checking 'new' operator return? have not seen such checking anywhere else in WK code.. Anyway, think, we should not simply return in such error cases even without logging. > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:543 > + Ewk_Touch_Point* touchPoint = static_cast<Ewk_Touch_Point*>(data); Does Ewk_Touch_Point have destructor to be invoked?

Created attachment 165347 [details] Patch

(In reply to comment #11) > (From update of attachment 165261 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165261&action=review > > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:134 > > +static void setEvasModifiers(Evas* evas, WKEventModifiers wkModifiers, bool state = true, bool isTouchModifier = false) > > This becomes a bit complicated. Since the behavior is slightly different, maybe we should share only the modifier to const char* converting? e.g. > static inline const char* toEvasModifier(unsigned modifier); > > What do you think? > Yes, it gets a little complicated. setEvasModifier() is used to set multiple modifiers whereas setTouchModifier() deals with only one modifier at a time. With setTouchModifier() we can have multiple modifiers enabled at a time whereas with setEvasModifiers() only one modifier will be enabled. As discussed, I have made the change to use modifierNames in both modifier related APIs. > > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:488 > > + Ewk_Touch_Point* touchPoint = new Ewk_Touch_Point; > > I would add the needed constructor. It looks weird to assign the members afterwards. > I think ewk_touch.h is public header and it is not possible to add constructor. I have made other changes as per comments. Please check.

(In reply to comment #12) > (From update of attachment 165261 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165261&action=review > > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:489 > > + if (!touchPoint) > > Checking 'new' operator return? have not seen such checking anywhere else in WK code.. Anyway, think, we should not simply return in such error cases even without logging. > I have removed the check to make it consistent with code. Please review. Thank you. > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:543 > > + Ewk_Touch_Point* touchPoint = static_cast<Ewk_Touch_Point*>(data); > > Does Ewk_Touch_Point have destructor to be invoked? > Ewk_Touch_Point does not have a destructor. Memory for touch point is allocated in addTouchPoint().

Comment on attachment 165347 [details] Patch LGTM but please make sure to run the layout tests in debug mode before landing this.

Comment on attachment 165347 [details] Patch Looks good, thanks.

(In reply to comment #16) > (From update of attachment 165347 [details]) > LGTM but please make sure to run the layout tests in debug mode before landing this. > Yes, I tried running touch related layout tests (LayoutTests/fast/events/touch) in debug mode before creating the patch. revision 129339.

Comment on attachment 165347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165347&action=review > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:445 > +inline Evas_Object* EventSenderProxy::getPlatformView() ALWAYS_INLINE? but you need to declare it in the header with implementation, but then again it is really not needed to be inlined for testing and you are kind of cluttering the api with this method. Why not just call that directly > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:458 > + for (Eina_List* list = m_touchPoints; list; ) { Why not use the eina iteration methods? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:475 > + int id = 0; unsigned? > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:479 > + Ewk_Touch_Point* touchPoint = static_cast<Ewk_Touch_Point*>(eina_list_data_get(last)); > + ASSERT(touchPoint); Why are we actually using eina lists here and not a Vector<OwnPtr<Ewk_Touch_Point> > or similar?

Comment on attachment 165347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165347&action=review > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:458 > + for (Eina_List* list = m_touchPoints; list; ) { If you use EINA_LIST_FOREACH() instead of for loop, can't we reduce unnecessary codes ? See also : http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_touch_event.cpp#L81

Comment on attachment 165347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165347&action=review >> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:475 >> + int id = 0; > > unsigned? Well, it is defined as an int in Ewk_Touch_Point so it is consistent. Otherwise we should fix Ewk_Touch_Point declaration in ewk_touch.h >> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:479 >> + ASSERT(touchPoint); > > Why are we actually using eina lists here and not a Vector<OwnPtr<Ewk_Touch_Point> > or similar? because ewk_view_feed_touch_event() takes an Eina_List in argument. If we use a Vector internally (tempting I know), then we need to convert before calling Ewk_View API which is bad.

Created attachment 165362 [details] Patch

(In reply to comment #19) > (From update of attachment 165347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165347&action=review > > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:445 > > +inline Evas_Object* EventSenderProxy::getPlatformView() > > ALWAYS_INLINE? but you need to declare it in the header with implementation, but then again it is really not needed to be inlined for testing and you are kind of cluttering the api with this method. > > Why not just call that directly > I have modified the code to remove that api. Please check. > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:458 > > + for (Eina_List* list = m_touchPoints; list; ) { > > Why not use the eina iteration methods? > I've modified code to use EINA_LIST_FOREACH_SAFE since there is a possibility of deleting current node. Please check. Thank you.

(In reply to comment #20) > (From update of attachment 165347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165347&action=review > > > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:458 > > + for (Eina_List* list = m_touchPoints; list; ) { > > If you use EINA_LIST_FOREACH() instead of for loop, can't we reduce unnecessary codes ? > > See also : http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_touch_event.cpp#L81 I'm using EINA_LIST_FOREACH_SAFE now. Please check. Thank you.

Comment on attachment 165362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165362&action=review I think this looks fine > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:462 > + delete touchPoint; > + m_touchPoints = eina_list_remove_list(m_touchPoints, list); Is this safe when iterating?

Comment on attachment 165362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165362&action=review LGTM. >> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:462 >> + m_touchPoints = eina_list_remove_list(m_touchPoints, list); > > Is this safe when iterating? According to the doc (http://docs.enlightenment.org/auto/eina/group__Eina__List__Group.html#ga5b4d2aac696cd2558aaed03e3929d873), yes: "Since this macro stores a pointer to the next list node in l_next, deleting the current node and continuing looping is safe."

Comment on attachment 165362 [details] Patch Clearing flags on attachment: 165362 Committed r129437: <http://trac.webkit.org/changeset/129437>

All reviewed patches have been landed. Closing bug.