Bug 99365 – Add MessageEncoder and MessageDecoder subclasses

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

Add MessageEncoder and MessageDecoder subclasses

[Anders Carlsson]

Created attachment 168780 [details]
Patch

[Eric Seidel (no email)]

Attachment 168780 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/Platform/CoreIPC/Connection.h:193:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:254:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:256:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:257:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:343:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/MessageEncoder.cpp:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Created attachment 168798 [details]
Patch

[Eric Seidel (no email)]

Attachment 168798 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/Platform/CoreIPC/Connection.h:193:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:254:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:256:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:257:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:343:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/MessageEncoder.cpp:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Created attachment 168825 [details]
Patch

[Darin Adler]

Comment on attachment 168825 [details]
Patch

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_arguments.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_arguments.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_arguments.leakPtr())));

Same thought about static_pointer_cast as above.

[Gyuyoung Kim]

Comment on attachment 168825 [details]
Patch

Attachment 168825 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14289872

[Rik Cabanier]

Attachment 168825 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/Target.pri', u'Source/WebKi..." exit_code: 1
WARNING: Patch's size is 79 kbytes.
Patches 20k or smaller are more likely to be reviewed. Larger patches may sit unreviewed for a long time.
Source/WebKit2/Platform/CoreIPC/Connection.h:193:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:254:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:256:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:257:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:343:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/MessageEncoder.cpp:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Created attachment 169021 [details]
Patch

[WebKit Review Bot]

Attachment 169021 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/Platform/CoreIPC/Connection.h:193:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:254:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:256:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:257:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:343:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/MessageEncoder.cpp:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 169021 [details]
Patch

Attachment 169021 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14384340

[Anders Carlsson]

(In reply to comment #6)
> (From update of attachment 168825 [details])
> 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.

Compared to the overhead of a message send I think it's fine. If we had a StringReference class (pointer + length) it would be even better. 

> 
> > Source/WebKit2/Platform/CoreIPC/MessageDecoder.h:30
> > +#include "DataReference.h"
> 
> Why include this instead of forward-declaring it?

Fixed.

> 
> > Source/WebKit2/Platform/CoreIPC/MessageDecoder.h:32
> > +#include <wtf/PassOwnPtr.h>
> > +#include <wtf/text/CString.h>
> 
> These includes are not needed.

I'm planning to add the message receiver name + message name as member variables, so the CString.h include will be required in the future. However, there's no need to add it now so I moved it to the .cpp file.


> 
> > 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.
> 

Fixed.

> > Source/WebKit2/Platform/CoreIPC/MessageEncoder.h:30
> > +#include <wtf/Forward.h>
> 
> I don’t think this include is needed.

It's needed for the CString using declaration.

> 
> > 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?

Good point. I added it as a reminder to myself but the FIXME is good enough.

> 
> > Source/WebKit2/Scripts/webkit2/messages.py:505
> > +            result.append('    bool result = m_connection->sendSyncReply(adoptPtr(static_cast<CoreIPC::MessageEncoder*>(m_arguments.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.

I agree. Will fix.

> 
> > 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.

Fixed.

[Anders Carlsson]

Created attachment 169065 [details]
Patch

[WebKit Review Bot]

Attachment 169065 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/Platform/CoreIPC/Connection.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/Platform/CoreIPC/Connection.h:194:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:255:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:257:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:258:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:344:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 6 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 169065 [details]
Patch

Attachment 169065 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14389378

[Early Warning System Bot]

Comment on attachment 169065 [details]
Patch

Attachment 169065 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14395349

[Gyuyoung Kim]

Comment on attachment 169065 [details]
Patch

Attachment 169065 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14392372

[Anders Carlsson]

Created attachment 169208 [details]
Patch

[WebKit Review Bot]

Attachment 169208 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/Platform/CoreIPC/Connection.h:194:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:255:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:257:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:258:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:344:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 5 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 169208 [details]
Patch

Attachment 169208 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14386675

[Anders Carlsson]

(In reply to comment #20)
> (From update of attachment 169208 [details])
> Attachment 169208 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/14386675

Something is wrong with the EFL build scripts - they won't rebuild anything when the message scripts are changed. I'll make a tiny change to the .messages.in files to work around this.

[Anders Carlsson]

Created attachment 169262 [details]
Patch

[WebKit Review Bot]

Attachment 169262 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/Platform/CoreIPC/Connection.h:194:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:255:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:257:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:258:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:344:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 5 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Committed r131679: <http://trac.webkit.org/changeset/131679>