[webkit-reviews] review denied: [Bug 68108] [WK2] [Mac] Implement a more-complete MouseDown/MouseUp/MouseMoveTo functions for WebKit2 EventSender : [Attachment 107855] patch 1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 20 13:20:51 PDT 2011


Darin Adler <darin at apple.com> has denied Chang Shu <cshu at webkit.org>'s request
for review:
Bug 68108: [WK2] [Mac] Implement a more-complete MouseDown/MouseUp/MouseMoveTo
functions for WebKit2 EventSender
https://bugs.webkit.org/show_bug.cgi?id=68108

Attachment 107855: patch 1
https://bugs.webkit.org/attachment.cgi?id=107855&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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".


More information about the webkit-reviews mailing list