Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10388135:
Add common code for extensions event filtering. (Closed)
|
Created:
8 years, 7 months ago by koz (OOO until 15th September) Modified:
8 years, 7 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, mpcomplete, not at google - send to devlin, benwells Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd common code for extensions event filtering.
FilteredEventRouter will be used in the browser and the renderer to match
events to interested parties. In the browser it will be used to send events
only to renderers that are interested in them, and in the renderer it will be
used to send events to the correct JS listeners.
BUG=121479
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137829
Patch Set 1 #Patch Set 2 : self review #
Total comments: 51
Patch Set 3 : respond to comments #
Total comments: 21
Patch Set 4 : respond to comments #
Total comments: 20
Patch Set 5 : respond to comments #Patch Set 6 : rebase #
Messages
Total messages: 17 (0 generated)
FilteredEventRouter is intended to be used analogously on the browser and the renderer to route events to the right renderer (in the browser case) and JS listener (in the renderer case). It's dead code currently, but the test should illustrate its correctness (and intended use). (See the filtered events design doc for more details.)
battre: am I using URLMatcher correctly? mpcomplete: does this align with your vision of general event filtering, not just URL filtering?
https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_filtering_info.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_filtering_info.h:30: bool has_url_; optional: could you use GURL::is_empty()? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_filtering_info.h:31: GURL url_; DISALLOW_COPY_AND_ASSIGN https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_matcher.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.cc:5: #include "base/logging.h" not necessary https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_matcher.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.h:9: #include "base/compiler_specific.h" not necessary https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.h:12: #include "chrome/common/extensions/event_filtering_info.h" forward declaration? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.h:23: virtual ~EventMatcher(); why virtual? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.h:39: scoped_ptr<base::ListValue> url_filters_; DISALLOW_COPY_AND_ASSIGN https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:20: scoped_ptr<EventMatcher> matcher) { nit: indentation https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:22: event_matchers_[event_name]; what is this? Move it down by two lines to move it into the vincinity of the event_matchers_.find? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:27: matcher_map->second[id] = linked_ptr<EventMatcher>(matcher.release()); could you simplify lines 22, 25-27 to event_matchers_[event_name][id] = linked_ptr<...>(...); ? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:31: void FilteredEventRouter::AddURLConditionsToURLMatcher(int id, nit: indentation (move parameter to next line or line up parameters at parenthesis) https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:38: CHECK(url_filters->GetDictionary(i, &url_filter)); This is passed by an extension, right? I think we should not CHECK on invalid input. Or is it guarded by the schema validation? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:45: &error)); Handle error? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:60: std::set<int> FilteredEventRouter::MatchEvent(const std::string& event_name, nit: indentation https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:22: class FilteredEventRouter { Given that we are dealing with so many IDs, could you replace all ints with some typedefs. This should simplify tracking which IDs are actually referenced. E.g. in the condition_set_id_to_event_matcher_id_, we would use URLMatcherConditionSet::ID as the first template parameter. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:27: int AddEventMatcher(const std::string& event_name, Please document return values https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:59: std::map<int, std::string> id_to_event_name_; DISALLOW_COPY_AND_ASSIGN https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:59: std::map<int, std::string> id_to_event_name_; this class has a lot of maps. I wonder whether we can simplify this. Can we get rid of id_to_event_name_ if we change the signature of RemoveEventMatcher to take both the event_name and the id? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router_unittest.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router_unittest.cc:26: scoped_ptr<base::ListValue> Singleton(scoped_ptr<base::Value> value) { Rename this to ListWithValue? My understanding of a singleton would be this: http://en.wikipedia.org/wiki/Singleton_pattern https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router_unittest.cc:35: matcher->set_url_filters(Singleton(HostSuffixDict(""))); I think from the extension API, AllURLs should be represented by an empty list.
https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_filtering_info.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_filtering_info.cc:9: EventFilteringInfo::EventFilteringInfo() { need to init has_url_ https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_filtering_info.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_filtering_info.h:23: EventFilteringInfo(); any complex class should define an empty destructor in the .cc file. otherwise an inline default one will be generated for every compilation unit. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:22: class FilteredEventRouter { How will this work in the multiprocess world of event pages? Will the event IDs be synchronized between browser and extension processes? What happens when the extension process is shut down and restarted? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router_unittest.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router_unittest.cc:23: return scoped_ptr<base::Value>(dict.release()); does dict.Pass() not work here?
Thanks, guys. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_filtering_info.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_filtering_info.cc:9: EventFilteringInfo::EventFilteringInfo() { On 2012/05/15 20:42:59, Matt Perry wrote: > need to init has_url_ Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_filtering_info.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_filtering_info.h:23: EventFilteringInfo(); On 2012/05/15 20:42:59, Matt Perry wrote: > any complex class should define an empty destructor in the .cc file. otherwise > an inline default one will be generated for every compilation unit. Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_filtering_info.h:30: bool has_url_; On 2012/05/15 13:12:32, battre wrote: > optional: could you use GURL::is_empty()? Better to be explicit, I think. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_filtering_info.h:31: GURL url_; On 2012/05/15 13:12:32, battre wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_matcher.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.cc:5: #include "base/logging.h" On 2012/05/15 13:12:32, battre wrote: > not necessary Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/event_matcher.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.h:9: #include "base/compiler_specific.h" On 2012/05/15 13:12:32, battre wrote: > not necessary Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.h:12: #include "chrome/common/extensions/event_filtering_info.h" On 2012/05/15 13:12:32, battre wrote: > forward declaration? Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.h:23: virtual ~EventMatcher(); On 2012/05/15 13:12:32, battre wrote: > why virtual? Ah, cheers. Done. Originally I had implemented this in a more opaque fashion, and had subclasses like AllEvents() and NoEvents(), but I realised that in order for FilteredEventRouter to be able to match over multiple EventMatcher's URLs simultaneously I'd have to expose the data they hold anyway, so I got rid of them. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/event_matcher.h:39: scoped_ptr<base::ListValue> url_filters_; On 2012/05/15 13:12:32, battre wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:20: scoped_ptr<EventMatcher> matcher) { On 2012/05/15 13:12:32, battre wrote: > nit: indentation Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:22: event_matchers_[event_name]; On 2012/05/15 13:12:32, battre wrote: > what is this? Move it down by two lines to move it into the vincinity of the > event_matchers_.find? With your next comment this becomes unnecessary. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:27: matcher_map->second[id] = linked_ptr<EventMatcher>(matcher.release()); On 2012/05/15 13:12:32, battre wrote: > could you simplify lines 22, 25-27 to event_matchers_[event_name][id] = > linked_ptr<...>(...); ? True! Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:31: void FilteredEventRouter::AddURLConditionsToURLMatcher(int id, On 2012/05/15 13:12:32, battre wrote: > nit: indentation (move parameter to next line or line up parameters at > parenthesis) Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:38: CHECK(url_filters->GetDictionary(i, &url_filter)); On 2012/05/15 13:12:32, battre wrote: > This is passed by an extension, right? I think we should not CHECK on invalid > input. Or is it guarded by the schema validation? Good point, I've handled the error cases now. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:45: &error)); On 2012/05/15 13:12:32, battre wrote: > Handle error? Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:60: std::set<int> FilteredEventRouter::MatchEvent(const std::string& event_name, On 2012/05/15 13:12:32, battre wrote: > nit: indentation Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:22: class FilteredEventRouter { On 2012/05/15 13:12:32, battre wrote: > Given that we are dealing with so many IDs, could you replace all ints with some > typedefs. This should simplify tracking which IDs are actually referenced. > > E.g. in the condition_set_id_to_event_matcher_id_, we would use > URLMatcherConditionSet::ID as the first template parameter. Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:22: class FilteredEventRouter { On 2012/05/15 20:42:59, Matt Perry wrote: > How will this work in the multiprocess world of event pages? Will the event IDs > be synchronized between browser and extension processes? What happens when the > extension process is shut down and restarted? The idea is that each process will maintain their own FilteredEventRouter which will not synchronize event IDs. When an extension process is started and calls to addListener() are made they will be sent to the browser in an IPC where it is determined if that event/filter is already being listened to and does nothing if it is. Though thinking about it this deduplicating of event/filter pairs will have to be done in linear time (on both the renderer and the browser). If we have users specify an ID for their filtered event listeners then we can do it in constant time. It would also mean we wouldn't need to have a FilteredEventRouter on the renderer to do matching. I think we talked about this before briefly and adding an ID parameter wasn't considered as completely insane. What do you think? Does a linear time lookup on addListener() warrant changing the interface? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:27: int AddEventMatcher(const std::string& event_name, On 2012/05/15 13:12:32, battre wrote: > Please document return values Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:59: std::map<int, std::string> id_to_event_name_; On 2012/05/15 13:12:32, battre wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:59: std::map<int, std::string> id_to_event_name_; On 2012/05/15 13:12:32, battre wrote: > this class has a lot of maps. I wonder whether we can simplify this. Can we get > rid of id_to_event_name_ if we change the signature of RemoveEventMatcher to > take both the event_name and the id? We could, but wouldn't that just make the callers have to do more work by remembering the id and the name? https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router_unittest.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router_unittest.cc:23: return scoped_ptr<base::Value>(dict.release()); On 2012/05/15 20:42:59, Matt Perry wrote: > does dict.Pass() not work here? No, because dict.Pass() will return a scoped_ptr<base::DictionaryValue> which the compiler complains about. https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router_unittest.cc:26: scoped_ptr<base::ListValue> Singleton(scoped_ptr<base::Value> value) { On 2012/05/15 13:12:32, battre wrote: > Rename this to ListWithValue? My understanding of a singleton would be this: > http://en.wikipedia.org/wiki/Singleton_pattern Done (ValueAsList okay?). https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router_unittest.cc:35: matcher->set_url_filters(Singleton(HostSuffixDict(""))); On 2012/05/15 13:12:32, battre wrote: > I think from the extension API, AllURLs should be represented by an empty list. Done.
lgtm https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:22: class FilteredEventRouter { On 2012/05/16 00:14:00, koz wrote: > On 2012/05/15 20:42:59, Matt Perry wrote: > > How will this work in the multiprocess world of event pages? Will the event > IDs > > be synchronized between browser and extension processes? What happens when the > > extension process is shut down and restarted? > > The idea is that each process will maintain their own FilteredEventRouter which > will not synchronize event IDs. When an extension process is started and calls > to addListener() are made they will be sent to the browser in an IPC where it is > determined if that event/filter is already being listened to and does nothing if > it is. > > Though thinking about it this deduplicating of event/filter pairs will have to > be done in linear time (on both the renderer and the browser). If we have users > specify an ID for their filtered event listeners then we can do it in constant > time. It would also mean we wouldn't need to have a FilteredEventRouter on the > renderer to do matching. > > I think we talked about this before briefly and adding an ID parameter wasn't > considered as completely insane. What do you think? Does a linear time lookup on > addListener() warrant changing the interface? If I understand correctly, you're saying the events will have to be filtered twice. i.e. once in the browser process to decide which processes to dispatch to, then again in the renderer to decide which listeners to call. Maybe that's not a big deal. I would expect process creation and IPC to take up the majority of the time in event dispatch. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... File chrome/common/extensions/event_matcher.h (right): https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/event_matcher.h:20: class EventMatcher { Maybe FilteredEventMatcher (and likewise FilteredEventInfo) for symmetry?
https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:22: class FilteredEventRouter { On 2012/05/16 00:37:32, Matt Perry wrote: > On 2012/05/16 00:14:00, koz wrote: > > On 2012/05/15 20:42:59, Matt Perry wrote: > > > How will this work in the multiprocess world of event pages? Will the event > > IDs > > > be synchronized between browser and extension processes? What happens when > the > > > extension process is shut down and restarted? > > > > The idea is that each process will maintain their own FilteredEventRouter > which > > will not synchronize event IDs. When an extension process is started and calls > > to addListener() are made they will be sent to the browser in an IPC where it > is > > determined if that event/filter is already being listened to and does nothing > if > > it is. > > > > Though thinking about it this deduplicating of event/filter pairs will have to > > be done in linear time (on both the renderer and the browser). If we have > users > > specify an ID for their filtered event listeners then we can do it in constant > > time. It would also mean we wouldn't need to have a FilteredEventRouter on the > > renderer to do matching. > > > > I think we talked about this before briefly and adding an ID parameter wasn't > > considered as completely insane. What do you think? Does a linear time lookup > on > > addListener() warrant changing the interface? > > If I understand correctly, you're saying the events will have to be filtered > twice. i.e. once in the browser process to decide which processes to dispatch > to, then again in the renderer to decide which listeners to call. Maybe that's > not a big deal. I would expect process creation and IPC to take up the majority > of the time in event dispatch. Actually I was more thinking about the cost of addListener() / removeListener(), not dispatch. When the browser receives IPCs for adding an event/filter pair, it will have to look through the filters already registered for that event and see if this new one is a duplicate (otherwise we have that infinite listener problem with event pages always adding fresh listeners). We also need to perform that duplicate check on the renderer so that code like this works: chrome.filteredEvent.addListener(cb, filter); // Sends IPC chrome.filteredEvent.addListener(cb, filter); // Doesn't send IPC chrome.filteredEvent.removeListener(cb); // Doesn't send IPC - shouldn't stop browser from sending us 'filteredEvent's. It'll only be linear in the number of listeners for a particular event on a particular extension, so perhaps it wouldn't be that bad...
https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... File chrome/common/extensions/filtered_event_router.h (right): https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:22: class FilteredEventRouter { On 2012/05/16 01:01:21, koz wrote: > On 2012/05/16 00:37:32, Matt Perry wrote: > > On 2012/05/16 00:14:00, koz wrote: > > > On 2012/05/15 20:42:59, Matt Perry wrote: > > > > How will this work in the multiprocess world of event pages? Will the > event > > > IDs > > > > be synchronized between browser and extension processes? What happens when > > the > > > > extension process is shut down and restarted? > > > > > > The idea is that each process will maintain their own FilteredEventRouter > > which > > > will not synchronize event IDs. When an extension process is started and > calls > > > to addListener() are made they will be sent to the browser in an IPC where > it > > is > > > determined if that event/filter is already being listened to and does > nothing > > if > > > it is. > > > > > > Though thinking about it this deduplicating of event/filter pairs will have > to > > > be done in linear time (on both the renderer and the browser). If we have > > users > > > specify an ID for their filtered event listeners then we can do it in > constant > > > time. It would also mean we wouldn't need to have a FilteredEventRouter on > the > > > renderer to do matching. > > > > > > I think we talked about this before briefly and adding an ID parameter > wasn't > > > considered as completely insane. What do you think? Does a linear time > lookup > > on > > > addListener() warrant changing the interface? > > > > If I understand correctly, you're saying the events will have to be filtered > > twice. i.e. once in the browser process to decide which processes to dispatch > > to, then again in the renderer to decide which listeners to call. Maybe that's > > not a big deal. I would expect process creation and IPC to take up the > majority > > of the time in event dispatch. > > Actually I was more thinking about the cost of addListener() / removeListener(), > not dispatch. When the browser receives IPCs for adding an event/filter pair, it > will have to look through the filters already registered for that event and see > if this new one is a duplicate (otherwise we have that infinite listener problem > with event pages always adding fresh listeners). > > We also need to perform that duplicate check on the renderer so that code like > this works: > > chrome.filteredEvent.addListener(cb, filter); // Sends IPC > chrome.filteredEvent.addListener(cb, filter); // Doesn't send IPC > chrome.filteredEvent.removeListener(cb); // Doesn't send IPC - shouldn't stop > browser from sending us 'filteredEvent's. What about: chrome.filteredEvent.addListener(cb1, filter1); chrome.filteredEvent.addListener(cb1, filter2); chrome.filteredEvent.addListener(cb2, filter1); Does the second or third call send an IPC? I don't think we need to optimize for the case where both cb and filter are the same across 2 listeners.
On 2012/05/16 01:17:06, Matt Perry wrote: > https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... > File chrome/common/extensions/filtered_event_router.h (right): > > https://chromiumcodereview.appspot.com/10388135/diff/2001/chrome/common/exten... > chrome/common/extensions/filtered_event_router.h:22: class FilteredEventRouter { > On 2012/05/16 01:01:21, koz wrote: > > On 2012/05/16 00:37:32, Matt Perry wrote: > > > On 2012/05/16 00:14:00, koz wrote: > > > > On 2012/05/15 20:42:59, Matt Perry wrote: > > > > > How will this work in the multiprocess world of event pages? Will the > > event > > > > IDs > > > > > be synchronized between browser and extension processes? What happens > when > > > the > > > > > extension process is shut down and restarted? > > > > > > > > The idea is that each process will maintain their own FilteredEventRouter > > > which > > > > will not synchronize event IDs. When an extension process is started and > > calls > > > > to addListener() are made they will be sent to the browser in an IPC where > > it > > > is > > > > determined if that event/filter is already being listened to and does > > nothing > > > if > > > > it is. > > > > > > > > Though thinking about it this deduplicating of event/filter pairs will > have > > to > > > > be done in linear time (on both the renderer and the browser). If we have > > > users > > > > specify an ID for their filtered event listeners then we can do it in > > constant > > > > time. It would also mean we wouldn't need to have a FilteredEventRouter on > > the > > > > renderer to do matching. > > > > > > > > I think we talked about this before briefly and adding an ID parameter > > wasn't > > > > considered as completely insane. What do you think? Does a linear time > > lookup > > > on > > > > addListener() warrant changing the interface? > > > > > > If I understand correctly, you're saying the events will have to be filtered > > > twice. i.e. once in the browser process to decide which processes to > dispatch > > > to, then again in the renderer to decide which listeners to call. Maybe > that's > > > not a big deal. I would expect process creation and IPC to take up the > > majority > > > of the time in event dispatch. > > > > Actually I was more thinking about the cost of addListener() / > removeListener(), > > not dispatch. When the browser receives IPCs for adding an event/filter pair, > it > > will have to look through the filters already registered for that event and > see > > if this new one is a duplicate (otherwise we have that infinite listener > problem > > with event pages always adding fresh listeners). > > > > We also need to perform that duplicate check on the renderer so that code like > > this works: > > > > chrome.filteredEvent.addListener(cb, filter); // Sends IPC > > chrome.filteredEvent.addListener(cb, filter); // Doesn't send IPC > > chrome.filteredEvent.removeListener(cb); // Doesn't send IPC - shouldn't stop > > browser from sending us 'filteredEvent's. > > What about: > > chrome.filteredEvent.addListener(cb1, filter1); > chrome.filteredEvent.addListener(cb1, filter2); > chrome.filteredEvent.addListener(cb2, filter1); > > Does the second or third call send an IPC? I don't think we need to optimize for > the case where both cb and filter are the same across 2 listeners. I guess the rule should be that IPCs are fired on the first listener added of any event/filter pair, and on the last listener removed of any event/filter pair. So that would mean #2 yes, #3 no. And just to be clear the expensive case here is not when the filters are the same, but when we add lots of filters to the same event. It'll be N^2 cost to add N listeners because we need to know when duplicates are added.
https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... File chrome/common/extensions/event_matcher.h (right): https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/event_matcher.h:20: class EventMatcher { On 2012/05/16 00:37:32, Matt Perry wrote: > Maybe FilteredEventMatcher (and likewise FilteredEventInfo) for symmetry? Or EventFilter? https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... File chrome/common/extensions/filtered_event_router.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:47: LOG(ERROR) << "CreateFromURLFilterDictionary failed: " << error; Here we have to add url_matcher_.ClearUnusedConditionSets(); In order to optimize for memory consumption, the ConditionSets contain only references to strings that are owned by the URLMatcherConditionFactory. These may leak here. I have created a CL to improve the documentation: https://chromiumcodereview.appspot.com/10398054/ https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:57: std::string FilteredEventRouter::RemoveEventMatcher(MatcherID id) { you should remove the event condition sets from url_matcher_ as well. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:75: std::set<MatcherID> matching_condition_set_ids = this should be std::set<URLMatcherConditionSet::ID> https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:77: for (std::set<MatcherID>::iterator it = matching_condition_set_ids.begin(); same here https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:78: it != matching_condition_set_ids.end(); it++) { nit: +1 space https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:84: it != matcher_map->second.end(); it++) { nit: +1 space https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:84: it != matcher_map->second.end(); it++) { This looks inefficient and defeats the purpose of the URLMatcher a little bit, because you need to do the linear scan. I think it would be better to add some code before line 53: if (condition_sets.empty()) { // Add a new condition set using URLMatcherFactory::CreateFromURLFilterDictionary that contains an empty url_filter. } Then you could iterate over url_matched_matchers, lookup the respective EventMatchers and do the test? for (... it = url_matched_matchers.begin(); it != ...; ++it) { ...iterator it2 = matcher_map.find(*id); if (it2 != matcher_map.end() && it2->MatchNonURLCriteria) { ... } } WDYT? https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... File chrome/common/extensions/filtered_event_router.h (right): https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:31: scoped_ptr<EventMatcher> matcher); As the event filters will come from extension code, it will be useful to pass an error message here. Does not need to go into this CL. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:45: typedef std::map<std::string, EventMatcherMap> EventMatcherMultiMap; optional: typedef std::string EventName; typedef std::map<EventName, EventMatcherMap> EventMatcherMultiMap; alternatively: add comment?
Thanks for the thorough review! https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... File chrome/common/extensions/event_matcher.h (right): https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/event_matcher.h:20: class EventMatcher { On 2012/05/16 13:35:12, battre wrote: > On 2012/05/16 00:37:32, Matt Perry wrote: > > Maybe FilteredEventMatcher (and likewise FilteredEventInfo) for symmetry? > > Or EventFilter? EventFilter is a good one, I'll go with that. (I disliked router because there is already an ExtensionEventRouter which operates at a higher level than my suggested FilteredEventRouter.) https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... File chrome/common/extensions/filtered_event_router.cc (right): https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:47: LOG(ERROR) << "CreateFromURLFilterDictionary failed: " << error; On 2012/05/16 13:35:12, battre wrote: > Here we have to add > url_matcher_.ClearUnusedConditionSets(); > > In order to optimize for memory consumption, the ConditionSets contain only > references to strings that are owned by the URLMatcherConditionFactory. These > may leak here. I have created a CL to improve the documentation: > https://chromiumcodereview.appspot.com/10398054/ Done. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:57: std::string FilteredEventRouter::RemoveEventMatcher(MatcherID id) { On 2012/05/16 13:35:12, battre wrote: > you should remove the event condition sets from url_matcher_ as well. Done. I've added an EventMatcherEntry class that handles the addition and removal of condition sets to the URLMatcher on creation/destruction. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:75: std::set<MatcherID> matching_condition_set_ids = On 2012/05/16 13:35:12, battre wrote: > this should be std::set<URLMatcherConditionSet::ID> Done. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:77: for (std::set<MatcherID>::iterator it = matching_condition_set_ids.begin(); On 2012/05/16 13:35:12, battre wrote: > same here Done. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:78: it != matching_condition_set_ids.end(); it++) { On 2012/05/16 13:35:12, battre wrote: > nit: +1 space Fixed here and elsewhere. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:84: it != matcher_map->second.end(); it++) { On 2012/05/16 13:35:12, battre wrote: > nit: +1 space Done. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.cc:84: it != matcher_map->second.end(); it++) { On 2012/05/16 13:35:12, battre wrote: > This looks inefficient and defeats the purpose of the URLMatcher a little bit, > because you need to do the linear scan. > > I think it would be better to add some code before line 53: > > if (condition_sets.empty()) { > // Add a new condition set using > URLMatcherFactory::CreateFromURLFilterDictionary that contains an empty > url_filter. > } > > Then you could iterate over url_matched_matchers, lookup the respective > EventMatchers and do the test? > for (... it = url_matched_matchers.begin(); it != ...; ++it) { > ...iterator it2 = matcher_map.find(*id); > if (it2 != matcher_map.end() && it2->MatchNonURLCriteria) { ... } > } > > WDYT? That's a good idea! Done. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... File chrome/common/extensions/filtered_event_router.h (right): https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:31: scoped_ptr<EventMatcher> matcher); On 2012/05/16 13:35:12, battre wrote: > As the event filters will come from extension code, it will be useful to pass an > error message here. Does not need to go into this CL. Cool, I've added a TODO. https://chromiumcodereview.appspot.com/10388135/diff/7003/chrome/common/exten... chrome/common/extensions/filtered_event_router.h:45: typedef std::map<std::string, EventMatcherMap> EventMatcherMultiMap; On 2012/05/16 13:35:12, battre wrote: > optional: > typedef std::string EventName; > typedef std::map<EventName, EventMatcherMap> EventMatcherMultiMap; > > alternatively: add comment? I added comments here.
LGTM http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... File chrome/common/extensions/event_filter.cc (right): http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.cc:24: url_matcher_->RemoveConditionSets(condition_set_ids_); This architecture makes the destruction of the EventFilter at browser shutdown somewhat expensive. Maybe add a function to clear condition_set_ids_ and call it from ~EventFilter(), because they are cleared when the URLMatcher dies anyway? http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.cc:37: scoped_ptr<EventMatcher> matcher) { nit: indent +1 http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.cc:113: for (std::set<URLMatcherConditionSet::ID>::iterator it = I would recommend to combine these two loop. I don't think it is necessary to store everything into url_matched_matchers first. I would add a matchers.reserve(url_matched_matchers.size()) infront of this loop. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... File chrome/common/extensions/event_filter.h (right): http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.h:69: URLMatcher* url_matcher_; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.h:89: EventMatcherMultiMap event_matchers_; Add a comment that event_matchers_ must appear after url_matcher_ because it deregisters its content from url_matcher_ on destruction. Alternatively: add event_matchers_.clear() with such a comment in ~EventFilter(). http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... File chrome/common/extensions/event_filter_unittest.cc (right): http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter_unittest.cc:24: } remove? http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter_unittest.cc:83: ASSERT_TRUE(matches.count(id) > 0); nit: ASSERT_EQ(1u, matches.count(id)); http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter_unittest.cc:190: int id1 = event_filter_.AddEventMatcher("event1", matcher.Pass()); add EXPECT_TRUE(matcher->IsURLMatcherEmpty()); http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... File chrome/common/extensions/event_matcher.h (right): http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_matcher.h:19: // MatchNonURLCriteria() - URL matching is handled by EventRouter. s/EventRouter/EventFilter/? http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_matcher.h:20: class EventMatcher { optional: rename EventMatcher to EventFilterCriterion as EventFilter and EventMatcher are somewhat close?
Thanks, Dominic! http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... File chrome/common/extensions/event_filter.cc (right): http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.cc:24: url_matcher_->RemoveConditionSets(condition_set_ids_); On 2012/05/17 08:50:51, battre wrote: > This architecture makes the destruction of the EventFilter at browser shutdown > somewhat expensive. Maybe add a function to clear condition_set_ids_ and call it > from ~EventFilter(), because they are cleared when the URLMatcher dies anyway? Good point. Done. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.cc:37: scoped_ptr<EventMatcher> matcher) { On 2012/05/17 08:50:51, battre wrote: > nit: indent +1 Done. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.cc:113: for (std::set<URLMatcherConditionSet::ID>::iterator it = On 2012/05/17 08:50:51, battre wrote: > I would recommend to combine these two loop. I don't think it is necessary to > store everything into url_matched_matchers first. I would add a > matchers.reserve(url_matched_matchers.size()) infront of this loop. I merged the loops but there is no reserve method on std::set. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... File chrome/common/extensions/event_filter.h (right): http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.h:69: URLMatcher* url_matcher_; On 2012/05/17 08:50:51, battre wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter.h:89: EventMatcherMultiMap event_matchers_; On 2012/05/17 08:50:51, battre wrote: > Add a comment that event_matchers_ must appear after url_matcher_ because it > deregisters its content from url_matcher_ on destruction. Alternatively: add > event_matchers_.clear() with such a comment in ~EventFilter(). I added the comment in ~EventFilter(). http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... File chrome/common/extensions/event_filter_unittest.cc (right): http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter_unittest.cc:24: } On 2012/05/17 08:50:51, battre wrote: > remove? Done. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter_unittest.cc:83: ASSERT_TRUE(matches.count(id) > 0); On 2012/05/17 08:50:51, battre wrote: > nit: ASSERT_EQ(1u, matches.count(id)); Done. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_filter_unittest.cc:190: int id1 = event_filter_.AddEventMatcher("event1", matcher.Pass()); On 2012/05/17 08:50:51, battre wrote: > add EXPECT_TRUE(matcher->IsURLMatcherEmpty()); Done. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... File chrome/common/extensions/event_matcher.h (right): http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_matcher.h:19: // MatchNonURLCriteria() - URL matching is handled by EventRouter. On 2012/05/17 08:50:51, battre wrote: > s/EventRouter/EventFilter/? Done. http://codereview.chromium.org/10388135/diff/12001/chrome/common/extensions/e... chrome/common/extensions/event_matcher.h:20: class EventMatcher { On 2012/05/17 08:50:51, battre wrote: > optional: rename EventMatcher to EventFilterCriterion as EventFilter and > EventMatcher are somewhat close? I think I prefer the more operational EventMatcher here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/10388135/15001
Try job failure for 10388135-15001 (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...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/10388135/15001
Change committed as 137829 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
