IPC::Message -> base::Pickle

IPC::Message -> base::Pickle

This changes consumers of IPC::Message to generally refer
refer to base::Pickle instead when performing serialization
and deserialization operations. The few specific instances
where the IPC::Message interface (Read/WriteAttachent) is
needed can safely be static_cast'd, as IPC::Message is the
only public subclass of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org

Committed: https://crrev.com/502c94ff1a9f99b5c4ecfe700415cc4ed14228dd
Cr-Commit-Position: refs/heads/master@{#373323}

[Ken Rockot(use gerrit already), 2016-02-02 16:41:09]

Description was changed from

==========
IPC::Message -> base::Pickle

This changes all consumers of IPC::Message to generally
refer to base::Pickle instead. The few specific instances
where the IPC::Message interface is needed can safely be
static_cast'd, as IPC::Message is the only public subclass
of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To facilitate this, base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment class to support this usage. The base
implementation of these methods is NOTREACHED() to guard
against misuse.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

to

==========
IPC::Message -> base::Pickle

This changes all consumers of IPC::Message to generally
refer to base::Pickle instead. The few specific instances
where the IPC::Message interface is needed can safely be
static_cast'd, as IPC::Message is the only public subclass
of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment class to support this usage. The base
implementation of these methods is NOTREACHED() to guard
against misuse.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-02-02 16:59:31]

Description was changed from

==========
IPC::Message -> base::Pickle

This changes all consumers of IPC::Message to generally
refer to base::Pickle instead. The few specific instances
where the IPC::Message interface is needed can safely be
static_cast'd, as IPC::Message is the only public subclass
of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment class to support this usage. The base
implementation of these methods is NOTREACHED() to guard
against misuse.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

to

==========
IPC::Message -> base::Pickle

This changes consumers of IPC::Message to generally refer
refer to base::Pickle instead when performing serialization
and deserialization operations. The few specific instances
where the IPC::Message interface (Read/WriteAttachent) is
needed can safely be static_cast'd, as IPC::Message is the
only public subclass of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment class to support this usage. The base
implementation of these methods is NOTREACHED() to guard
against misuse, which could realistically only occur if
one were to attempt passing messages-with-attachments over
a Mojo message pipe.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-02-02 17:00:04]

Description was changed from

==========
IPC::Message -> base::Pickle

This changes consumers of IPC::Message to generally refer
refer to base::Pickle instead when performing serialization
and deserialization operations. The few specific instances
where the IPC::Message interface (Read/WriteAttachent) is
needed can safely be static_cast'd, as IPC::Message is the
only public subclass of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment class to support this usage. The base
implementation of these methods is NOTREACHED() to guard
against misuse, which could realistically only occur if
one were to attempt passing messages-with-attachments over
a Mojo message pipe.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

to

==========
IPC::Message -> base::Pickle

This changes consumers of IPC::Message to generally refer
refer to base::Pickle instead when performing serialization
and deserialization operations. The few specific instances
where the IPC::Message interface (Read/WriteAttachent) is
needed can safely be static_cast'd, as IPC::Message is the
only public subclass of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment. The base implementation of these methods
is NOTREACHED() to guard against misuse, which should
realistically only occur if one were to attempt passing
messages-with-attachments over a Mojo message pipe.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

[jam, 2016-02-02 18:00:46]

lgtm

https://codereview.chromium.org/1659003003/diff/20001/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/1659003003/diff/20001/base/pickle.h#newcode127
base/pickle.h:127: //
nit: add comment

[jam, 2016-02-02 18:01:12]

(i'm not enough of a review for all these files, you'll want a security person
too)

[Ken Rockot(use gerrit already), 2016-02-02 18:02:15]

rockot@chromium.org changed reviewers:
+ tsepez@chromium.org

[Ken Rockot(use gerrit already), 2016-02-02 18:02:15]

tsepez@, please take a look. This is a pretty rote conversion, but there are a
handful of places where some static_casting is done.

[Tom Sepez, 2016-02-02 19:25:55]

It seems like there must be a cleaner way of doing this.  Maybe there's some
pedantic C++ developer around here who
could weigh in on alternatives.  I'm not it unfortunately.

What if the pickle::Atachement knew the type of attachments, and/or supported
virtual methods for doing the things you do in MessageAttachment?  That might
eliminate one source of downcasts.

https://codereview.chromium.org/1659003003/diff/60001/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/1659003003/diff/60001/base/pickle.h#newcode226
base/pickle.h:226: // NOTE: It is NOT OK to call this or ReadAttachment() on a
simple
It feels wrong to design a virtual API that requires knowing the concrete type
of the underlying class to avoid a NOTREACHED().  I'd be happier if we returned
false in that case and that false means the set is full or the pickle doesn't
support attachments.

[Ken Rockot(use gerrit already), 2016-02-02 19:50:43]

On Tue, Feb 2, 2016 at 11:25 AM, <tsepez@chromium.org> wrote:

> It seems like there must be a cleaner way of doing this.  Maybe there's some
> pedantic C++ developer around here who
> could weigh in on alternatives.  I'm not it unfortunately.
>
> What if the pickle::Atachement knew the type of attachments, and/or supported
> virtual methods for doing the things you do in MessageAttachment?  That might
> eliminate one source of downcasts.
>
> Moving the Type enum to Pickle::Attachment and making Read/WriteAttachment
fail gracefully seem reasonable. I'll make those changes.


>
> https://codereview.chromium.org/1659003003/diff/60001/base/pickle.h
> File base/pickle.h (right):
>
https://codereview.chromium.org/1659003003/diff/60001/base/pickle.h#newcode226
> base/pickle.h:226
<https://codereview.chromium.org/1659003003/diff/60001/base/pickle.h#newcode22...>:
// NOTE: It is NOT OK to call this or
> ReadAttachment() on a simple
> It feels wrong to design a virtual API that requires knowing the
> concrete type of the underlying class to avoid a NOTREACHED().  I'd be
> happier if we returned false in that case and that false means the set
> is full or the pickle doesn't support attachments.
> https://codereview.chromium.org/1659003003/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2016-02-02 20:38:35]

quick note: this change to Pickle is only temporary. once all the IPCs that use
attachements are converted to mojo, it can be removed

On 2016/02/02 19:25:55, Tom Sepez wrote:
> It seems like there must be a cleaner way of doing this.  Maybe there's some
> pedantic C++ developer around here who
> could weigh in on alternatives.  I'm not it unfortunately.
> 
> What if the pickle::Atachement knew the type of attachments, and/or supported
> virtual methods for doing the things you do in MessageAttachment?  That might
> eliminate one source of downcasts.
> 
> https://codereview.chromium.org/1659003003/diff/60001/base/pickle.h
> File base/pickle.h (right):
> 
> https://codereview.chromium.org/1659003003/diff/60001/base/pickle.h#newcode226
> base/pickle.h:226: // NOTE: It is NOT OK to call this or ReadAttachment() on a
> simple
> It feels wrong to design a virtual API that requires knowing the concrete type
> of the underlying class to avoid a NOTREACHED().  I'd be happier if we
returned
> false in that case and that false means the set is full or the pickle doesn't
> support attachments.

[Ken Rockot(use gerrit already), 2016-02-02 21:37:01]

No more downcasts! I fully moved the attachment concept into
base::Pickle::Attachment; IPC::MessageAttachment is now just an alias which can
be deleted at some later time. WDYT?

[jam, 2016-02-02 21:45:14]

On 2016/02/02 21:37:01, Ken Rockot wrote:
> No more downcasts! I fully moved the attachment concept into
> base::Pickle::Attachment; IPC::MessageAttachment is now just an alias which
can
> be deleted at some later time. WDYT?

hmm, it seems like a layering violation that base would know about mojo handles
and brokering. these are concepts that belong in layers above it in the stack.
why not just do the part where the base code returns false, and the derived
class is in src/ipc as before?

[Ken Rockot(use gerrit already), 2016-02-02 21:55:22]

Hmm. Not sure what's really worse - nominal awareness of attachment types
or potentially unsafe downcasts...

On Tue, Feb 2, 2016 at 1:45 PM, <jam@chromium.org> wrote:

> On 2016/02/02 21:37:01, Ken Rockot wrote:
> > No more downcasts! I fully moved the attachment concept into
> > base::Pickle::Attachment; IPC::MessageAttachment is now just an alias which
> can
> > be deleted at some later time. WDYT?
>
> hmm, it seems like a layering violation that base would know about mojo
handles
> and brokering. these are concepts that belong in layers above it in the
stack.
> why not just do the part where the base code returns false, and the derived
> class is in src/ipc as before?
> https://codereview.chromium.org/1659003003/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2016-02-02 23:54:09]

On 2016/02/02 21:55:22, Ken Rockot wrote:
> Hmm. Not sure what's really worse - nominal awareness of attachment types
> or potentially unsafe downcasts...

The only time it would be unsafe is if src/ipc gets a pickle that's not an
IPC::Message right? I don't see how that can actually happen, since it's the one
that creates the IPC::Message objects?

[Tom Sepez, 2016-02-03 00:00:53]

On 2016/02/02 23:54:09, jam wrote:
> On 2016/02/02 21:55:22, Ken Rockot wrote:
> > Hmm. Not sure what's really worse - nominal awareness of attachment types
> > or potentially unsafe downcasts...
> 
> The only time it would be unsafe is if src/ipc gets a pickle that's not an
> IPC::Message right? I don't see how that can actually happen, since it's the
one
> that creates the IPC::Message objects?

I'm OK with the casts in this situation, if it makes things cleaner, btw.

[Ken Rockot(use gerrit already), 2016-02-03 14:39:39]

Description was changed from

==========
IPC::Message -> base::Pickle

This changes consumers of IPC::Message to generally refer
refer to base::Pickle instead when performing serialization
and deserialization operations. The few specific instances
where the IPC::Message interface (Read/WriteAttachent) is
needed can safely be static_cast'd, as IPC::Message is the
only public subclass of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment. The base implementation of these methods
is NOTREACHED() to guard against misuse, which should
realistically only occur if one were to attempt passing
messages-with-attachments over a Mojo message pipe.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

to

==========
IPC::Message -> base::Pickle

This changes consumers of IPC::Message to generally refer
refer to base::Pickle instead when performing serialization
and deserialization operations. The few specific instances
where the IPC::Message interface (Read/WriteAttachent) is
needed can safely be static_cast'd, as IPC::Message is the
only public subclass of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-02-03 15:47:13]

OK - thank you both for enduring multiple iterations of this patch :)  I've
moved details back into IPC::MessageAttachment, and downcasts of
base::Pickle::Attachment => IPC::MessageAttachment are done when extracting
attachment details. Downcasts are completely avoided on the Pickles themselves,
so this should all be pretty safe.

[jam, 2016-02-03 16:36:17]

On 2016/02/03 15:47:13, Ken Rockot wrote:
> OK - thank you both for enduring multiple iterations of this patch :)  I've
> moved details back into IPC::MessageAttachment, and downcasts of
> base::Pickle::Attachment => IPC::MessageAttachment are done when extracting
> attachment details. Downcasts are completely avoided on the Pickles
themselves,
> so this should all be pretty safe.

lgtm, thanks

[Tom Sepez, 2016-02-03 17:44:05]

lgtm

[Ken Rockot(use gerrit already), 2016-02-03 18:45:04]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-02-03 18:47:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1659003003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1659003003/140001

[commit-bot: I haz the power, 2016-02-03 20:20:27]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-03 20:21:55]

Description was changed from

==========
IPC::Message -> base::Pickle

This changes consumers of IPC::Message to generally refer
refer to base::Pickle instead when performing serialization
and deserialization operations. The few specific instances
where the IPC::Message interface (Read/WriteAttachent) is
needed can safely be static_cast'd, as IPC::Message is the
only public subclass of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org
==========

to

==========
IPC::Message -> base::Pickle

This changes consumers of IPC::Message to generally refer
refer to base::Pickle instead when performing serialization
and deserialization operations. The few specific instances
where the IPC::Message interface (Read/WriteAttachent) is
needed can safely be static_cast'd, as IPC::Message is the
only public subclass of base::Pickle.

The purpose of this change is to facilitate the transition
to Mojo IPC, as we've trained the Mojo bindings layer to
use existing ParamTraits<T> parameterized over base::Pickle.

To support this base::Pickle has had some stub interfaces
added for WriteAttachment and ReadAttachment, along with a
Pickle::Attachment.

A follow-up patch will add sizing traits to the standard
IPC_STRUCT macros, enabling the majority of Chrome IPC
structs to be carried over Mojo message pipes as-is.

BUG=577685
R=jam@chromium.org

Committed: https://crrev.com/502c94ff1a9f99b5c4ecfe700415cc4ed14228dd
Cr-Commit-Position: refs/heads/master@{#373323}
==========

[commit-bot: I haz the power, 2016-02-03 20:21:58]

Patchset 8 (id:??) landed as
https://crrev.com/502c94ff1a9f99b5c4ecfe700415cc4ed14228dd
Cr-Commit-Position: refs/heads/master@{#373323}