Bug 46359 - Message autogeneration script should parse sync message syntax
Summary: Message autogeneration script should parse sync message syntax
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-23 08:47 PDT by Anders Carlsson
Modified: 2010-09-23 09:47 PDT (History)
0 users

See Also:


Attachments
Patch (8.18 KB, patch)
2010-09-23 08:53 PDT, Anders Carlsson
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2010-09-23 08:47:20 PDT
Message autogeneration script should parse sync message syntax
Comment 1 Anders Carlsson 2010-09-23 08:53:37 PDT
Created attachment 68526 [details]
Patch
Comment 2 Adam Roben (:aroben) 2010-09-23 09:07:04 PDT
Comment on attachment 68526 [details]
Patch

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.
Comment 3 Anders Carlsson 2010-09-23 09:47:06 PDT
Committed r68153: <http://trac.webkit.org/changeset/68153>