Bug 68108

Any comments for the attempt, Darin?

Created attachment 107855 [details] patch 1

ping reviewer. :)

Comment on attachment 107855 [details] patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=107855&action=review A step in the right direction, but still needs some work. Thanks for tackling this! > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:72 > -WK_EXPORT void WKPageSetShouldSendKeyboardEventSynchronously(WKPageRef page, bool sync); > +WK_EXPORT void WKPageSetShouldSendEventSynchronously(WKPageRef page, bool sync); This, and all the other names, should probably be plural, since it affects all events, not just one event. > Tools/WebKitTestRunner/EventSenderProxy.h:45 > + : m_testController(testController) > + , m_time(0) > + , m_position() > + , m_leftMouseButtonDown(false) > + , m_clickCount(0) > + , m_clickTime(0) > + , m_clickPosition() > + , m_clickButton(kWKEventMouseButtonNoButton) > + , eventNumber(0) This is not the normal WebKit formatting. We typically indent initializer lists, and I think this is covered in the style guide. > Tools/WebKitTestRunner/TestController.cpp:537 > + } else if (WKStringIsEqualToUTF8CString(subMessageName, "MouseDown") || WKStringIsEqualToUTF8CString(subMessageName, "MouseUp")) { We don’t do else after return. > Tools/WebKitTestRunner/TestController.cpp:552 > + } else if (WKStringIsEqualToUTF8CString(subMessageName, "MouseMoveTo")) { Ditto. > Tools/WebKitTestRunner/TestController.cpp:564 > + } else if (WKStringIsEqualToUTF8CString(subMessageName, "LeapForward")) { Ditto. > Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:44 > +#if !PLATFORM(MAC) I think this conditionals are mystifying. Instead we should probably define something in the header that controls whether we use WKBundlePageSimulateMouseDown or EventSender MouseDown and use that instead of saying PLATFORM(MAC) each time. > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:121 > +// [[[mainFrame frameView] documentView] layout]; Lets not leave commented-out code like this in here. Also, it’s not needed. > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:140 > + NSView *subView = [m_testController->mainWebView()->platformView() hitTest:[event locationInWindow]]; > + if (subView) { > + [subView mouseDown:event]; > + if (buttonNumber == LeftMouseButton) > + m_leftMouseButtonDown = true; > + } I don’t understand why this is needed. A WKView doesn’t have any subviews, does it? Also, the word is subview, not sub view, so it should be subview, not subView. > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:187 > + [subView mouseMoved:event]; // FIXME: [subView mouseDragged:event]; That FIXME is unclear. Why is it a FIXME and not working that way. > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:193 > +void EventSenderProxy::keyDown(WKStringRef keyRef, WKEventModifiers modifiersRef, unsigned int keyLocation) Should be unsigned, not unsigned int. Here in the implementation it should just be key and modifiers, no need to say "ref".

> A step in the right direction, but still needs some work. Thanks for tackling this! Thanks for the review! > > > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:140 > > + NSView *subView = [m_testController->mainWebView()->platformView() hitTest:[event locationInWindow]]; > > + if (subView) { > > + [subView mouseDown:event]; > > + if (buttonNumber == LeftMouseButton) > > + m_leftMouseButtonDown = true; > > + } > > I don’t understand why this is needed. A WKView doesn’t have any subviews, does it? Good point. I need to take a deeper look. > > Also, the word is subview, not sub view, so it should be subview, not subView. > > > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:187 > > + [subView mouseMoved:event]; // FIXME: [subView mouseDragged:event]; > > That FIXME is unclear. Why is it a FIXME and not working that way. The drag support is taken out from the original code from WK1. When it's supported, we should call mouseDragged instead. I will add drag support later.

> > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:140 > > + NSView *subView = [m_testController->mainWebView()->platformView() hitTest:[event locationInWindow]]; > > + if (subView) { > > + [subView mouseDown:event]; > > + if (buttonNumber == LeftMouseButton) > > + m_leftMouseButtonDown = true; > > + } > > I don’t understand why this is needed. A WKView doesn’t have any subviews, does it? Do you mean why hitTest is needed? For example, when the mouse moves out of the window, platformView() still returns a valid pointer to the main window but hitTest will return 0 so subView(will rename it to targetView) is 0.

Created attachment 108150 [details] patch 2: update per review

Created attachment 108165 [details] patch 3: rebaseline

Comment on attachment 108165 [details] patch 3: rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=108165&action=review > Source/WebKit2/UIProcess/WebPageProxy.h:938 > + bool m_ShouldSendEventsSynchronously; This should be a lowercase "s" here, m_shouldSendEventsSynchronously.

> > Source/WebKit2/UIProcess/WebPageProxy.h:938 > > + bool m_ShouldSendEventsSynchronously; > > This should be a lowercase "s" here, m_shouldSendEventsSynchronously. silly copy/paste error. :) thanks!

Created attachment 108197 [details] patch 4: to land

Comment on attachment 108197 [details] patch 4: to land Clearing flags on attachment: 108197 Committed r95660: <http://trac.webkit.org/changeset/95660>

All reviewed patches have been landed. Closing bug.