aura: Fix unit_tests on linux

aura: Fix unit_tests on linux

Make sure the aura::Desktop is destroyed with the message-loop. This is
done by adding a DestroyMessagePump notification to MessagePumpObserver.

Also remove some views_unittests that were added to test
NativeWidgetViews (which aren't used anymore), and do proper cleanup
in AccessibilityEventRouter unittest.

TBR=darin@chromium.org
BUG=104559, 105613
TEST=unit_tests

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

[sadrul, 2011-11-18 05:56:33]

This is probably the simplest workaround/fix.

A more involved fix would be to send a notification to MessagePumpObserver when
the message-pump gets destroyed, and install the DesktopHost as an observer, so
that it can reset itself when the message-pump gets destroyed, and re-initialize
when a new message-pump is created. But that didn't quite feel like a clean
solution.

[oshima, 2011-11-18 06:12:37]

ui_test_utils is also used by browser_tests, ui_tests and interactive_uitests
that need native events.


On 2011/11/18 05:56:33, sadrul wrote:
> This is probably the simplest workaround/fix.
> 
> A more involved fix would be to send a notification to MessagePumpObserver
when
> the message-pump gets destroyed, and install the DesktopHost as an observer,
so
> that it can reset itself when the message-pump gets destroyed, and
re-initialize
> when a new message-pump is created. But that didn't quite feel like a clean
> solution.

[sadrul, 2011-11-18 06:21:19]

On 2011/11/18 06:12:37, oshima wrote:
> ui_test_utils is also used by browser_tests, ui_tests and interactive_uitests
> that need native events.

I did run browser_tests locally, and it looks like at least the ones I ran
(FindInPage*, DesktopNotifications* etc.) work OK with this change. But I
suspect this won't be sufficient for tests that do expect native-events (e.g.
ones sending events using XSendEvent?).

> 
> 
> On 2011/11/18 05:56:33, sadrul wrote:
> > This is probably the simplest workaround/fix.
> > 
> > A more involved fix would be to send a notification to MessagePumpObserver
> when
> > the message-pump gets destroyed, and install the DesktopHost as an observer,
> so
> > that it can reset itself when the message-pump gets destroyed, and
> re-initialize
> > when a new message-pump is created. But that didn't quite feel like a clean
> > solution.

[oshima_google, 2011-11-18 07:28:45]

On Thu, Nov 17, 2011 at 10:21 PM, <sadrul@chromium.org> wrote:

> On 2011/11/18 06:12:37, oshima wrote:
>
>> ui_test_utils is also used by browser_tests, ui_tests and
>> interactive_uitests
>> that need native events.
>>
>
> I did run browser_tests locally, and it looks like at least the ones I ran
> (FindInPage*, DesktopNotifications* etc.) work OK with this change. But I
> suspect this won't be sufficient for tests that do expect native-events
> (e.g.
> ones sending events using XSendEvent?).


Yes, that is correct. There is another issue, where MessagePumpX may hang
when there is an event in the queue because it never remove events from
event queue when there is no dispatcher. If we want to run tests (that
doesn't need native event) without dispatcher,  you need to remove events
from the queue.

- oshima



>
>
>
>
>  On 2011/11/18 05:56:33, sadrul wrote:
>> > This is probably the simplest workaround/fix.
>> >
>> > A more involved fix would be to send a notification to
>> MessagePumpObserver
>> when
>> > the message-pump gets destroyed, and install the DesktopHost as an
>> observer,
>> so
>> > that it can reset itself when the message-pump gets destroyed, and
>> re-initialize
>> > when a new message-pump is created. But that didn't quite feel like a
>> clean
>> > solution.
>>
>
>
>
>
http://codereview.chromium.**org/8566037/<http://codereview.chromium.org/8566...
>

[sadrul, 2011-11-18 17:26:10]

I have changed the CL to take a different approach. I think this is better than
what I had done in the first patch-set, and still simpler than adding a new
notification to MessagePumpObserver. PTAL.

On 2011/11/18 07:28:45, oshima1 wrote:
> On Thu, Nov 17, 2011 at 10:21 PM, <mailto:sadrul@chromium.org> wrote:
> 
> > On 2011/11/18 06:12:37, oshima wrote:
> >
> >> ui_test_utils is also used by browser_tests, ui_tests and
> >> interactive_uitests
> >> that need native events.
> >>
> >
> > I did run browser_tests locally, and it looks like at least the ones I ran
> > (FindInPage*, DesktopNotifications* etc.) work OK with this change. But I
> > suspect this won't be sufficient for tests that do expect native-events
> > (e.g.
> > ones sending events using XSendEvent?).
> 
> 
> Yes, that is correct. There is another issue, where MessagePumpX may hang
> when there is an event in the queue because it never remove events from
> event queue when there is no dispatcher. If we want to run tests (that
> doesn't need native event) without dispatcher,  you need to remove events
> from the queue.
> 
> - oshima
> 
> 
> 
> >
> >
> >
> >
> >  On 2011/11/18 05:56:33, sadrul wrote:
> >> > This is probably the simplest workaround/fix.
> >> >
> >> > A more involved fix would be to send a notification to
> >> MessagePumpObserver
> >> when
> >> > the message-pump gets destroyed, and install the DesktopHost as an
> >> observer,
> >> so
> >> > that it can reset itself when the message-pump gets destroyed, and
> >> re-initialize
> >> > when a new message-pump is created. But that didn't quite feel like a
> >> clean
> >> > solution.
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/8566037/%3Chttp://codereview.chromium.org/85...>
> >

[sadrul, 2011-11-29 16:15:59]

Requesting suggestion as to which approach is better:

1) Add AuraMessageLoopForTest in ui/aura, require all unit_tests that have a
MessageLoopForUI attribute conditionally use AuraMessageLoopForTest instead.
This may be necessary in only a small number of tests, so possibly will not be
very intrusive.

2) Dispatch a NULL event from ~MessagePumpX, notifying the termination of the
message-pump. aura::Desktop can then clean up its state/destroy itself.

3) Add MessagePumpObserver::Destroying. Install the aura::DesktopHost as an
observer for the message-pump.

Patchset4 here is a crude implementation of 1 (with code duplication in two
places that seem to require this change at the moment). But perhaps 3 would be
the better fix?

[sadrul, 2011-11-29 18:31:37]

I have gone ahead and done the change for MessagePumpObserver in patchset5. PTAL

[sadrul, 2011-11-29 20:49:12]

The trybots are happy; linux_aura runs unit_tests and it's green.

[oshima, 2011-11-29 21:08:11]

nice work!

http://codereview.chromium.org/8566037/diff/14009/base/message_pump_observer.h
File base/message_pump_observer.h (right):

http://codereview.chromium.org/8566037/diff/14009/base/message_pump_observer....
base/message_pump_observer.h:42: virtual void DestroyMessagePump() {}
OnMessagePumpDestroying. how much work do we need to make this pure virtual?

http://codereview.chromium.org/8566037/diff/14009/ui/views/widget/widget_unit...
File ui/views/widget/widget_unittest.cc (left):

http://codereview.chromium.org/8566037/diff/14009/ui/views/widget/widget_unit...
ui/views/widget/widget_unittest.cc:175: TEST_F(WidgetTest,
GetTopLevelWidget_Synthetic) {
Are these removal intentional?

[sadrul, 2011-11-29 21:21:33]

http://codereview.chromium.org/8566037/diff/14009/base/message_pump_observer.h
File base/message_pump_observer.h (right):

http://codereview.chromium.org/8566037/diff/14009/base/message_pump_observer....
base/message_pump_observer.h:42: virtual void DestroyMessagePump() {}
On 2011/11/29 21:08:11, oshima wrote:
> OnMessagePumpDestroying. how much work do we need to make this pure virtual?

Renamed.

To make it pure virtual: I am not sure how much work that would need (probably
not much). But I tend to think that 'Observer' functions should not be pure
virtual: an observer may be interested in only some events, and not all, and
should not be forced to have implementation for every notification.

http://codereview.chromium.org/8566037/diff/14009/ui/views/widget/widget_unit...
File ui/views/widget/widget_unittest.cc (left):

http://codereview.chromium.org/8566037/diff/14009/ui/views/widget/widget_unit...
ui/views/widget/widget_unittest.cc:175: TEST_F(WidgetTest,
GetTopLevelWidget_Synthetic) {
On 2011/11/29 21:08:11, oshima wrote:
> Are these removal intentional?

They are. The _Synthetic tests were introduced for NativeWidgetView(s), and
since those don't exist anymore, these tests aren't necessary.

[oshima, 2011-11-30 00:04:16]

LGTM

http://codereview.chromium.org/8566037/diff/14009/base/message_pump_observer.h
File base/message_pump_observer.h (right):

http://codereview.chromium.org/8566037/diff/14009/base/message_pump_observer....
base/message_pump_observer.h:42: virtual void DestroyMessagePump() {}
On 2011/11/29 21:21:33, sadrul wrote:
> On 2011/11/29 21:08:11, oshima wrote:
> > OnMessagePumpDestroying. how much work do we need to make this pure virtual?
> 
> Renamed.
> 
> To make it pure virtual: I am not sure how much work that would need (probably
> not much). But I tend to think that 'Observer' functions should not be pure
> virtual: an observer may be interested in only some events, and not all, and
> should not be forced to have implementation for every notification.

That's true if Observer interface is broad (like WindowObserver). I prefer pure
virtual for narrow interface, but I don't feel strongly about it if we add this
to MessagePumpObserver. I'll leave it to you and owners.

http://codereview.chromium.org/8566037/diff/14009/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8566037/diff/14009/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:311:
MessageLoopForUI::current()->RemoveObserver(this);
nit: it may be better to put them in reverse order to match the order in
constructor.

[sadrul, 2011-11-30 01:09:43]

+darin for base/
+davemoore for chrome/browser/profiles/

[darin (slow to review), 2011-11-30 06:30:31]

Is there some reason why MessageLoop::DestructionObserver is insufficient?  Why
do you need to separately know when the MessagePump is being destroyed?

http://codereview.chromium.org/8566037/diff/19001/base/message_pump_observer.h
File base/message_pump_observer.h (right):

http://codereview.chromium.org/8566037/diff/19001/base/message_pump_observer....
base/message_pump_observer.h:42: virtual void OnMessagePumpDestroying() {}
nit: I think a better name would be WillDestroyMessagePump.  Notice how that
makes the comment unnecessary / redundant?

I also recommend listing destruction notifications first.

[sadrul, 2011-11-30 13:49:00]

On 2011/11/30 06:30:31, darin wrote:
> Is there some reason why MessageLoop::DestructionObserver is insufficient? 
Why
> do you need to separately know when the MessagePump is being destroyed?

That's wonderful. Using MessageLoop::DestructionObserver is sufficient (and
cleaner), I didn't know it existed. I switched to using that. Thank you very
much for the pointer.

For base/, there is now only a small change remaining related to how
message_pump_x calls the observers.

[sadrul, 2011-11-30 13:54:58]

http://codereview.chromium.org/8566037/diff/14009/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8566037/diff/14009/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:311:
MessageLoopForUI::current()->RemoveObserver(this);
On 2011/11/30 00:04:16, oshima wrote:
> nit: it may be better to put them in reverse order to match the order in
> constructor.

Done.

[Ben Goodger (Google), 2011-11-30 20:51:55]

UI bits lgtm.

[DaveMoore, 2011-11-30 23:00:26]

LGTM

[sadrul, 2011-12-01 16:21:19]

Thanks for the reviews! I am going to TBR the change in base/