EventSender requires implementation of leapForward function in order to test drag'n'drop, wheel and other mouse events.
Created attachment 136421 [details] Patch Implementation of leapForward in EFL's DRT EventSender
Attachment 136421 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Tools/DumpRenderTree/efl/EventSender.cpp:135: Missing space inside { }. [whitespace/braces] [5] Tools/DumpRenderTree/efl/EventSender.cpp:137: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/DumpRenderTree/efl/EventSender.cpp:143: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/DumpRenderTree/efl/EventSender.cpp:143: Missing space inside { }. [whitespace/braces] [5] Tools/DumpRenderTree/efl/EventSender.cpp:145: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/DumpRenderTree/efl/EventSender.cpp:151: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/DumpRenderTree/efl/EventSender.cpp:151: Missing space inside { }. [whitespace/braces] [5] Tools/DumpRenderTree/efl/EventSender.cpp:160: The parameter name "eventInfo" adds no information, so it should be removed. [readability/parameter_name] [5] Tools/DumpRenderTree/efl/EventSender.cpp:161: The parameter name "eventInfo" adds no information, so it should be removed. [readability/parameter_name] [5] Tools/DumpRenderTree/efl/EventSender.cpp:658: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/efl/EventSender.cpp:666: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 11 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 136427 [details] Patch Fixed stylebot errors
Comment on attachment 136427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136427&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:269 > + MouseEventInfo* eventInfo = new MouseEventInfo(EvasMouseEventDown, modifiers, (EvasMouseButton)button); Use static_cast instead of c-type casting. > Tools/DumpRenderTree/efl/EventSender.cpp:352 > + const int horizontal = - static_cast<int>(JSValueToNumber(context, arguments[0], exception)); To expression above line more clearly, why don't you modify like this. int horizontal = -(static_cast<int>(JSValueToNumber(context, arguments[0], exception)));
Created attachment 136499 [details] Patch Applied review changes
Created attachment 136723 [details] Patch Updated Skipped list.
Comment on attachment 136723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136723&action=review It looks this patch is based on GTK port implementation. Could you please explain this patch on irc again ? > Tools/ChangeLog:6 > + Implemented leapForward function I think this patch description is poor beside patch size. Could you write more detail description ?
Comment on attachment 136723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136723&action=review > LayoutTests/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). This line usually comes before the change's longer description. > Tools/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). Ditto. > Tools/DumpRenderTree/efl/EventSender.cpp:69 > + ulong delay; This value is always set from an int below, so why is it a ulong here? > Tools/DumpRenderTree/efl/EventSender.cpp:75 > +static const int gDelayedQueueSize = 1024; > +static DelayedEvent gEventQueue[gDelayedQueueSize]; > +static unsigned gStartOfEventQueue; > +static unsigned gEndOfEventQueue; Why do all this manual bookkeeping instead of using a WTF::Vector? > Tools/DumpRenderTree/efl/EventSender.cpp:154 > + MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers) > + : event(event) > + , modifiers(modifiers) > + , button(EvasMouseButtonNone) > + , horizontalDelta(0) > + , verticalDelta(0) > + { > + } > + > + MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers, EvasMouseButton button) > + : event(event) > + , modifiers(modifiers) > + , button(button) > + , horizontalDelta(0) > + , verticalDelta(0) > + { > + } > + > + MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers, int horizontalDelta, int verticalDelta) > + : event(event) > + , modifiers(modifiers) > + , button(EvasMouseButtonNone) > + , horizontalDelta(horizontalDelta) > + , verticalDelta(verticalDelta) > + { > + } This set of constructors could be replaced by a single one: MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers, EvasMouseButton button = EvasMouseButtonNone, int horizontalDelta = 0, int verticalDelta = 0) > Tools/DumpRenderTree/efl/EventSender.cpp:659 > +static void feedOrQueueMouseEvent(MouseEventInfo* eventInfo, bool shouldFeedQueuedEvents) It's not clear to me when a function is supposed to pass true or false to shouldFeeedQueuedEvents. Can you elaborate? > Tools/DumpRenderTree/efl/EventSender.cpp:661 > + if ((gDragMode || gStartOfEventQueue != gEndOfEventQueue || gEventQueue[gEndOfEventQueue].delay) Ditto about the values of gDragMode (ie. why it is in this check). > Tools/DumpRenderTree/efl/EventSender.cpp:688 > + // FIXME: We need to pass additional information with our events, so that > + // we could construct correct PlatformWheelEvent. At the moment, max number > + // of clicks is 3 Are there tests which are broken due to this?
Created attachment 140570 [details] Patch Fixed according to review comments
(In reply to comment #8) > (From update of attachment 136723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136723&action=review > > > LayoutTests/ChangeLog:9 > > + Reviewed by NOBODY (OOPS!). > > This line usually comes before the change's longer description. Fixed > > Tools/ChangeLog:8 > > + Reviewed by NOBODY (OOPS!). > > Ditto. Fixed > > Tools/DumpRenderTree/efl/EventSender.cpp:69 > > + ulong delay; > > This value is always set from an int below, so why is it a ulong here? Fixed > > Tools/DumpRenderTree/efl/EventSender.cpp:75 > > +static const int gDelayedQueueSize = 1024; > > +static DelayedEvent gEventQueue[gDelayedQueueSize]; > > +static unsigned gStartOfEventQueue; > > +static unsigned gEndOfEventQueue; > > Why do all this manual bookkeeping instead of using a WTF::Vector? Fixed (Tried to follow style:) > > Tools/DumpRenderTree/efl/EventSender.cpp:154 > > + MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers) > > + : event(event) > > + , modifiers(modifiers) > > + , button(EvasMouseButtonNone) > > + , horizontalDelta(0) > > + , verticalDelta(0) > > + { > > + } ... > > This set of constructors could be replaced by a single one: Fixed > MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers, EvasMouseButton button = EvasMouseButtonNone, int horizontalDelta = 0, int verticalDelta = 0) > > > Tools/DumpRenderTree/efl/EventSender.cpp:659 > > +static void feedOrQueueMouseEvent(MouseEventInfo* eventInfo, bool shouldFeedQueuedEvents) > > It's not clear to me when a function is supposed to pass true or false to shouldFeeedQueuedEvents. Can you elaborate? All mouse events except move should fire all events that are in the queue (same as in other ports). Usually, test code would programatically press mouse button and then send move command to select or drag element and when button is released, all events with their delays would be re-played in order to simulate user actions. > > Tools/DumpRenderTree/efl/EventSender.cpp:661 > > + if ((gDragMode || gStartOfEventQueue != gEndOfEventQueue || gEventQueue[gEndOfEventQueue].delay) > > Ditto about the values of gDragMode (ie. why it is in this check). Removed, not required at the moment. > > Tools/DumpRenderTree/efl/EventSender.cpp:688 > > + // FIXME: We need to pass additional information with our events, so that > > + // we could construct correct PlatformWheelEvent. At the moment, max number > > + // of clicks is 3 > > Are there tests which are broken due to this? Yes. fast/events/click-count.html WebKit doesn't care about 1-2-3-ple clicks. It needs click count, right now, in EFL max number of clicks is 3. GTK has same limitation, but guys have implemented custom click counter that fixes that issue.
Created attachment 140572 [details] Patch Fixed style.
Comment on attachment 140572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140572&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:66 > +struct MouseEventInfo; Nit: You could move the DetalyedEvent declaration after MouseEventInfo and avoid this forward declaration here. > Tools/DumpRenderTree/efl/EventSender.cpp:78 > +static WTF::Vector<DelayedEvent> gEventQueue; Constructing global static objects this way is dangerous -- in general, we use the DEFINE_STATIC_LOCAL macro for that. > Tools/DumpRenderTree/efl/EventSender.cpp:148 > +static void feedOrQueueMouseEvent(MouseEventInfo*, bool shouldFeedQueuedEvents = true); I think this can be made more clear, even if it requires a few more lines of code. A boolean parameter in a function with this name is not very expressive, and someone looking at a place which calls this function for the first time will probably get confused. I suggest using an enum instead of a boolean and removing the default value from it, so you end up calling something like feedOrQueueMouseEvent(myMouseEvent, FeedQueuedEvents); or feedOrQueueMouseEvent(myMouseEvent, DoNotFeedQueuedEvents); > Tools/DumpRenderTree/efl/EventSender.cpp:334 > + // We need to invert scolling values since in EFL negative z value means that Nit: scolling -> scrolling
Created attachment 140579 [details] Patch Fixed: * Forward declaration of MouseEventInfo * Use DEFINE_STATIC_LOCAL instead of "static WTF::Vector<DelayedEvent> gEventQueue;" * Fix typo: scolling -> scrolling * Use enum FeedQueuedEvents, DoNotFeedQueuedEvents to make code readable
Comment on attachment 140579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140579&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:150 > +DEFINE_STATIC_LOCAL(WTF::Vector<DelayedEvent>, gEventQueue, ()); We're almost there. The idea of DEFINE_STATIC_LOCAL is to use it inside a function; there are example uses in ewk and in WebCore.
Created attachment 140679 [details] Patch Moved DEFINE_STATIC_LOCAL to static getter
Created attachment 140689 [details] Patch Skipping few tests that were unskipped after rebasing
Comment on attachment 140689 [details] Patch LGTM now, thanks.
Comment on attachment 140689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140689&action=review Seems reasonable. rs=me. > Tools/DumpRenderTree/efl/EventSender.cpp:122 > +struct MouseEventInfo { Structs tend to very quickly become classes. :) > Tools/DumpRenderTree/efl/EventSender.cpp:302 > + MouseEventInfo* eventInfo = new MouseEventInfo(EvasMouseEventMove); OwnPtr? > Tools/DumpRenderTree/efl/EventSender.cpp:700 > + delete eventInfo; :( Manual memory management is sad times. :( > Tools/DumpRenderTree/efl/EventSender.cpp:709 > + usleep(delayedEvent.delay * 1000); This is sad. That we need to artificially slow down the testing?
Comment on attachment 140689 [details] Patch Clearing flags on attachment: 140689 Committed r116464: <http://trac.webkit.org/changeset/116464>
All reviewed patches have been landed. Closing bug.