Implement StructTraits for various cc and gpu types

Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken
4. cc::ReturnedResource

This CL also implements unit tests in both gpu/ipc/common
and cc/ipc that test these traits.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/3de222455772a473df1585b47e3395a0c174fc38
Cr-Commit-Position: refs/heads/master@{#397067}

[Fady Samuel, 2016-05-27 22:33:33]

Description was changed from

==========
Implement StructTraits for gpu::Mailbox, gpu::SyncToken, and
cc::ReturnedResource

BUG=
==========

to

==========
Implement StructTraits for gpu::Mailbox, gpu::SyncToken, and
cc::ReturnedResource

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Fady Samuel, 2016-05-27 22:33:33]

Description was changed from

==========
Implement StructTraits for gpu::Mailbox, gpu::SyncToken, and
cc::ReturnedResource

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Implement StructTraits for gpu::Mailbox, gpu::SyncToken, and
cc::ReturnedResource

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-05-27 22:39:22]

Description was changed from

==========
Implement StructTraits for gpu::Mailbox, gpu::SyncToken, and
cc::ReturnedResource

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement StructTraits for gpu::Mailbox, gpu::SyncToken, and
cc::ReturnedResource

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-05-27 22:40:03]

fsamuel@chromium.org changed reviewers:
+ ben@chromium.org, piman@chromium.org, tsepez@chromium.org

[Fady Samuel, 2016-05-27 22:40:03]

+tsepez@ for ipc.
+piman@ for gpu and cc.
+ben@ for mus.

[Fady Samuel, 2016-05-27 22:43:18]

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

[commit-bot: I haz the power, 2016-05-27 22:43:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019833002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019833002/40001

[commit-bot: I haz the power, 2016-05-27 22:45:57]

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

[commit-bot: I haz the power, 2016-05-27 22:45:58]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Fady Samuel, 2016-05-29 19:48:08]

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

[commit-bot: I haz the power, 2016-05-29 19:48:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019833002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019833002/60001

[Fady Samuel, 2016-05-29 20:38:02]

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

[commit-bot: I haz the power, 2016-05-29 20:38:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019833002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019833002/80001

[commit-bot: I haz the power, 2016-05-29 21:27:51]

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

[commit-bot: I haz the power, 2016-05-29 21:27:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2016-05-31 00:36:41]

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

[commit-bot: I haz the power, 2016-05-31 00:36:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019833002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019833002/180001

[commit-bot: I haz the power, 2016-05-31 00:43:27]

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

[commit-bot: I haz the power, 2016-05-31 00:43:27]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Fady Samuel, 2016-05-31 01:03:05]

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

[commit-bot: I haz the power, 2016-05-31 01:03:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019833002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019833002/200001

[commit-bot: I haz the power, 2016-05-31 01:10:52]

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

[commit-bot: I haz the power, 2016-05-31 01:10:54]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Fady Samuel, 2016-05-31 01:41:38]

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

[commit-bot: I haz the power, 2016-05-31 01:41:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019833002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019833002/220001

[commit-bot: I haz the power, 2016-05-31 01:49:27]

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

[commit-bot: I haz the power, 2016-05-31 01:49:28]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Fady Samuel, 2016-05-31 03:05:34]

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

[commit-bot: I haz the power, 2016-05-31 03:05:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019833002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019833002/240001

[commit-bot: I haz the power, 2016-05-31 04:18:43]

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

[commit-bot: I haz the power, 2016-05-31 04:18:45]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-05-31 15:33:23]

Description was changed from

==========
Implement StructTraits for gpu::Mailbox, gpu::SyncToken, and
cc::ReturnedResource

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-05-31 15:33:56]

Description was changed from

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken
4. cc::ReturnedResource

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-05-31 15:34:57]

Description was changed from

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken
4. cc::ReturnedResource

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken
4. cc::ReturnedResource

This CL also implements unit tests in both gpu/ipc/common
and cc/ipc that test these traits.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-05-31 15:35:15]

PTAL

[Fady Samuel, 2016-05-31 15:35:15]

PTAL

[piman, 2016-05-31 16:08:50]

LGTM, I think. The duplication with IPC traits is unfortunate... I don't know if
there's a better solution.

https://codereview.chromium.org/2019833002/diff/240001/gpu/ipc/common/mailbox...
File gpu/ipc/common/mailbox.mojom (right):

https://codereview.chromium.org/2019833002/diff/240001/gpu/ipc/common/mailbox...
gpu/ipc/common/mailbox.mojom:9: array<int8, 64> name;
I'm not well versed in the mojo semantics - does this means that the reader will
always expect 64 int8s, and that it will report failure if there aren't exactly
64?

https://codereview.chromium.org/2019833002/diff/240001/gpu/ipc/common/mailbox...
File gpu/ipc/common/mailbox_struct_traits.cc (right):

https://codereview.chromium.org/2019833002/diff/240001/gpu/ipc/common/mailbox...
gpu/ipc/common/mailbox_struct_traits.cc:46: CHECK(GL_MAILBOX_SIZE_CHROMIUM ==
size);
nit: DCHECK

[Ben Goodger (Google), 2016-05-31 16:32:04]

lgtm

[Ken Rockot(use gerrit already), 2016-05-31 17:19:46]

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

[Ken Rockot(use gerrit already), 2016-05-31 17:19:48]

https://codereview.chromium.org/2019833002/diff/240001/gpu/ipc/common/mailbox...
File gpu/ipc/common/mailbox.mojom (right):

https://codereview.chromium.org/2019833002/diff/240001/gpu/ipc/common/mailbox...
gpu/ipc/common/mailbox.mojom:9: array<int8, 64> name;
On 2016/05/31 at 16:08:49, piman OOO back 2016-5-31 wrote:
> I'm not well versed in the mojo semantics - does this means that the reader
will always expect 64 int8s, and that it will report failure if there aren't
exactly 64?

Yes. Receiving anything but an array with exactly 64 elements is equivalent to
receiving a malformed message. As such the bindings layer will reject the
message (i.e. never dispatch) and the message pipe will be closed.

[Fady Samuel, 2016-05-31 17:23:58]

PTAL Tom! Thanks!

[Tom Sepez, 2016-05-31 17:30:24]

lgtm

https://codereview.chromium.org/2019833002/diff/260001/gpu/ipc/common/struct_...
File gpu/ipc/common/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2019833002/diff/260001/gpu/ipc/common/struct_...
gpu/ipc/common/struct_traits_unittest.cc:49: 0, 9, 8, 7, 6, 5, 4, 3, 2, 1, 9, 7,
5, 3, 1, 2, 4, 6, 8, 0, 0, 9,
nit: Any particular reason for this pattern?  If so, maybe comment.

https://codereview.chromium.org/2019833002/diff/260001/gpu/ipc/common/struct_...
gpu/ipc/common/struct_traits_unittest.cc:77: const uint64_t release_count =
0xdeadbeefdead;
nit: Probably should have an L suffix?

[Fady Samuel, 2016-05-31 17:59:59]

Will CQ once the previous patch lands

https://codereview.chromium.org/2019833002/diff/260001/gpu/ipc/common/struct_...
File gpu/ipc/common/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2019833002/diff/260001/gpu/ipc/common/struct_...
gpu/ipc/common/struct_traits_unittest.cc:49: 0, 9, 8, 7, 6, 5, 4, 3, 2, 1, 9, 7,
5, 3, 1, 2, 4, 6, 8, 0, 0, 9,
On 2016/05/31 17:30:24, Tom Sepez wrote:
> nit: Any particular reason for this pattern?  If so, maybe comment.

No reason.

https://codereview.chromium.org/2019833002/diff/260001/gpu/ipc/common/struct_...
gpu/ipc/common/struct_traits_unittest.cc:77: const uint64_t release_count =
0xdeadbeefdead;
On 2016/05/31 17:30:24, Tom Sepez wrote:
> nit: Probably should have an L suffix?

Done.

[Fady Samuel, 2016-05-31 21:55:25]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-05-31 21:55:26]

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

[commit-bot: I haz the power, 2016-05-31 21:56:20]

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

[commit-bot: I haz the power, 2016-05-31 22:14:54]

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

[commit-bot: I haz the power, 2016-05-31 22:14:55]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Fady Samuel, 2016-05-31 23:16:08]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-05-31 23:16:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
ben@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2019833002/#ps320001
(title: "Rebased")

[commit-bot: I haz the power, 2016-05-31 23:17:04]

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

[commit-bot: I haz the power, 2016-06-01 00:21:36]

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

[commit-bot: I haz the power, 2016-06-01 00:21:38]

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

[Fady Samuel, 2016-06-01 04:19:14]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-01 04:19:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
ben@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2019833002/#ps340001
(title: "mailbox_name by ref to make windows happy")

[commit-bot: I haz the power, 2016-06-01 04:19:44]

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

[commit-bot: I haz the power, 2016-06-01 05:37:14]

Description was changed from

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken
4. cc::ReturnedResource

This CL also implements unit tests in both gpu/ipc/common
and cc/ipc that test these traits.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken
4. cc::ReturnedResource

This CL also implements unit tests in both gpu/ipc/common
and cc/ipc that test these traits.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-01 05:37:16]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-06-01 05:39:00]

Description was changed from

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken
4. cc::ReturnedResource

This CL also implements unit tests in both gpu/ipc/common
and cc/ipc that test these traits.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement StructTraits for various cc and gpu types

This CL implements StructTraits for the following types:

1. gpu::Mailbox
2. gpu::MailboxHolder
3. gpu::SyncToken
4. cc::ReturnedResource

This CL also implements unit tests in both gpu/ipc/common
and cc/ipc that test these traits.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/3de222455772a473df1585b47e3395a0c174fc38
Cr-Commit-Position: refs/heads/master@{#397067}
==========

[commit-bot: I haz the power, 2016-06-01 05:39:02]

Patchset 18 (id:??) landed as
https://crrev.com/3de222455772a473df1585b47e3395a0c174fc38
Cr-Commit-Position: refs/heads/master@{#397067}

[yzshen1, 2016-06-01 15:41:29]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-06-01 15:41:30]

https://codereview.chromium.org/2019833002/diff/300001/gpu/ipc/common/mailbox...
File gpu/ipc/common/mailbox_struct_traits.cc (right):

https://codereview.chromium.org/2019833002/diff/300001/gpu/ipc/common/mailbox...
gpu/ipc/common/mailbox_struct_traits.cc:16: b->data = nullptr;
This deson't actually affect gpu::Mailbox's contents, right?

If you never use a nullable MailboxName, you could consider removing IsNull and
SetToNull altogether.

[Fady Samuel, 2016-06-01 19:42:52]

On 2016/06/01 15:41:30, yzshen1 wrote:
>
https://codereview.chromium.org/2019833002/diff/300001/gpu/ipc/common/mailbox...
> File gpu/ipc/common/mailbox_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2019833002/diff/300001/gpu/ipc/common/mailbox...
> gpu/ipc/common/mailbox_struct_traits.cc:16: b->data = nullptr;
> This deson't actually affect gpu::Mailbox's contents, right?
> 
> If you never use a nullable MailboxName, you could consider removing IsNull
and
> SetToNull altogether.

LANDED: https://codereview.chromium.org/2027223002/