Prevent the context menu from showing when long pressing the window controls

Make PostEventDispatchHandler::OnGestureEvent() only show the
context menu on long press if the events are targetted at a view
which can show the context menu. If a different view is the gesture
handler and the event was targetted at a different view (e.g.
because the maximize button handled ET_GESTURE_TAP_DOWN) the parent
view should not show a context menu on long press.

Previously, we were assuming that events were in the coordinates of
|PostEventDispatchHandler::owner_| and were sometimes passing an
incorrect position to BrowserFrame::ShowContextMenuForView().

BUG=375232
TEST=RootViewTest.ContextMenuFromLongPress

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

[pkotwicz, 2014-05-22 01:22:55]

Sadrul, PTAL

[sadrul, 2014-05-22 01:27:44]

+tdanderson@ mind reviewing this change?

Is the right fix to not do anything if target != owner_?

Can this have a test?

[tdanderson, 2014-05-22 14:40:37]

Peter, please see my comments below. Also:

- I would suggest re-wording the double negative in the issue description and
saying that your patch is correct because you're now correctly converting from
the coordinate space of the event target.

- nit: Please break the lines of the issue description to <= 80 characters for
readability in the logs.

- I agree with Sadrul that it would be nice to have a test for this patch.

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc#newcode104
ui/views/view.cc:104: virtual void OnGestureEvent(ui::GestureEvent* event)
OVERRIDE {
In response to Sadrul's suggestion, I actually don't think we want an early
return here if owner_ != event->target(). 

Consider view A with the child view B, both having post-target handlers
installed. Say an event is targeted at B, but B is not draggable and does not
have a context menu controller. It might still be the case that A is
draggable/have a context menu controller, and the early return would disallow
dragging/a context menu for A.

Peter, can you double-check this?

[sadrul, 2014-05-22 14:50:53]

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc#newcode104
ui/views/view.cc:104: virtual void OnGestureEvent(ui::GestureEvent* event)
OVERRIDE {
On 2014/05/22 14:40:37, tdanderson wrote:
> In response to Sadrul's suggestion, I actually don't think we want an early
> return here if owner_ != event->target(). 
> 
> Consider view A with the child view B, both having post-target handlers
> installed. Say an event is targeted at B, but B is not draggable and does not
> have a context menu controller. It might still be the case that A is
> draggable/have a context menu controller, and the early return would disallow
> dragging/a context menu for A.

But the event is going to be retargeted to A if B did not handle it, right? If
there is a pre-target handler on A that wants to do something else (or if the
default handler of A does something with the event), then we should not do this
drag?

> 
> Peter, can you double-check this?

[tdanderson, 2014-05-22 16:08:20]

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc#newcode104
ui/views/view.cc:104: virtual void OnGestureEvent(ui::GestureEvent* event)
OVERRIDE {
On 2014/05/22 14:50:53, sadrul wrote:
> On 2014/05/22 14:40:37, tdanderson wrote:
> > In response to Sadrul's suggestion, I actually don't think we want an early
> > return here if owner_ != event->target(). 
> > 
> > Consider view A with the child view B, both having post-target handlers
> > installed. Say an event is targeted at B, but B is not draggable and does
not
> > have a context menu controller. It might still be the case that A is
> > draggable/have a context menu controller, and the early return would
disallow
> > dragging/a context menu for A.
> 
> But the event is going to be retargeted to A if B did not handle it, right? If
> there is a pre-target handler on A that wants to do something else (or if the
> default handler of A does something with the event), then we should not do
this
> drag?

Wouldn't the early return when owner_ != event->target() defeat the purpose of
considering the post-target handlers of the target as well as all of the
post-target handlers of the target's ancestors during the post-target phase?

If a pre-target handler of A wanted to do something with the event, it had the
chance to do so during the pre-target phase before the event reaches B.

If A itself wants to do something with the event, it should only be allowed to
do so after the post-target phase for B's dispatch completes (which includes
possibly showing a context menu for A).

> 
> > 
> > Peter, can you double-check this?
>

[sadrul, 2014-05-22 16:26:21]

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc#newcode104
ui/views/view.cc:104: virtual void OnGestureEvent(ui::GestureEvent* event)
OVERRIDE {
On 2014/05/22 16:08:20, tdanderson wrote:
> On 2014/05/22 14:50:53, sadrul wrote:
> > On 2014/05/22 14:40:37, tdanderson wrote:
> > > In response to Sadrul's suggestion, I actually don't think we want an
early
> > > return here if owner_ != event->target(). 
> > > 
> > > Consider view A with the child view B, both having post-target handlers
> > > installed. Say an event is targeted at B, but B is not draggable and does
> not
> > > have a context menu controller. It might still be the case that A is
> > > draggable/have a context menu controller, and the early return would
> disallow
> > > dragging/a context menu for A.
> > 
> > But the event is going to be retargeted to A if B did not handle it, right?
If
> > there is a pre-target handler on A that wants to do something else (or if
the
> > default handler of A does something with the event), then we should not do
> this
> > drag?
> 
> Wouldn't the early return when owner_ != event->target() defeat the purpose of
> considering the post-target handlers of the target as well as all of the
> post-target handlers of the target's ancestors during the post-target phase?
> 
> If a pre-target handler of A wanted to do something with the event, it had the
> chance to do so during the pre-target phase before the event reaches B.
> 
> If A itself wants to do something with the event, it should only be allowed to
> do so after the post-target phase for B's dispatch completes (which includes
> possibly showing a context menu for A).

Consider the case of B: whether or not the context menu of B is shown (or a drag
starts from B) depends on whether the default handler of B processed the event
or not. The same should apply for A. The default handler of A should get a
chance to process the event first.

It is entirely possible for a post-target handler of A do something during the
post-dispatch phase for target B, but in this case for context-menu/dnd, doing
that would be wrong. (if the code showed the context menu of the target instead
of owner_, then it would have made more sense).

Maybe the better fix is to install this handler on the root-view instead, and
show the context-menu/start dnd on the target.

> 
> > 
> > > 
> > > Peter, can you double-check this?
> > 
>

[tdanderson, 2014-05-22 18:35:10]

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/291273007/diff/90001/ui/views/view.cc#newcode104
ui/views/view.cc:104: virtual void OnGestureEvent(ui::GestureEvent* event)
OVERRIDE {
On 2014/05/22 16:26:22, sadrul wrote:
> On 2014/05/22 16:08:20, tdanderson wrote:
> > On 2014/05/22 14:50:53, sadrul wrote:
> > > On 2014/05/22 14:40:37, tdanderson wrote:
> > > > In response to Sadrul's suggestion, I actually don't think we want an
> early
> > > > return here if owner_ != event->target(). 
> > > > 
> > > > Consider view A with the child view B, both having post-target handlers
> > > > installed. Say an event is targeted at B, but B is not draggable and
does
> > not
> > > > have a context menu controller. It might still be the case that A is
> > > > draggable/have a context menu controller, and the early return would
> > disallow
> > > > dragging/a context menu for A.
> > > 
> > > But the event is going to be retargeted to A if B did not handle it,
right?
> If
> > > there is a pre-target handler on A that wants to do something else (or if
> the
> > > default handler of A does something with the event), then we should not do
> > this
> > > drag?
> > 
> > Wouldn't the early return when owner_ != event->target() defeat the purpose
of
> > considering the post-target handlers of the target as well as all of the
> > post-target handlers of the target's ancestors during the post-target phase?
> > 
> > If a pre-target handler of A wanted to do something with the event, it had
the
> > chance to do so during the pre-target phase before the event reaches B.
> > 
> > If A itself wants to do something with the event, it should only be allowed
to
> > do so after the post-target phase for B's dispatch completes (which includes
> > possibly showing a context menu for A).
> 
> Consider the case of B: whether or not the context menu of B is shown (or a
drag
> starts from B) depends on whether the default handler of B processed the event
> or not. The same should apply for A. The default handler of A should get a
> chance to process the event first.
> 
> It is entirely possible for a post-target handler of A do something during the
> post-dispatch phase for target B, but in this case for context-menu/dnd, doing
> that would be wrong. (if the code showed the context menu of the target
instead
> of owner_, then it would have made more sense).

OK, so your concern here is specific to context menus / touch d&d and you are
not objecting in general to the order of handlers visited during dispatch. In
that case, I am now on the same page as you.

> 
> Maybe the better fix is to install this handler on the root-view instead, and
> show the context-menu/start dnd on the target.

I agree. Doing so will avoid the need to have a separate EventHandler for each
View and will also make the point conversion at line 112 unnecessary.

> 
> > 
> > > 
> > > > 
> > > > Peter, can you double-check this?
> > > 
> > 
>

[pkotwicz, 2014-05-23 03:46:34]

Sadrul, can you please take another look?

[tdanderson, 2014-05-23 15:02:37]

LGTM with the nits addressed regarding the issue description in my original
comments. Plus please also replace TEST=... with
TEST=RootViewTest.ContextMenuFromLongPress

[pkotwicz, 2014-05-23 16:05:57]

Sadrul, can you please take a look?

Fixed the CL description as per tdanderson@'s request.
I do not think that I need to worry about the length of the lines in the CL
description?

[sadrul, 2014-05-26 14:53:58]

On 2014/05/23 16:05:57, pkotwicz wrote:
> Sadrul, can you please take a look?
> 
> Fixed the CL description as per tdanderson@'s request.
> I do not think that I need to worry about the length of the lines in the CL
> description?

Shorter lengths of line in the description makes it easier to look at the logs
from git log etc. We don't have formal restrictions on line lengths, but a lot
of developers (myself included) prefer shorter lines. (see, for example:
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html,
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/sRRWJwvy2C8)

LGTM

[pkotwicz, 2014-05-26 15:09:00]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-05-26 15:10:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/291273007/120001

[commit-bot: I haz the power, 2014-05-26 19:42:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-26 22:46:19]

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

[commit-bot: I haz the power, 2014-05-26 22:46:19]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[pkotwicz, 2014-05-26 23:28:33]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-05-26 23:29:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/291273007/120001

[commit-bot: I haz the power, 2014-05-27 01:13:42]

Change committed as 272907