Bug 61962

Created attachment 95810 [details] Patch

Comment on attachment 95810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95810&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:102 > + if (!modifiers) { > + for (unsigned modifier = 0; modifier < 4; ++modifier) > + evas_key_modifier_off(evas, modifierNames[modifier]); > + } else { This is redundant, the rest of the loop has this same behavior when !modifiers. > Tools/DumpRenderTree/efl/EventSender.cpp:164 > + if (vertical && horizontal) { > + if (event & EVAS_MOUSE_EVENT_SCROLL_UP) > + evas_event_feed_mouse_wheel(evas, 0, 10, timestamp, 0); > + else > + evas_event_feed_mouse_wheel(evas, 0, -10, timestamp, 0); > + > + if (event & EVAS_MOUSE_EVENT_SCROLL_LEFT) > + evas_event_feed_mouse_wheel(evas, 1, 10, timestamp, 0); > + else > + evas_event_feed_mouse_wheel(evas, 1, -10, timestamp, 0); > + > + ++timestamp; > + } else { > + if (event & EVAS_MOUSE_EVENT_SCROLL_UP) > + evas_event_feed_mouse_wheel(evas, 0, 10, timestamp++, 0); > + else if (event & EVAS_MOUSE_EVENT_SCROLL_DOWN) > + evas_event_feed_mouse_wheel(evas, 0, -10, timestamp++, 0); > + > + if (event & EVAS_MOUSE_EVENT_SCROLL_LEFT) > + evas_event_feed_mouse_wheel(evas, 1, 10, timestamp++, 0); > + else if (event & EVAS_MOUSE_EVENT_SCROLL_RIGHT) > + evas_event_feed_mouse_wheel(evas, 1, -10, timestamp++, 0); > + } Can't this be written more concisely with some smrart ifs to set a value -1 or 1 to be multipled by the 10? > Tools/DumpRenderTree/efl/EventSender.cpp:173 > + Evas* evas = evas_object_evas_get(mainFrame); No free, I assume? If not, this whole function can be 1 line. > Tools/DumpRenderTree/efl/EventSender.cpp:197 > + JSStringRef string = JSValueToStringCopy(context, value, 0); Don't we have a smart pointer for the JSC API? We must. Some flavor of RetainPtr maybe? > Tools/DumpRenderTree/efl/EventSender.cpp:273 > + lastClickPositionX = lastMousePositionX; > + lastClickPositionY = lastMousePositionY; > + lastClickButton = buttonCurrentlyDown; > + lastClickTimeOffset = timeOffset; Are these globals? Can we name them in such a way to distinguish that? > Tools/DumpRenderTree/efl/EventSender.cpp:317 > + unsigned mouseEvent = 0; > + if (vertical > 0) > + mouseEvent |= EVAS_MOUSE_EVENT_SCROLL_UP; > + else if (vertical < 0) > + mouseEvent |= EVAS_MOUSE_EVENT_SCROLL_DOWN; > + > + if (horizontal > 0) > + mouseEvent |= EVAS_MOUSE_EVENT_SCROLL_RIGHT; > + else if (horizontal < 0) > + mouseEvent |= EVAS_MOUSE_EVENT_SCROLL_LEFT; I might have made this a helper function. > Tools/DumpRenderTree/efl/EventSender.cpp:357 > + if (location == DOM_KEY_LOCATION_NUMPAD) { > + if (JSStringIsEqualToUTF8CString(character, "leftArrow")) > + keyName = "KP_Left"; > + else if (JSStringIsEqualToUTF8CString(character, "rightArrow")) > + keyName = "KP_Right"; > + else if (JSStringIsEqualToUTF8CString(character, "upArrow")) > + keyName = "KP_Up"; > + else if (JSStringIsEqualToUTF8CString(character, "downArrow")) > + keyName = "KP_Down"; > + else if (JSStringIsEqualToUTF8CString(character, "pageUp")) I definitely would have made these two huge if blocks helper functions. :) > Tools/DumpRenderTree/efl/EventSender.cpp:466 > + zoomIn(FALSE); Bools are a horrible way to specify behavior. They're unreadable at teh callsite. Enums are much preferred. But I'm nto sure you even want such. This is dead code at the moment and woudl be better to just remove for now.

Created attachment 97331 [details] EventSender, take 2 This version should hopefully be more readable. check-webkit-style will raise a false positive due to context being called context in EventSender.h.

Attachment 97331 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/e..." exit_code: 1 Tools/DumpRenderTree/efl/EventSender.h:38: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 97334 [details] Take 2, now pleasing the style bot

Eric, is there anything you'd still like us to change in the patch?

Comment on attachment 97334 [details] Take 2, now pleasing the style bot View in context: https://bugs.webkit.org/attachment.cgi?id=97334&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:68 > + DOM_KEY_LOCATION_STANDARD = 0x00, > + DOM_KEY_LOCATION_LEFT = 0x01, > + DOM_KEY_LOCATION_RIGHT = 0x02, > + DOM_KEY_LOCATION_NUMPAD = 0x03 Why define the values in hex? > Tools/DumpRenderTree/efl/EventSender.cpp:96 > +enum EvasKeyModifier { > + EVAS_KEY_MODIFIER_NONE = 0, > + EVAS_KEY_MODIFIER_CONTROL = 1 << 0, > + EVAS_KEY_MODIFIER_SHIFT = 1 << 1, > + EVAS_KEY_MODIFIER_ALT = 1 << 2, > + EVAS_KEY_MODIFIER_META = 1 << 3 > +}; > + > +enum EvasMouseButton { > + EVAS_MOUSE_BUTTON_NONE = 0x00, > + EVAS_MOUSE_BUTTON_LEFT = 0x01, > + EVAS_MOUSE_BUTTON_MIDDLE = 0x02, > + EVAS_MOUSE_BUTTON_RIGHT = 0x03 > +}; > + > +enum EvasMouseEvent { > + EVAS_MOUSE_EVENT_NONE = 0, > + EVAS_MOUSE_EVENT_DOWN = 1 << 0, > + EVAS_MOUSE_EVENT_UP = 1 << 1, > + EVAS_MOUSE_EVENT_MOVE = 1 << 2, > + EVAS_MOUSE_EVENT_SCROLL_UP = 1 << 3, > + EVAS_MOUSE_EVENT_SCROLL_DOWN = 1 << 4, > + EVAS_MOUSE_EVENT_SCROLL_LEFT = 1 << 5, > + EVAS_MOUSE_EVENT_SCROLL_RIGHT = 1 << 6, > + EVAS_MOUSE_EVENT_CLICK = EVAS_MOUSE_EVENT_MOVE | EVAS_MOUSE_EVENT_DOWN | EVAS_MOUSE_EVENT_UP, > +}; Are these enums which are defined in some EFL header which we should be importing instead of duplicating? > Tools/DumpRenderTree/efl/EventSender.cpp:179 > + if (JSStringIsEqualToUTF8CString(jsKeyValue.get(), "ctrlKey") || JSStringIsEqualToUTF8CString(jsKeyValue.get(), "addSelectionKey")) I feel like we talked about some sort of equals() helper in one of these patches.

I don't really want to be involved in EFL patches anymore. The c-style code is just depressing.

(In reply to comment #7) > (From update of attachment 97334 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97334&action=review > > > Tools/DumpRenderTree/efl/EventSender.cpp:68 > > + DOM_KEY_LOCATION_STANDARD = 0x00, > > + DOM_KEY_LOCATION_LEFT = 0x01, > > + DOM_KEY_LOCATION_RIGHT = 0x02, > > + DOM_KEY_LOCATION_NUMPAD = 0x03 > > Why define the values in hex? Fixed locally. > > Tools/DumpRenderTree/efl/EventSender.cpp:96 > > +enum EvasKeyModifier { > > + EVAS_KEY_MODIFIER_NONE = 0, > > + EVAS_KEY_MODIFIER_CONTROL = 1 << 0, > > + EVAS_KEY_MODIFIER_SHIFT = 1 << 1, > > + EVAS_KEY_MODIFIER_ALT = 1 << 2, > > + EVAS_KEY_MODIFIER_META = 1 << 3 > > +}; > > + > > +enum EvasMouseButton { > > + EVAS_MOUSE_BUTTON_NONE = 0x00, > > + EVAS_MOUSE_BUTTON_LEFT = 0x01, > > + EVAS_MOUSE_BUTTON_MIDDLE = 0x02, > > + EVAS_MOUSE_BUTTON_RIGHT = 0x03 > > +}; > > + > > +enum EvasMouseEvent { > > + EVAS_MOUSE_EVENT_NONE = 0, > > + EVAS_MOUSE_EVENT_DOWN = 1 << 0, > > + EVAS_MOUSE_EVENT_UP = 1 << 1, > > + EVAS_MOUSE_EVENT_MOVE = 1 << 2, > > + EVAS_MOUSE_EVENT_SCROLL_UP = 1 << 3, > > + EVAS_MOUSE_EVENT_SCROLL_DOWN = 1 << 4, > > + EVAS_MOUSE_EVENT_SCROLL_LEFT = 1 << 5, > > + EVAS_MOUSE_EVENT_SCROLL_RIGHT = 1 << 6, > > + EVAS_MOUSE_EVENT_CLICK = EVAS_MOUSE_EVENT_MOVE | EVAS_MOUSE_EVENT_DOWN | EVAS_MOUSE_EVENT_UP, > > +}; > > Are these enums which are defined in some EFL header which we should be importing instead of duplicating? No. > > Tools/DumpRenderTree/efl/EventSender.cpp:179 > > + if (JSStringIsEqualToUTF8CString(jsKeyValue.get(), "ctrlKey") || JSStringIsEqualToUTF8CString(jsKeyValue.get(), "addSelectionKey")) > > I feel like we talked about some sort of equals() helper in one of these patches. Fixed locally.

Created attachment 98010 [details] Patch

Comment on attachment 98010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98010&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:294 > + int horizontal = (int)JSValueToNumber(context, arguments[0], exception); > + if (exception && *exception) > + return JSValueMakeUndefined(context); > + int vertical = (int)JSValueToNumber(context, arguments[1], exception); C-style cast is not recommended in WebKit. > Tools/DumpRenderTree/efl/EventSender.h:38 > +JSObjectRef makeEventSender(JSContextRef, bool); We avoid using bool for a function parameter, and use enum instead. If you need to use bool, you should write an explanation of the argument in the header.

Created attachment 98717 [details] New patch following Kent's suggestions

CC'ing Kent after adding a new patch after his suggestions.

Comment on attachment 98717 [details] New patch following Kent's suggestions Rubber-stamped by tkent

Comment on attachment 98717 [details] New patch following Kent's suggestions Clearing flags on attachment: 98717 Committed r89820: <http://trac.webkit.org/changeset/89820>

All reviewed patches have been landed. Closing bug.