[WK2] [Mac] Implement a more-complete MouseDown/MouseUp/MouseMoveTo functions for WebKit2 EventSender

[Chang Shu]

We are ready to do a more complete implementation for the above functions in which the events go across processes for a true platform simulation.

[Chang Shu]

Any comments for the attempt, Darin?

[Chang Shu]

Created attachment 107855 [details]
patch 1

[Chang Shu]

ping reviewer. :)

[Darin Adler]

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".

[Chang Shu]

> 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.

[Chang Shu]

> > 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.

[Chang Shu]

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

[Chang Shu]

Created attachment 108165 [details]
patch 3: rebaseline

[Darin Adler]

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.

[Chang Shu]

> > Source/WebKit2/UIProcess/WebPageProxy.h:938
> > +    bool m_ShouldSendEventsSynchronously;
> 
> This should be a lowercase "s" here, m_shouldSendEventsSynchronously.

silly copy/paste error. :) thanks!

[Chang Shu]

Created attachment 108197 [details]
patch 4: to land

[WebKit Review Bot]

Comment on attachment 108197 [details]
patch 4: to land

Clearing flags on attachment: 108197

Committed r95660: <http://trac.webkit.org/changeset/95660>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.