Corrected issues with queued touch events. This fixes gestures in Windows.

Corrected issues with queued touch events.  This fixes gestures in Windows.

The underlying issue is that the GR expects individual touch events (one touch per event) while javascript expects composite events (containing information on each touch point in a single event.)

BUG=130205
TEST=


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

[sadrul, 2012-07-05 17:00:53]

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

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:542:
touch_count_.push( touch_event_.data().touchesLength);
no space after (

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:548: int
get_touch_count() {
Perhaps GetNextTouchCount would be a better name for this.

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:572: //
(ForwardTouchEvent) takes a set of touch events. _touchCount_ tracks how
|touch_count_| instead of _touchCount_

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2317:
touch_state_->QueueEvents(this,gesture_recognizer_.get());
space after ,

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2320: const
LocalTouchEvent *touch_event = touch_state_->ui_touch_event();
'LocatedTouchEvent* touch_event'

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2325:
this->ProcessGestures(gestures.get());
Is this |this->| bit necessary here?

http://codereview.chromium.org/10702096/diff/2001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10702096/diff/2001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:239: DCHECK(!new_point->in_use());
lines 238/239 are checking the same thing?

[girard, 2012-07-05 18:43:44]

Thanks for the review, sadrul.  New patch uploaded.

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

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:542:
touch_count_.push( touch_event_.data().touchesLength);
On 2012/07/05 17:00:54, sadrul wrote:
> no space after (

Done.

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:548: int
get_touch_count() {
On 2012/07/05 17:00:54, sadrul wrote:
> Perhaps GetNextTouchCount would be a better name for this.

Done.

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:572: //
(ForwardTouchEvent) takes a set of touch events. _touchCount_ tracks how
On 2012/07/05 17:00:54, sadrul wrote:
> |touch_count_| instead of _touchCount_

Done.  Thanks.

http://codereview.chromium.org/10702096/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2325:
this->ProcessGestures(gestures.get());
On 2012/07/05 17:00:54, sadrul wrote:
> Is this |this->| bit necessary here?

Nope. Removed.

http://codereview.chromium.org/10702096/diff/2001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10702096/diff/2001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:239: DCHECK(!new_point->in_use());
On 2012/07/05 17:00:54, sadrul wrote:
> lines 238/239 are checking the same thing?
Yes.  Since the rest of the code uses the new_point variable, I thought the
DCHECK should use it too.

[sadrul, 2012-07-09 07:12:06]

http://codereview.chromium.org/10702096/diff/2001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10702096/diff/2001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:239: DCHECK(!new_point->in_use());
On 2012/07/05 18:43:44, girard wrote:
> On 2012/07/05 17:00:54, sadrul wrote:
> > lines 238/239 are checking the same thing?
> Yes.  Since the rest of the code uses the new_point variable, I thought the
> DCHECK should use it too.

Ah, in that case, remove line 238?

[sadrul, 2012-07-11 15:39:07]

+Ben for the windows related code (since sky@ is OOO)

The change in RenderWidgetHostViewWin looks mostly reasonable.

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

http://codereview.chromium.org/10702096/diff/7001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1231: int
touchCount = touch_state_->GetNextTouchCount();
touch_count

http://codereview.chromium.org/10702096/diff/7001/ui/views/widget/native_widg...
File ui/views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/10702096/diff/7001/ui/views/widget/native_widg...
ui/views/widget/native_widget_win.cc:1486: return 0;
Perhaps you didn't mean this code to be included in this patch?

[Ben Goodger (Google), 2012-07-11 16:12:58]

lgtm

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

http://codereview.chromium.org/10702096/diff/1009/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2320: const
LocalTouchEvent *touch_event = touch_state_->ui_touch_event();
LocalTouchEvent*

[commit-bot: I haz the power, 2012-07-11 16:53:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/10702096/12001

[commit-bot: I haz the power, 2012-07-11 17:51:21]

Try job failure for 10702096-12001 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-11 18:26:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/10702096/6004

[commit-bot: I haz the power, 2012-07-11 19:55:05]

Change committed as 146180

[girard, 2012-07-26 12:36:43]

https://chromiumcodereview.appspot.com/10702096/diff/2001/ui/base/gestures/ge...
File ui/base/gestures/gesture_sequence.cc (right):

https://chromiumcodereview.appspot.com/10702096/diff/2001/ui/base/gestures/ge...
ui/base/gestures/gesture_sequence.cc:239: DCHECK(!new_point->in_use());
On 2012/07/09 07:12:06, sadrul wrote:
> On 2012/07/05 18:43:44, girard wrote:
> > On 2012/07/05 17:00:54, sadrul wrote:
> > > lines 238/239 are checking the same thing?
> > Yes.  Since the rest of the code uses the new_point variable, I thought the
> > DCHECK should use it too.
> 
> Ah, in that case, remove line 238?

Done.