[Aura] Support additonal event filters in DesktopEventFilter.

[Aura] Support additonal event filters in DesktopEventFilter.

BUG=none.
TEST=DesktopEventFilter.AddtionalFilters test should pass.


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

[sky, 2011-11-14 21:29:56]

http://codereview.chromium.org/8565018/diff/1/ui/aura/test/test_event_filter.h
File ui/aura/test/test_event_filter.h (right):

http://codereview.chromium.org/8565018/diff/1/ui/aura/test/test_event_filter....
ui/aura/test/test_event_filter.h:18: class TestEventFilter : public EventFilter
{
Add description.

http://codereview.chromium.org/8565018/diff/1/ui/aura/test/test_event_filter....
ui/aura/test/test_event_filter.h:23: void ResetCounts();
Add description.

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
File ui/aura_shell/desktop_event_filter.cc (right):

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
ui/aura_shell/desktop_event_filter.cc:52: }
Should the destructor delete any filters?

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
ui/aura_shell/desktop_event_filter.cc:132: for (EventFilters::iterator it =
filters_.begin();
Shouldn't this have the same semantics as Desktop that if one filter handles it
no other filters get it and true is returned?

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
File ui/aura_shell/desktop_event_filter.h (right):

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
ui/aura_shell/desktop_event_filter.h:51: typedef std::vector<aura::EventFilter*>
EventFilters;
typedefs first in a section. That said, you should use ObserverList for this to
handle the case where an observer removes itself during notification.

[xiyuan, 2011-11-14 22:42:42]

All done. Please take another look. Thanks.

http://codereview.chromium.org/8565018/diff/1/ui/aura/test/test_event_filter.h
File ui/aura/test/test_event_filter.h (right):

http://codereview.chromium.org/8565018/diff/1/ui/aura/test/test_event_filter....
ui/aura/test/test_event_filter.h:18: class TestEventFilter : public EventFilter
{
On 2011/11/14 21:29:56, sky wrote:
> Add description.

Done.

http://codereview.chromium.org/8565018/diff/1/ui/aura/test/test_event_filter....
ui/aura/test/test_event_filter.h:23: void ResetCounts();
On 2011/11/14 21:29:56, sky wrote:
> Add description.

Done.

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
File ui/aura_shell/desktop_event_filter.cc (right):

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
ui/aura_shell/desktop_event_filter.cc:52: }
On 2011/11/14 21:29:56, sky wrote:
> Should the destructor delete any filters?

Added comments and use an ObserverList that check emtpy on destruction for
filters_.

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
ui/aura_shell/desktop_event_filter.cc:132: for (EventFilters::iterator it =
filters_.begin();
On 2011/11/14 21:29:56, sky wrote:
> Shouldn't this have the same semantics as Desktop that if one filter handles
it
> no other filters get it and true is returned?

I actually did that and it's part of the loop end condition. :)

The code is changed to use ObserverList iterator. handled is checked as part of
the while condition. If any filter consumes the event, the loop will break out
and other filters will not get it.

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
File ui/aura_shell/desktop_event_filter.h (right):

http://codereview.chromium.org/8565018/diff/1/ui/aura_shell/desktop_event_fil...
ui/aura_shell/desktop_event_filter.h:51: typedef std::vector<aura::EventFilter*>
EventFilters;
On 2011/11/14 21:29:56, sky wrote:
> typedefs first in a section. That said, you should use ObserverList for this
to
> handle the case where an observer removes itself during notification.

Done.

[sky, 2011-11-15 00:08:07]

LGTM

[commit-bot: I haz the power, 2011-11-15 00:59:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/8565018/6001

[commit-bot: I haz the power, 2011-11-15 02:52:15]

Change committed as 110022