Fix showing the sibling menu on mouse move

Fix showing the sibling menu on mouse move

Using GetTopWindowContainingPoint() was returning the ExoShellSurfce
in the case of ARC++ devices, which was different from the window of
the widget that owns the menu. This prevented showing the sibling menu
when the mouse moved and hovered over the neighboring menu button.

We decided to remove GetTopWindowContainingPoint() as it's not needed
anymore.

BUG=594439
Review-Url: https://codereview.chromium.org/2715513006
Cr-Commit-Position: refs/heads/master@{#460995}
Committed: https://chromium.googlesource.com/chromium/src/+/6ad1835ebdf60d5363f0df85437f34a4c3e46052

[afakhry, 2017-02-24 18:05:49]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 18:06:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 18:34:45]

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

[commit-bot: I haz the power, 2017-02-24 18:34:46]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-02-25 00:58:36]

afakhry@chromium.org changed reviewers:
+ oshima@chromium.org

[afakhry, 2017-02-25 00:58:36]

Oshima-san, I need to ask you about this CL. How can I write a browser test for
this that simulates the existence of the ExoShellSurface for Arc++?

[oshima, 2017-02-28 02:05:35]

On 2017/02/25 00:58:36, afakhry wrote:
> Oshima-san, I need to ask you about this CL. How can I write a browser test
for
> this that simulates the existence of the ExoShellSurface for Arc++?

There are unit tests for exo. Do you need browser tests?

[afakhry, 2017-02-28 02:16:26]

On 2017/02/28 02:05:35, oshima wrote:
> On 2017/02/25 00:58:36, afakhry wrote:
> > Oshima-san, I need to ask you about this CL. How can I write a browser test
> for
> > this that simulates the existence of the ExoShellSurface for Arc++?
> 
> There are unit tests for exo. Do you need browser tests?

I want to write a test that opens sibling menus in a browser with the exo window
present.

[oshima, 2017-02-28 07:54:45]

On 2017/02/28 02:16:26, afakhry wrote:
> On 2017/02/28 02:05:35, oshima wrote:
> > On 2017/02/25 00:58:36, afakhry wrote:
> > > Oshima-san, I need to ask you about this CL. How can I write a browser
test
> > for
> > > this that simulates the existence of the ExoShellSurface for Arc++?
> > 
> > There are unit tests for exo. Do you need browser tests?
> 
> I want to write a test that opens sibling menus in a browser with the exo
window
> present.

You can create surface shell widget in the same way as exo unit test does.

[afakhry, 2017-03-21 17:35:47]

afakhry@chromium.org changed reviewers:
+ sadrul@chromium.org

[afakhry, 2017-03-21 17:35:47]

https://codereview.chromium.org/2715513006/diff/1/ash/display/screen_ash.cc
File ash/display/screen_ash.cc (left):

https://codereview.chromium.org/2715513006/diff/1/ash/display/screen_ash.cc#o...
ash/display/screen_ash.cc:107: return
root_window->GetTopWindowContainingPoint(local_point);
+Sadrul.

Oshima and talked about deprecating this GetTopWindowContainingPoint() as it is
not used in many places but we want to confirm that its use here in this context
is wrong. WDYT?

[sadrul, 2017-03-22 20:04:31]

https://codereview.chromium.org/2715513006/diff/1/ash/display/screen_ash.cc
File ash/display/screen_ash.cc (left):

https://codereview.chromium.org/2715513006/diff/1/ash/display/screen_ash.cc#o...
ash/display/screen_ash.cc:107: return
root_window->GetTopWindowContainingPoint(local_point);
On 2017/03/21 17:35:47, afakhry wrote:
> +Sadrul.
> 
> Oshima and talked about deprecating this GetTopWindowContainingPoint() as it
is
> not used in many places but we want to confirm that its use here in this
context
> is wrong. WDYT?

Can you clarify how this change fixes the bug you are trying to fix (and what is
causing the bug in the first place)?

If GetTopWindowContainingPoint() ends up not being necessary, then yes please!
rm-rf code not used/necessary.

[afakhry, 2017-03-22 20:29:33]

https://codereview.chromium.org/2715513006/diff/1/ash/display/screen_ash.cc
File ash/display/screen_ash.cc (left):

https://codereview.chromium.org/2715513006/diff/1/ash/display/screen_ash.cc#o...
ash/display/screen_ash.cc:107: return
root_window->GetTopWindowContainingPoint(local_point);
On 2017/03/22 20:04:31, sadrul wrote:
> On 2017/03/21 17:35:47, afakhry wrote:
> > +Sadrul.
> > 
> > Oshima and talked about deprecating this GetTopWindowContainingPoint() as it
> is
> > not used in many places but we want to confirm that its use here in this
> context
> > is wrong. WDYT?
> 
> Can you clarify how this change fixes the bug you are trying to fix (and what
is
> causing the bug in the first place)?
> 
> If GetTopWindowContainingPoint() ends up not being necessary, then yes please!
> rm-rf code not used/necessary.

Thanks.

GetTopWindowContaingPoint() on ARC++ devices return the ExoShellSurface window
rather than the window that's the owner widget of the window controller.
Resulting in returning early here
https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_controller.c...
without showing the sibling window.

More details are in this comment on the bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=594439#c28

[oshima, 2017-03-23 12:52:29]

On 2017/03/22 20:29:33, afakhry wrote:
> https://codereview.chromium.org/2715513006/diff/1/ash/display/screen_ash.cc
> File ash/display/screen_ash.cc (left):
> 
>
https://codereview.chromium.org/2715513006/diff/1/ash/display/screen_ash.cc#o...
> ash/display/screen_ash.cc:107: return
> root_window->GetTopWindowContainingPoint(local_point);
> On 2017/03/22 20:04:31, sadrul wrote:
> > On 2017/03/21 17:35:47, afakhry wrote:
> > > +Sadrul.
> > > 
> > > Oshima and talked about deprecating this GetTopWindowContainingPoint() as
it
> > is
> > > not used in many places but we want to confirm that its use here in this
> > context
> > > is wrong. WDYT?
> > 
> > Can you clarify how this change fixes the bug you are trying to fix (and
what
> is
> > causing the bug in the first place)?
> > 
> > If GetTopWindowContainingPoint() ends up not being necessary, then yes
please!
> > rm-rf code not used/necessary.
> 
> Thanks.
> 
> GetTopWindowContaingPoint() on ARC++ devices return the ExoShellSurface window
> rather than the window that's the owner widget of the window controller.
> Resulting in returning early here
>
https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_controller.c...
> without showing the sibling window.
> 
> More details are in this comment on the bug:
> https://bugs.chromium.org/p/chromium/issues/detail?id=594439#c28

GetTopWindowContainingPoint predates EventTargeter (added 2011)
and doesn't use the targeter it to find the target window. 

I believe this is supposed to be replaced with GetEventHandlerForPoint,
and this is the last one.

[afakhry, 2017-03-25 01:03:11]

Description was changed from

==========
Fix showing the sibling menu on mouse move

BUG=594439
==========

to

==========
Fix showing the sibling menu on mouse move

Using GetTopWindowContainingPoint() was returning the ExoShellSurfce
in the case of ARC++ devices, which was different from the window of
the widget that owns the menu. This prevented showing the sibling menu
when the mouse moved and hovered over the neighboring menu button.

We decided to remove GetTopWindowContainingPoint() as it's not needed
anymore.

BUG=594439
==========

[afakhry, 2017-03-25 01:07:21]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 01:08:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 01:24:55]

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

[commit-bot: I haz the power, 2017-03-25 01:24:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2017-03-25 01:32:16]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 01:32:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 02:44:31]

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

[commit-bot: I haz the power, 2017-03-25 02:44:32]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-03-25 02:47:35]

Sadrul & Oshima, Please take a look! Thanks.

I removed the GetTopWindowContainingPoint() as agreed.

[sadrul, 2017-03-28 19:05:02]

lgtm

I suspect it is possible for this change to introduce some regressions
(specifically, with some edge-cases for drag-drop, and/or tooltips). So please
keep an eye out for that.

[afakhry, 2017-03-29 01:59:08]

afakhry@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[afakhry, 2017-03-29 01:59:09]

Thanks, Sadrul. I will keep an eye on any regressions.

Devlin for extensions and Oshima for ash.

[Devlin, 2017-03-30 22:24:20]

extensions lgtm.  This code is a little magical to me, but this seems sane. :)

[oshima, 2017-03-31 01:10:08]

lgtm

[afakhry, 2017-03-31 01:11:06]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-03-31 01:11:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 02:08:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490922666768070,
"parent_rev": "986a2209658887e85eeb3e00c933db92383bef73", "commit_rev":
"6ad1835ebdf60d5363f0df85437f34a4c3e46052"}

[commit-bot: I haz the power, 2017-03-31 02:09:47]

Description was changed from

==========
Fix showing the sibling menu on mouse move

Using GetTopWindowContainingPoint() was returning the ExoShellSurfce
in the case of ARC++ devices, which was different from the window of
the widget that owns the menu. This prevented showing the sibling menu
when the mouse moved and hovered over the neighboring menu button.

We decided to remove GetTopWindowContainingPoint() as it's not needed
anymore.

BUG=594439
==========

to

==========
Fix showing the sibling menu on mouse move

Using GetTopWindowContainingPoint() was returning the ExoShellSurfce
in the case of ARC++ devices, which was different from the window of
the widget that owns the menu. This prevented showing the sibling menu
when the mouse moved and hovered over the neighboring menu button.

We decided to remove GetTopWindowContainingPoint() as it's not needed
anymore.

BUG=594439

Review-Url: https://codereview.chromium.org/2715513006
Cr-Commit-Position: refs/heads/master@{#460995}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ad1835ebdf60d5363f0df85437f...
==========

[commit-bot: I haz the power, 2017-03-31 02:09:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6ad1835ebdf60d5363f0df85437f...