Addressing sadruls comments from the previous CL https://codereview.chromium.org/22865036/ which was

Addressing sadruls comments from the previous CL https://codereview.chromium.org/22865036/ which was 
already committed.

Changes as below:-
1. We dispatch the GESTURE_BEGIN event before the TAP_DOWN in the repost code as some recipients may rely on
   that behavior.
2. A new test GestureRepostEventOrder has been added to validate that we receive reposted and subsequent events
   in the correct order.

BUG=277018
R=sadrul@chromium.org

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

[sadrul, 2013-08-23 15:00:06]

LGTM. Thanks!

In the test, since the EventFilter is added on the RootWindow, we need to verify
that the target of the event is correct. I have left couple of comments for that
inline:

https://codereview.chromium.org/23202016/diff/10001/ui/aura/root_window_unitt...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/10001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:927: events().clear();
We should verify that repost_target_ is not already the target:
 EXPECT_NE(repost_target_, event->target());

https://codereview.chromium.org/23202016/diff/10001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:931: }
We should also add:
  if (reposted_)
    EXPECT_EQ(repost_target_, event->target()) << event->name();

[ananta, 2013-08-23 20:27:32]

https://codereview.chromium.org/23202016/diff/10001/ui/aura/root_window_unitt...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/10001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:927: events().clear();
On 2013/08/23 15:00:06, sadrul wrote:
> We should verify that repost_target_ is not already the target:
>  EXPECT_NE(repost_target_, event->target());

Done.

https://codereview.chromium.org/23202016/diff/10001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:931: }
On 2013/08/23 15:00:06, sadrul wrote:
> We should also add:
>   if (reposted_)
>     EXPECT_EQ(repost_target_, event->target()) << event->name();

I had to redo the test as we need different root windows to ensure
that the gesture event is posted correctly to the target.

PTAL.

[sadrul, 2013-08-26 17:07:18]

https://codereview.chromium.org/23202016/diff/5001/ui/aura/root_window_unitte...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/5001/ui/aura/root_window_unitte...
ui/aura/root_window_unittest.cc:972: // We create two windows in different root
window hierarchies. The
Can you explain why it is necessary to create two root windows here? Because
reposting should work OK in the same root-window.

[ananta, 2013-08-26 19:19:02]

https://codereview.chromium.org/23202016/diff/5001/ui/aura/root_window_unitte...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/5001/ui/aura/root_window_unitte...
ui/aura/root_window_unittest.cc:972: // We create two windows in different root
window hierarchies. The
On 2013/08/26 17:07:18, sadrul wrote:
> Can you explain why it is necessary to create two root windows here? Because
> reposting should work OK in the same root-window.

As per our discussion, changed the test to only use one root window. I added
code to remove the repost source window after we repost the gesture event.

[sadrul, 2013-08-26 19:52:50]

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:917: explicit
RepostGestureEventRecorder(aura::Window* repost_source,
remove 'explicit'

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:935:
EXPECT_EQ(static_cast<aura::Window*>(event->target()), repost_target_);
This should be outside of the == TAP_DOWN check in line 924. i.e.

  virtual void ... {
    EXPECT_EQ(reposted_ ? repost_target_ : repost_source_, event->target());

    if (event->type() == TAP_DOWN) {
      .. repost
    }

    EFR::OnGestureEvent(event);
  }

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:986: DLOG(INFO) << "Event types: " <<
event_string;
We shouldn't add this in the log.

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:996: EXPECT_EQ(tap_down_count, 2);
Why did we switch from checking the sequence of events in an earlier patch set
to counting just the number of tap-downs? Checking the sequence of events would
be better. (also, if you change the test-expectations between patchsets, please
add a comment about it, preferably explaining the reason for the change).

Changing the target of gestures in the middle of a gesture-sequence can be
problematic (e.g. when doing captures, etc.). So I want to make sure when we
repost an event, with the specific intention of changing the target of the
gesture in the process, the event-targetting of all the subsequent
touch/gesture-events in the same gesture-sequence is done correctly. Simply
counting the number of tap-downs, or checking the target of tap-down gesture
does not accomplish that.

[ananta, 2013-08-26 21:23:16]

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:917: explicit
RepostGestureEventRecorder(aura::Window* repost_source,
On 2013/08/26 19:52:50, sadrul wrote:
> remove 'explicit'

Done.

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:935:
EXPECT_EQ(static_cast<aura::Window*>(event->target()), repost_target_);
On 2013/08/26 19:52:50, sadrul wrote:
> This should be outside of the == TAP_DOWN check in line 924. i.e.
> 
>   virtual void ... {
>     EXPECT_EQ(reposted_ ? repost_target_ : repost_source_, event->target());
> 
>     if (event->type() == TAP_DOWN) {
>       .. repost
>     }
> 
>     EFR::OnGestureEvent(event);
>   }

Done.

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:986: DLOG(INFO) << "Event types: " <<
event_string;
On 2013/08/26 19:52:50, sadrul wrote:
> We shouldn't add this in the log.

Done.

https://codereview.chromium.org/23202016/diff/79001/ui/aura/root_window_unitt...
ui/aura/root_window_unittest.cc:996: EXPECT_EQ(tap_down_count, 2);
On 2013/08/26 19:52:50, sadrul wrote:
> Why did we switch from checking the sequence of events in an earlier patch set
> to counting just the number of tap-downs? Checking the sequence of events
would
> be better. (also, if you change the test-expectations between patchsets,
please
> add a comment about it, preferably explaining the reason for the change).
> 
> Changing the target of gestures in the middle of a gesture-sequence can be
> problematic (e.g. when doing captures, etc.). So I want to make sure when we
> repost an event, with the specific intention of changing the target of the
> gesture in the process, the event-targetting of all the subsequent
> touch/gesture-events in the same gesture-sequence is done correctly. Simply
> counting the number of tap-downs, or checking the target of tap-down gesture
> does not accomplish that.

I removed that with a comment in the gcl upload indicating that I did that
because it looked more readable. Sadly the comment did not make it through. 

The event list check has been restored.

PTAL

[sadrul, 2013-08-28 15:16:59]

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:922: 
You should add a virtual destructor here too.

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:943: return;
Don't need this.

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:963: ui::ET_TOUCH_RELEASED,
Do you know why we don't get a GESTURE_END here?

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:1025: repost_event_recorder->events().size()),
0);
Please go back to using EventTypeToString (it'd be easier to understand what
went wrong when the test fails with string outputs).

[ananta, 2013-08-28 18:39:10]

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:922: 
On 2013/08/28 15:16:59, sadrul wrote:
> You should add a virtual destructor here too.

Done.

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:943: return;
On 2013/08/28 15:16:59, sadrul wrote:
> Don't need this.

Done.

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:963: ui::ET_TOUCH_RELEASED,
On 2013/08/28 15:16:59, sadrul wrote:
> Do you know why we don't get a GESTURE_END here?

If you look at the RootWindow::ProcessGestures function, there is this
line:-
if (event_dispatch_target_ != target)  // |target| has been destroyed
  break;
The event_dispatch_target_ member is set to NULL because the
RepostGestureEventRecorder::OnGestureEvent function after reposting the tap down
gesture, removes the repost_source_ window from the root window.

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:1025: repost_event_recorder->events().size()),
0);
On 2013/08/28 15:16:59, sadrul wrote:
> Please go back to using EventTypeToString (it'd be easier to understand what
> went wrong when the test fails with string outputs).

Done.

[sadrul, 2013-08-28 18:44:43]

LGTM

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
File ui/aura/root_window_unittest.cc (right):

https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
ui/aura/root_window_unittest.cc:963: ui::ET_TOUCH_RELEASED,
On 2013/08/28 18:39:10, ananta wrote:
> On 2013/08/28 15:16:59, sadrul wrote:
> > Do you know why we don't get a GESTURE_END here?
> 
> If you look at the RootWindow::ProcessGestures function, there is this
> line:-
> if (event_dispatch_target_ != target)  // |target| has been destroyed
>   break;
> The event_dispatch_target_ member is set to NULL because the
> RepostGestureEventRecorder::OnGestureEvent function after reposting the tap
down
> gesture, removes the repost_source_ window from the root window.
> 
The first window is destroyed/removed from the RootWindow in response to the
touch-press/tap-down event. And the second window does receive the
TOUCH_RELEASED event. It's curious that it doesn't receive the GESTURE_END event
(which is generated from the TOUCH_RELEASED event). It'd be good to understand
why.

[ananta, 2013-08-28 19:10:28]

On 2013/08/28 18:44:43, sadrul wrote:
> LGTM
> 
>
https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
> File ui/aura/root_window_unittest.cc (right):
> 
>
https://codereview.chromium.org/23202016/diff/112001/ui/aura/root_window_unit...
> ui/aura/root_window_unittest.cc:963: ui::ET_TOUCH_RELEASED,
> On 2013/08/28 18:39:10, ananta wrote:
> > On 2013/08/28 15:16:59, sadrul wrote:
> > > Do you know why we don't get a GESTURE_END here?
> > 
> > If you look at the RootWindow::ProcessGestures function, there is this
> > line:-
> > if (event_dispatch_target_ != target)  // |target| has been destroyed
> >   break;
> > The event_dispatch_target_ member is set to NULL because the
> > RepostGestureEventRecorder::OnGestureEvent function after reposting the tap
> down
> > gesture, removes the repost_source_ window from the root window.
> > 
> The first window is destroyed/removed from the RootWindow in response to the
> touch-press/tap-down event. And the second window does receive the
> TOUCH_RELEASED event. It's curious that it doesn't receive the GESTURE_END
event
> (which is generated from the TOUCH_RELEASED event). It'd be good to understand
> why.

It is because we directly post the GESTURE_BEGIN  and GESTURE_TAP_DOWN events to
the new window.
The gesture sequence for this window is still in the gs_no_gesture state which
causes it
to not receive GESTURE_END when the TOUCH_RELEASED event is processed. Should we
post the GESTURE_END
to the new target on the same lines as GESTURE_BEGIN



d

[ananta, 2013-08-30 23:31:39]

Committed patchset #14 manually as r220693 (presubmit successful).