Creat a KeyboardEventManager class

Create a KeyboardEventManager class

Extract Keyboard related logic and events from
EventHandler and put it in a dedicate class.

BUG=616836
Committed: https://crrev.com/68da48f3c013555d779a0a0c86634e53dfb65ce4
Cr-Commit-Position: refs/heads/master@{#398678}

[Navid Zolghadr, 2016-06-03 19:32:45]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org, mustaq@chromium.org

[dtapuska, 2016-06-03 20:13:39]

https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/KeyboardEventManager.h (right):

https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/KeyboardEventManager.h:26: explicit
KeyboardEventManager(LocalFrame*);
Disallow copy?

Can't the KeyboardEventManager have a handle to the ScrollManager instead of
passing it in to each?

[Navid Zolghadr, 2016-06-03 20:18:52]

https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/KeyboardEventManager.h (right):

https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/KeyboardEventManager.h:26: explicit
KeyboardEventManager(LocalFrame*);
On 2016/06/03 20:13:39, dtapuska wrote:
> Disallow copy?

Sure. Will do.

> 
> Can't the KeyboardEventManager have a handle to the ScrollManager instead of
> passing it in to each?

I was not sure passing the instance in that two functions is better or just
passing the instance in the constructor and keeping a reference to that in this
class. Do you think the latter is better?

[Navid Zolghadr, 2016-06-06 18:29:03]

nzolghadr@chromium.org changed reviewers:
+ bokan@chromium.org

[Navid Zolghadr, 2016-06-06 18:32:10]

Description was changed from

==========
Creat a KeyboardEventManager class

Extract Keyboard related logic and events from
EventHandler and put it in a dedicate class.

BUG=616836
==========

to

==========
Create a KeyboardEventManager class

Extract Keyboard related logic and events from
EventHandler and put it in a dedicate class.

BUG=616836
==========

[bokan, 2016-06-06 18:34:14]

On 2016/06/03 20:18:52, Navid Zolghadr wrote:
>
https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/KeyboardEventManager.h (right):
> 
>
https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/KeyboardEventManager.h:26: explicit
> KeyboardEventManager(LocalFrame*);
> On 2016/06/03 20:13:39, dtapuska wrote:
> > Disallow copy?
> 
> Sure. Will do.
> 
> > 
> > Can't the KeyboardEventManager have a handle to the ScrollManager instead of
> > passing it in to each?
> 
> I was not sure passing the instance in that two functions is better or just
> passing the instance in the constructor and keeping a reference to that in
this
> class. Do you think the latter is better?

I agree with Dave it's somewhat simpler to just pass the reference once at
construction time.

[Navid Zolghadr, 2016-06-06 20:21:49]

ptal.

https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/KeyboardEventManager.h (right):

https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/KeyboardEventManager.h:26: explicit
KeyboardEventManager(LocalFrame*);
On 2016/06/03 20:18:51, Navid Zolghadr wrote:
> On 2016/06/03 20:13:39, dtapuska wrote:
> > Disallow copy?
> 
> Sure. Will do.
> 
> > 
> > Can't the KeyboardEventManager have a handle to the ScrollManager instead of
> > passing it in to each?
> 
> I was not sure passing the instance in that two functions is better or just
> passing the instance in the constructor and keeping a reference to that in
this
> class. Do you think the latter is better?
> 

Done.

[dtapuska, 2016-06-06 20:43:22]

On 2016/06/06 20:21:49, Navid Zolghadr wrote:
> ptal.
> 
>
https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/KeyboardEventManager.h (right):
> 
>
https://codereview.chromium.org/2036643003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/KeyboardEventManager.h:26: explicit
> KeyboardEventManager(LocalFrame*);
> On 2016/06/03 20:18:51, Navid Zolghadr wrote:
> > On 2016/06/03 20:13:39, dtapuska wrote:
> > > Disallow copy?
> > 
> > Sure. Will do.
> > 
> > > 
> > > Can't the KeyboardEventManager have a handle to the ScrollManager instead
of
> > > passing it in to each?
> > 
> > I was not sure passing the instance in that two functions is better or just
> > passing the instance in the constructor and keeping a reference to that in
> this
> > class. Do you think the latter is better?
> > 
> 
> Done.

lgtm % nit

[dtapuska, 2016-06-06 20:43:31]

https://codereview.chromium.org/2036643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2036643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:1: #include
"KeyboardEventManager.h"
Chromium copyright?

[Navid Zolghadr, 2016-06-06 21:37:50]

https://codereview.chromium.org/2036643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2036643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:1: #include
"KeyboardEventManager.h"
On 2016/06/06 20:43:30, dtapuska wrote:
> Chromium copyright?

Done.

[dtapuska, 2016-06-06 23:12:00]

On 2016/06/06 21:37:50, Navid Zolghadr wrote:
>
https://codereview.chromium.org/2036643003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):
> 
>
https://codereview.chromium.org/2036643003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:1: #include
> "KeyboardEventManager.h"
> On 2016/06/06 20:43:30, dtapuska wrote:
> > Chromium copyright?
> 
> Done.

Lgtm

[bokan, 2016-06-07 13:06:53]

Nit: in description s/dedicate/dedicated

lgtm

https://codereview.chromium.org/2036643003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.h (right):

https://codereview.chromium.org/2036643003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.h:48: // cleared in
|PointerEventManager::clear()|.
You mean adding KeyboardManager::clear() and calling it from
PointerEventManager::clear()? IMO, we don't need this note, it depends on the
semantics of the field (e.g. we don't clear either of the current fields).

[Navid Zolghadr, 2016-06-07 14:21:07]

https://codereview.chromium.org/2036643003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.h (right):

https://codereview.chromium.org/2036643003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.h:48: // cleared in
|PointerEventManager::clear()|.
On 2016/06/07 13:06:52, bokan wrote:
> You mean adding KeyboardManager::clear() and calling it from
> PointerEventManager::clear()? IMO, we don't need this note, it depends on the
> semantics of the field (e.g. we don't clear either of the current fields).

Yeah. Makes sense. Particularly since there is no field right now needing this I
remove the comment and probably the first field that needs something like this
would add such function.

[mustaq, 2016-06-07 15:28:36]

LGTM with a nit.

https://codereview.chromium.org/2036643003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2036643003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:50:
PlatformEvent::Modifiers KeyboardEventManager::accessKeyModifiers()
This is not specific to event handling, belongs to PlatformKeyboardEvent I
think.

[Navid Zolghadr, 2016-06-07 15:59:17]

Can you guys have another last look? I was not sure if I need to add
PLATFORM_EXPORT for accessKeyModifiers in PlatformKeyboardEvent or not as I
don't know what exactly that means.

https://codereview.chromium.org/2036643003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2036643003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:50:
PlatformEvent::Modifiers KeyboardEventManager::accessKeyModifiers()
On 2016/06/07 15:28:36, mustaq wrote:
> This is not specific to event handling, belongs to PlatformKeyboardEvent I
> think.

Done.

[bokan, 2016-06-07 16:01:58]

On 2016/06/07 15:59:17, Navid Zolghadr wrote:
> Can you guys have another last look? I was not sure if I need to add
> PLATFORM_EXPORT for accessKeyModifiers in PlatformKeyboardEvent or not as I
> don't know what exactly that means.

You need PLATFORM_EXPORT since accessKeyModifiers is used in a file from Core.
Source/core and Source/platform are built into separate objects. If you're
referencing a name from one to the other, it needs to be exported so the linker
can see it.

[Navid Zolghadr, 2016-06-07 16:24:24]

nzolghadr@chromium.org changed reviewers:
+ rbyers@chromium.org

[Navid Zolghadr, 2016-06-07 16:24:25]

rbyers@chromium.org: Please review changes in 

third_party/WebKit/Source/platform/PlatformKeyboardEvent.h
third_party/WebKit/Source/platform/PlatformKeyboardEvent.cpp

[Rick Byers, 2016-06-08 17:58:16]

https://codereview.chromium.org/2036643003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformKeyboardEvent.cpp (right):

https://codereview.chromium.org/2036643003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformKeyboardEvent.cpp:76: #if OS(MACOSX)
normally we prefer to avoid #ifdefs in blink for platform-specific behavior like
this, and instead use a setting that the embedder can set as appropriate to the
platform (I believe this is how we handle the contextmenu difference).  This
allows us to tests both behaviors on all OSes and have consistent test output.

But I see you're just moving this existing code from EventHandler, so cleaning
it up is not required.  Can you add a TODO though saying to move to a setting?

https://codereview.chromium.org/2036643003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformKeyboardEvent.h (right):

https://codereview.chromium.org/2036643003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformKeyboardEvent.h:92: PLATFORM_EXPORT
static PlatformEvent::Modifiers accessKeyModifiers();
please add a comment describing what this means (i.e. what is an "access key") -
it's not clear to me.  I know you're just moving existing code, so just do you
best to explain it based on how it's used today.

[Navid Zolghadr, 2016-06-08 18:55:08]

ptal.

https://codereview.chromium.org/2036643003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformKeyboardEvent.cpp (right):

https://codereview.chromium.org/2036643003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformKeyboardEvent.cpp:76: #if OS(MACOSX)
On 2016/06/08 17:58:16, Rick Byers wrote:
> normally we prefer to avoid #ifdefs in blink for platform-specific behavior
like
> this, and instead use a setting that the embedder can set as appropriate to
the
> platform (I believe this is how we handle the contextmenu difference).  This
> allows us to tests both behaviors on all OSes and have consistent test output.
> 
> But I see you're just moving this existing code from EventHandler, so cleaning
> it up is not required.  Can you add a TODO though saying to move to a setting?

Done.

https://codereview.chromium.org/2036643003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformKeyboardEvent.h (right):

https://codereview.chromium.org/2036643003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformKeyboardEvent.h:92: PLATFORM_EXPORT
static PlatformEvent::Modifiers accessKeyModifiers();
On 2016/06/08 17:58:16, Rick Byers wrote:
> please add a comment describing what this means (i.e. what is an "access key")
-
> it's not clear to me.  I know you're just moving existing code, so just do you
> best to explain it based on how it's used today.

Done.

[dtapuska, 2016-06-08 19:11:45]

lgtm % comment nit

https://codereview.chromium.org/2036643003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformKeyboardEvent.h (right):

https://codereview.chromium.org/2036643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformKeyboardEvent.h:94: // See
http://www.w3schools.com/tags/att_global_accesskey.asp.
Oh no commenting from w3schools?

Can we change it to the MDN reference?

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/accesskey

[Navid Zolghadr, 2016-06-08 19:22:48]

https://codereview.chromium.org/2036643003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformKeyboardEvent.h (right):

https://codereview.chromium.org/2036643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformKeyboardEvent.h:94: // See
http://www.w3schools.com/tags/att_global_accesskey.asp.
On 2016/06/08 19:11:44, dtapuska wrote:
> Oh no commenting from w3schools?
> 
> Can we change it to the MDN reference?
> 
> https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/accesskey

Done.

[Rick Byers, 2016-06-08 20:01:43]

platform LGTM

[Navid Zolghadr, 2016-06-08 20:18:28]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-06-08 20:18:29]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
mustaq@chromium.org, dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2036643003/#ps180001
(title: " ")

[commit-bot: I haz the power, 2016-06-08 20:19:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036643003/180001

[commit-bot: I haz the power, 2016-06-08 21:18:16]

Description was changed from

==========
Create a KeyboardEventManager class

Extract Keyboard related logic and events from
EventHandler and put it in a dedicate class.

BUG=616836
==========

to

==========
Create a KeyboardEventManager class

Extract Keyboard related logic and events from
EventHandler and put it in a dedicate class.

BUG=616836
==========

[commit-bot: I haz the power, 2016-06-08 21:18:18]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-08 21:21:20]

Description was changed from

==========
Create a KeyboardEventManager class

Extract Keyboard related logic and events from
EventHandler and put it in a dedicate class.

BUG=616836
==========

to

==========
Create a KeyboardEventManager class

Extract Keyboard related logic and events from
EventHandler and put it in a dedicate class.

BUG=616836

Committed: https://crrev.com/68da48f3c013555d779a0a0c86634e53dfb65ce4
Cr-Commit-Position: refs/heads/master@{#398678}
==========

[commit-bot: I haz the power, 2016-06-08 21:21:21]

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