[webkit-reviews] review granted: [Bug 99365] Add MessageEncoder and MessageDecoder subclasses : [Attachment 168825] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 99365: Add MessageEncoder and MessageDecoder subclasses
https://bugs.webkit.org/show_bug.cgi?id=99365

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=168825&action=review


> Source/WebKit2/Platform/CoreIPC/Connection.h:412
> +    OwnPtr<MessageEncoder> encoder = MessageEncoder::create("", "",
destinationID);

Is this an efficient idiom? Seems possibly costly to create the two CString
objects each time.

> Source/WebKit2/Platform/CoreIPC/MessageDecoder.h:30
> +#include "DataReference.h"

Why include this instead of forward-declaring it?

> Source/WebKit2/Platform/CoreIPC/MessageDecoder.h:32
> +#include <wtf/PassOwnPtr.h>
> +#include <wtf/text/CString.h>

These includes are not needed.

> Source/WebKit2/Platform/CoreIPC/MessageEncoder.cpp:33
> +    PassOwnPtr<MessageEncoder> MessageEncoder::create(const CString&
messageReceiverName, const CString& messageName, uint64_t destinationID)

Xcode indented this for you and you forgot to undo that.

> Source/WebKit2/Platform/CoreIPC/MessageEncoder.h:30
> +#include <wtf/Forward.h>

I don’t think this include is needed.

> Source/WebKit2/Platform/CoreIPC/mac/ConnectionMac.cpp:374
>	       // FIXME: Disconnect.
> +	       ASSERT_NOT_REACHED();

How can both of these lines be true? If this can’t be reached, then why do we
need to disconnect? If this can be reached, then it’s not right to assert, is
it?

> Source/WebKit2/Scripts/webkit2/messages.py:505
> +	       result.append('	  bool result =
m_connection->sendSyncReply(adoptPtr(static_cast<CoreIPC::MessageEncoder*>(m_ar
guments.leakPtr())));\n')

We should overload static_pointer_cast for PassOwnPtr like we do for RefPtr, so
you don’t have to leakPtr and adoptPtr just to cast.

> Source/WebKit2/Scripts/webkit2/messages_unittest.py:327
> -    class ArgumentEncoder;
> +    class MessageEncoder;
>      class Connection;

This was sorted alphabetically. Would be nice to keep it so.

> Source/WebKit2/Scripts/webkit2/messages_unittest.py:658
> +    bool result =
m_connection->sendSyncReply(adoptPtr(static_cast<CoreIPC::MessageEncoder*>(m_ar
guments.leakPtr())));

Same thought about static_pointer_cast as above.

> Source/WebKit2/Scripts/webkit2/messages_unittest.py:677
> +    bool result =
m_connection->sendSyncReply(adoptPtr(static_cast<CoreIPC::MessageEncoder*>(m_ar
guments.leakPtr())));

Same thought about static_pointer_cast as above.