Bug 93979 - [WK2] Implement eventSender.scheduleAsynchronousKeyDown
Summary: [WK2] Implement eventSender.scheduleAsynchronousKeyDown
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-14 09:32 PDT by Sergio Villar Senin
Modified: 2013-01-09 14:38 PST (History)
10 users (show)

See Also:


Attachments
Patch (10.03 KB, patch)
2013-01-01 05:27 PST, Chris Dumez
benjamin: review-
Details | Formatted Diff | Diff
Patch (13.09 KB, patch)
2013-01-09 01:04 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2012-08-14 09:32:25 PDT
Needed by: fast/mutation/end-of-task-delivery.html
Comment 1 Chris Dumez 2013-01-01 05:27:26 PST
Created attachment 180989 [details]
Patch
Comment 2 Benjamin Poulain 2013-01-08 17:56:53 PST
Comment on attachment 180989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180989&action=review

The patch looks good. Some nitpick:

> Tools/ChangeLog:14
> +        (WTR):

You can remove this.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:269
> +    return EventSenderMessageBody.leakRef();

I think it would be reasonable to return a WKRetainPtr here to avoid accidents if someone forgets to use Adopt.

> Tools/WebKitTestRunner/TestController.cpp:800
> +            WKRetainPtr<WKStringRef> keyKey = adoptWK(WKStringCreateWithUTF8CString("Key"));
> +            WKStringRef key = static_cast<WKStringRef>(WKDictionaryGetItemForKey(messageBodyDictionary, keyKey.get()));
> +
> +            WKRetainPtr<WKStringRef> modifiersKey = adoptWK(WKStringCreateWithUTF8CString("Modifiers"));
> +            WKEventModifiers modifiers = static_cast<WKEventModifiers>(WKUInt64GetValue(static_cast<WKUInt64Ref>(WKDictionaryGetItemForKey(messageBodyDictionary, modifiersKey.get()))));
> +
> +            WKRetainPtr<WKStringRef> locationKey = adoptWK(WKStringCreateWithUTF8CString("Location"));
> +            unsigned location = static_cast<unsigned>(WKUInt64GetValue(static_cast<WKUInt64Ref>(WKDictionaryGetItemForKey(messageBodyDictionary, locationKey.get()))));
> +
> +            // Forward to WebProcess
> +            WKPageSetShouldSendEventsSynchronously(mainWebView()->page(), false);
> +            m_eventSenderProxy->keyDown(key, modifiers, location);
> +
> +            return;

This is duplicated code from the synchronous message handler.
Please refactor to make a common path as much as possible.
Comment 3 Chris Dumez 2013-01-09 01:04:55 PST
Created attachment 181870 [details]
Patch

Take Benjamin's feedback into consideration.
Comment 4 Benjamin Poulain 2013-01-09 14:21:29 PST
Comment on attachment 181870 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181870&action=review

Looks good.
One comment but it is not blocking:

> Tools/WebKitTestRunner/TestController.cpp:815
> +            didReceiveKeyDownMessageFromInjectedBundle(messageBodyDictionary, false);

You can avoid this kind of code by using a enum instead of a boolean
    didReceiveKeyDownMessageFromInjectedBundle(messageBodyDictionary, SendSynchronously);
    didReceiveKeyDownMessageFromInjectedBundle(messageBodyDictionary, SendAsynchronously);
Comment 5 WebKit Review Bot 2013-01-09 14:38:11 PST
Comment on attachment 181870 [details]
Patch

Clearing flags on attachment: 181870

Committed r139242: <http://trac.webkit.org/changeset/139242>
Comment 6 WebKit Review Bot 2013-01-09 14:38:16 PST
All reviewed patches have been landed.  Closing bug.