[Autofill] Handle the Tab Key in the new UI

[Autofill] Handle the Tab Key in the new UI

The tab key accepts the selected element and always
moves the cursor to the next field.

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

[csharp, 2013-04-17 13:51:25]

The tab key caused a bit of trouble, because it doesn't say it accepts the key
press (since we want the key press to propagate to move focus), but it hides the
autofill ui, which causes the keyboard listener to disappear. The change to the
RenderWidgetHostImpl handles this case where a listener could delete itself and
not accept the key press, but I'm unsure if this is the best way to handle the
issue.

For testing this, I looked at adding something to autofill_browsertests.cc, but
all the tests in there that deal with key presses are marked as flaky. Is there
a non-flaky way to add a test that deals with key presses?

[Ilya Sherman, 2013-04-18 00:33:32]

> For testing this, I looked at adding something to autofill_browsertests.cc,
but
> all the tests in there that deal with key presses are marked as flaky. Is
there
> a non-flaky way to add a test that deals with key presses?

The test code has changed a fair bit since those were marked flaky.  We should
investigate whether they're still flaky; and if so, where they're timing out. 
In principle, the pattern being used for those tests should be fine, and should
be possible to use in a non-flaky way.

https://codereview.chromium.org/13912016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/13912016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:2225: if
((*(it++))->HandleKeyPressEvent(event))
This isn't guaranteed to work.  You should (a) use an ObserverList, (b) make a
local copy of the list, or (c) ensure that it's not possible for the listener to
delete itself when handling the key press.  Option (a) is probably the best if
we can use it, since this is pretty much what the ObserverList is designed for.

There are at least a couple of ways that you could achieve that last option:
  (1) Have the listener return that it handled the event, then re-post it.
  (2) Delay the destruction of the listener until after this loop completes, by
posting a task to the current MessageLoop.

[csharp, 2013-04-18 19:53:02]

https://codereview.chromium.org/13912016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/13912016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:2225: if
((*(it++))->HandleKeyPressEvent(event))
On 2013/04/18 00:33:32, Ilya Sherman wrote:
> This isn't guaranteed to work.  You should (a) use an ObserverList, (b) make a
> local copy of the list, or (c) ensure that it's not possible for the listener
to
> delete itself when handling the key press.  Option (a) is probably the best if
> we can use it, since this is pretty much what the ObserverList is designed
for.
> 
> There are at least a couple of ways that you could achieve that last option:
>   (1) Have the listener return that it handled the event, then re-post it.
>   (2) Delay the destruction of the listener until after this loop completes,
by
> posting a task to the current MessageLoop.

Ah, I didn't know about ObserverList, awesome.

[Ilya Sherman, 2013-04-18 21:27:03]

LGTM, thanks

[commit-bot: I haz the power, 2013-04-19 14:56:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/13912016/5001

[commit-bot: I haz the power, 2013-04-19 14:56:37]

Presubmit check for 13912016-5001 failed and returned exit status 1.

INFO:root:Found 4 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/browser/renderer_host/render_widget_host_impl.cc
    content/browser/renderer_host/render_widget_host_impl.h

Presubmit checks took 2.3s to calculate.

[csharp, 2013-04-19 15:03:37]

jam@ for renderer_host owner's approval

[jam, 2013-04-19 17:02:41]

https://codereview.chromium.org/13912016/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/13912016/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:2209: while ((listener
= it.GetNext()) != NULL) {
!= NULL seems redundant?

[csharp, 2013-04-19 18:04:42]

https://codereview.chromium.org/13912016/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/13912016/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:2209: while ((listener
= it.GetNext()) != NULL) {
On 2013/04/19 17:02:41, jam wrote:
> != NULL seems redundant?

Yup, but all the instances in the code base that manually iterator over an
ObserverListener do it this way. (I assume its to make it clear that "listener =
it.GetNext()" is an assignment, not a comparison. So went with consistency.

I've got no problem removing the != NULL if you still want it gone through.

[jam, 2013-04-19 18:27:07]

i see, ok lgtm

[commit-bot: I haz the power, 2013-04-19 18:45:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/13912016/5001

[commit-bot: I haz the power, 2013-04-20 00:01:09]

Change committed as 195343