Extend EventFilter to handle the case where EventMatchers from event X get

Fix bug in EventFilter that arises when matchers are added to multiple events.

This change fixes a bug in EventFilter to handle the case where the existence of matching EventMatchers for event X causes matching against an event with name Y to crash. It also changes the representation of EventMatcher so that it owns the entire filter dictionary.


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

[koz (OOO until 15th September), 2012-06-12 22:50:18]

These changes support the implementation of event filtering in
http://codereview.chromium.org/10514013/.

[Matt Perry, 2012-06-12 23:33:37]

https://chromiumcodereview.appspot.com/10534125/diff/1/chrome/common/extensio...
File chrome/common/extensions/event_filtering_info.cc (right):

https://chromiumcodereview.appspot.com/10534125/diff/1/chrome/common/extensio...
chrome/common/extensions/event_filtering_info.cc:30: value.SetString("testing",
"hello");
remove?

[koz (OOO until 15th September), 2012-06-12 23:47:09]

https://chromiumcodereview.appspot.com/10534125/diff/1/chrome/common/extensio...
File chrome/common/extensions/event_filtering_info.cc (right):

https://chromiumcodereview.appspot.com/10534125/diff/1/chrome/common/extensio...
chrome/common/extensions/event_filtering_info.cc:30: value.SetString("testing",
"hello");
On 2012/06/12 23:33:37, Matt Perry wrote:
> remove?

Done.

[battre, 2012-06-13 09:22:04]

https://chromiumcodereview.appspot.com/10534125/diff/6001/chrome/common/exten...
File chrome/common/extensions/event_filter.h (right):

https://chromiumcodereview.appspot.com/10534125/diff/6001/chrome/common/exten...
chrome/common/extensions/event_filter.h:75: EventMatcher* event_matcher() const
{
remove this const?

https://chromiumcodereview.appspot.com/10534125/diff/6001/chrome/common/exten...
File chrome/common/extensions/event_filtering_info.h (left):

https://chromiumcodereview.appspot.com/10534125/diff/6001/chrome/common/exten...
chrome/common/extensions/event_filtering_info.h:34:
DISALLOW_COPY_AND_ASSIGN(EventFilteringInfo);
Can you add a comment "// Allow implicit copy and assignment"?

[koz (OOO until 15th September), 2012-06-14 02:22:25]

https://chromiumcodereview.appspot.com/10534125/diff/6001/chrome/common/exten...
File chrome/common/extensions/event_filter.h (right):

https://chromiumcodereview.appspot.com/10534125/diff/6001/chrome/common/exten...
chrome/common/extensions/event_filter.h:75: EventMatcher* event_matcher() const
{
On 2012/06/13 09:22:04, battre wrote:
> remove this const?

Done.

https://chromiumcodereview.appspot.com/10534125/diff/6001/chrome/common/exten...
File chrome/common/extensions/event_filtering_info.h (left):

https://chromiumcodereview.appspot.com/10534125/diff/6001/chrome/common/exten...
chrome/common/extensions/event_filtering_info.h:34:
DISALLOW_COPY_AND_ASSIGN(EventFilteringInfo);
On 2012/06/13 09:22:04, battre wrote:
> Can you add a comment "// Allow implicit copy and assignment"?

Done.

[koz (OOO until 15th September), 2012-06-18 00:57:20]

Friendly ping.

[battre, 2012-06-18 12:29:17]

LGTM with nits.

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
File chrome/common/extensions/event_filter.cc (right):

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_filter.cc:68: EventMatcher*
EventFilter::GetEventMatcher(MatcherID id) {
opt: DCHECK(id_to_event_name_.find(id) != id_to_event_name_.end());

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_filter.cc:73: const std::string&
EventFilter::GetEventName(MatcherID id) {
same here

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
File chrome/common/extensions/event_filter.h (right):

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_filter.h:94: // Adds the list of filters to the
URL matcher, having matches for those URLs
can up update the comment?

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
File chrome/common/extensions/event_matcher.cc (right):

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_matcher.cc:28: base::ListValue* url_filters;
nit: initialize to NULL?

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_matcher.cc:35: base::ListValue* url_filters;
nit: initialize to NULL?

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
File chrome/common/extensions/event_matcher.h (right):

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_matcher.h:33: return GetURLFilterCount() != 0;
move to .cc file?

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_matcher.h:41: scoped_ptr<base::DictionaryValue>
filter_;
can we add a comment about the content of filter_? I suppose this corresponds to
a JSON type definition.

[Matt Perry, 2012-06-18 18:17:12]

lgtm

[koz (OOO until 15th September), 2012-06-19 00:07:57]

Thanks, guys.

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
File chrome/common/extensions/event_filter.cc (right):

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_filter.cc:68: EventMatcher*
EventFilter::GetEventMatcher(MatcherID id) {
On 2012/06/18 12:29:17, battre wrote:
> opt: DCHECK(id_to_event_name_.find(id) != id_to_event_name_.end());

Done.

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_filter.cc:73: const std::string&
EventFilter::GetEventName(MatcherID id) {
On 2012/06/18 12:29:17, battre wrote:
> same here

Done.

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
File chrome/common/extensions/event_filter.h (right):

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_filter.h:94: // Adds the list of filters to the
URL matcher, having matches for those URLs
On 2012/06/18 12:29:17, battre wrote:
> can up update the comment?

Done.

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
File chrome/common/extensions/event_matcher.cc (right):

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_matcher.cc:28: base::ListValue* url_filters;
On 2012/06/18 12:29:17, battre wrote:
> nit: initialize to NULL?

Done.

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_matcher.cc:35: base::ListValue* url_filters;
On 2012/06/18 12:29:17, battre wrote:
> nit: initialize to NULL?

Done.

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
File chrome/common/extensions/event_matcher.h (right):

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_matcher.h:33: return GetURLFilterCount() != 0;
On 2012/06/18 12:29:17, battre wrote:
> move to .cc file?

Done.

https://chromiumcodereview.appspot.com/10534125/diff/4002/chrome/common/exten...
chrome/common/extensions/event_matcher.h:41: scoped_ptr<base::DictionaryValue>
filter_;
On 2012/06/18 12:29:17, battre wrote:
> can we add a comment about the content of filter_? I suppose this corresponds
to
> a JSON type definition.

Done.

[commit-bot: I haz the power, 2012-06-19 00:08:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10534125/17001

[commit-bot: I haz the power, 2012-06-19 04:15:34]

Try job failure for 10534125-17001 (retry) (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-19 04:24:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10534125/17001

[commit-bot: I haz the power, 2012-06-19 06:15:13]

Try job failure for 10534125-17001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-19 06:49:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10534125/21002

[commit-bot: I haz the power, 2012-06-19 12:55:42]

Try job failure for 10534125-21002 (retry) (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-20 03:00:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10534125/21002

[commit-bot: I haz the power, 2012-06-20 04:45:35]

Change committed as 143142