Context menu on views must show on mouse dpwn for non-WIN.

Context menu on views must show on mouse down for non-WIN.

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

[sadrul, 2013-03-04 23:22:04]

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

https://codereview.chromium.org/12395010/diff/1/ui/views/view.cc#newcode2056
ui/views/view.cc:2056: context_menu_on_mouse_press) {
Note the WARNING on 2050.

I think you should first check here that the view didn't already process the
mouse-press event (i.e. |result| == false). So this condition would be:

  if (!result && context_menu_on_mouse_press && context_menu_controller) { ... }

That should take care of the warning (I think it is a reasonable requirement
that if a View destroys itself from an event-handler, then it should claim that
it has processed the event).

[varunjain, 2013-03-05 00:04:28]

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

https://codereview.chromium.org/12395010/diff/1/ui/views/view.cc#newcode2056
ui/views/view.cc:2056: context_menu_on_mouse_press) {
On 2013/03/04 23:22:04, sadrul wrote:
> Note the WARNING on 2050.
> 
> I think you should first check here that the view didn't already process the
> mouse-press event (i.e. |result| == false). So this condition would be:
> 
>   if (!result && context_menu_on_mouse_press && context_menu_controller) { ...
}
> 
> That should take care of the warning (I think it is a reasonable requirement
> that if a View destroys itself from an event-handler, then it should claim
that
> it has processed the event).

Ha! I missed that warning. Although, the GetDragInfo below does not seem to
care.
The problem with checking for result is that some views return true even if they
are not deleted. What you are suggesting changes the existing behavior which is
that we show context menu even if a view returns true for OnMousePressed. I
looked around a bit and saw that ProcessMouseReleased has the same problem (view
may be deleted in OnMouseReleased). There, existence of a
context_menu_controller is considered evidence that the view is not deleted (see
comment on line 2097). Although I dont understand the validity of that
assertion, I think we can do the same thing here?

[sadrul, 2013-03-05 16:30:11]

Your first patchset is simpler, you simply need to change the condition to check
context_menu_controller instead of context_menu_controller_ and add the comment
explaining the assumption about a view not being destroyed.

There is also a nit in patchset2 that applies to both patchsets.

LGTM with those applied.

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

https://codereview.chromium.org/12395010/diff/1/ui/views/view.cc#newcode2056
ui/views/view.cc:2056: context_menu_on_mouse_press) {
On 2013/03/05 00:04:28, varunjain wrote:
> On 2013/03/04 23:22:04, sadrul wrote:
> > Note the WARNING on 2050.
> > 
> > I think you should first check here that the view didn't already process the
> > mouse-press event (i.e. |result| == false). So this condition would be:
> > 
> >   if (!result && context_menu_on_mouse_press && context_menu_controller) {
...
> }
> > 
> > That should take care of the warning (I think it is a reasonable requirement
> > that if a View destroys itself from an event-handler, then it should claim
> that
> > it has processed the event).
> 
> Ha! I missed that warning. Although, the GetDragInfo below does not seem to
> care.

Good catch. Perhaps GetDragInfo() should be cached before the call to
OnMousePressed too.

> The problem with checking for result is that some views return true even if
they
> are not deleted. What you are suggesting changes the existing behavior which
is
> that we show context menu even if a view returns true for OnMousePressed. I
> looked around a bit and saw that ProcessMouseReleased has the same problem
(view
> may be deleted in OnMouseReleased). There, existence of a
> context_menu_controller is considered evidence that the view is not deleted
(see
> comment on line 2097). Although I dont understand the validity of that
> assertion, I think we can do the same thing here?

Ew, but I suppose it should hold true in both places, yeah.

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

https://codereview.chromium.org/12395010/diff/4001/ui/views/view.cc#newcode61
ui/views/view.cc:61: bool context_menu_on_mouse_press = false;
this value never changes, right? It should be 'const bool
kContextMenuOnMousePress'

[varunjain, 2013-03-05 21:14:21]

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

https://codereview.chromium.org/12395010/diff/1/ui/views/view.cc#newcode2056
ui/views/view.cc:2056: context_menu_on_mouse_press) {
On 2013/03/05 16:30:11, sadrul wrote:
> On 2013/03/05 00:04:28, varunjain wrote:
> > On 2013/03/04 23:22:04, sadrul wrote:
> > > Note the WARNING on 2050.
> > > 
> > > I think you should first check here that the view didn't already process
the
> > > mouse-press event (i.e. |result| == false). So this condition would be:
> > > 
> > >   if (!result && context_menu_on_mouse_press && context_menu_controller) {
> ...
> > }
> > > 
> > > That should take care of the warning (I think it is a reasonable
requirement
> > > that if a View destroys itself from an event-handler, then it should claim
> > that
> > > it has processed the event).
> > 
> > Ha! I missed that warning. Although, the GetDragInfo below does not seem to
> > care.
> 
> Good catch. Perhaps GetDragInfo() should be cached before the call to
> OnMousePressed too.
> 
> > The problem with checking for result is that some views return true even if
> they
> > are not deleted. What you are suggesting changes the existing behavior which
> is
> > that we show context menu even if a view returns true for OnMousePressed. I
> > looked around a bit and saw that ProcessMouseReleased has the same problem
> (view
> > may be deleted in OnMouseReleased). There, existence of a
> > context_menu_controller is considered evidence that the view is not deleted
> (see
> > comment on line 2097). Although I dont understand the validity of that
> > assertion, I think we can do the same thing here?
> 
> Ew, but I suppose it should hold true in both places, yeah.

Done.

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

https://codereview.chromium.org/12395010/diff/4001/ui/views/view.cc#newcode61
ui/views/view.cc:61: bool context_menu_on_mouse_press = false;
On 2013/03/05 16:30:11, sadrul wrote:
> this value never changes, right? It should be 'const bool
> kContextMenuOnMousePress'

Done.

[commit-bot: I haz the power, 2013-03-05 21:16:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12395010/14001

[commit-bot: I haz the power, 2013-03-05 21:22:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-03-05 21:43:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12395010/14001

[commit-bot: I haz the power, 2013-03-06 01:27:22]

Change committed as 186317