Bug 218516

Created attachment 413061 [details]
Patch

Comment on attachment 413061 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413061&action=review

r=me

> Source/WebKit/Platform/IPC/Encoder.cpp:76
> +    : m_messageName(static_cast<MessageName>(0))

If you just want some initialization, and you don't have a specific use for 0, "m_messageName()" is probably better here.

> Source/WebKit/Platform/IPC/Encoder.h:104
> +    static RefPtr<WebCore::SharedBuffer> encodeObject(const T& object)

Not sure what distinction "encodeObject" intends to make here, in comparison to Encoder's "encode" functions. Maybe this should just be called "encode"?

Thanks for the review!

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 413061 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413061&action=review
> 
> r=me
> 
> > Source/WebKit/Platform/IPC/Encoder.cpp:76
> > +    : m_messageName(static_cast<MessageName>(0))
> 
> If you just want some initialization, and you don't have a specific use for
> 0, "m_messageName()" is probably better here.

Sounds good — changed to m_messageName().

> 
> > Source/WebKit/Platform/IPC/Encoder.h:104
> > +    static RefPtr<WebCore::SharedBuffer> encodeObject(const T& object)
> 
> Not sure what distinction "encodeObject" intends to make here, in comparison
> to Encoder's "encode" functions. Maybe this should just be called "encode"?

Hm...so I originally just wanted to call this `encode` as well, but I ran into compilation errors due to ambiguous calls to `encode` (since it's also defined as an instance method with the same signature :/).

Created attachment 413069 [details]
For landing

Comment on attachment 413069 [details]
For landing

View in context: https://bugs.webkit.org/attachment.cgi?id=413069&action=review

> Source/WebKit/Platform/IPC/Decoder.h:178
> +    static Optional<T> decodeObject(const uint8_t* buffer, size_t bufferSize)

I would have called 'buffer' 'sourceBytes' or 'source'.

> > > Source/WebKit/Platform/IPC/Encoder.h:104
> > > +    static RefPtr<WebCore::SharedBuffer> encodeObject(const T& object)
> > 
> > Not sure what distinction "encodeObject" intends to make here, in comparison
> > to Encoder's "encode" functions. Maybe this should just be called "encode"?
> 
> Hm...so I originally just wanted to call this `encode` as well, but I ran
> into compilation errors due to ambiguous calls to `encode` (since it's also
> defined as an instance method with the same signature :/).

I guess the distinction is that the static methods encode/decode just one object, and into a standalone buffer without attachments, and without any message name or destination.

Maybe we should call that "encodeSingleObject".

But also, can we just create a separate class? It's not clear why this new behavior is a part of a class that also has attachments, message names, destination ID, alignment adjustment, etc. Can we make an object that's more specific to the task of serializing one object into shared memory, and that doesn't advertise member functions that would be broken if you called them?

If there really is a significant chunk of code to share, maybe we can do that by making this new simpler class a base class, or by making stand-alone helper functions.

Comment on attachment 413069 [details]
For landing

View in context: https://bugs.webkit.org/attachment.cgi?id=413069&action=review

>> Source/WebKit/Platform/IPC/Decoder.h:178
>> +    static Optional<T> decodeObject(const uint8_t* buffer, size_t bufferSize)
> 
> I would have called 'buffer' 'sourceBytes' or 'source'.

👍🏻 renamed these parameters to `source` and `numberOfBytes`.

(In reply to Geoffrey Garen from comment #6)
> > > > Source/WebKit/Platform/IPC/Encoder.h:104
> > > > +    static RefPtr<WebCore::SharedBuffer> encodeObject(const T& object)
> > > 
> > > Not sure what distinction "encodeObject" intends to make here, in comparison
> > > to Encoder's "encode" functions. Maybe this should just be called "encode"?
> > 
> > Hm...so I originally just wanted to call this `encode` as well, but I ran
> > into compilation errors due to ambiguous calls to `encode` (since it's also
> > defined as an instance method with the same signature :/).
> 
> I guess the distinction is that the static methods encode/decode just one
> object, and into a standalone buffer without attachments, and without any
> message name or destination.
> 
> Maybe we should call that "encodeSingleObject".
> 
> But also, can we just create a separate class? It's not clear why this new
> behavior is a part of a class that also has attachments, message names,
> destination ID, alignment adjustment, etc. Can we make an object that's more
> specific to the task of serializing one object into shared memory, and that
> doesn't advertise member functions that would be broken if you called them?
> 
> If there really is a significant chunk of code to share, maybe we can do
> that by making this new simpler class a base class, or by making stand-alone
> helper functions.

encodeSingleObject/decodeSingleObject sound good to me — I renamed these new static methods to those. I did consider making separate Encoder/Decoder classes (e.g. RawDataEncoder/RawDataDecoder), but felt that the functionality we require is so close to what's already in Encoder/Decoder, that exposing a couple of helper methods to encode/decode data using these existing classes would make more sense.

I think some refactoring to either pull IPC::Attachments and IPC header logic out of Encoder/Decoder and into a derived class, or write new DataEncoder/DataDecoder classes  makes sense to me. If possible, I'd like to tackle this separately.

Created attachment 413081 [details]
Patch for landing

> encodeSingleObject/decodeSingleObject sound good to me — I renamed these new
> static methods to those. I did consider making separate Encoder/Decoder
> classes (e.g. RawDataEncoder/RawDataDecoder), but felt that the
> functionality we require is so close to what's already in Encoder/Decoder,
> that exposing a couple of helper methods to encode/decode data using these
> existing classes would make more sense.

👍🏻

> I think some refactoring to either pull IPC::Attachments and IPC header
> logic out of Encoder/Decoder and into a derived class, or write new
> DataEncoder/DataDecoder classes  makes sense to me. If possible, I'd like to
> tackle this separately.

👍🏻

Committed r269327: <https://trac.webkit.org/changeset/269327>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413081 [details].

<rdar://problem/71005847>