Couple of tweaks to ui_controls_linux:...

Couple of tweaks to ui_controls_linux:

. Moves listening from DidProcess to WillProcess. This is necessitated
  by the bookmark bar (chrome menu) tests that spawn a nested run
  loop, which results in DidProcess not getting sent and the test
  wedging. This brings the code closer in line with Windows.
. If there is a mouse/keyboard grab, mouse/keyboard events are sent to
  the grabbed widget.
. Deals with a NULL widget for sending keyboard events (tries to find
  focused widget).

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=24420

[Evan Stade, 2009-08-25 21:24:33]

won't the event be sent to the grab widget automagically?

http://codereview.chromium.org/173392/diff/1/4
File chrome/browser/automation/ui_controls_linux.cc (right):

http://codereview.chromium.org/173392/diff/1/4#newcode48
Line 48: MessageLoop::current()->PostDelayedTask(
can this just be PostTask()?

[sky, 2009-08-25 21:32:52]

On 2009/08/25 21:24:33, Evan Stade wrote:
> won't the event be sent to the grab widget automagically?

That wasn't the case in my testing.

> http://codereview.chromium.org/173392/diff/1/4
> File chrome/browser/automation/ui_controls_linux.cc (right):
> 
> http://codereview.chromium.org/173392/diff/1/4#newcode48
> Line 48: MessageLoop::current()->PostDelayedTask(
> can this just be PostTask()?

Done.

New patch uploaded.

  -Scott

[Evan Stade, 2009-08-25 21:43:44]

http://codereview.chromium.org/173392/diff/1002/16
File chrome/browser/automation/ui_controls_linux.cc (right):

http://codereview.chromium.org/173392/diff/1002/16#newcode108
Line 108: if (gtk_window_get_focus(window))
why do we check for a focused widget here, but not in the if (window) case? It
seems the two cases should be consistent.

I think keyboard events all get sent to the focused widget anyhow, so there is
no point to check for it explicitly. But I could be wrong.

http://codereview.chromium.org/173392/diff/1002/16#newcode116
Line 116: DCHECK(event->key.window);
can we move this DCHECK outside the conditional?

[sky, 2009-08-25 21:54:06]

New snapshot uploaded.

http://codereview.chromium.org/173392/diff/1002/16
File chrome/browser/automation/ui_controls_linux.cc (right):

http://codereview.chromium.org/173392/diff/1002/16#newcode108
Line 108: if (gtk_window_get_focus(window))
On 2009/08/25 21:43:46, Evan Stade wrote:
> why do we check for a focused widget here, but not in the if (window) case? It
> seems the two cases should be consistent.

Done.

> I think keyboard events all get sent to the focused widget anyhow, so there is
> no point to check for it explicitly. But I could be wrong.

I'm not positive there. I ended up changing this code because the bookmark test
is passing in NULL and assuming the event would target the focused widget. Gtk
didn't like a NULL widget. I then changed it to get the focused widget, but that
is some times null. That's how I arrived at these cases.

http://codereview.chromium.org/173392/diff/1002/16#newcode116
Line 116: DCHECK(event->key.window);
On 2009/08/25 21:43:46, Evan Stade wrote:
> can we move this DCHECK outside the conditional?

Done.

[Evan Stade, 2009-08-25 22:03:27]

right, I've been changing all the tests to pass in an actual browser window, but
didn't change the bookmark bar one because it only affected windows and windows
didn't use that parameter anyhow. But this approach seems ok too.

Since the focused widget is a gtk concept (that X knows nothing about), gtk has
to manually redirect events sent to it (from X) to the focused widget. So I am
pretty sure we don't need to explicitly send it to the focused widget. Can you
test it without that bit, and take it out if it still works?

with that, LGTM

[sky, 2009-08-25 22:36:00]

You're right. Looking for the focused widget is not needed.

Thanks,

  -Scott