Fix the wrong relative coordinates from EventDispatch

Fix the wrong relative coordinates from EventDispatch

The problem is that the event's coordrinates are moved to the
negative location when testing MouseLeaveTest.MouseDownOnBrowserCaption
in interactive_ui_tests. Becasue it tries to calculate the relative
coordinates one more in WindowEventDispatcher::DispatchMouseEnterOrExit
if target window is null.
We should use the target window from PreDispatchMouseEvent to
prevent that window is null.

BUG=409601
R=sky@chromium.org
TEST=WindowEventDispatcherTest.MouseEventWithoutTargetWindow

Committed: https://crrev.com/50bcf1b20f123424d2a176260b61881c55c19dc7
Cr-Commit-Position: refs/heads/master@{#310445}

[Miyoung Shin, 2014-11-25 01:57:49]

myid.shin@samsung.com changed reviewers:
+ avi@chromium.org

[Miyoung Shin, 2014-11-25 01:58:12]

avi@, PTAL

[Avi (use Gerrit), 2014-11-25 04:51:39]

lgtm

[Miyoung Shin, 2014-11-25 05:02:07]

The CQ bit was checked by myid.shin@samsung.com

[commit-bot: I haz the power, 2014-11-25 05:02:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/754013007/1

[commit-bot: I haz the power, 2014-11-25 05:06:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 05:06:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Miyoung Shin, 2014-11-25 05:14:55]

On 2014/11/25 05:06:40, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Avi@, Do I need another reviewer ?

[Miyoung Shin, 2014-11-25 05:20:47]

myid.shin@samsung.com changed reviewers:
+ sky@chromium.org

[Avi (use Gerrit), 2014-11-25 05:24:55]

On 2014/11/25 05:14:55, Miyoung Shin wrote:
> On 2014/11/25 05:06:40, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Avi@, Do I need another reviewer ?

Yes. Apparently I am an OWNER of chrome/test, but not of the general chrome/
directory. You need an OK from someone who owns chrome/browser.

http://www.chromium.org/developers/owners-files

[Miyoung Shin(g), 2014-11-25 05:31:45]

On 2014/11/25 05:24:55, Avi wrote:
> On 2014/11/25 05:14:55, Miyoung Shin wrote:
> > On 2014/11/25 05:06:40, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   chromium_presubmit on tryserver.chromium.linux
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > 
> > Avi@, Do I need another reviewer ?
> 
> Yes. Apparently I am an OWNER of chrome/test, but not of the general chrome/
> directory. You need an OK from someone who owns chrome/browser.
> 
> http://www.chromium.org/developers/owners-files

Avi@, Thanks, I've add sky@ for reviewer.
sky@, PTAL.

[Miyoung Shin, 2014-12-01 06:55:36]

sky@, PTAL.

[sky, 2014-12-01 17:28:28]

https://codereview.chromium.org/754013007/diff/1/chrome/browser/mouseleave_br...
File chrome/browser/mouseleave_browsertest.cc (right):

https://codereview.chromium.org/754013007/diff/1/chrome/browser/mouseleave_br...
chrome/browser/mouseleave_browsertest.cc:32: tab_view_bounds.y() + 200);
I'm not familiar with this test and it has a lot of magic numbers that aren't
validated in anyway. There should be comments here describing why 200 is better
than 10 and why 200 is right. I would also thing there is EXPECTS that 200 is
smaller than tab_view_bounds.

[Miyoung Shin, 2014-12-02 02:07:20]

On 2014/12/01 17:28:28, sky wrote:
>
https://codereview.chromium.org/754013007/diff/1/chrome/browser/mouseleave_br...
> File chrome/browser/mouseleave_browsertest.cc (right):
> 
>
https://codereview.chromium.org/754013007/diff/1/chrome/browser/mouseleave_br...
> chrome/browser/mouseleave_browsertest.cc:32: tab_view_bounds.y() + 200);
> I'm not familiar with this test and it has a lot of magic numbers that aren't
> validated in anyway. There should be comments here describing why 200 is
better
> than 10 and why 200 is right. I would also thing there is EXPECTS that 200 is
> smaller than tab_view_bounds.

sky@, I've upload the patch to add the comment in my patch.
and I changed the position to the center instead of using +200.

_______________________________
| tab ui                      | 
|-----------------------------|      <--- tab_view_bounds.y() 
| some notification UI        | 
|-----------------------------|
|                             |
|                             |                    
|     content area(DIV 80%)   |      <--- + height/2
|                             |
|                             |
|_____________________________|

[sky, 2014-12-02 15:50:30]

I'm confused then. tab->GetContainerBounds() should the return
coordinates in the content area. So the +10 that was there before
should be fine. When this fails, what is the mouse over?

On Mon, Dec 1, 2014 at 6:07 PM,  <myid.shin@samsung.com> wrote:
> On 2014/12/01 17:28:28, sky wrote:
>
>
https://codereview.chromium.org/754013007/diff/1/chrome/browser/mouseleave_br...
>>
>> File chrome/browser/mouseleave_browsertest.cc (right):
>
>
>
>
https://codereview.chromium.org/754013007/diff/1/chrome/browser/mouseleave_br...
>>
>> chrome/browser/mouseleave_browsertest.cc:32: tab_view_bounds.y() + 200);
>> I'm not familiar with this test and it has a lot of magic numbers that
>> aren't
>> validated in anyway. There should be comments here describing why 200 is
>
> better
>>
>> than 10 and why 200 is right. I would also thing there is EXPECTS that 200
>> is
>> smaller than tab_view_bounds.
>
>
> sky@, I've upload the patch to add the comment in my patch.
> and I changed the position to the center instead of using +200.
>
> _______________________________
> | tab ui                      |
> |-----------------------------|      <--- tab_view_bounds.y()
> | some notification UI        |
> |-----------------------------|
> |                             |
> |                             |
> |     content area(DIV 80%)   |      <--- + height/2
> |                             |
> |                             |
> |_____________________________|
>
> https://codereview.chromium.org/754013007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miyoung Shin, 2014-12-02 23:31:23]

On 2014/12/02 15:50:30, sky wrote:
> I'm confused then. tab->GetContainerBounds() should the return
> coordinates in the content area. So the +10 that was there before
> should be fine. When this fails, what is the mouse over?

Actually, it's happened on only window. Some graphic layer for the native UI in
window affects the mouse-move event coordinates and Blink(Webkit) gets the
negative coordinates of the mouse-move event.
so I had guessed it would be notification UI. but I'm not sure because I could
see only a black screen when I run MouseLeaveTest.MouseDownOnBrowserCaption. if
I run mouseleave.html on chrome-window, it works fine.

[sky, 2014-12-02 23:46:54]

Seems to me you don't have a good grasp on the bug and are working
around it by fiddling with coordinates. That isn't the right way to
fix this.

  -Scott

On Tue, Dec 2, 2014 at 3:31 PM,  <myid.shin@samsung.com> wrote:
> On 2014/12/02 15:50:30, sky wrote:
>>
>> I'm confused then. tab->GetContainerBounds() should the return
>> coordinates in the content area. So the +10 that was there before
>> should be fine. When this fails, what is the mouse over?
>
>
> Actually, it's happened on only window. Some graphic layer for the native UI
> in
> window affects the mouse-move event coordinates and Blink(Webkit) gets the
> negative coordinates of the mouse-move event.
> so I had guessed it would be notification UI. but I'm not sure because I
> could
> see only a black screen when I run MouseLeaveTest.MouseDownOnBrowserCaption.
> if
> I run mouseleave.html on chrome-window, it works fine.
>
>
> https://codereview.chromium.org/754013007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miyoung Shin, 2014-12-03 01:25:56]

On 2014/12/02 23:46:54, sky wrote:
> Seems to me you don't have a good grasp on the bug and are working
> around it by fiddling with coordinates. That isn't the right way to
> fix this.
> 
>   -Scott

I got your point. I will check it in more detail and update it.

[Miyoung Shin, 2014-12-04 01:44:13]

On 2014/12/03 01:25:56, Miyoung Shin wrote:
> On 2014/12/02 23:46:54, sky wrote:
> > Seems to me you don't have a good grasp on the bug and are working
> > around it by fiddling with coordinates. That isn't the right way to
> > fix this.
> > 
> >   -Scott
> 
> I got your point. I will check it in more detail and update it.

sky@,
I'm not familiar with the event process on Window.
So, I'd like to get your advice about this issue.

This is my debugging contents and function call flow.

EventProcessor::OnEventFromSource - 
{
....
  target = targeter->FindTargetForEvent(root, event_to_dispatch);  // during
calling FindTargetForEvent to find target, the event's coordinates moves to the
relative location from target.
}

==> ..... ==>

void WindowEventDispatcher::PreDispatchMouseEvent(Window* target,      // target
is here.
                                                  ui::MouseEvent* event) 
{
    case ui::ET_MOUSE_MOVED:
        mouse_moved_handler_ = target;
        details = DispatchMouseEnterOrExit(*event, ui::ET_MOUSE_ENTERED);
}

==>

ui::EventDispatchDetails WindowEventDispatcher::DispatchMouseEnterOrExit(
{
....
  aura::Window* target = static_cast<Window*>(event.target());
  if (!target)
    target = window();                                          //
event.target() is null, and target is updated to the root-window.
  ui::MouseEvent translated_event(event,
                                  target,
                                  mouse_moved_handler_,
                                  type,
                                  event.flags() | ui::EF_IS_SYNTHESIZED); // the
event's coordinates moves to the negative location because target is root-window
.
}

So, I think if event.target() is null them target should be gotten from previous
function, PreDispatchMouseEvent(), instead of event.target().

Could you give me any comment about it ?

[sky, 2014-12-04 04:24:43]

What are the coordinates of the mouse at the time it enters HWNDMessageHandler?

On Wed, Dec 3, 2014 at 5:44 PM,  <myid.shin@samsung.com> wrote:
> On 2014/12/03 01:25:56, Miyoung Shin wrote:
>>
>> On 2014/12/02 23:46:54, sky wrote:
>> > Seems to me you don't have a good grasp on the bug and are working
>> > around it by fiddling with coordinates. That isn't the right way to
>> > fix this.
>> >
>> >   -Scott
>
>
>> I got your point. I will check it in more detail and update it.
>
>
> sky@,
> I'm not familiar with the event process on Window.
> So, I'd like to get your advice about this issue.
>
> This is my debugging contents and function call flow.
>
> EventProcessor::OnEventFromSource -
> {
> ....
>   target = targeter->FindTargetForEvent(root, event_to_dispatch);  // during
> calling FindTargetForEvent to find target, the event's coordinates moves to
> the
> relative location from target.
> }
>
> ==> ..... ==>
>
> void WindowEventDispatcher::PreDispatchMouseEvent(Window* target,      //
> target
> is here.
>                                                   ui::MouseEvent* event)
> {
>     case ui::ET_MOUSE_MOVED:
>         mouse_moved_handler_ = target;
>         details = DispatchMouseEnterOrExit(*event, ui::ET_MOUSE_ENTERED);
> }
>
> ==>
>
> ui::EventDispatchDetails WindowEventDispatcher::DispatchMouseEnterOrExit(
> {
> ....
>   aura::Window* target = static_cast<Window*>(event.target());
>   if (!target)
>     target = window();                                          //
> event.target() is null, and target is updated to the root-window.
>   ui::MouseEvent translated_event(event,
>                                   target,
>                                   mouse_moved_handler_,
>                                   type,
>                                   event.flags() | ui::EF_IS_SYNTHESIZED); //
> the
> event's coordinates moves to the negative location because target is
> root-window
> .
> }
>
> So, I think if event.target() is null them target should be gotten from
> previous
> function, PreDispatchMouseEvent(), instead of event.target().
>
> Could you give me any comment about it ?
>
> https://codereview.chromium.org/754013007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miyoung Shin, 2014-12-04 06:06:25]

On 2014/12/04 04:24:43, sky wrote:
> What are the coordinates of the mouse at the time it enters
HWNDMessageHandler?

at that time, the coordinates of the mouse are same with
EventProcessor::OnEventFromSource.
I mean they are the relative location from top-window.

HWNDMessageHandler -  the relative coordinates from top-window
....
EventSource::SendEventToProcessor - the relative coordinates from top-window
EventProcessor::OnEventFromSource - move the relative coordinates based on
target-window(content) in this function.
....
WindowEventDispatcher::PreDispatchMouseEvent - the relative coordinates from
target-window
WindowEventDispatcher::DispatchMouseEnterOrExit - move the relative coordinates
based on root-window again and they become the negative coordinates.

[sky, 2014-12-04 15:58:29]

On Wed, Dec 3, 2014 at 10:06 PM,  <myid.shin@samsung.com> wrote:
> On 2014/12/04 04:24:43, sky wrote:
>>
>> What are the coordinates of the mouse at the time it enters
>
> HWNDMessageHandler?
>
> at that time, the coordinates of the mouse are same with
> EventProcessor::OnEventFromSource.
> I mean they are the relative location from top-window.

My question is more of do the coordinates look correct at the the time
they enter HWNDMessageHandler? That is, does it look like the
coordinates are over the webcontents?

  -Scott

>
> HWNDMessageHandler -  the relative coordinates from top-window
> ....
> EventSource::SendEventToProcessor - the relative coordinates from top-window
> EventProcessor::OnEventFromSource - move the relative coordinates based on
> target-window(content) in this function.
> ....
> WindowEventDispatcher::PreDispatchMouseEvent - the relative coordinates from
> target-window
> WindowEventDispatcher::DispatchMouseEnterOrExit - move the relative
> coordinates
> based on root-window again and they become the negative coordinates.
>
> https://codereview.chromium.org/754013007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miyoung Shin, 2014-12-04 22:03:58]

On 2014/12/04 15:58:29, sky wrote:
> On Wed, Dec 3, 2014 at 10:06 PM,  <mailto:myid.shin@samsung.com> wrote:
> > On 2014/12/04 04:24:43, sky wrote:
> >>
> >> What are the coordinates of the mouse at the time it enters
> >
> > HWNDMessageHandler?
> >
> > at that time, the coordinates of the mouse are same with
> > EventProcessor::OnEventFromSource.
> > I mean they are the relative location from top-window.
> 
> My question is more of do the coordinates look correct at the the time
> they enter HWNDMessageHandler? That is, does it look like the
> coordinates are over the webcontents?
> 
Yes, I can make a guess that coordinates of the mouse are over the wbecontents
though I could see a black screen.

[sky, 2014-12-05 00:02:44]

There's a switch to not see black, --enable-pixel-output-in-tests .

On Thu, Dec 4, 2014 at 2:03 PM,  <myid.shin@samsung.com> wrote:
> On 2014/12/04 15:58:29, sky wrote:
>>
>> On Wed, Dec 3, 2014 at 10:06 PM,  <mailto:myid.shin@samsung.com> wrote:
>> > On 2014/12/04 04:24:43, sky wrote:
>> >>
>> >> What are the coordinates of the mouse at the time it enters
>> >
>> > HWNDMessageHandler?
>> >
>> > at that time, the coordinates of the mouse are same with
>> > EventProcessor::OnEventFromSource.
>> > I mean they are the relative location from top-window.
>
>
>> My question is more of do the coordinates look correct at the the time
>> they enter HWNDMessageHandler? That is, does it look like the
>> coordinates are over the webcontents?
>
>
> Yes, I can make a guess that coordinates of the mouse are over the
> wbecontents
> though I could see a black screen.
>
> https://codereview.chromium.org/754013007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miyoung Shin, 2014-12-05 13:01:27]

On 2014/12/05 00:02:44, sky wrote:
> There's a switch to not see black, --enable-pixel-output-in-tests .

Thanks. I can see the screen with -enable-pixel-output-in-tests.
I'm sure that coordinates of the mouse are over the wbecontents.

[Miyoung Shin, 2014-12-11 15:58:53]

On 2014/12/05 13:01:27, Miyoung Shin wrote:
> On 2014/12/05 00:02:44, sky wrote:
> > There's a switch to not see black, --enable-pixel-output-in-tests .
> 
> Thanks. I can see the screen with -enable-pixel-output-in-tests.
> I'm sure that coordinates of the mouse are over the wbecontents.

sky@, Could you give me your answer about #19 ?

[sky, 2014-12-11 16:44:40]

On 2014/12/04 01:44:13, Miyoung Shin wrote:
> On 2014/12/03 01:25:56, Miyoung Shin wrote:
> > On 2014/12/02 23:46:54, sky wrote:
> > > Seems to me you don't have a good grasp on the bug and are working
> > > around it by fiddling with coordinates. That isn't the right way to
> > > fix this.
> > > 
> > >   -Scott
> > 
> > I got your point. I will check it in more detail and update it.
> 
> sky@,
> I'm not familiar with the event process on Window.
> So, I'd like to get your advice about this issue.
> 
> This is my debugging contents and function call flow.
> 
> EventProcessor::OnEventFromSource - 
> {
> ....
>   target = targeter->FindTargetForEvent(root, event_to_dispatch);  // during
> calling FindTargetForEvent to find target, the event's coordinates moves to
the
> relative location from target.
> }
> 
> ==> ..... ==>
> 
> void WindowEventDispatcher::PreDispatchMouseEvent(Window* target,      //
target
> is here.
>                                                   ui::MouseEvent* event) 
> {
>     case ui::ET_MOUSE_MOVED:
>         mouse_moved_handler_ = target;
>         details = DispatchMouseEnterOrExit(*event, ui::ET_MOUSE_ENTERED);
> }
> 
> ==>
> 
> ui::EventDispatchDetails WindowEventDispatcher::DispatchMouseEnterOrExit(
> {
> ....
>   aura::Window* target = static_cast<Window*>(event.target());
>   if (!target)
>     target = window();                                          //
> event.target() is null, and target is updated to the root-window.
>
>   ui::MouseEvent translated_event(event,
>                                   target,
>                                   mouse_moved_handler_,
>                                   type,
>                                   event.flags() | ui::EF_IS_SYNTHESIZED); //
the
> event's coordinates moves to the negative location because target is
root-window

This event is going to be dispatched to mouse_moved_handler_. The event needs to
be converted from it's current coordinates to that of mouse_moved_handler_.

> .
> }
> 
> So, I think if event.target() is null them target should be gotten from
previous
> function, PreDispatchMouseEvent(), instead of event.target().
> 
> Could you give me any comment about it ?

I think you're right. Seems as though it should be using the target from
PreDispatchMouseEvent.

You should be able to write a test that exercises this code patch.

[Miyoung Shin, 2014-12-14 07:38:47]

sky@, I've uploaded the patch, PTAL.

[sky, 2014-12-15 16:58:41]

Please add test coverage.

[Miyoung Shin, 2014-12-16 02:30:37]

On 2014/12/15 16:58:41, sky wrote:
> Please add test coverage.

Do you mean to add test case for this patch ?

[sky, 2014-12-16 03:48:54]

Yes.

On Mon, Dec 15, 2014 at 6:30 PM,  <myid.shin@samsung.com> wrote:
> On 2014/12/15 16:58:41, sky wrote:
>>
>> Please add test coverage.
>
>
> Do you mean to add test case for this patch ?
>
> https://codereview.chromium.org/754013007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miyoung Shin, 2014-12-16 15:45:14]

On 2014/12/16 03:48:54, sky wrote:
> Yes.
> 
> On Mon, Dec 15, 2014 at 6:30 PM,  <mailto:myid.shin@samsung.com> wrote:
> > On 2014/12/15 16:58:41, sky wrote:
> >>
> >> Please add test coverage.
> >
> >
> > Do you mean to add test case for this patch ?
> >
> > https://codereview.chromium.org/754013007/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

sky@, I added to compare the offset of mousemove event in
mouseleave_browsertest.cc. PTAL.

[sky, 2014-12-16 16:45:05]

Sorry, by test I meant a test in aura exercising the aura change. You
shouldn't need to write a browser test for this. A unit test in aura
should be possible here.

  -Scott

On Tue, Dec 16, 2014 at 7:45 AM,  <myid.shin@samsung.com> wrote:
> On 2014/12/16 03:48:54, sky wrote:
>>
>> Yes.
>
>
>> On Mon, Dec 15, 2014 at 6:30 PM,  <mailto:myid.shin@samsung.com> wrote:
>> > On 2014/12/15 16:58:41, sky wrote:
>> >>
>> >> Please add test coverage.
>> >
>> >
>> > Do you mean to add test case for this patch ?
>> >
>> > https://codereview.chromium.org/754013007/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> sky@, I added to compare the offset of mousemove event in
> mouseleave_browsertest.cc. PTAL.
>
> https://codereview.chromium.org/754013007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miyoung Shin, 2014-12-18 15:13:08]

On 2014/12/16 16:45:05, sky wrote:
> Sorry, by test I meant a test in aura exercising the aura change. You
> shouldn't need to write a browser test for this. A unit test in aura
> should be possible here.
> 
>   -Scott
sky@, I've uploaded the patch to add unit test in aura, PTAL

[sky, 2014-12-18 20:29:29]

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:270: if (event.target())
Why do we need both the supplied target and event.target()? 
WindowEventDispatcher::DispatchMouseExitAtPoint is creating an event, can't it
set the window on the target?

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:410: DispatchDetails details =
DispatchMouseExitAtPoint(NULL,
nullptr

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:81: ui::EventDispatchDetails
DispatchMouseExitAtPoint(Window* window,
Run git cl format.

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher_unittest.cc:833:
EXPECT_FALSE(recorder_second.events().empty());
Doesn't 834 handle this assertion?

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher_unittest.cc:836: EXPECT_EQ(gfx::Point(2,
3).ToString(),
You need to assert mouse_locations() is non-empty, otherwise if it is empty this
line crashes.

[Miyoung Shin, 2014-12-19 14:39:43]

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:270: if (event.target())
On 2014/12/18 20:29:29, sky wrote:
> Why do we need both the supplied target and event.target()? 
> WindowEventDispatcher::DispatchMouseExitAtPoint is creating an event, can't it
> set the window on the target?

You're right.
I think I can use only target to consider both of DispatchMouseExitAtPoint and
PreDispatchMouseEvent. I will change.

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:410: DispatchDetails details =
DispatchMouseExitAtPoint(NULL,
On 2014/12/18 20:29:29, sky wrote:
> nullptr

ok

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:81: ui::EventDispatchDetails
DispatchMouseExitAtPoint(Window* window,
On 2014/12/18 20:29:29, sky wrote:
> Run git cl format.

OK

[Miyoung Shin, 2014-12-19 14:40:17]

sky@, 
I've upload the patch to apply your comment. PTAL

[Miyoung Shin, 2014-12-28 05:44:22]

On 2014/12/19 14:40:17, Miyoung Shin wrote:
> sky@, 
> I've upload the patch to apply your comment. PTAL

sky@, PTAL

[sky, 2015-01-05 17:23:03]

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:270: if (event.target())
On 2014/12/19 14:39:42, Miyoung Shin wrote:
> On 2014/12/18 20:29:29, sky wrote:
> > Why do we need both the supplied target and event.target()? 
> > WindowEventDispatcher::DispatchMouseExitAtPoint is creating an event, can't
it
> > set the window on the target?
> 
> You're right.
> I think I can use only target to consider both of DispatchMouseExitAtPoint and
> PreDispatchMouseEvent. I will change.

You didn't quite end up with what I was suggesting. I'm wondering if when
WindowEventDispatcher::DispatchMouseExitAtPoint creates the MouseEvent it can
set the target on the MouseEvent. I believe this would mean you don't need to
supply an additional target.

https://codereview.chromium.org/754013007/diff/120001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/754013007/diff/120001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.cc:155: NULL,
dispatcher()->GetLastMouseLocationInRoot());
nullptr

[Miyoung Shin, 2015-01-06 01:59:13]

Patchset #8 (id:140001) has been deleted

[Miyoung Shin, 2015-01-06 13:20:44]

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/754013007/diff/100001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:270: if (event.target())
On 2015/01/05 17:23:02, sky wrote:
> On 2014/12/19 14:39:42, Miyoung Shin wrote:
> > On 2014/12/18 20:29:29, sky wrote:
> > > Why do we need both the supplied target and event.target()? 
> > > WindowEventDispatcher::DispatchMouseExitAtPoint is creating an event,
can't
> it
> > > set the window on the target?
> > 
> > You're right.
> > I think I can use only target to consider both of DispatchMouseExitAtPoint
and
> > PreDispatchMouseEvent. I will change.
> 
> You didn't quite end up with what I was suggesting. I'm wondering if when
> WindowEventDispatcher::DispatchMouseExitAtPoint creates the MouseEvent it can
> set the target on the MouseEvent. I believe this would mean you don't need to
> supply an additional target.

Sorry, I don't understand your point yet and why you mentioned
DispatchMouseExitAtPoint.
I can set the target on the MouseEvent in 
WindowEventDispatcher::DispatchMouseExitAtPoint.
But I have no idea how to set the target in
WindowEventDispatcher::PreDispatchMouseEvent that has originally the issue
without supplying an additional target.
Do you mean I should add setTarget method in Event class for
PreDispatchMouseEvent?
If I'm wrong, please correct me.

[sky, 2015-01-06 16:51:30]

You're right. Incorporate my latest suggestions and I'll approve.

https://codereview.chromium.org/754013007/diff/120001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/754013007/diff/120001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:81: ui::EventDispatchDetails
DispatchMouseExitAtPoint(Window* window,
You need to document what |window| and |target| on 155 are. You should rename
|window| here to |target| for better consistency.

[Miyoung Shin, 2015-01-07 07:30:48]

I've uploaded the patchset8 and remained the comment for adding target
parameter. PTAL.

https://codereview.chromium.org/754013007/diff/120001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/754013007/diff/120001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:81: ui::EventDispatchDetails
DispatchMouseExitAtPoint(Window* window,
On 2015/01/06 16:51:30, sky wrote:
> You need to document what |window| and |target| on 155 are. You should rename
> |window| here to |target| for better consistency.
ok.

https://codereview.chromium.org/754013007/diff/120001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/754013007/diff/120001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.cc:155: NULL,
dispatcher()->GetLastMouseLocationInRoot());
On 2015/01/05 17:23:02, sky wrote:
> nullptr

ok

[sky, 2015-01-07 17:07:10]

LGTM

[Miyoung Shin, 2015-01-08 00:39:06]

The CQ bit was checked by myid.shin@samsung.com

[commit-bot: I haz the power, 2015-01-08 00:40:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/754013007/160001

[commit-bot: I haz the power, 2015-01-08 02:42:48]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-01-08 02:43:31]

Patchset 8 (id:??) landed as
https://crrev.com/50bcf1b20f123424d2a176260b61881c55c19dc7
Cr-Commit-Position: refs/heads/master@{#310445}