Enable touch for autofill popup view

Enable touch for autofill popup view

Previously, items in autofill popup couldn't be selected using touch.
Now, gesture events are handled on autofill popup to enable touch.

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

[mohsen, 2013-10-25 17:14:02]

rbyers@: can you take a look at gesture handling logic, please...

[Rick Byers, 2013-10-25 17:22:22]

LGTM.  Gesture handling looks fine to me.

As we discussed it's a little weird to be handling scroll gestures for the sake
of scrubbing/activating, but as you pointed out it's consistent with some of our
other non-scrollable menus and matches the mouse behavior (eg. mouse down on
one, drag, and lift on another activates the second one).  I'd also be OK with
not handling scroll gestures at all (like the wrench menu).

https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right):

https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:173:
controller_->MouseHovered(event->x(), event->y());
nit: would be nice to update these method names to make it clear they're no
longer specific to mouse.  Maybe:
MouseHovered -> HighlightAt
MouseExitedPopup -> Dismiss
MouseClicked -> ActivateAt

or something like that.

[mohsen, 2013-10-25 18:22:36]

dbeam@: can you take a look, please...

rbyers@: I have talked to kuscher@ about the scrubbing and he said that the main
problem with scrubbing is when the menu is scrollable and since the popup will
never be scrollable, it's fine to have scrubbing here.

[Dan Beam, 2013-10-25 18:38:23]

generally lg

https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right):

https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:190:
event->SetHandled();
don't you only want to set the event handled in some cases?

[Dan Beam, 2013-10-25 18:38:58]

also, CC csharp@ as FYI

[csharp, 2013-10-25 18:46:13]

Nice to see this getting added :)

[mohsen, 2013-10-25 21:36:40]

dbeam@: What do you think about rbyers@ suggestion to change mouse-specific
names of AutofillPopupController's methods?

https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right):

https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:190:
event->SetHandled();
On 2013/10/25 18:38:23, Dan Beam wrote:
> don't you only want to set the event handled in some cases?
I think it is not making any difference in this case, because we don't have a
hierarchy of Views here. But, I've limited the SetHandled() to consumed events,
as it seems generally more correct.

[Dan Beam, 2013-10-26 01:55:32]

On 2013/10/25 21:36:40, mohsen wrote:
> dbeam@: What do you think about rbyers@ suggestion to change mouse-specific
> names of AutofillPopupController's methods?

this would be more up to estade@ or csharp@ than me.

imo, it makes slightly more sense to leave them as SomethingHappened than
DoSomething and let the controller decide the side-effect (i.e. MouseHovered ->
means highlight) but it'd probably result in a larger API (e.g. TapCancelled,
TapDown(x, y)).

> 
>
https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
> File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right):
> 
>
https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
> chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:190:
> event->SetHandled();
> On 2013/10/25 18:38:23, Dan Beam wrote:
> > don't you only want to set the event handled in some cases?
> I think it is not making any difference in this case, because we don't have a
> hierarchy of Views here. But, I've limited the SetHandled() to consumed
events,
> as it seems generally more correct.

[Dan Beam, 2013-10-26 01:56:09]

https://codereview.chromium.org/44543002/diff/180001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right):

https://codereview.chromium.org/44543002/diff/180001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:192:
event->SetHandled();
^ this doesn't seem done

[mohsen, 2013-10-26 03:29:59]

https://codereview.chromium.org/44543002/diff/180001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right):

https://codereview.chromium.org/44543002/diff/180001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:192:
event->SetHandled();
On 2013/10/26 01:56:09, Dan Beam wrote:
> ^ this doesn't seem done
non-handled events return from the function before reaching here (i.e. in the
'default' case of switch statement).

[Dan Beam, 2013-10-28 19:31:18]

lgtm

https://codereview.chromium.org/44543002/diff/180001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right):

https://codereview.chromium.org/44543002/diff/180001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:192:
event->SetHandled();
On 2013/10/26 03:30:00, mohsen wrote:
> On 2013/10/26 01:56:09, Dan Beam wrote:
> > ^ this doesn't seem done
> non-handled events return from the function before reaching here (i.e. in the
> 'default' case of switch statement).

ah, i see

[Rick Byers, 2013-10-28 20:09:41]

On 2013/10/26 01:55:32, Dan Beam wrote:
> On 2013/10/25 21:36:40, mohsen wrote:
> > dbeam@: What do you think about rbyers@ suggestion to change mouse-specific
> > names of AutofillPopupController's methods?
> 
> this would be more up to estade@ or csharp@ than me.
> 
> imo, it makes slightly more sense to leave them as SomethingHappened than
> DoSomething and let the controller decide the side-effect (i.e. MouseHovered
->
> means highlight) but it'd probably result in a larger API (e.g. TapCancelled,
> TapDown(x, y)).

That's OK with me too (although you'd still want to delegate to common handling
code internally for the APIs with identical behavior, so you'd still need all
the DoSomething functions internally - not sure what you're buying).  My main
concern was that calling the mouse-related-events in response to touch gesture
events seemed wrong - asking to have some touch-specific bugs in the future...
 
> 
> > 
> >
>
https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
> > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right):
> > 
> >
>
https://codereview.chromium.org/44543002/diff/50001/chrome/browser/ui/views/a...
> > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:190:
> > event->SetHandled();
> > On 2013/10/25 18:38:23, Dan Beam wrote:
> > > don't you only want to set the event handled in some cases?
> > I think it is not making any difference in this case, because we don't have
a
> > hierarchy of Views here. But, I've limited the SetHandled() to consumed
> events,
> > as it seems generally more correct.

[mohsen, 2013-10-31 00:13:01]

estade@: Please take a look...

[Evan Stade, 2013-10-31 00:26:39]

lgtm

[commit-bot: I haz the power, 2013-11-01 03:15:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/44543002/490001

[commit-bot: I haz the power, 2013-11-01 03:36:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mohsen, 2013-11-01 03:41:34]

isherman@: OWNERS for
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm, please...

[Ilya Sherman, 2013-11-01 06:45:49]

https://codereview.chromium.org/44543002/diff/490001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller.h (right):

https://codereview.chromium.org/44543002/diff/490001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller.h:40: virtual void
PointPreselected(int x, int y) = 0;
It doesn't make sense, at least in terminology that I am aware of, to "select"
or "preselect" a point.  Please find better names for these methods, or possibly
just keep the Mouse* methods, and add touch-specific ones.

https://codereview.chromium.org/44543002/diff/490001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/44543002/diff/490001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.h:55: virtual void
DidPreselectSuggestion(int identifier) OVERRIDE;
Please preserve the previous name, i.e. "select" rather than "preselect".  This
comment applies throughout the CL.  This use of "select" vs. "accept" is
standard terminology in UI code, and seems unrelated to adding support for touch
events.

[mohsen, 2013-11-05 15:54:06]

I have undone the "preselection" renamings and only changed mouse-specific names
of AutofillPopupController methods. Please take another look...

[Ilya Sherman, 2013-11-05 18:39:44]

Thanks, LGTM % nits.

https://codereview.chromium.org/44543002/diff/870001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller.h (right):

https://codereview.chromium.org/44543002/diff/870001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller.h:39: // The line at (x, y)
is selected (e.g., mouse hovered).
nit: "is" -> "was"; or better yet, something like "The user selected the line at
(x, y), e.g. by hovering the mouse cursor."  Please also document that (x, y)
must be within the bounds of the popup.  Ditto for the docs below.

https://codereview.chromium.org/44543002/diff/870001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/44543002/diff/870001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:269: // A tab press
should cause the selected line to be accepted, but still
Thanks for correcting this comment :)

[mohsen, 2013-11-05 19:52:35]

https://codereview.chromium.org/44543002/diff/870001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller.h (right):

https://codereview.chromium.org/44543002/diff/870001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller.h:39: // The line at (x, y)
is selected (e.g., mouse hovered).
On 2013/11/05 18:39:45, Ilya Sherman wrote:
> nit: "is" -> "was"; or better yet, something like "The user selected the line
at
> (x, y), e.g. by hovering the mouse cursor."  Please also document that (x, y)
> must be within the bounds of the popup.  Ditto for the docs below.

Done.

[commit-bot: I haz the power, 2013-11-05 20:04:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/44543002/970001

[commit-bot: I haz the power, 2013-11-05 20:43:28]

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=win&number...

[commit-bot: I haz the power, 2013-11-05 20:44:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/44543002/970001

[commit-bot: I haz the power, 2013-11-06 01:13:47]

Change committed as 233172