Add struct_traits and typemap for blink::WebGamepad

Add struct_traits and typemap for blink::WebGamepad

This is the prepare patch for Mojo-ification of Gamepad.
The original GamepadMsg_GamepadConnected/Disconnected messages use the
blink::WebGamepad as parameter, so before we convert above messages to mojo
interface, we have to add the typemap and struct_traits for struct
blink::WebGamepad first.

Unittest of this new-added typemap and struct_traits are added into gn target
device_unittests.

BUG=612330
Committed: https://crrev.com/303e0f93e55f92b45c5fcea420379c0802583f36
Cr-Commit-Position: refs/heads/master@{#433789}

[ke.he, 2016-11-11 07:26:07]

ke.he@intel.com changed reviewers:
+ blundell@chromium.org, leon.han@intel.com

[ke.he, 2016-11-11 07:26:08]

blundell@, PTAL, thanks! 
Based on this preparation patch, I'll continue the mojo-ification work of
gamepad module.

[leonhsl(Using Gerrit), 2016-11-11 07:39:33]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 07:39:43]

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, 2016-11-11 08:19:07]

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

[commit-bot: I haz the power, 2016-11-11 08:19:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ke.he, 2016-11-11 08:42:30]

Patchset #2 (id:20001) has been deleted

[leonhsl(Using Gerrit), 2016-11-11 08:58:51]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 08:59:02]

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

[ke.he, 2016-11-11 09:10:34]

Patchset #2 (id:40001) has been deleted

[leonhsl(Using Gerrit), 2016-11-11 09:12:28]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 09:12:39]

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, 2016-11-11 09:56:56]

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

[commit-bot: I haz the power, 2016-11-11 09:57:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-11-11 12:33:02]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 12:33:11]

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, 2016-11-11 13:45:55]

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

[commit-bot: I haz the power, 2016-11-11 13:45:56]

Dry run: 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_...)

[ke.he, 2016-11-14 03:55:26]

ke.he@intel.com changed reviewers:
+ yzshen@chromium.org

[ke.he, 2016-11-14 03:55:26]

Currently this patch only breaks the win_chromium_x64_rel_ng build, and I'm
blocked. I think it is quite a mojo-specific problem so I also invite yzshen@ to
have a look.

the log shows:
------
e:\b\c\b\win\src\device\gamepad\public\interfaces\webgamepad_struct_traits.cc(144):
error C2220: warning treated as error - no 'object' file generated
e:\b\c\b\win\src\device\gamepad\public\interfaces\webgamepad_struct_traits.cc(144):
warning C4366: The result of the unary '&' operator may be unaligned
------

The root cause is: blink::WebGamepad is 1 byte aligned by "#pragma pack(push,
1)", and mojo auto-generates function like: "bool ReadHand(UserType* output)". 
Then in win64bit it complains warning(and chromium treat it as error)

From link ( https://msdn.microsoft.com/en-us/library/ms173684.aspx ), it says
"To resolve C4366, either change the alignment of the structure or declare the
pointer with the __unaligned keyword."

So my question is how to tell mojo to add the "__unaligned" keyword?(the
cleanest way to solve this problem). Or, is it ok to remove the "#pragma
pack(push, 1)" out of the blink::webgamepad?(seems not the best choice).

Please help to give some comments on this, thanks very much.

[ke.he, 2016-11-14 06:51:56]

Patchset #4 (id:90001) has been deleted

[leonhsl(Using Gerrit), 2016-11-14 08:39:30]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 08:39:49]

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

[ke.he, 2016-11-14 08:54:47]

Problem solved. Big thanks to leonhsl's suggestion.
Apologize for the long and useless mail I sent :(

blundell@, CL updated, PTAL.
Thanks all!

[commit-bot: I haz the power, 2016-11-14 09:30:20]

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

[commit-bot: I haz the power, 2016-11-14 09:30:21]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-11-14 12:33:16]

Yuzhu, can you do the initial review here? I'm still not that familiar with how
all of the typemap stuff works (either correctness or conventions of naming,
locations in the codebase, etc). Thanks!

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad.mojom (right):

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:3: struct WebGamepadQuaternion{
nit: "Web" is a Blink-ism that shouldn't be part of the Mojo descriptions of any
of these.

[blundell, 2016-11-14 12:34:19]

On 2016/11/14 12:33:16, blundell wrote:
> Yuzhu, can you do the initial review here? I'm still not that familiar with
how
> all of the typemap stuff works (either correctness or conventions of naming,
> locations in the codebase, etc). Thanks!
> 
>
https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
> File device/gamepad/public/interfaces/gamepad.mojom (right):
> 
>
https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
> device/gamepad/public/interfaces/gamepad.mojom:3: struct WebGamepadQuaternion{
> nit: "Web" is a Blink-ism that shouldn't be part of the Mojo descriptions of
any
> of these.

Alo, Ke He, can you explain what the solution to the problem that you described
earlier was?

[yzshen1, 2016-11-14 21:13:17]

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad.mojom (right):

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:1: module device.mojom;
Please add the copyright boilerplate.

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:4: bool notNull;
please use naming format like this: not_null. (Here and below.)

There are other coding style issues. For example,
- there should be a space between WebGamepadQuaternion and "{".
- two spaces between code and same-line comment.

It is probably useful to review the coding style doc:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:4: bool notNull;
Could we use the nullable feature instead of an explicit notNull field?

For example, 

struct WebGamepadPose {
  ...
  WebGamepadQuaternion? orientation;
};

If you want, you could still make nullable/non-nullable WebGamepadQuaternion to
the same custom type.

Please see:
nullable: https://www.chromium.org/developers/design-documents/mojo/mojom-syntax
nullable_is_same_type:
https://www.chromium.org/developers/design-documents/mojo/type-mapping

[ke.he, 2016-11-15 10:55:18]

blundell@, yzshen1@  CL updated, PTAL, thanks!

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad.mojom (right):

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:1: module device.mojom;
On 2016/11/14 21:13:17, yzshen1 wrote:
> Please add the copyright boilerplate.

Done.

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:3: struct WebGamepadQuaternion{
On 2016/11/14 12:33:16, blundell wrote:
> nit: "Web" is a Blink-ism that shouldn't be part of the Mojo descriptions of
any
> of these.

I changed all the "WebGamepadxxx" into "Gamepadxxx", only keep some functions
names in unittests which are really closed to blink::WebGamepad.
Done.

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:4: bool notNull;
On 2016/11/14 21:13:17, yzshen1 wrote:
> please use naming format like this: not_null. (Here and below.)
> 
> There are other coding style issues. For example,
> - there should be a space between WebGamepadQuaternion and "{".
> - two spaces between code and same-line comment.
> 
> It is probably useful to review the coding style doc:
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

Oh, it is not correct to just copy those webxxx structs which
are in blink style directly into mojom file. In mojom it should follow the
chromium style. Thanks for reminding me this.
So besides "notNull" to "not_null", other members like "hasPosition" should be
changed as "has_position". I'll change all of them.
Thanks!
Done.

https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:4: bool notNull;
On 2016/11/14 21:13:17, yzshen1 wrote:
> Could we use the nullable feature instead of an explicit notNull field?
> 
> For example, 
> 
> struct WebGamepadPose {
>   ...
>   WebGamepadQuaternion? orientation;
> };
> 
> If you want, you could still make nullable/non-nullable WebGamepadQuaternion
to
> the same custom type.
> 
> Please see:
> nullable:
https://www.chromium.org/developers/design-documents/mojo/mojom-syntax
> nullable_is_same_type:
> https://www.chromium.org/developers/design-documents/mojo/type-mapping
> 
> 

Done.

[ke.he, 2016-11-15 11:09:32]

On 2016/11/14 12:34:19, blundell wrote:
> On 2016/11/14 12:33:16, blundell wrote:
> > Yuzhu, can you do the initial review here? I'm still not that familiar with
> how
> > all of the typemap stuff works (either correctness or conventions of naming,
> > locations in the codebase, etc). Thanks!
> > 
> >
>
https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
> > File device/gamepad/public/interfaces/gamepad.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2492183002/diff/130001/device/gamepad/public/...
> > device/gamepad/public/interfaces/gamepad.mojom:3: struct
WebGamepadQuaternion{
> > nit: "Web" is a Blink-ism that shouldn't be part of the Mojo descriptions of
> any
> > of these.
> 
> Alo, Ke He, can you explain what the solution to the problem that you
described
> earlier was?

The problem I met earlier was: In windows 64bit build, it complains warning if I
try to call: data.ReadHand(&out->hand).
The mojo generates the function defination as: "bool ReadHand(UserType* output)"
Because the struct "out" here is 1 bytes aligned, so compiler complains C4366
warning.

Solution is simple, we avoid pass the struct member pointer to function
ReadHead(), we get the content into a local variable first, then set the value
into the struct:
    blink::WebGamepadHand hand;
    if (!data.ReadHand(&hand))
       return false;
    out->hand = hand;

Then everything is OK.

[leonhsl(Using Gerrit), 2016-11-15 14:14:16]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 14:14:26]

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, 2016-11-15 14:55:23]

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

[commit-bot: I haz the power, 2016-11-15 14:55:24]

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

[yzshen1, 2016-11-15 19:17:33]

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad.mojom (right):

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:45: uint32 axes_length;
If axes length is variable, how about making |axes| *not* fixed-size and
eliminate |axes_length|? (and also |buttons_length|) It is a waste to pass
unnecessary bytes.

We could verify that they don't exceed the max length in the struct traits.

Question: are |id| and |mapping| of variable size too? I noticed the word "Cap"
in the comment.

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits.cc (right):

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:13: memset(out, 0,
sizeof(blink::WebGamepadPose));
Should it be sizeof(blink::WebGamepadQuaternion)?

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:143: if
(out->axesLength > blink::WebGamepad::axesLengthCap) {
It is okay to have {} for one-line body, or omit it.
But please be consistent. (For example, line 139 doesn't have it, but line 144
does.)

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits.h (right):

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.h:99:
reinterpret_cast<uint16_t*>(const_cast<blink::WebUChar*>(&r.id[0]))};
You could return a ConstCArray, that way you don't need to do const cast. (Here
and below)

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits_unittest.cc (right):

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits_unittest.cc:89: if
(lhs.pressed == rhs.pressed && lhs.touched == rhs.touched &&
Please directly return what you currently use as the if-condition.

[ke.he, 2016-11-16 09:51:41]

yzshen1@, CL updated, PTAL.
Thanks!

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad.mojom (right):

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad.mojom:45: uint32 axes_length;
On 2016/11/15 19:17:33, yzshen1 wrote:
> If axes length is variable, how about making |axes| *not* fixed-size and
> eliminate |axes_length|? (and also |buttons_length|) It is a waste to pass
> unnecessary bytes.
> 
> We could verify that they don't exceed the max length in the struct traits.
> 
> Question: are |id| and |mapping| of variable size too? I noticed the word
"Cap"
> in the comment.

Yes, to avoid passing unnecessary bytes, |axes_length| and |buttons_length|
should be eliminated.
As for |id| and |mapping|, they are "fixed array"s which are used to place
string(WebUChars), the "Cap" are their capacity. so we can pass only the
no-empty part of the fixed-array to avoid wastes.

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits.cc (right):

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:13: memset(out, 0,
sizeof(blink::WebGamepadPose));
On 2016/11/15 19:17:33, yzshen1 wrote:
> Should it be sizeof(blink::WebGamepadQuaternion)?

Done.

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:143: if
(out->axesLength > blink::WebGamepad::axesLengthCap) {
On 2016/11/15 19:17:33, yzshen1 wrote:
> It is okay to have {} for one-line body, or omit it.
> But please be consistent. (For example, line 139 doesn't have it, but line 144
> does.)

Done.

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits.h (right):

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.h:99:
reinterpret_cast<uint16_t*>(const_cast<blink::WebUChar*>(&r.id[0]))};
On 2016/11/15 19:17:33, yzshen1 wrote:
> You could return a ConstCArray, that way you don't need to do const cast.
(Here
> and below)

Done.

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits_unittest.cc (right):

https://codereview.chromium.org/2492183002/diff/150001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits_unittest.cc:89: if
(lhs.pressed == rhs.pressed && lhs.touched == rhs.touched &&
On 2016/11/15 19:17:33, yzshen1 wrote:
> Please directly return what you currently use as the if-condition.

Done.

[leonhsl(Using Gerrit), 2016-11-16 09:53:58]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 09:54:13]

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, 2016-11-16 11:32:10]

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

[commit-bot: I haz the power, 2016-11-16 11:32:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yzshen1, 2016-11-16 19:20:18]

Mostly looks good. Thanks!

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits.cc (right):

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:22: memset(out, 0,
sizeof(blink::WebGamepadQuaternion));
why is this needed?

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:44: memset(out, 0,
sizeof(blink::WebGamepadVector));
ditto

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:57: memset(out, 0,
sizeof(blink::WebGamepadButton));
ditto

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:75: memset(out, 0,
sizeof(blink::WebGamepadPose));
ditto

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:144: while
(r.id[id_length] != 0 && id_length < blink::WebGamepad::idLengthCap) {
You need to swap the position of the two conditions to avoid out-of-bound
access.

Besides, I don't know much about Gamepad, I don't know whether the first 0 marks
the end of id. So this needs to be reviewed by some expert.

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:155: while
(r.mapping[mapping_length] != 0 &&
ditto

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:166: memset(out, 0,
sizeof(blink::WebGamepad));
Does it make sense to only memset those necessary fields, i.e., id/mapping?

Usually I would avoid memseting an object's contents directly.

[leonhsl(Using Gerrit), 2016-11-17 02:46:24]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 02:47:52]

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

[leonhsl(Using Gerrit), 2016-11-17 04:48:50]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 04:49:37]

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

[ke.he, 2016-11-17 05:51:49]

yzshen1@, Thanks very much for your comments. PTAL.

blundell@, Could you PTAL at
https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
?

Big thanks to both of you:)

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits.cc (right):

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:22: memset(out, 0,
sizeof(blink::WebGamepadQuaternion));
On 2016/11/16 19:20:18, yzshen1 wrote:
> why is this needed?

Done.

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:44: memset(out, 0,
sizeof(blink::WebGamepadVector));
On 2016/11/16 19:20:18, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:57: memset(out, 0,
sizeof(blink::WebGamepadButton));
On 2016/11/16 19:20:18, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:75: memset(out, 0,
sizeof(blink::WebGamepadPose));
On 2016/11/16 19:20:18, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:144: while
(r.id[id_length] != 0 && id_length < blink::WebGamepad::idLengthCap) {
On 2016/11/16 19:20:18, yzshen1 wrote:
> You need to swap the position of the two conditions to avoid out-of-bound
> access.
> 
> Besides, I don't know much about Gamepad, I don't know whether the first 0
marks
> the end of id. So this needs to be reviewed by some expert.

blundell@, I think it is correct way to check the end of id[] and mapping[] by
"r.id[id_length] != 0",
Could you PTAL at this? Thanks.

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:155: while
(r.mapping[mapping_length] != 0 &&
On 2016/11/16 19:20:18, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:166: memset(out, 0,
sizeof(blink::WebGamepad));
On 2016/11/16 19:20:18, yzshen1 wrote:
> Does it make sense to only memset those necessary fields, i.e., id/mapping?
> 
> Usually I would avoid memseting an object's contents directly.

Yes, it make sense to only memset necessary fields.
Mojo didn't do memset before it passes "out" into Read(). the out are filled
with random data.
But I noticed all the members will be assigned so it is OK to skip the memset
whole object, and only memset necessary fields.
It is more efficient in this way.

Done.

[commit-bot: I haz the power, 2016-11-17 05:59:08]

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

[commit-bot: I haz the power, 2016-11-17 05:59:09]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-11-17 14:55:41]

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
File device/gamepad/public/interfaces/gamepad_struct_traits.cc (right):

https://codereview.chromium.org/2492183002/diff/170001/device/gamepad/public/...
device/gamepad/public/interfaces/gamepad_struct_traits.cc:144: while
(r.id[id_length] != 0 && id_length < blink::WebGamepad::idLengthCap) {
On 2016/11/17 05:51:48, ke.he wrote:
> On 2016/11/16 19:20:18, yzshen1 wrote:
> > You need to swap the position of the two conditions to avoid out-of-bound
> > access.
> > 
> > Besides, I don't know much about Gamepad, I don't know whether the first 0
> marks
> > the end of id. So this needs to be reviewed by some expert.
> 
> blundell@, I think it is correct way to check the end of id[] and mapping[] by
> "r.id[id_length] != 0",
> Could you PTAL at this? Thanks.

I'm not a domain expert on Gamepad either, but this (and the below) look correct
to me based on
https://cs.chromium.org/chromium/src/device/gamepad/gamepad_platform_data_fet....

bajones@ or scottmg@ should be able to definitively confirm.

[yzshen1, 2016-11-17 17:37:07]

LGTM (please also wait for other reviewer's LGs.)

Thanks!

[ke.he, 2016-11-18 01:51:39]

ke.he@intel.com changed reviewers:
+ bajones@chromium.org

[ke.he, 2016-11-18 01:51:40]

bajones@, Could you PTAL on this patch, thanks!

[bajones, 2016-11-18 18:38:23]

On 2016/11/18 01:51:40, ke.he wrote:
> bajones@, Could you PTAL on this patch, thanks!

device/gamepad/ LGTM

[leonhsl(Using Gerrit), 2016-11-19 11:28:28]

leon.han@intel.com changed reviewers:
+ tsepez@chromium.org

[leonhsl(Using Gerrit), 2016-11-19 11:28:29]

Hi, Tom, would you please help to take security review? Thanks.

[Tom Sepez, 2016-11-21 17:55:50]

lgtm

[ke.he, 2016-11-22 01:38:03]

The CQ bit was checked by ke.he@intel.com

[commit-bot: I haz the power, 2016-11-22 01:38:37]

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, 2016-11-22 02:53:22]

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

[commit-bot: I haz the power, 2016-11-22 02:53:24]

Failed to apply patch for device/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: device/BUILD.gn:61
  error: device/BUILD.gn: patch does not apply

Patch:       device/BUILD.gn
Index: device/BUILD.gn
diff --git a/device/BUILD.gn b/device/BUILD.gn
index
7b9c0042eae8952c8044bcc47c8220a531c93442..48390e57ffabd318c45d0de4f613bae07e543e10
100644
--- a/device/BUILD.gn
+++ b/device/BUILD.gn
@@ -61,6 +61,7 @@ test("device_unittests") {
     "bluetooth/test/test_bluetooth_local_gatt_service_delegate.cc",
     "bluetooth/test/test_bluetooth_local_gatt_service_delegate.h",
     "gamepad/gamepad_provider_unittest.cc",
+    "gamepad/public/interfaces/gamepad_struct_traits_unittest.cc",
     "generic_sensor/platform_sensor_and_provider_unittest_win.cc",
     "generic_sensor/platform_sensor_provider_unittest.cc",
     "test/run_all_unittests.cc",
@@ -77,6 +78,8 @@ test("device_unittests") {
     "//device/bluetooth:mocks",
     "//device/gamepad",
     "//device/gamepad:test_helpers",
+    "//device/gamepad/public/interfaces",
+    "//device/gamepad/public/interfaces:gamepad_struct_traits_test",
     "//device/generic_sensor",
     "//device/generic_sensor:testing",
     "//device/geolocation:unittests",
@@ -88,6 +91,7 @@ test("device_unittests") {
     "//net",
     "//testing/gmock",
     "//testing/gtest",
+    "//third_party/WebKit/public:blink_headers",
     "//tools/usb_gadget",
     "//url",
   ]

[ke.he, 2016-11-22 03:04:31]

The CQ bit was checked by ke.he@intel.com

[commit-bot: I haz the power, 2016-11-22 03:05:03]

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, 2016-11-22 03:09:21]

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

[commit-bot: I haz the power, 2016-11-22 03:09:22]

Failed to apply patch for device/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: device/BUILD.gn:61
  error: device/BUILD.gn: patch does not apply

Patch:       device/BUILD.gn
Index: device/BUILD.gn
diff --git a/device/BUILD.gn b/device/BUILD.gn
index
7b9c0042eae8952c8044bcc47c8220a531c93442..48390e57ffabd318c45d0de4f613bae07e543e10
100644
--- a/device/BUILD.gn
+++ b/device/BUILD.gn
@@ -61,6 +61,7 @@ test("device_unittests") {
     "bluetooth/test/test_bluetooth_local_gatt_service_delegate.cc",
     "bluetooth/test/test_bluetooth_local_gatt_service_delegate.h",
     "gamepad/gamepad_provider_unittest.cc",
+    "gamepad/public/interfaces/gamepad_struct_traits_unittest.cc",
     "generic_sensor/platform_sensor_and_provider_unittest_win.cc",
     "generic_sensor/platform_sensor_provider_unittest.cc",
     "test/run_all_unittests.cc",
@@ -77,6 +78,8 @@ test("device_unittests") {
     "//device/bluetooth:mocks",
     "//device/gamepad",
     "//device/gamepad:test_helpers",
+    "//device/gamepad/public/interfaces",
+    "//device/gamepad/public/interfaces:gamepad_struct_traits_test",
     "//device/generic_sensor",
     "//device/generic_sensor:testing",
     "//device/geolocation:unittests",
@@ -88,6 +91,7 @@ test("device_unittests") {
     "//net",
     "//testing/gmock",
     "//testing/gtest",
+    "//third_party/WebKit/public:blink_headers",
     "//tools/usb_gadget",
     "//url",
   ]

[ke.he, 2016-11-22 03:33:35]

The CQ bit was checked by ke.he@intel.com

[ke.he, 2016-11-22 03:33:35]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
yzshen@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2492183002/#ps230001
(title: "code rebase")

[commit-bot: I haz the power, 2016-11-22 03:33:55]

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, 2016-11-22 04:38:16]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1479785615412870,
"parent_rev": "f74d1d95828a1e0a7a2d7c8d46c22c7581c33c82", "commit_rev":
"ee18ab2bbd163eb82db8ef2e8fb638a1d0b4ad1b"}

[commit-bot: I haz the power, 2016-11-22 04:38:43]

Committed patchset #10 (id:230001)

[commit-bot: I haz the power, 2016-11-22 04:41:16]

Description was changed from

==========
Add struct_traits and typemap for blink::WebGamepad

This is the prepare patch for Mojo-ification of Gamepad.
The original GamepadMsg_GamepadConnected/Disconnected messages use the
blink::WebGamepad as parameter, so before we convert above messages to mojo
interface, we have to add the typemap and struct_traits for struct
blink::WebGamepad first.

Unittest of this new-added typemap and struct_traits are added into gn target
device_unittests.

BUG=612330
==========

to

==========
Add struct_traits and typemap for blink::WebGamepad

This is the prepare patch for Mojo-ification of Gamepad.
The original GamepadMsg_GamepadConnected/Disconnected messages use the
blink::WebGamepad as parameter, so before we convert above messages to mojo
interface, we have to add the typemap and struct_traits for struct
blink::WebGamepad first.

Unittest of this new-added typemap and struct_traits are added into gn target
device_unittests.

BUG=612330

Committed: https://crrev.com/303e0f93e55f92b45c5fcea420379c0802583f36
Cr-Commit-Position: refs/heads/master@{#433789}
==========

[commit-bot: I haz the power, 2016-11-22 04:41:17]

Patchset 10 (id:??) landed as
https://crrev.com/303e0f93e55f92b45c5fcea420379c0802583f36
Cr-Commit-Position: refs/heads/master@{#433789}