[webkit-reviews] review granted: [Bug 46359] Message autogeneration script should parse sync message syntax : [Attachment 68526] 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 46359: Message autogeneration script should parse sync message syntax
https://bugs.webkit.org/show_bug.cgi?id=46359

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

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

> WebKit2/Scripts/webkit2/messages.py:87
> +		       reply_parameters = [Parameter(*type_and_name.split(' '))
for type_and_name in reply_parameters_string.split(', ')]

Maybe we should add a function that does this instead of duplicating the code.

> WebKit2/Scripts/webkit2/messages.py:102
> +	   if self.reply_parameters != None:

I think "is not" is preferred over "!=".

> WebKit2/Scripts/webkit2/messages.py:136
> +    builtin_types = ['bool', 'uint8_t', 'uint16_t', 'uint32_t', 'uint64_t']

Wrapping this in set() would be good.

I don't see a test that passes a type with :: as a non-reference.

> WebKit2/Scripts/webkit2/messages.py:166
> +def reply_base_class(message):
> +    reply_base_class = 'CoreIPC::Arguments%d' %
len(message.reply_parameters)
> +    if len(message.reply_parameters):
> +	   reply_base_class = '%s<%s>' % (reply_base_class, ',
'.join(reply_parameter_type(parameter.type) for parameter in
message.reply_parameters))
> +    return reply_base_class
> +
> +
> +def delayed_reply_base_class(message):
> +    delayed_reply_base_class = 'CoreIPC::Arguments%d' %
len(message.reply_parameters)
> +    if len(message.reply_parameters):
> +	   delayed_reply_base_class = '%s<%s>' % (delayed_reply_base_class, ',
'.join(function_parameter_type(parameter.type) for parameter in
message.reply_parameters))
> +    return delayed_reply_base_class

So much code duplication! Can we just pass the *_parameter_type function as a
parameter?

> WebKit2/Scripts/webkit2/messages_unittest.py:154
> +	   if message.reply_parameters != None:
> +	       for index, parameter in enumerate(message.reply_parameters):
> +		   self.assertEquals(parameter.type,
expected_message['reply_parameters'][index][0])
> +		   self.assertEquals(parameter.name,
expected_message['reply_parameters'][index][1])

Hm, this won't catch cases where we expect there to be reply parameters but for
some reason the parsing screwed up and found none.

> WebKit2/Scripts/webkit2/messages_unittest.py:173
> +	       if message.reply_parameters != None:
> +		   self.assertEquals(messages.reply_base_class(message),
_expected_results['messages'][index]['reply_base_class'])
> +		   if message.delayed:
> +		      
self.assertEquals(messages.delayed_reply_base_class(message),
_expected_results['messages'][index]['delayed_reply_base_class'])

Ditto.