Add a KeyboardLayoutEngine::SetCurrentLayoutFromBuffer implementation

Add a KeyboardLayoutEngine::SetCurrentLayoutFromBuffer implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout string.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout string not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
Review-Url: https://codereview.chromium.org/2645703005
Cr-Commit-Position: refs/heads/master@{#445171}
Committed: https://chromium.googlesource.com/chromium/src/+/e5a535bb5a2fc99a090f81bde71ed50e974efa60

[tonikitoo, 2017-01-20 13:43:00]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 13:43:14]

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

[tonikitoo, 2017-01-20 13:45:32]

Description was changed from

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutByFD implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with an file descriptor that can be mmap'ed in order to retrieve
the keyboard layout.

Today the method SetCurrentLayoutByName of XkbKeyboardLayoutEngine is
(ab)used to support this case (where the client provices the keyboard
layout not the name), which is not ideal.

CL adds a method that solely handles this casei, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

to

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutByFD implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with an file descriptor that can be mmap'ed in order to retrieve
the keyboard layout.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

[tonikitoo, 2017-01-20 13:45:49]

Description was changed from

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutByFD implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with an file descriptor that can be mmap'ed in order to retrieve
the keyboard layout.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

to

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutByFD implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

[fwang, 2017-01-20 14:11:33]

fwang@igalia.com changed reviewers:
+ fwang@igalia.com

[fwang, 2017-01-20 14:11:33]

informal l g t m

[commit-bot: I haz the power, 2017-01-20 15:15:07]

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

[commit-bot: I haz the power, 2017-01-20 15:15:08]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-01-20 15:27:19]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 15:27:27]

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

[tonikitoo, 2017-01-20 15:27:53]

tonikitoo@igalia.com changed reviewers:
+ forney@google.com, rjkroege@chromium.org, spang@chromium.org

[tonikitoo, 2017-01-20 15:27:54]

PTAL

[tonikitoo, 2017-01-20 15:42:16]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 15:42: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, 2017-01-20 16:26:18]

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

[commit-bot: I haz the power, 2017-01-20 16:26:19]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-01-20 17:10:53]

Description was changed from

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutByFD implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

to

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutFromString implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

[tonikitoo, 2017-01-20 17:11:17]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 17:11:35]

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

[tonikitoo, 2017-01-20 17:11:56]

Description was changed from

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutFromString implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

to

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutFromString implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout string.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout string not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

[spang, 2017-01-20 17:41:14]

https://codereview.chromium.org/2645703005/diff/60001/ui/events/ozone/layout/...
File ui/events/ozone/layout/no/no_keyboard_layout_engine.h (right):

https://codereview.chromium.org/2645703005/diff/60001/ui/events/ozone/layout/...
ui/events/ozone/layout/no/no_keyboard_layout_engine.h:22: bool
SetCurrentLayoutFromString(const std::string& keymap_str) override;
Can you not say SetCurrentLayoutFromBuffer(const char* buf, size_t length) and
use xkb_keymap_new_from_buffer? It would avoid a copy.

https://codereview.chromium.org/2645703005/diff/60001/ui/events/ozone/layout/...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right):

https://codereview.chromium.org/2645703005/diff/60001/ui/events/ozone/layout/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:697: LOG(ERROR) <<
"Currently the only evdev client is ChromeOS."
Please don't remove the return statement.

This error message doesn't mention what failed (changing keymaps) and is also
not very clear about why (it just isn't supported/implemented). Just say

NOTIMPLEMENTED();

[commit-bot: I haz the power, 2017-01-20 17:57:15]

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

[commit-bot: I haz the power, 2017-01-20 17:57:15]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-01-20 18:58:11]

https://codereview.chromium.org/2645703005/diff/60001/ui/events/ozone/layout/...
File ui/events/ozone/layout/no/no_keyboard_layout_engine.h (right):

https://codereview.chromium.org/2645703005/diff/60001/ui/events/ozone/layout/...
ui/events/ozone/layout/no/no_keyboard_layout_engine.h:22: bool
SetCurrentLayoutFromString(const std::string& keymap_str) override;
On 2017/01/20 17:41:14, spang wrote:
> Can you not say SetCurrentLayoutFromBuffer(const char* buf, size_t length) and
> use xkb_keymap_new_from_buffer? It would avoid a copy.

Done.

https://codereview.chromium.org/2645703005/diff/60001/ui/events/ozone/layout/...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right):

https://codereview.chromium.org/2645703005/diff/60001/ui/events/ozone/layout/...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:697: LOG(ERROR) <<
"Currently the only evdev client is ChromeOS."
On 2017/01/20 17:41:14, spang wrote:
> Please don't remove the return statement.
> 
> This error message doesn't mention what failed (changing keymaps) and is also
> not very clear about why (it just isn't supported/implemented). Just say
> 
> NOTIMPLEMENTED();

Done.

[tonikitoo, 2017-01-20 18:58:18]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 18:58:38]

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

[spang, 2017-01-20 19:00:28]

lgtm

[spang, 2017-01-20 19:00:53]

Description was changed from

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutFromString implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout string.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout string not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

to

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutFromBuffer implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout string.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout string not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

[Michael Forney, 2017-01-20 19:31:43]

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
ui/events/ozone/layout/keyboard_layout_engine.h:42: // that represent keyboard
mapping description; returns true on success.
Rewrap.

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
File ui/events/ozone/layout/no/no_keyboard_layout_engine.h (right):

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
ui/events/ozone/layout/no/no_keyboard_layout_engine.h:23: uint32_t size)
override;
Shouldn't this be size_t size?

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (left):

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:810: if (keymap)
This check is unnecessary. You already returned if !keymap.

[commit-bot: I haz the power, 2017-01-20 19:51:15]

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

[commit-bot: I haz the power, 2017-01-20 19:51:16]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-01-20 20:01:13]

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
File ui/events/ozone/layout/keyboard_layout_engine.h (right):

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
ui/events/ozone/layout/keyboard_layout_engine.h:42: // that represent keyboard
mapping description; returns true on success.
On 2017/01/20 19:31:42, Michael Forney wrote:
> Rewrap.

Done.

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
File ui/events/ozone/layout/no/no_keyboard_layout_engine.h (right):

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
ui/events/ozone/layout/no/no_keyboard_layout_engine.h:23: uint32_t size)
override;
On 2017/01/20 19:31:43, Michael Forney wrote:
> Shouldn't this be size_t size?

Done.

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (left):

https://codereview.chromium.org/2645703005/diff/100001/ui/events/ozone/layout...
ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:810: if (keymap)
On 2017/01/20 19:31:43, Michael Forney wrote:
> This check is unnecessary. You already returned if !keymap.

Done.

[tonikitoo, 2017-01-20 20:32:34]

The CQ bit was checked by tonikitoo@igalia.com

[tonikitoo, 2017-01-20 20:32:34]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/2645703005/#ps140001
(title: "for landing")

[commit-bot: I haz the power, 2017-01-20 20:33:19]

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-01-20 21:29:48]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484944354232380,
"parent_rev": "6b07546314db5956f7f793d660bee51f8067ed14", "commit_rev":
"e5a535bb5a2fc99a090f81bde71ed50e974efa60"}

[commit-bot: I haz the power, 2017-01-20 21:30:20]

Description was changed from

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutFromBuffer implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout string.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout string not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890
==========

to

==========
Add a KeyboardLayoutEngine::SetCurrentLayoutFromBuffer implementation

Some servers (read Wayland here) do the XKB initialization and provide
clients with a file descriptor that can be mmap'ed in order to retrieve
the keyboard layout string.

Today the method XkbKeyboardLayoutEngine::SetCurrentLayoutByName is
(ab)used to support this case (where the client provides the keyboard
layout string not the name), which is not ideal.

CL adds a method that solely handles this case, cleaning up
XkbKeyboardLayoutEngine::SetCurrentLayoutByName. The method will be used
later on when the CL that adds keyboard/wayland support lands.

BUG=578890

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

[commit-bot: I haz the power, 2017-01-20 21:30:21]

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