Forward Windows touch messages to the renderer

Forward Windows touch messages to the renderer

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

[jschuh, 2011-11-05 16:16:03]

I probably have some nits to fix, but figured I'd get it in your review queue.

[cpu_(ooo_6.6-7.5), 2011-11-08 02:34:04]

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:898:
DCHECK(RegisterTouchWindow(m_hWnd, 0));
bug: on release RTE is not called.

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:900:
sizeof(GESTURECONFIG))) {
886 to 893 should be moved inside this block and there is a redundant if
(base::win::GetVersion()

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.h:344: void
FinishTouchEvent();
You are going to hate me but I have the urge to tell you that there is a little
class that is screaming to be born. I'll let you name it but here is what it
look s like to me:

class Fooo {
public:
  AddTouchPoint(...)
  UpdateTouchPoint(..)
  FinishTouchEvent(..)
private:
  WebKit::WebTouchEvent touch_event_;
}

[jschuh, 2011-11-08 17:12:41]

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:898:
DCHECK(RegisterTouchWindow(m_hWnd, 0));
On 2011/11/08 02:34:04, cpu wrote:
> bug: on release RTE is not called.

I'm confused. Are you saying I need a call to UnregisterTouchWindow in the
OnDestroy handler?

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:900:
sizeof(GESTURECONFIG))) {
On 2011/11/08 02:34:04, cpu wrote:
> 886 to 893 should be moved inside this block and there is a redundant if
> (base::win::GetVersion()

Ugh, merge noise. Sorry, I should have caught that.

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.h:344: void
FinishTouchEvent();
On 2011/11/08 02:34:04, cpu wrote:
> You are going to hate me but I have the urge to tell you that there is a
little
> class that is screaming to be born. I'll let you name it but here is what it
> look s like to me:
> 
> class Fooo {
> public:
>   AddTouchPoint(...)
>   UpdateTouchPoint(..)
>   FinishTouchEvent(..)
> private:
>   WebKit::WebTouchEvent touch_event_;
> }

Yeah, I was thinking about that but I anticipate significantly changing
WebTouchEvent class in the near future. Also, I didn't want to dupe the hwnd
into the class (but that's not a big deal). I think it would be better to factor
it all out at once, but I can do it now if you think it's better.

[cpu_(ooo_6.6-7.5), 2011-11-08 19:20:14]

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:898:
DCHECK(RegisterTouchWindow(m_hWnd, 0));
On 2011/11/08 17:12:41, Justin Schuh wrote:
> On 2011/11/08 02:34:04, cpu wrote:
> > bug: on release RTE is not called.
> 
> I'm confused. Are you saying I need a call to UnregisterTouchWindow in the
> OnDestroy handler?

No. release build. dcheck goes *puff*

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/8341125/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.h:344: void
FinishTouchEvent();
I think now it's a good time. If you want you can put the class declaration in
this file and the impl in the .cc

Mostly what I want is logical separation.


On 2011/11/08 17:12:41, Justin Schuh wrote:
> On 2011/11/08 02:34:04, cpu wrote:
> > You are going to hate me but I have the urge to tell you that there is a
> little
> > class that is screaming to be born. I'll let you name it but here is what it
> > look s like to me:
> > 
> > class Fooo {
> > public:
> >   AddTouchPoint(...)
> >   UpdateTouchPoint(..)
> >   FinishTouchEvent(..)
> > private:
> >   WebKit::WebTouchEvent touch_event_;
> > }
> 
> Yeah, I was thinking about that but I anticipate significantly changing
> WebTouchEvent class in the near future. Also, I didn't want to dupe the hwnd
> into the class (but that's not a big deal). I think it would be better to
factor
> it all out at once, but I can do it now if you think it's better.

[ananta, 2011-11-08 20:00:25]

Looks good.

http://codereview.chromium.org/8341125/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8341125/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1585: void
RenderWidgetHostViewWin::FinishTouchEvent() {
+1 for moving this inside a class and into a separate file.

http://codereview.chromium.org/8341125/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1657: if
(points_count > WebKit::WebTouchEvent::touchesLengthCap)
The std::min statement above should ensure that this never happens?

http://codereview.chromium.org/8341125/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1678: if (point &&
(points[i].dwFlags & 0x7) == TOUCHEVENTF_DOWN)
Can you add a define for the 0x7 mask?

[jschuh, 2011-11-09 19:53:25]

Okay, fixed all the nits and created a contained class, but I also a made a more
significant change. It turns out that I was over coalescing touches, because
touches of different type (down vs. up) cannot be sent in the same event. So,
the new code iterates through the touch input array and sends a message only for
each contiguous group of the same event type.

On 2011/11/08 20:00:25, ananta wrote:
> Looks good.
> 
>
http://codereview.chromium.org/8341125/diff/17001/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_view_win.cc (right):
> 
>
http://codereview.chromium.org/8341125/diff/17001/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_win.cc:1585: void
> RenderWidgetHostViewWin::FinishTouchEvent() {
> +1 for moving this inside a class and into a separate file.
> 
>
http://codereview.chromium.org/8341125/diff/17001/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_win.cc:1657: if
> (points_count > WebKit::WebTouchEvent::touchesLengthCap)
> The std::min statement above should ensure that this never happens?
> 
>
http://codereview.chromium.org/8341125/diff/17001/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_win.cc:1678: if (point
&&
> (points[i].dwFlags & 0x7) == TOUCHEVENTF_DOWN)
> Can you add a define for the 0x7 mask?

[ananta, 2011-11-10 02:15:23]

http://codereview.chromium.org/8341125/diff/30001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8341125/diff/30001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1602:
WebKit::WebTouchPoint* point = touch_event_.touches;
Can you add some comments here explaining the logic?. It would be great if you
can explain the whole function.

[jschuh, 2011-11-10 21:29:10]

Now with better comments.

http://codereview.chromium.org/8341125/diff/30001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8341125/diff/30001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1602:
WebKit::WebTouchPoint* point = touch_event_.touches;
On 2011/11/10 02:15:24, ananta wrote:
> Can you add some comments here explaining the logic?. It would be great if you
> can explain the whole function.
> 

Done.

[jschuh, 2011-11-10 22:15:11]

@jam - Adding you in for owners review now that the main review is finishing up.

[jam, 2011-11-10 22:18:30]

lgtm

[ananta, 2011-11-10 22:31:14]

LGTM

http://codereview.chromium.org/8341125/diff/38001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8341125/diff/38001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:295: inline int
GetTouchType(const TOUCHINPUT& point) {
These functions can be moved to the new class?

[commit-bot: I haz the power, 2011-11-11 06:32:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/8341125/38001

[commit-bot: I haz the power, 2011-11-11 07:41:05]

Change committed as 109594