[webkit-reviews] review granted: [Bug 75986] DispatchOnConnectionQueue messages should have a Connection parameter : [Attachment 121895] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 75986: DispatchOnConnectionQueue messages should have a Connection
parameter
https://bugs.webkit.org/show_bug.cgi?id=75986

Attachment 121895: Patch
https://bugs.webkit.org/attachment.cgi?id=121895&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=121895&action=review


> Source/WebKit2/Platform/CoreIPC/HandleMessage.h:211
> +// Dispatch functions with connection parameter.
> +template<typename C, typename MF, typename P1, typename P2>
> +void callMemberFunction(Connection* connection, const Arguments2<P1, P2>&
args, C* object, MF function)
> +{
> +    (object->*function)(connection, args.argument1, args.argument2);
> +}
> +
> +// Dispatch functions with connection parameter.
> +template<typename C, typename MF, typename P1>
> +void callMemberFunction(Connection* connection, const Arguments1<P1>& args,
C* object, MF function)
> +{
> +    (object->*function)(connection, args.argument1);
> +}

I was expecting these to be listed in the opposite order to match the rest of
the file.

> Source/WebKit2/Scripts/webkit2/messages.py:317
>      dispatch_function = 'handleMessage'
>      if message_is_variadic(message):
>	   dispatch_function += 'Variadic'

Maybe you should add after this:

if message.has_attribute(DISPATCH_ON_CONNECTION_QUEUE_ATTRIBUTE):
    dispatch_function += 'OnConnectionQueue'

> Source/WebKit2/Scripts/webkit2/messages.py:325
> +    if message.has_attribute(DISPATCH_ON_CONNECTION_QUEUE_ATTRIBUTE):
> +	   result.append('	 
CoreIPC::%sOnConnectionQueue<Messages::%s::%s>(connection, arguments, this,
&%s);\n' % (dispatch_function, receiver.name, message.name,
handler_function(receiver, message)))
> +	   result.append('	  didHandleMessage = true;\n')
> +    else:
> +	   result.append('	  CoreIPC::%s<Messages::%s::%s>(arguments,
this, &%s);\n' % (dispatch_function, receiver.name, message.name,
handler_function(receiver, message)))

…and then this can become:

dispatch_function_args = ['arguments, 'this', '&%s' %
handler_function(receiver, message)]
if message.has_attribute(DISPATCH_ON_CONNECTION_QUEUE_ATTRIBUTE):
    dispatch_function_args.insert(0, 'connection')
result.append('        CoreIPC::%s<Messages::%s::%s>(%s);\n' %
(dispatch_function, receiver.name, message.name, ',
'.join(dispatch_function_args))
if message.has_attribute(DISPATCH_ON_CONNECTION_QUEUE_ATTRIBUTE):
    result.append('	   didHandleMessage = true;\n')

In the end, I'm not that sure if this is better. But it does reduce some
duplication between the two cases.