Explicit interface for synchronous InputHandlerProxy input handling

Explicit interface for synchronous InputHandlerProxy input handling

The move of compositor input handling logic from WebKit to content gives us 
guarantees on how and when input events might be handled.  As InputHandlerProxy
handles input events synchronously, we now simply return the EventDisposition
directly, rather than calling into a client interface with the result.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203844

[jdduke (slow), 2013-06-03 20:01:08]

In short, this removes the Did/DidNotHandle callbacks from the
InputHandlerProxyClient, instead returning the result of HandleInputEvent calls
(both of InputHandlerProxy and InputHandlerManager) directly.  PTAL.

[jamesr, 2013-06-03 20:45:59]

I'll review this (i'm owners here too).  Looks really nice, just needs a bit of
style massaging.

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
File content/renderer/gpu/input_handler_manager.cc (right):

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
content/renderer/gpu/input_handler_manager.cc:29: default:
i would omit the default: so the compiler yells at you if one of the enum values
is not handled and put the NOTREACHED()/return outside of the switch

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
File content/renderer/gpu/input_handler_proxy.h (right):

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
content/renderer/gpu/input_handler_proxy.h:37: DidHandle,
enums in chromium code should always be SHOUTING_STYLE - see
http://www.chromium.org/developers/coding-style#Naming

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
File content/renderer/gpu/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
content/renderer/gpu/input_handler_proxy_unittest.cc:18: using
content::InputHandlerProxy;
one pattern i've seen for tests is to put the body of the test in this
construct:

namespace content {
namespace {
.....


which lets you avoid 'using ' statements for everything and lets you avoid all
the content::

[jdduke (slow), 2013-06-03 21:40:42]

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
File content/renderer/gpu/input_handler_manager.cc (right):

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
content/renderer/gpu/input_handler_manager.cc:29: default:
On 2013/06/03 20:45:59, jamesr wrote:
> i would omit the default: so the compiler yells at you if one of the enum
values
> is not handled and put the NOTREACHED()/return outside of the switch

I like it, done.

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
File content/renderer/gpu/input_handler_proxy.h (right):

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
content/renderer/gpu/input_handler_proxy.h:37: DidHandle,
On 2013/06/03 20:45:59, jamesr wrote:
> enums in chromium code should always be SHOUTING_STYLE - see
> http://www.chromium.org/developers/coding-style#Naming

Yeah, I really just moved the internal, intermediate enum that was already being
used, but this is the perfect time to refactor I suppose.

Do you think we should keep this enum (appropriately styled). or simply switch
to InputEventAckState? I somewhat prefer keeping them separate; this class
really isn't concerned with IPC's or ACK's, and it lets the caller interpret the
disposition as appropriate to whatever the context is.

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
File content/renderer/gpu/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
content/renderer/gpu/input_handler_proxy_unittest.cc:18: using
content::InputHandlerProxy;
On 2013/06/03 20:45:59, jamesr wrote:
> one pattern i've seen for tests is to put the body of the test in this
> construct:
> 
> namespace content {
> namespace {
> .....
> 
> 
> which lets you avoid 'using ' statements for everything and lets you avoid all
> the content::

Done.

[jamesr, 2013-06-03 21:42:40]

On 2013/06/03 21:40:42, jdduke wrote:
>
https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
> File content/renderer/gpu/input_handler_proxy.h (right):
> 
>
https://codereview.chromium.org/15927023/diff/1/content/renderer/gpu/input_ha...
> content/renderer/gpu/input_handler_proxy.h:37: DidHandle,
> On 2013/06/03 20:45:59, jamesr wrote:
> > enums in chromium code should always be SHOUTING_STYLE - see
> > http://www.chromium.org/developers/coding-style#Naming
> 
> Yeah, I really just moved the internal, intermediate enum that was already
being
> used, but this is the perfect time to refactor I suppose.

Oops, you're right - my bad!

> 
> Do you think we should keep this enum (appropriately styled). or simply switch
> to InputEventAckState? I somewhat prefer keeping them separate; this class
> really isn't concerned with IPC's or ACK's, and it lets the caller interpret
the
> disposition as appropriate to whatever the context is.

I like keeping them separate.

[jdduke (slow), 2013-06-03 21:56:59]

OK, latest version has the enum refactor and style fixes.  Thanks for the
review.

[jamesr, 2013-06-03 22:07:44]

lgtm. this is really nice.

I think you can be shorter on the enum values (see inline comment)

https://codereview.chromium.org/15927023/diff/3002/content/renderer/gpu/input...
File content/renderer/gpu/input_handler_proxy.h (right):

https://codereview.chromium.org/15927023/diff/3002/content/renderer/gpu/input...
content/renderer/gpu/input_handler_proxy.h:37: EVENT_DISPOSITION_DID_HANDLE,
nit: the whole "put the enum name in the enum values" thing is a holdover from
the WebKit API style. you don't need to do that here - so long as the enum
values don't collide they can be shorter. up to you

[jdduke (slow), 2013-06-03 22:31:03]

https://codereview.chromium.org/15927023/diff/3002/content/renderer/gpu/input...
File content/renderer/gpu/input_handler_proxy.h (right):

https://codereview.chromium.org/15927023/diff/3002/content/renderer/gpu/input...
content/renderer/gpu/input_handler_proxy.h:37: EVENT_DISPOSITION_DID_HANDLE,
On 2013/06/03 22:07:44, jamesr wrote:
> nit: the whole "put the enum name in the enum values" thing is a holdover from
> the WebKit API style. you don't need to do that here - so long as the enum
> values don't collide they can be shorter. up to you

Oh heck yes.  I'll update.

[commit-bot: I haz the power, 2013-06-03 22:53:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/15927023/1012

[commit-bot: I haz the power, 2013-06-04 04:18:38]

Change committed as 203844