Adds mojom structure for ui::Accelerator and struct traits

Adds mojom structure for ui::Accelerator and struct traits

BUG=701815
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org

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

[Tom Sepez, 2017-03-16 21:48:09]

lgtm

https://codereview.chromium.org/2749183005/diff/1/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/2749183005/diff/1/ui/base/BUILD.gn#newcode878
ui/base/BUILD.gn:878: deps += [ "//ui/base/accelerators/mojo:unittests" ]
nit: It doesn't sound like //ui/base/accelerators/mojo:unittests" provides
analogous functionality to //ui/resources:ui_test_pak_bundle_data, so maybe it
should have its own if-statement.

https://codereview.chromium.org/2749183005/diff/1/ui/base/accelerators/mojo/B...
File ui/base/accelerators/mojo/BUILD.gn (right):

https://codereview.chromium.org/2749183005/diff/1/ui/base/accelerators/mojo/B...
ui/base/accelerators/mojo/BUILD.gn:11: 
nit: stray blank line.

[sky, 2017-03-16 22:02:44]

https://codereview.chromium.org/2749183005/diff/1/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/2749183005/diff/1/ui/base/BUILD.gn#newcode878
ui/base/BUILD.gn:878: deps += [ "//ui/base/accelerators/mojo:unittests" ]
On 2017/03/16 21:48:09, Tom Sepez wrote:
> nit: It doesn't sound like //ui/base/accelerators/mojo:unittests" provides
> analogous functionality to //ui/resources:ui_test_pak_bundle_data, so maybe it
> should have its own if-statement.

I'm keeping this as I prefer less conditionals. And I'm going to resist trying
to cleanup this file here as there are too many repeated conditionals (line 881
and 901 are the same!)

https://codereview.chromium.org/2749183005/diff/1/ui/base/accelerators/mojo/B...
File ui/base/accelerators/mojo/BUILD.gn (right):

https://codereview.chromium.org/2749183005/diff/1/ui/base/accelerators/mojo/B...
ui/base/accelerators/mojo/BUILD.gn:11: 
On 2017/03/16 21:48:09, Tom Sepez wrote:
> nit: stray blank line.

Done.

[sky, 2017-03-16 22:02:44]

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

[commit-bot: I haz the power, 2017-03-16 22:03:12]

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

[msw, 2017-03-16 22:50:39]

lgtm with nits

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
File ui/base/accelerators/mojo/BUILD.gn (right):

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
ui/base/accelerators/mojo/BUILD.gn:32: deps = [
optional nit: you might be able to omit the public_deps of ":struct_traits"?

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
File ui/base/accelerators/mojo/accelerator.mojom (right):

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
ui/base/accelerators/mojo/accelerator.mojom:10: enum AcceleratorKeyState {
nit: comment, mention that this matches ui::Accelerator::KeyState

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
ui/base/accelerators/mojo/accelerator.mojom:15: // See ui::Accelerator for
details on these values.
optional nit: // This structure matches ui::Accelerator; see details there.
(James has actually been encouraging me to document mojoms equally or better
than the corresponding types in our c++ headers)

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
ui/base/accelerators/mojo/accelerator.mojom:17: // Accelerator::key_code_ is a
KeyboardCode, but it may also contain other
optional nit: It might help to elaborate on what other values it may contain.

[commit-bot: I haz the power, 2017-03-16 23:03:19]

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

[commit-bot: I haz the power, 2017-03-16 23:03:20]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-16 23:30:46]

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
File ui/base/accelerators/mojo/BUILD.gn (right):

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
ui/base/accelerators/mojo/BUILD.gn:32: deps = [
On 2017/03/16 22:50:39, msw wrote:
> optional nit: you might be able to omit the public_deps of ":struct_traits"?

The reason to make the deps public_deps is that the header includes all those
files, which means anyone including the header needs to include them as well.

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
File ui/base/accelerators/mojo/accelerator.mojom (right):

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
ui/base/accelerators/mojo/accelerator.mojom:10: enum AcceleratorKeyState {
On 2017/03/16 22:50:39, msw wrote:
> nit: comment, mention that this matches ui::Accelerator::KeyState

Done.

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
ui/base/accelerators/mojo/accelerator.mojom:15: // See ui::Accelerator for
details on these values.
On 2017/03/16 22:50:39, msw wrote:
> optional nit: // This structure matches ui::Accelerator; see details there.
> (James has actually been encouraging me to document mojoms equally or better
> than the corresponding types in our c++ headers)

Done.

https://codereview.chromium.org/2749183005/diff/20001/ui/base/accelerators/mo...
ui/base/accelerators/mojo/accelerator.mojom:17: // Accelerator::key_code_ is a
KeyboardCode, but it may also contain other
On 2017/03/16 22:50:39, msw wrote:
> optional nit: It might help to elaborate on what other values it may contain.

Done.

[sky, 2017-03-16 23:31:01]

The CQ bit was checked by sky@chromium.org

[sky, 2017-03-16 23:31:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2749183005/#ps2 (title:
"feedback")

[commit-bot: I haz the power, 2017-03-16 23:31:43]

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-03-17 00:38:59]

CQ is committing da patch.

Bot data: {"patchset_id": 2, "attempt_start_ts": 1489707061736760, "parent_rev":
"def538a8cd9e4d982d150d4308e44a2aa4c504ff", "commit_rev":
"e59dfe3023b250e823ca9267d94db30a6bf7c1f8"}

[commit-bot: I haz the power, 2017-03-17 00:40:27]

Description was changed from

==========
Adds mojom structure for ui::Accelerator and struct traits

BUG=701815
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org
==========

to

==========
Adds mojom structure for ui::Accelerator and struct traits

BUG=701815
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org

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

[commit-bot: I haz the power, 2017-03-17 00:40:32]

Committed patchset #3 (id:2) as
https://chromium.googlesource.com/chromium/src/+/e59dfe3023b250e823ca9267d94d...