Add GetCharacter() and GetUnmodifiedCharacter() methods to views::Event.

Add GetCharacter() and GetUnmodifiedCharacter() methods to views::Event.

BUG=75003
TEST=views_unittests --gtest_filter=EventTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78943

[James Su, 2011-03-18 03:43:06]

Hi,
  It's the second CL of the new Views input method API. These two methods will
be used for:

1. Replace NativeTextfieldViews::GetPrintableChar()
2. Help create NativeWebKeyboardEvent object
3. Support XKB keyboard layouts on Linux

Regards
James Su

[Yusuke Sato, 2011-03-18 12:07:41]

I'll revisit this change when all CLs get ready, but let me ask some questions
first.

http://codereview.chromium.org/6713027/diff/1/views/events/event.cc
File views/events/event.cc (right):

http://codereview.chromium.org/6713027/diff/1/views/events/event.cc#newcode80
views/events/event.cc:80: // Following Windows behavior to map ctrl-z ~ ctrl-z
to \x01 ~ \x1A.
the first ctrl-z should be ctrl-a?

http://codereview.chromium.org/6713027/diff/1/views/events/event.cc#newcode125
views/events/event.cc:125: return shift ? ")!@#$%^&*("[key_code - ui::VKEY_0] :
key_code;
It seems that the US keyboard layout is assumed here. Is it okay? If it's okay,
please add a comment that explains why we can safely assume the US layout here.

http://codereview.chromium.org/6713027/diff/1/views/events/event_gtk.cc
File views/events/event_gtk.cc (right):

http://codereview.chromium.org/6713027/diff/1/views/events/event_gtk.cc#newco...
views/events/event_gtk.cc:194: if (IsControlDown() || !native_event())
question: when native_event() can be NULL?

http://codereview.chromium.org/6713027/diff/1/views/events/event_gtk.cc#newco...
views/events/event_gtk.cc:208: if (!native_event())
when native_event() can be NULL?

http://codereview.chromium.org/6713027/diff/1/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/6713027/diff/1/views/events/event_x.cc#newcode220
views/events/event_x.cc:220: char buf[7];
I guess it should be either [6]; or [7] = {};

http://codereview.chromium.org/6713027/diff/1/views/events/event_x.cc#newcode265
views/events/event_x.cc:265: if (!native_event_2())
when native_event_2() can be NULL?

http://codereview.chromium.org/6713027/diff/1/views/events/event_x.cc#newcode279
views/events/event_x.cc:279: if (!native_event_2())
ditto.

[James Su, 2011-03-18 15:54:08]

http://codereview.chromium.org/6713027/diff/1/views/events/event.cc
File views/events/event.cc (right):

http://codereview.chromium.org/6713027/diff/1/views/events/event.cc#newcode80
views/events/event.cc:80: // Following Windows behavior to map ctrl-z ~ ctrl-z
to \x01 ~ \x1A.
On 2011/03/18 12:07:41, Yusuke Sato wrote:
> the first ctrl-z should be ctrl-a?

Done.

http://codereview.chromium.org/6713027/diff/1/views/events/event.cc#newcode125
views/events/event.cc:125: return shift ? ")!@#$%^&*("[key_code - ui::VKEY_0] :
key_code;
On 2011/03/18 12:07:41, Yusuke Sato wrote:
> It seems that the US keyboard layout is assumed here. Is it okay? If it's
okay,
> please add a comment that explains why we can safely assume the US layout
here.

Updated the comment in the header file.

http://codereview.chromium.org/6713027/diff/1/views/events/event_gtk.cc
File views/events/event_gtk.cc (right):

http://codereview.chromium.org/6713027/diff/1/views/events/event_gtk.cc#newco...
views/events/event_gtk.cc:194: if (IsControlDown() || !native_event())
On 2011/03/18 12:07:41, Yusuke Sato wrote:
> question: when native_event() can be NULL?

It could be NULL if it's a fabricated event.

http://codereview.chromium.org/6713027/diff/1/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/6713027/diff/1/views/events/event_x.cc#newcode220
views/events/event_x.cc:220: char buf[7];
On 2011/03/18 12:07:41, Yusuke Sato wrote:
> I guess it should be either [6]; or [7] = {};
> 

Done.

[Evan Stade, 2011-03-18 21:13:07]

mostly LG but I can't say I feel super qualified to review

http://codereview.chromium.org/6713027/diff/4001/views/events/event_gtk.cc
File views/events/event_gtk.cc (right):

http://codereview.chromium.org/6713027/diff/4001/views/events/event_gtk.cc#ne...
views/events/event_gtk.cc:203: static const guint kIgnoredModifiers =
declare this right above where it's used (and near the comment that pretains to
it)

http://codereview.chromium.org/6713027/diff/4001/views/events/event_gtk.cc#ne...
views/events/event_gtk.cc:221: NULL, NULL, NULL))
curlies

http://codereview.chromium.org/6713027/diff/4001/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/6713027/diff/4001/views/events/event_x.cc#newc...
views/events/event_x.cc:222: DCHECK(bytes_written <= 6);
DCHECK_LE

http://codereview.chromium.org/6713027/diff/4001/views/events/event_x.cc#newc...
views/events/event_x.cc:276: static const unsigned int kIgnoredModifiers =
ControlMask | LockMask |
ditto

[James Su, 2011-03-18 21:22:14]

http://codereview.chromium.org/6713027/diff/4001/views/events/event_gtk.cc
File views/events/event_gtk.cc (right):

http://codereview.chromium.org/6713027/diff/4001/views/events/event_gtk.cc#ne...
views/events/event_gtk.cc:203: static const guint kIgnoredModifiers =
On 2011/03/18 21:13:07, Evan Stade wrote:
> declare this right above where it's used (and near the comment that pretains
to
> it)

Done.

http://codereview.chromium.org/6713027/diff/4001/views/events/event_gtk.cc#ne...
views/events/event_gtk.cc:221: NULL, NULL, NULL))
On 2011/03/18 21:13:07, Evan Stade wrote:
> curlies

Done.

http://codereview.chromium.org/6713027/diff/4001/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/6713027/diff/4001/views/events/event_x.cc#newc...
views/events/event_x.cc:222: DCHECK(bytes_written <= 6);
On 2011/03/18 21:13:07, Evan Stade wrote:
> DCHECK_LE

Done.

http://codereview.chromium.org/6713027/diff/4001/views/events/event_x.cc#newc...
views/events/event_x.cc:276: static const unsigned int kIgnoredModifiers =
ControlMask | LockMask |
On 2011/03/18 21:13:07, Evan Stade wrote:
> ditto

Done.

[oshima, 2011-03-21 18:35:42]

http://codereview.chromium.org/6713027/diff/2003/views/events/event.cc
File views/events/event.cc (right):

http://codereview.chromium.org/6713027/diff/2003/views/events/event.cc#newcode72
views/events/event.cc:72: // KeyEvent, private:
please use new format. convert others if possible.

// KeyEvent, privaite: ------------

http://codereview.chromium.org/6713027/diff/2003/views/events/event.h
File views/events/event.h (right):

http://codereview.chromium.org/6713027/diff/2003/views/events/event.h#newcode300
views/events/event.h:300: // BMP characters.
What do we do for non-BMP?

http://codereview.chromium.org/6713027/diff/2003/views/events/event.h#newcode311
views/events/event.h:311: // is no native event or no better way to get the
character.
I indeed assume us keyboard, but it's still not clear why we need or when this
is usefl. Can you clarify?

http://codereview.chromium.org/6713027/diff/2003/views/events/event_gtk.cc
File views/events/event_gtk.cc (right):

http://codereview.chromium.org/6713027/diff/2003/views/events/event_gtk.cc#ne...
views/events/event_gtk.cc:221: NULL, NULL, NULL)) {
indent

http://codereview.chromium.org/6713027/diff/2003/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/6713027/diff/2003/views/events/event_x.cc#newc...
views/events/event_x.cc:219: uint16 GetCharacterFromXKeyEvent(XKeyEvent* key) {
can key be const?

[James Su, 2011-03-21 18:59:22]

http://codereview.chromium.org/6713027/diff/2003/views/events/event.cc
File views/events/event.cc (right):

http://codereview.chromium.org/6713027/diff/2003/views/events/event.cc#newcode72
views/events/event.cc:72: // KeyEvent, private:
On 2011/03/21 18:35:42, oshima wrote:
> please use new format. convert others if possible.
> 
> // KeyEvent, privaite: ------------

Done.

http://codereview.chromium.org/6713027/diff/2003/views/events/event.h
File views/events/event.h (right):

http://codereview.chromium.org/6713027/diff/2003/views/events/event.h#newcode300
views/events/event.h:300: // BMP characters.
On 2011/03/21 18:35:42, oshima wrote:
> What do we do for non-BMP? 

non-BMP characters could be input via TextInputClient::InsertText().

And seems that there is no keyboard layout supporting non-BMP characters. At
least I don't know.

I was thinking to string16 here, but I'm skeptical if it's a good idea to add
such dependency.

http://codereview.chromium.org/6713027/diff/2003/views/events/event.h#newcode311
views/events/event.h:311: // is no native event or no better way to get the
character.
On 2011/03/21 18:35:42, oshima wrote:
> I indeed assume us keyboard, but it's still not clear why we need or when this
> is usefl. Can you clarify?

Key events generated by virtual keyboard or input method won't have
native_event. This method is mainly for those kind of events.

http://codereview.chromium.org/6713027/diff/2003/views/events/event_gtk.cc
File views/events/event_gtk.cc (right):

http://codereview.chromium.org/6713027/diff/2003/views/events/event_gtk.cc#ne...
views/events/event_gtk.cc:221: NULL, NULL, NULL)) {
On 2011/03/21 18:35:42, oshima wrote:
> indent

Done.

http://codereview.chromium.org/6713027/diff/2003/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/6713027/diff/2003/views/events/event_x.cc#newc...
views/events/event_x.cc:219: uint16 GetCharacterFromXKeyEvent(XKeyEvent* key) {
On 2011/03/21 18:35:42, oshima wrote:
> can key be const?

No. XLookupString() accepts non-const pointer.

[oshima, 2011-03-21 22:53:41]

LGTM