Forward touch events to a RenderWidget's associated WebWidget

Forward touch events to a RenderWidget's associated WebWidget

Receives WebTouchEvents over the IPC bridge and forwards
them to the RenderWidget's WebWidget. Sends each touch event
to an interim GestureManager.

BUG=
TEST=unit_tests

[rjkroege, 2010-09-21 21:54:38]

Note: depends on https://bugs.webkit.org/show_bug.cgi?id=45560.

[sky, 2010-09-21 22:28:40]

http://codereview.chromium.org/3388013/diff/1/3
File chrome/renderer/gesture_manager.cc (right):

http://codereview.chromium.org/3388013/diff/1/3#newcode6
chrome/renderer/gesture_manager.cc:6: #ifndef NDEBUG
newline between 5 and 6.

http://codereview.chromium.org/3388013/diff/1/3#newcode22
chrome/renderer/gesture_manager.cc:22: GestureManager::~GestureManager() {
Make sure the order of methods in the .cc matches that of the .h

http://codereview.chromium.org/3388013/diff/1/3#newcode32
chrome/renderer/gesture_manager.cc:32: DLOG(INFO) <<
"ProcessTouchEventForGesture...";
We generally don't use DLOGs. See threads on chromium-dev for details.

http://codereview.chromium.org/3388013/diff/1/3#newcode35
chrome/renderer/gesture_manager.cc:35: const WebTouchPoint &p =
event.touchPoints[i];
Point& p

http://codereview.chromium.org/3388013/diff/1/3#newcode37
chrome/renderer/gesture_manager.cc:37: case NoGesture:
indenting is wrong.

http://codereview.chromium.org/3388013/diff/1/3#newcode48
chrome/renderer/gesture_manager.cc:48: DLOG(INFO) << ">> error shouldn't
happen\n";
NOTREACHED if it shouldn't happen.

http://codereview.chromium.org/3388013/diff/1/3#newcode52
chrome/renderer/gesture_manager.cc:52: &&
isInClickTimeWindow(event.timeStampSeconds)
indent to align with (

http://codereview.chromium.org/3388013/diff/1/3#newcode98
chrome/renderer/gesture_manager.cc:98: // Dispatch a mouseDown/mouseUp/Click
sequence
Descriptions should be in the header.

http://codereview.chromium.org/3388013/diff/1/4
File chrome/renderer/gesture_manager.h (right):

http://codereview.chromium.org/3388013/diff/1/4#newcode5
chrome/renderer/gesture_manager.h:5: 
remove this line

http://codereview.chromium.org/3388013/diff/1/4#newcode20
chrome/renderer/gesture_manager.h:20: 
remove this line

http://codereview.chromium.org/3388013/diff/1/4#newcode38
chrome/renderer/gesture_manager.h:38: // The states of the gesture manager state
machine.
Enums should appear first.

http://codereview.chromium.org/3388013/diff/1/4#newcode40
chrome/renderer/gesture_manager.h:40: NoGesture,
Use all caps for enums, eg NO_GESTURE.

http://codereview.chromium.org/3388013/diff/1/4#newcode58
chrome/renderer/gesture_manager.h:58: bool isInClickTimeWindow(const double ts);
Methods start with capital.

http://codereview.chromium.org/3388013/diff/1/4#newcode69
chrome/renderer/gesture_manager.h:69: static const double MaxClickDownTime = .8;
Constants are named with k, and the value assigned in the definition.

http://codereview.chromium.org/3388013/diff/1/4#newcode80
chrome/renderer/gesture_manager.h:80: SyntheticCandidateGesture
m_candidateGesture;
Only use webkit style (m_ if you're in webkit).

[rjkroege, 2010-09-23 01:51:57]

http://codereview.chromium.org/3388013/diff/1/3
File chrome/renderer/gesture_manager.cc (right):

http://codereview.chromium.org/3388013/diff/1/3#newcode6
chrome/renderer/gesture_manager.cc:6: #ifndef NDEBUG
On 2010/09/21 22:28:40, sky wrote:
> newline between 5 and 6.

Done.

http://codereview.chromium.org/3388013/diff/1/3#newcode22
chrome/renderer/gesture_manager.cc:22: GestureManager::~GestureManager() {
On 2010/09/21 22:28:40, sky wrote:
> Make sure the order of methods in the .cc matches that of the .h

Done.

http://codereview.chromium.org/3388013/diff/1/3#newcode32
chrome/renderer/gesture_manager.cc:32: DLOG(INFO) <<
"ProcessTouchEventForGesture...";
On 2010/09/21 22:28:40, sky wrote:
> We generally don't use DLOGs. See threads on chromium-dev for details.

I can only find a thread asking for when one should use LOG vs DLOG so remain in
the dark. Can you clarify?

http://codereview.chromium.org/3388013/diff/1/3#newcode35
chrome/renderer/gesture_manager.cc:35: const WebTouchPoint &p =
event.touchPoints[i];
On 2010/09/21 22:28:40, sky wrote:
> Point& p

Done.

http://codereview.chromium.org/3388013/diff/1/3#newcode37
chrome/renderer/gesture_manager.cc:37: case NoGesture:
On 2010/09/21 22:28:40, sky wrote:
> indenting is wrong.

Done.

http://codereview.chromium.org/3388013/diff/1/3#newcode48
chrome/renderer/gesture_manager.cc:48: DLOG(INFO) << ">> error shouldn't
happen\n";
On 2010/09/21 22:28:40, sky wrote:
> NOTREACHED if it shouldn't happen.

Done.

http://codereview.chromium.org/3388013/diff/1/3#newcode52
chrome/renderer/gesture_manager.cc:52: &&
isInClickTimeWindow(event.timeStampSeconds)
On 2010/09/21 22:28:40, sky wrote:
> indent to align with (

Done.

http://codereview.chromium.org/3388013/diff/1/3#newcode98
chrome/renderer/gesture_manager.cc:98: // Dispatch a mouseDown/mouseUp/Click
sequence
On 2010/09/21 22:28:40, sky wrote:
> Descriptions should be in the header.

Done.

http://codereview.chromium.org/3388013/diff/1/4
File chrome/renderer/gesture_manager.h (right):

http://codereview.chromium.org/3388013/diff/1/4#newcode5
chrome/renderer/gesture_manager.h:5: 
On 2010/09/21 22:28:40, sky wrote:
> remove this line

Done.

http://codereview.chromium.org/3388013/diff/1/4#newcode20
chrome/renderer/gesture_manager.h:20: 
On 2010/09/21 22:28:40, sky wrote:
> remove this line

Done.

http://codereview.chromium.org/3388013/diff/1/4#newcode38
chrome/renderer/gesture_manager.h:38: // The states of the gesture manager state
machine.
On 2010/09/21 22:28:40, sky wrote:
> Enums should appear first.

Done.

http://codereview.chromium.org/3388013/diff/1/4#newcode40
chrome/renderer/gesture_manager.h:40: NoGesture,
On 2010/09/21 22:28:40, sky wrote:
> Use all caps for enums, eg NO_GESTURE.

Done.

http://codereview.chromium.org/3388013/diff/1/4#newcode58
chrome/renderer/gesture_manager.h:58: bool isInClickTimeWindow(const double ts);
On 2010/09/21 22:28:40, sky wrote:
> Methods start with capital.

Done.

I had written all of this code originally to be in WebKit and forgot to update
the coding conventions when I moved it to Chrome. Sorry.

http://codereview.chromium.org/3388013/diff/1/4#newcode69
chrome/renderer/gesture_manager.h:69: static const double MaxClickDownTime = .8;
On 2010/09/21 22:28:40, sky wrote:
> Constants are named with k, and the value assigned in the definition.

Done.

http://codereview.chromium.org/3388013/diff/1/4#newcode80
chrome/renderer/gesture_manager.h:80: SyntheticCandidateGesture
m_candidateGesture;
On 2010/09/21 22:28:40, sky wrote:
> Only use webkit style (m_ if you're in webkit).

Done.

[sky, 2010-09-23 17:13:03]

I don't think I'm really adding any value here other than style nits...

http://codereview.chromium.org/3388013/diff/5001/6002
File chrome/renderer/gesture_manager.cc (right):

http://codereview.chromium.org/3388013/diff/5001/6002#newcode33
chrome/renderer/gesture_manager.cc:33: DLOG(INFO) <<
"ProcessTouchEventForGesture";
The problem with random DLOGs like this is if every developer added what they
thought was mildly interesting to DLOG then it rapidly adds up to a ton of
console spam and you can never find what you really care about. Think of it this
way, a year from now is this output going to be interesting to you?

http://codereview.chromium.org/3388013/diff/5001/6002#newcode61
chrome/renderer/gesture_manager.cc:61: || p.state == WebTouchPoint::StateMoved)
you should indent this one more space, otherwise it looks like it's at the same
nesting level as the && on the next line, which isn't right.

http://codereview.chromium.org/3388013/diff/5001/6003
File chrome/renderer/gesture_manager.h (right):

http://codereview.chromium.org/3388013/diff/5001/6003#newcode44
chrome/renderer/gesture_manager.h:44: // be the target of any generated
synthetic event. Finally, handled
what is handled? previously_handled?

http://codereview.chromium.org/3388013/diff/5001/6003#newcode49
chrome/renderer/gesture_manager.h:49: bool previouslyHandled);
previously_handled

http://codereview.chromium.org/3388013/diff/5001/6003#newcode57
chrome/renderer/gesture_manager.h:57: static const double kMaxClickDownTime =
.8;
nit: Values for constants are typically in the .cc, but apparently the style
guide doesn't say either way so this is fine although not typical of Chrome.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode67
chrome/renderer/gesture_manager.h:67: void DispatchSyntheticClick(RenderWidget*,
const WebKit::WebTouchEvent&,
Parameters should have names, and the definition/declaration should match. Also,
when you wrap put each parameter on its own line.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode71
chrome/renderer/gesture_manager.h:71: bool IsInClickTimeWindow(const double ts);
no point in using const here.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode80
chrome/renderer/gesture_manager.h:80: void ScrollViaTouchMotion(const
WebKit::WebTouchPoint& p, RenderWidget *w);
RenderWidget* w

http://codereview.chromium.org/3388013/diff/5001/6003#newcode92
chrome/renderer/gesture_manager.h:92: double firstTouchTime_;
first_touch_time_

[rjkroege, 2010-09-23 21:36:55]

I think I've addressed the comments. Hopefully I'll have the differences between
the C++ style guide and the Java/JavaScript guide internalized soon.

Should someone else review the renderer changes? They're tiny and I'll make sure
that that I get a passing try result before committing.

http://codereview.chromium.org/3388013/diff/5001/6002
File chrome/renderer/gesture_manager.cc (right):

http://codereview.chromium.org/3388013/diff/5001/6002#newcode33
chrome/renderer/gesture_manager.cc:33: DLOG(INFO) <<
"ProcessTouchEventForGesture";
On 2010/09/23 17:13:03, sky wrote:
> The problem with random DLOGs like this is if every developer added what they
> thought was mildly interesting to DLOG then it rapidly adds up to a ton of
> console spam and you can never find what you really care about. Think of it
this
> way, a year from now is this output going to be interesting to you?

True. Removed.

http://codereview.chromium.org/3388013/diff/5001/6002#newcode61
chrome/renderer/gesture_manager.cc:61: || p.state == WebTouchPoint::StateMoved)
On 2010/09/23 17:13:03, sky wrote:
> you should indent this one more space, otherwise it looks like it's at the
same
> nesting level as the && on the next line, which isn't right.

Done.

http://codereview.chromium.org/3388013/diff/5001/6003
File chrome/renderer/gesture_manager.h (right):

http://codereview.chromium.org/3388013/diff/5001/6003#newcode44
chrome/renderer/gesture_manager.h:44: // be the target of any generated
synthetic event. Finally, handled
On 2010/09/23 17:13:03, sky wrote:
> what is handled? previously_handled?

Done.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode49
chrome/renderer/gesture_manager.h:49: bool previouslyHandled);
On 2010/09/23 17:13:03, sky wrote:
> previously_handled

Done.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode57
chrome/renderer/gesture_manager.h:57: static const double kMaxClickDownTime =
.8;
On 2010/09/23 17:13:03, sky wrote:
> nit: Values for constants are typically in the .cc, but apparently the style
> guide doesn't say either way so this is fine although not typical of Chrome.

I moved them.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode67
chrome/renderer/gesture_manager.h:67: void DispatchSyntheticClick(RenderWidget*,
const WebKit::WebTouchEvent&,
On 2010/09/23 17:13:03, sky wrote:
> Parameters should have names, and the definition/declaration should match.
Also,
> when you wrap put each parameter on its own line.

Done.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode71
chrome/renderer/gesture_manager.h:71: bool IsInClickTimeWindow(const double ts);
On 2010/09/23 17:13:03, sky wrote:
> no point in using const here.

Done.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode80
chrome/renderer/gesture_manager.h:80: void ScrollViaTouchMotion(const
WebKit::WebTouchPoint& p, RenderWidget *w);
On 2010/09/23 17:13:03, sky wrote:
> RenderWidget* w

Done.

http://codereview.chromium.org/3388013/diff/5001/6003#newcode92
chrome/renderer/gesture_manager.h:92: double firstTouchTime_;
On 2010/09/23 17:13:03, sky wrote:
> first_touch_time_

Done.

And the others members too.

[sky, 2010-09-23 21:57:13]

LGTM

Yes to someone else reviewing the renderer change.

  -Scott

http://codereview.chromium.org/3388013/diff/11001/12002
File chrome/renderer/gesture_manager.cc (right):

http://codereview.chromium.org/3388013/diff/11001/12002#newcode7
chrome/renderer/gesture_manager.cc:7: //#ifndef NDEBUG
Remove this and line 11.

http://codereview.chromium.org/3388013/diff/11001/12002#newcode22
chrome/renderer/gesture_manager.cc:22: 
remove one newline here.

[rjkroege, 2010-09-24 20:09:49]

Darin: could you have a look at the RenderWidget changes?

[darin (slow to review), 2010-09-25 15:21:06]

http://codereview.chromium.org/3388013/diff/15001/16004
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/3388013/diff/15001/16004#newcode360
chrome/renderer/render_widget.cc:360: if
(WebInputEvent::isTouchEventType(input_event->type)) {
why is this code here instead of inside the WebViewImpl::handleInputEvent
function?

if you put the code in handleInputEvent, then you can write layout tests for
this feature more easily by using the eventSender

http://codereview.chromium.org/3388013/diff/15001/16005
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/3388013/diff/15001/16005#newcode355
chrome/renderer/render_widget.h:355: // A cached pointer to the GestureManager.
why cache this here?  GestureManager::Get() is fast.

[darin (slow to review), 2010-09-25 15:24:29]

On Sat, Sep 25, 2010 at 8:21 AM, <darin@chromium.org> wrote:

>
> http://codereview.chromium.org/3388013/diff/15001/16004
> File chrome/renderer/render_widget.cc (right):
>
> http://codereview.chromium.org/3388013/diff/15001/16004#newcode360
> chrome/renderer/render_widget.cc:360: if
> (WebInputEvent::isTouchEventType(input_event->type)) {
> why is this code here instead of inside the
> WebViewImpl::handleInputEvent function?
>
> if you put the code in handleInputEvent, then you can write layout tests
> for this feature more easily by using the eventSender
>

also, I recall objecting to the addition of WebWidget::gestureScroll.
 GestureManager, which is the caller of gestureScroll, could easily live in
WebKit.

-Darin



>
> http://codereview.chromium.org/3388013/diff/15001/16005
> File chrome/renderer/render_widget.h (right):
>
> http://codereview.chromium.org/3388013/diff/15001/16005#newcode355
> chrome/renderer/render_widget.h:355: // A cached pointer to the
> GestureManager.
> why cache this here?  GestureManager::Get() is fast.
>
>
> http://codereview.chromium.org/3388013/show
>

[dglazkov1, 2010-09-27 02:07:12]

On 2010/09/25 15:21:06, darin wrote:
> http://codereview.chromium.org/3388013/diff/15001/16004
> File chrome/renderer/render_widget.cc (right):
> 
> http://codereview.chromium.org/3388013/diff/15001/16004#newcode360
> chrome/renderer/render_widget.cc:360: if
> (WebInputEvent::isTouchEventType(input_event->type)) {
> why is this code here instead of inside the WebViewImpl::handleInputEvent
> function?
> 
> if you put the code in handleInputEvent, then you can write layout tests for
> this feature more easily by using the eventSender

I agree and I commented in https://bugs.webkit.org/show_bug.cgi?id=45560. I
think the line of separation of concerns needs to move and possibly put
GestureManager into WebCore/platform. Or something.

> http://codereview.chromium.org/3388013/diff/15001/16005
> File chrome/renderer/render_widget.h (right):
> 
> http://codereview.chromium.org/3388013/diff/15001/16005#newcode355
> chrome/renderer/render_widget.h:355: // A cached pointer to the
GestureManager.
> why cache this here?  GestureManager::Get() is fast.

[rjkroege, 2010-09-27 17:19:39]

On Sat, Sep 25, 2010 at 11:24 AM, Darin Fisher <darin@chromium.org> wrote:
> On Sat, Sep 25, 2010 at 8:21 AM, <darin@chromium.org> wrote:
>>
>> http://codereview.chromium.org/3388013/diff/15001/16004
>> File chrome/renderer/render_widget.cc (right):
>>
>> http://codereview.chromium.org/3388013/diff/15001/16004#newcode360
>> chrome/renderer/render_widget.cc:360: if
>> (WebInputEvent::isTouchEventType(input_event->type)) {
>> why is this code here instead of inside the
>> WebViewImpl::handleInputEvent function?
>>
>> if you put the code in handleInputEvent, then you can write layout tests
>> for this feature more easily by using the eventSender
>
> also, I recall objecting to the addition of WebWidget::gestureScroll.
>  GestureManager, which is the caller of gestureScroll, could easily live in
> WebKit.

Sorry for pestering you with this CL. I managed to misunderstand the
degree of your objection in the bugzilla comment. I agree though:
architecturally the GestureManager is much better living in WebKit.
I'll revise the WebKit CL shortly.

Rob.

> -Darin
>
>>
>> http://codereview.chromium.org/3388013/diff/15001/16005
>> File chrome/renderer/render_widget.h (right):
>>
>> http://codereview.chromium.org/3388013/diff/15001/16005#newcode355
>> chrome/renderer/render_widget.h:355: // A cached pointer to the
>> GestureManager.
>> why cache this here?  GestureManager::Get() is fast.
>>
>> http://codereview.chromium.org/3388013/show
>
>

[darin (slow to review), 2010-09-27 17:27:52]

On Mon, Sep 27, 2010 at 10:19 AM, Robert Kroeger <rjkroege@chromium.org>wrote:

> On Sat, Sep 25, 2010 at 11:24 AM, Darin Fisher <darin@chromium.org> wrote:
> > On Sat, Sep 25, 2010 at 8:21 AM, <darin@chromium.org> wrote:
> >>
> >> http://codereview.chromium.org/3388013/diff/15001/16004
> >> File chrome/renderer/render_widget.cc (right):
> >>
> >> http://codereview.chromium.org/3388013/diff/15001/16004#newcode360
> >> chrome/renderer/render_widget.cc:360: if
> >> (WebInputEvent::isTouchEventType(input_event->type)) {
> >> why is this code here instead of inside the
> >> WebViewImpl::handleInputEvent function?
> >>
> >> if you put the code in handleInputEvent, then you can write layout tests
> >> for this feature more easily by using the eventSender
> >
> > also, I recall objecting to the addition of WebWidget::gestureScroll.
> >  GestureManager, which is the caller of gestureScroll, could easily live
> in
> > WebKit.
>
> Sorry for pestering you with this CL. I managed to misunderstand the
> degree of your objection in the bugzilla comment. I agree though:
> architecturally the GestureManager is much better living in WebKit.
> I'll revise the WebKit CL shortly.
>
> Rob.
>

Thanks!




>
> > -Darin
> >
> >>
> >> http://codereview.chromium.org/3388013/diff/15001/16005
> >> File chrome/renderer/render_widget.h (right):
> >>
> >> http://codereview.chromium.org/3388013/diff/15001/16005#newcode355
> >> chrome/renderer/render_widget.h:355: // A cached pointer to the
> >> GestureManager.
> >> why cache this here?  GestureManager::Get() is fast.
> >>
> >> http://codereview.chromium.org/3388013/show
> >
> >
>