Add IPC ParamTraits for c-style arrays

Add IPC ParamTraits for c-style arrays

Currently, we don't have generic ParamTraits for c-style arrays (as we
do for vectors). This leads to a bit of copy-pasted code for each type
which sends a c-style array over IPC.

This change adds ParamTraits, allowing c-style array members to be
easily added via the IPC_STRUCT_TRAITS_MEMBER macro.

R=dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2686503004
Cr-Commit-Position: refs/heads/master@{#449650}
Committed: https://chromium.googlesource.com/chromium/src/+/b5de4603585dba1e41900cbc5dc3fd4a65d200ff

[ericrk, 2017-02-07 21:20:03]

Description was changed from

==========
Add IPC ParamTraits for c-style arrays

Currently, we don't have generic ParamTraits for c-style arrays (as we
do for vectors). This leads to a bit of copy-pasted code for each type
which sends a c-style array over IPC.

This change adds ParamTraits, allowing c-style array members to be
easily added via the IPC_STRUCT_TRAITS_MEMBER macro.

R=dcheng@chromium.org
==========

to

==========
Add IPC ParamTraits for c-style arrays

Currently, we don't have generic ParamTraits for c-style arrays (as we
do for vectors). This leads to a bit of copy-pasted code for each type
which sends a c-style array over IPC.

This change adds ParamTraits, allowing c-style array members to be
easily added via the IPC_STRUCT_TRAITS_MEMBER macro.

R=dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ericrk, 2017-02-07 21:38:07]

While converting the std::vector to a plain per your comment on
crrev.com/2654993004, I realized that we don't have ParamTraits to handle
c-style arrays. Decided to add one in, which allows some CC and ppapi code to be
simplified (as well as allowing the future code in crrev.com/2654993004). let me
know how this looks. Thanks!

[ericrk, 2017-02-07 21:38:31]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 21:39:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 21:52:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 21:52:58]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-02-08 00:13:37]

https://codereview.chromium.org/2686503004/diff/20001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/20001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:314: GetParamSize(sizer, static_cast<int>(Size));
Let's use strict_cast instead of static_cast throughout. I think it's highly
unlikely we'll have an array that will overflow the size here... but it's a
compile-time check, it should be free and can't hurt.

https://codereview.chromium.org/2686503004/diff/20001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:330: // For a fixed size array, a size mismatch should
be impossible.
It's not impossible if the renderer sends us bad data on purpose =)

[ericrk, 2017-02-08 18:52:37]

Patchset #3 (id:40001) has been deleted

[ericrk, 2017-02-08 18:57:49]

https://codereview.chromium.org/2686503004/diff/20001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/20001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:314: GetParamSize(sizer, static_cast<int>(Size));
On 2017/02/08 00:13:37, dcheng wrote:
> Let's use strict_cast instead of static_cast throughout. I think it's highly
> unlikely we'll have an array that will overflow the size here... but it's a
> compile-time check, it should be free and can't hurt.

So, strict_cast seems to only consider the types being cast between, not the
value being cast (even when the value is a compile-time constant), so
strict_cast raises an error here, as you can't safely go from size_t > int for
arbitrary values.

I added a static_assert at the beginning of the class to ensure that Size <=
std::numeric_limits<int>::max(), which should guard against the same issues.

https://codereview.chromium.org/2686503004/diff/20001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:330: // For a fixed size array, a size mismatch should
be impossible.
On 2017/02/08 00:13:37, dcheng wrote:
> It's not impossible if the renderer sends us bad data on purpose =)

heh, true enough, updated the comment

[dcheng, 2017-02-08 19:21:11]

LGTM

Btw, I talked with jschuh@ and checked_cast should work here (it's a
compile-time check if the values are known at compile time), but I don't feel
strongly about this.

https://codereview.chromium.org/2686503004/diff/60001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/60001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:333: // For a fixed size array, a size mismath indicates
invalid data.
Nit: mismatch

[vmpstr, 2017-02-08 20:06:11]

https://codereview.chromium.org/2686503004/diff/60001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/60001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:343: for (size_t i = 0; i < Size; ++i) {
drive-by: In the other files where we do something in the log, we also append [
and ] to the start and end respectively, so that it kind of looks like an array,
I guess. Should we do the same thing here?

[ericrk, 2017-02-08 21:02:43]

Patchset #4 (id:80001) has been deleted

[ericrk, 2017-02-08 21:07:15]

https://codereview.chromium.org/2686503004/diff/60001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/60001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:333: // For a fixed size array, a size mismath indicates
invalid data.
On 2017/02/08 19:21:11, dcheng wrote:
> Nit: mismatch

Done.

https://codereview.chromium.org/2686503004/diff/60001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:343: for (size_t i = 0; i < Size; ++i) {
On 2017/02/08 20:06:11, vmpstr wrote:
> drive-by: In the other files where we do something in the log, we also append
[
> and ] to the start and end respectively, so that it kind of looks like an
array,
> I guess. Should we do the same thing here?

This is pretty inconsistent - CC already has traits for a number of vectors,
which don't have "[", "]", it did use brackets on its arrays though... I guess
the brackets probably look better. Added.

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:314: GetParamSize(sizer, base::checked_cast<int>(Size));
Switched to checked_cast per your comment.

[ericrk, 2017-02-08 21:08:09]

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

[ericrk, 2017-02-08 21:08:09]

+tsepez for ipc/ owners. Thanks!

[ericrk, 2017-02-08 21:08:16]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 21:09:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 22:38:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 22:38:17]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-02-09 17:14:37]

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:314: GetParamSize(sizer, base::checked_cast<int>(Size));
Why are we passing the size over the wire?  These templates apply to arrays of
known size, and we should just read/write the expected number of members.

[Tom Sepez, 2017-02-09 17:17:11]

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:320: for (size_t i = 0; i < Size; i++)
nit: can we use a range-based for loop and avoid having an index variable at
all?

[ericrk, 2017-02-09 22:02:08]

thanks for the feedback.

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:314: GetParamSize(sizer, base::checked_cast<int>(Size));
On 2017/02/09 17:14:37, Tom Sepez wrote:
> Why are we passing the size over the wire?  These templates apply to arrays of
> known size, and we should just read/write the expected number of members.

True enough, I guess I was thinking it was more robust against malformed data,
but it does seem pretty unlikely that this would help us. Removed.

https://codereview.chromium.org/2686503004/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:320: for (size_t i = 0; i < Size; i++)
On 2017/02/09 17:17:10, Tom Sepez wrote:
> nit: can we use a range-based for loop and avoid having an index variable at
> all?

Yup, we can.

https://codereview.chromium.org/2686503004/diff/120001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/120001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:332: for (size_t i = 0; i < Size; ++i) {
Didn't move to range-based-for here, as it makes it somewhat more verbose to do
the "append a space on all but the first run" logic below.

[Tom Sepez, 2017-02-09 22:28:52]

lgtm

[Tom Sepez, 2017-02-09 22:39:07]

https://codereview.chromium.org/2686503004/diff/120001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/120001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:332: for (size_t i = 0; i < Size; ++i) {
On 2017/02/09 22:02:08, ericrk wrote:
> Didn't move to range-based-for here, as it makes it somewhat more verbose to
do
> the "append a space on all but the first run" logic below.
This is fine, but for future reference, I believe the idiom is something like

for (auto& element : container) {
  if (&element != &container.front())
    output(" ");
  ...
}

as you like

[ericrk, 2017-02-10 00:15:28]

https://codereview.chromium.org/2686503004/diff/120001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/2686503004/diff/120001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:332: for (size_t i = 0; i < Size; ++i) {
On 2017/02/09 22:39:07, Tom Sepez wrote:
> On 2017/02/09 22:02:08, ericrk wrote:
> > Didn't move to range-based-for here, as it makes it somewhat more verbose to
> do
> > the "append a space on all but the first run" logic below.
> This is fine, but for future reference, I believe the idiom is something like
> 
> for (auto& element : container) {
>   if (&element != &container.front())
>     output(" ");
>   ...
> }
> 
> as you like 

makes sense, done.

[ericrk, 2017-02-10 00:15:36]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2017-02-10 00:15:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2686503004/#ps140001
(title: "nit")

[commit-bot: I haz the power, 2017-02-10 00:16:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 01:06:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 01:06:33]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ericrk, 2017-02-10 01:31:13]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2017-02-10 01:32:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 08:26:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 08:26:19]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ericrk, 2017-02-10 16:44:46]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2017-02-10 16:45:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 17:49:40]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486745086516750,
"parent_rev": "202a5a81ed0c0929c55d28e44b822f5bbc194ade", "commit_rev":
"b5de4603585dba1e41900cbc5dc3fd4a65d200ff"}

[commit-bot: I haz the power, 2017-02-10 17:50:17]

Description was changed from

==========
Add IPC ParamTraits for c-style arrays

Currently, we don't have generic ParamTraits for c-style arrays (as we
do for vectors). This leads to a bit of copy-pasted code for each type
which sends a c-style array over IPC.

This change adds ParamTraits, allowing c-style array members to be
easily added via the IPC_STRUCT_TRAITS_MEMBER macro.

R=dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Add IPC ParamTraits for c-style arrays

Currently, we don't have generic ParamTraits for c-style arrays (as we
do for vectors). This leads to a bit of copy-pasted code for each type
which sends a c-style array over IPC.

This change adds ParamTraits, allowing c-style array members to be
easily added via the IPC_STRUCT_TRAITS_MEMBER macro.

R=dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2686503004
Cr-Commit-Position: refs/heads/master@{#449650}
Committed:
https://chromium.googlesource.com/chromium/src/+/b5de4603585dba1e41900cbc5dc3...
==========

[commit-bot: I haz the power, 2017-02-10 17:50:18]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/b5de4603585dba1e41900cbc5dc3...