Remove DisplayStructTraitsTest interface.

Remove DisplayStructTraitsTest interface.

The best practice is apparently not to do this and to use
Serialize()/Deserialize() functions directly. This also removes an
EnumTraits test that isn't super useful.

BUG=none

Review-Url: https://codereview.chromium.org/2857033002
Cr-Commit-Position: refs/heads/master@{#469402}
Committed: https://chromium.googlesource.com/chromium/src/+/5a762e451441fb1482e4b5adc274e03d4c9e6ba8

[kylechar, 2017-05-02 22:09:57]

Description was changed from

==========
Remove DisplayStructTraitsTest interface.

The best practice is apparently not to do this and to use
Serialize()/Deserialize() functions directly. This also removes an
EnumType test that isn't super useful.

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

to

==========
Remove DisplayStructTraitsTest interface.

The best practice is apparently not to do this and to use
Serialize()/Deserialize() functions directly. This also removes an
EnumType test that isn't super useful.

BUG=none
==========

[kylechar, 2017-05-03 13:53:11]

Patchset #2 (id:20001) has been deleted

[kylechar, 2017-05-03 13:57:52]

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

[kylechar, 2017-05-03 13:57:52]

I saw a thread on chromium-mojo@ that suggested a test interface with sync
methods for testing StructTraits is an anti-pattern and figured I'd fix the
couple I was responsible for. Please take a look at the StructTrait testing
parts before I send off to OWNERS? Thanks!

[yzshen1, 2017-05-03 16:39:46]

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right):

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
ui/display/mojo/display_snapshot_mojo_struct_traits.h:8: #include
"ipc/ipc_message_utils.h"
I thought keeping these includes in the .cc file should work. Have I missed
anything?

[kylechar, 2017-05-03 17:18:55]

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right):

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
ui/display/mojo/display_snapshot_mojo_struct_traits.h:8: #include
"ipc/ipc_message_utils.h"
On 2017/05/03 16:39:46, yzshen1 wrote:
> I thought keeping these includes in the .cc file should work. Have I missed
> anything?

There are errors in the display_unittests target looking for templates without
these changes.

If I undo the changes to display_snapshot_mojo_struct_straits.h/cc and
display_struct_traits.h/cc then I get the following compile errors:

../../mojo/public/cpp/bindings/lib/template_util.h:74:30: error: implicit
instantiation of undefined template
'mojo::StructTraits<gfx::mojom::RectDataView, gfx::Rect>'

../../mojo/public/cpp/bindings/lib/native_struct_serialization.h:36:13: error:
no member named 'GetSize' in 'IPC::ParamTraits<base::FilePath>'

gen/ui/display/mojo/display_constants.mojom-shared.h:142:36: error: implicit
instantiation of undefined template
'mojo::EnumTraits<display::mojom::DisplayConnectionType,
display::DisplayConnectionType>'

Plus lots of other related errors.

[yzshen1, 2017-05-03 17:22:03]

lgtm

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right):

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
ui/display/mojo/display_snapshot_mojo_struct_traits.h:8: #include
"ipc/ipc_message_utils.h"
On 2017/05/03 17:18:55, kylechar wrote:
> On 2017/05/03 16:39:46, yzshen1 wrote:
> > I thought keeping these includes in the .cc file should work. Have I missed
> > anything?
> 
> There are errors in the display_unittests target looking for templates without
> these changes.
> 
> If I undo the changes to display_snapshot_mojo_struct_straits.h/cc and
> display_struct_traits.h/cc then I get the following compile errors:
> 
> ../../mojo/public/cpp/bindings/lib/template_util.h:74:30: error: implicit
> instantiation of undefined template
> 'mojo::StructTraits<gfx::mojom::RectDataView, gfx::Rect>'
> 
> ../../mojo/public/cpp/bindings/lib/native_struct_serialization.h:36:13: error:
> no member named 'GetSize' in 'IPC::ParamTraits<base::FilePath>'
> 
> gen/ui/display/mojo/display_constants.mojom-shared.h:142:36: error: implicit
> instantiation of undefined template
> 'mojo::EnumTraits<display::mojom::DisplayConnectionType,
> display::DisplayConnectionType>'
> 
> Plus lots of other related errors.

Ah, right. Because the serialization is done outside of the .cc file
(deserialization should be fine because it uses the StructTraits::Read which is
implemented in the cc file). Thanks for explanation!

[kylechar, 2017-05-03 17:31:19]

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

[kylechar, 2017-05-03 17:31:37]

Description was changed from

==========
Remove DisplayStructTraitsTest interface.

The best practice is apparently not to do this and to use
Serialize()/Deserialize() functions directly. This also removes an
EnumType test that isn't super useful.

BUG=none
==========

to

==========
Remove DisplayStructTraitsTest interface.

The best practice is apparently not to do this and to use
Serialize()/Deserialize() functions directly. This also removes an
EnumTraits test that isn't super useful.

BUG=none
==========

[commit-bot: I haz the power, 2017-05-03 17:32:34]

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-05-03 18:52:39]

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

[commit-bot: I haz the power, 2017-05-03 18:52:40]

Dry run: This issue passed the CQ dry run.

[kylechar, 2017-05-04 12:45:43]

kylechar@chromium.org changed reviewers:
+ derat@chromium.org, tsepez@chromium.org

[kylechar, 2017-05-04 12:45:44]

Thanks yzshen!

derat: ui/display/*
tsepez: mojom/struct_traits.

[Daniel Erat, 2017-05-04 14:56:13]

lgtm with a comment

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
File ui/display/mojo/display_struct_traits_unittest.cc (right):

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
ui/display/mojo/display_struct_traits_unittest.cc:96: void
TestStructTraits(const Type& input, Type* output) {
this doesn't actually test anything. would SerializeAndDeserialize be more
accurate?

[kylechar, 2017-05-04 16:20:05]

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
File ui/display/mojo/display_struct_traits_unittest.cc (right):

https://codereview.chromium.org/2857033002/diff/40001/ui/display/mojo/display...
ui/display/mojo/display_struct_traits_unittest.cc:96: void
TestStructTraits(const Type& input, Type* output) {
On 2017/05/04 14:56:13, Daniel Erat wrote:
> this doesn't actually test anything. would SerializeAndDeserialize be more
> accurate?

Done.

[Tom Sepez, 2017-05-04 17:03:15]

lgtm

[kylechar, 2017-05-04 17:03:42]

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

[commit-bot: I haz the power, 2017-05-04 17:04:17]

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-05-04 18:39:17]

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

[commit-bot: I haz the power, 2017-05-04 18:39:18]

Dry run: This issue passed the CQ dry run.

[kylechar, 2017-05-04 18:41:35]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-05-04 18:41:36]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2857033002/#ps60001
(title: "SerializeAndDeserialize.")

[commit-bot: I haz the power, 2017-05-04 18:42:13]

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-05-04 18:51:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493923295791270,
"parent_rev": "30d7209be42dbfacdbe507d70474506ba9720f6a", "commit_rev":
"5a762e451441fb1482e4b5adc274e03d4c9e6ba8"}

[commit-bot: I haz the power, 2017-05-04 18:51:59]

Description was changed from

==========
Remove DisplayStructTraitsTest interface.

The best practice is apparently not to do this and to use
Serialize()/Deserialize() functions directly. This also removes an
EnumTraits test that isn't super useful.

BUG=none
==========

to

==========
Remove DisplayStructTraitsTest interface.

The best practice is apparently not to do this and to use
Serialize()/Deserialize() functions directly. This also removes an
EnumTraits test that isn't super useful.

BUG=none

Review-Url: https://codereview.chromium.org/2857033002
Cr-Commit-Position: refs/heads/master@{#469402}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a762e451441fb1482e4b5adc274...
==========

[commit-bot: I haz the power, 2017-05-04 18:52:00]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5a762e451441fb1482e4b5adc274...