Filtered events.

Filtered events.

Makes web_navigation events support filters, eg:

chrome.webNavigation.onBeforeCommitted.addListener(callback, {url: [{hostSuffix: 'google.com'}]});

Now callback will only be called when the event has a URL with a host suffix of google.com.

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=143874

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

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

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

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

This depends on https://chromiumcodereview.appspot.com/10534125/, and extends
upon and makes redundant http://codereview.chromium.org/10381143/

Sorry for the huge change, I can try splitting it up if that would make it
easier to review, but that may be non-trivial, and I'm not sure how much it can
be split up due to the cross-cutting nature of the change.

There are two main parts to this: the browser side and the renderer side. Most
of the rest of the code is just plumbing.

In the browser, in EventExtensionRouter, I've unified the data structures that
store listeners / lazy listeners into one class called EventListenerMap, to
which EventExtensionRouter now offloads the responsibility of managing
listeners. EventListenerMap also handles filtered events, and is written in a
way so that it can be extended to support other types of listeners with minimal
change - all types of listeners (regular, lazy, filtered) are all stored
together as instances of the generic EventListener class. Dispatching code
behaves differently depending on what type of EventListener it is dispatching
to. This approach will hopefully extend naturally to handle adding rules, where
a rule would be just another kind of listener.

The other non-plumbing browser change is in the web_navigation_api files, where
I add EventFilteringInfo to events fired from there.

The renderer side still doesn't support lists of filters as discussed in
http://codereview.chromium.org/10381143/, but I intend for that to be a change
on top of this patch. Most of the essence of the previous patch is still here,
but I continued to work on it in tandem with the browser side because they
evolved together.

[koz (OOO until 15th September), 2012-06-13 00:11:02]

Also, I have made the IPC for adding filtered events have a lazy flag instead of
a separate lazy version of the IPC. As I understand it there will never be a
situation where we fire the lazy version without firing the non-lazy version
(but please let me know if that's not a valid assumption).

[Matt Perry, 2012-06-13 01:24:27]

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:83:
info.SetURL(GURL(url_string));
This feels dirty. Let's just pass the URL in as a formal param to this function.
(Keep the JSONWriter change, though. I like that.)

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/event_listener_map.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.cc:56:
listeners_by_matcher_id_.insert(std::make_pair(id, listener.get()));
listeners_by_matcher_id_[id] = listener.get()

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.cc:65: scoped_ptr<EventMatcher>
EventListenerMap::ParseEventMatcer(
typo - Matcer

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.cc:147:
CHECK(filtered.GetDictionary(*it, &filter));
Prefs can get corrupted, so don't rely on them being valid.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:35: class EventListener {
put all new extensions code in the extensions namespace

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:60: ~EventListenerMap();
nit: newline before and after constructor/destructor pair

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:61: bool
AddListener(scoped_ptr<EventListener> listener);
comment all these methods

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:62: bool
RemoveListener(scoped_ptr<EventListener> listener);
use scoped_ptr in params only when the function needs to assume ownership of the
param. this could simple take a const ref.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:64: scoped_ptr<std::set<const
EventListener*> > GetEventTargets(
why not just return a bare std::set?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:128: std::string id =
extension_id;
unused

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:471: if
(listeners_.HasListener(listener.get()))
This is not quite right. IIUC, here |listener| is the lazy listener (i.e.
|process| is NULL). We want to check if there is a non-lazy listener before
dispatching the event.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:506: registered_events,
indent

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:510: case
chrome::NOTIFICATION_EXTENSION_UNLOADED: {
These top 4 notifications should be handled directly in EventListenerMap.
OnListenerRemoved can be a delegate call back to ExtensionEventRouter.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:112:
extensions::EventFilteringInfo info);
what about the other Dispatch methods?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:178: // If |extension_id| is
non-empty, we load only that extension's page
comment needs updating

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:184: void
DispatchEventToExtension(const std::string& extension_id,
We already have 2 DispatchEventToExtension functions... let's use a different
name.

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
File chrome/common/extensions/value_set.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
chrome/common/extensions/value_set.h:17: class ValueSet {
class comment

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
chrome/common/extensions/value_set.h:17: class ValueSet {
extensions namespace

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/extensions/...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/extensions/...
chrome/renderer/extensions/event_bindings.cc:73: explicit
ExtensionImpl(ExtensionDispatcher* dispatcher,
drop explicit

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:81: var
UnfilteredAttachmentStrategy = function(event) {
How much overhead would it be if we just treated unfiltered events as filtered
events with a NULL filter? That would probably simplify the code a ton, yes?

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:385: function getDeclarativeAPI()
{
is this used?

[battre, 2012-06-13 09:21:54]

A couple of comments. I have to admit, that I did not do a full review, yet,
though.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:33: // that an event can be
dispatched to. This is a lazy listener if |process| is
what is a lazy listener?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:40: scoped_ptr<DictionaryValue>
filter);
what goes into |filter|?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:47: const std::string event_name;
I think according to our style guide, these should either end in _ or be member
of a struct.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:51: int matcher_id;
private:
  DISALLOW_COPY_AND_ASSIGN(EventListener);

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:62: bool
RemoveListener(scoped_ptr<EventListener> listener);
On 2012/06/13 01:24:27, Matt Perry wrote:
> use scoped_ptr in params only when the function needs to assume ownership of
the
> param. this could simple take a const ref.

or a pointer?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:70: bool
AnyListenersForEvent(const std::string& event_name);
HasListenersForEvent? (see "HasListener(const EventListener*)" below)

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:71: bool
AnyListenersForExtension(const std::string& extension_id,
HasListenersForExtension?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:75: void
RemoveLazyListenersFor(const std::string extension_id);
const std::string&

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:79: std::set<std::string>&
unfiltered,
const std::set<...>&

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:57:
extensions::EventFilteringInfo info);
make |info| pass by const-ref?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:86: bool lazy);
what's lazy?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:964: ListValue* filter_list;
initialize to NULL

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1008: DictionaryValue* result;
initialize to NULL.

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
File chrome/common/extensions/value_set.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
chrome/common/extensions/value_set.h:24: int Add(const base::Value* value);
how is the ownership of |value| handled?

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
chrome/common/extensions/value_set.h:48: EntryList entries_;
DISALLOW_COPY_AND_ASSIGN

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

Thanks for the reviews, guys!

[+sky for OWNERS for chrome/browser/{history,bookmarks}]

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:83:
info.SetURL(GURL(url_string));
On 2012/06/13 01:24:27, Matt Perry wrote:
> This feels dirty. Let's just pass the URL in as a formal param to this
function.
> (Keep the JSONWriter change, though. I like that.)

Cool, done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/event_listener_map.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.cc:56:
listeners_by_matcher_id_.insert(std::make_pair(id, listener.get()));
On 2012/06/13 01:24:27, Matt Perry wrote:
> listeners_by_matcher_id_[id] = listener.get()

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.cc:65: scoped_ptr<EventMatcher>
EventListenerMap::ParseEventMatcer(
On 2012/06/13 01:24:27, Matt Perry wrote:
> typo - Matcer

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.cc:147:
CHECK(filtered.GetDictionary(*it, &filter));
On 2012/06/13 01:24:27, Matt Perry wrote:
> Prefs can get corrupted, so don't rely on them being valid.

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:33: // that an event can be
dispatched to. This is a lazy listener if |process| is
On 2012/06/13 09:21:54, battre wrote:
> what is a lazy listener?

I've added a comment.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:35: class EventListener {
On 2012/06/13 01:24:27, Matt Perry wrote:
> put all new extensions code in the extensions namespace

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:40: scoped_ptr<DictionaryValue>
filter);
On 2012/06/13 09:21:54, battre wrote:
> what goes into |filter|?

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:47: const std::string event_name;
On 2012/06/13 09:21:54, battre wrote:
> I think according to our style guide, these should either end in _ or be
member
> of a struct.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

Cool, I've changed this class to a struct.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:51: int matcher_id;
On 2012/06/13 09:21:54, battre wrote:
> private:
>   DISALLOW_COPY_AND_ASSIGN(EventListener);

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:60: ~EventListenerMap();
On 2012/06/13 01:24:27, Matt Perry wrote:
> nit: newline before and after constructor/destructor pair

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:61: bool
AddListener(scoped_ptr<EventListener> listener);
On 2012/06/13 01:24:27, Matt Perry wrote:
> comment all these methods

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:62: bool
RemoveListener(scoped_ptr<EventListener> listener);
On 2012/06/13 01:24:27, Matt Perry wrote:
> use scoped_ptr in params only when the function needs to assume ownership of
the
> param. this could simple take a const ref.

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:64: scoped_ptr<std::set<const
EventListener*> > GetEventTargets(
On 2012/06/13 01:24:27, Matt Perry wrote:
> why not just return a bare std::set?

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:70: bool
AnyListenersForEvent(const std::string& event_name);
On 2012/06/13 09:21:54, battre wrote:
> HasListenersForEvent? (see "HasListener(const EventListener*)" below)

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:71: bool
AnyListenersForExtension(const std::string& extension_id,
On 2012/06/13 09:21:54, battre wrote:
> HasListenersForExtension?

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:75: void
RemoveLazyListenersFor(const std::string extension_id);
On 2012/06/13 09:21:54, battre wrote:
> const std::string&

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/event_listener_map.h:79: std::set<std::string>&
unfiltered,
On 2012/06/13 09:21:54, battre wrote:
> const std::set<...>&

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:128: std::string id =
extension_id;
On 2012/06/13 01:24:27, Matt Perry wrote:
> unused

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:471: if
(listeners_.HasListener(listener.get()))
On 2012/06/13 01:24:27, Matt Perry wrote:
> This is not quite right. IIUC, here |listener| is the lazy listener (i.e.
> |process| is NULL). We want to check if there is a non-lazy listener before
> dispatching the event.

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:506: registered_events,
On 2012/06/13 01:24:27, Matt Perry wrote:
> indent

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:510: case
chrome::NOTIFICATION_EXTENSION_UNLOADED: {
On 2012/06/13 01:24:27, Matt Perry wrote:
> These top 4 notifications should be handled directly in EventListenerMap.
> OnListenerRemoved can be a delegate call back to ExtensionEventRouter.

I've added the callback for OnListenerRemoved(), but do you feel strongly that
the notifications should be handled by EventListenerMap? To me this class
handles communication with the outside world (eg: calls to AddListener() that
come in from IPCs) and so knowing which notifications are significant and how to
interpret them fits with the responsibilities of this class. To my mind
EventListenerMap should be "dumb", in that it is a data structure that we query,
and it shouldn't change behind our back.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:57:
extensions::EventFilteringInfo info);
On 2012/06/13 09:21:54, battre wrote:
> make |info| pass by const-ref?

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:86: bool lazy);
On 2012/06/13 09:21:54, battre wrote:
> what's lazy?

Actually, as this flag controls whether an extra, lazy version of the listener
is added, I've renamed this variable to "add_lazy_listener" and added a comment.

Same with below.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:112:
extensions::EventFilteringInfo info);
On 2012/06/13 01:24:27, Matt Perry wrote:
> what about the other Dispatch methods?

I was thinking of introducing some datatype to encapsulate all these different
calls, like an EventEnvelope, which callers can craft and call to a single
DispatchEvent() method, so I wasn't too eager to add all the extra plumbing for
the Dispatch*() methods.

What do you think?

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:178: // If |extension_id| is
non-empty, we load only that extension's page
On 2012/06/13 01:24:27, Matt Perry wrote:
> comment needs updating

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:184: void
DispatchEventToExtension(const std::string& extension_id,
On 2012/06/13 01:24:27, Matt Perry wrote:
> We already have 2 DispatchEventToExtension functions... let's use a different
> name.

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:964: ListValue* filter_list;
On 2012/06/13 09:21:54, battre wrote:
> initialize to NULL

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1008: DictionaryValue* result;
On 2012/06/13 09:21:54, battre wrote:
> initialize to NULL.

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
File chrome/common/extensions/value_set.h (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
chrome/common/extensions/value_set.h:17: class ValueSet {
On 2012/06/13 01:24:27, Matt Perry wrote:
> class comment

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
chrome/common/extensions/value_set.h:17: class ValueSet {
On 2012/06/13 01:24:27, Matt Perry wrote:
> extensions namespace

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
chrome/common/extensions/value_set.h:24: int Add(const base::Value* value);
On 2012/06/13 09:21:54, battre wrote:
> how is the ownership of |value| handled?

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/common/extensions/va...
chrome/common/extensions/value_set.h:48: EntryList entries_;
On 2012/06/13 09:21:54, battre wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/extensions/...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/extensions/...
chrome/renderer/extensions/event_bindings.cc:73: explicit
ExtensionImpl(ExtensionDispatcher* dispatcher,
On 2012/06/13 01:24:27, Matt Perry wrote:
> drop explicit

Done.

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:81: var
UnfilteredAttachmentStrategy = function(event) {
On 2012/06/13 01:24:27, Matt Perry wrote:
> How much overhead would it be if we just treated unfiltered events as filtered
> events with a NULL filter? That would probably simplify the code a ton, yes?

Yeah, it would. I don't think the overhead would be extremely high... I was
originally reluctant to remove the unfiltered events "fast path" because
filtered events are inherently less efficient.

The main consideration is that with filtered events it's O(N^2) to add N
listeners on a single event (because we need to figure out if another filter
equal to the added one already exists), whereas for unfiltered events it's O(N).
But probably extensions that add many listeners to the same event are rare?

The other cost would be adding unnecessary entries into EventFilter, which means
that its URLMatcher would have to rebuild its internal data structures on every
add, but we do this for filters without events for code simplicity.

I can investigate this path more today. If it proves feasible do you think I
should change this patch, or land this first and then remove the code that
handles unfiltered events? That approach seems more incremental to me.

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:385: function getDeclarativeAPI()
{
On 2012/06/13 01:24:27, Matt Perry wrote:
> is this used?

Hm, not sure where this came from. Removed.

[sky, 2012-06-14 03:21:24]

history/bookmarks LGTM

[Matt Perry, 2012-06-14 18:48:55]

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:81: var
UnfilteredAttachmentStrategy = function(event) {
On 2012/06/14 02:15:55, koz wrote:
> On 2012/06/13 01:24:27, Matt Perry wrote:
> > How much overhead would it be if we just treated unfiltered events as
filtered
> > events with a NULL filter? That would probably simplify the code a ton, yes?
> 
> Yeah, it would. I don't think the overhead would be extremely high... I was
> originally reluctant to remove the unfiltered events "fast path" because
> filtered events are inherently less efficient.
> 
> The main consideration is that with filtered events it's O(N^2) to add N
> listeners on a single event (because we need to figure out if another filter
> equal to the added one already exists), whereas for unfiltered events it's
O(N).
> But probably extensions that add many listeners to the same event are rare?
> 
> The other cost would be adding unnecessary entries into EventFilter, which
means
> that its URLMatcher would have to rebuild its internal data structures on
every
> add, but we do this for filters without events for code simplicity.

Could EventFilter skip rebuilding if there are no URL filters?

> I can investigate this path more today. If it proves feasible do you think I
> should change this patch, or land this first and then remove the code that
> handles unfiltered events? That approach seems more incremental to me.

Doesn't matter a whole lot to me. Though I think I disagree with your definition
of incremental :)

[battre, 2012-06-14 20:05:11]

I tried to reserve some time for coding myself today and now it is 10pm already.
So I did not get through the entire thing again. :-(

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:157: url);
nit: different formatting here, in line 137, 175 and 206

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.cc (right):

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:95: const std::string&
extension_id, const std::string& event_name) {
nit: linebreak after ,

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:168: DictionaryValue* filter;
initialize with NULL

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:42: // |filter| represents a
generic filter structure that EventFilter knows how to
nit: 80 characters

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:89: bool RemoveListener(const
EventListener& listener);
nit: const EventListener* for symmetry to HasListener and AddListener?

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map_unittest.cc (right):

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:23:
scoped_ptr<DictionaryValue> HostSuffixFilter(const std::string& suffix) {
Nit: CreateHostSuffixFilter

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:35:
scoped_ptr<ExtensionEvent> GetNamedEvent(const std::string& event_name) {
Nit: CreateNamedEvent

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:40: const GURL& url) {
nit: CreateEvent

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

That's okay Dominic, I appreciate your efforts! I hope you find some more time
to code tomorrow.

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:81: var
UnfilteredAttachmentStrategy = function(event) {
On 2012/06/14 18:48:56, Matt Perry wrote:
> On 2012/06/14 02:15:55, koz wrote:
> > On 2012/06/13 01:24:27, Matt Perry wrote:
> > > How much overhead would it be if we just treated unfiltered events as
> filtered
> > > events with a NULL filter? That would probably simplify the code a ton,
yes?
> > 
> > Yeah, it would. I don't think the overhead would be extremely high... I was
> > originally reluctant to remove the unfiltered events "fast path" because
> > filtered events are inherently less efficient.
> > 
> > The main consideration is that with filtered events it's O(N^2) to add N
> > listeners on a single event (because we need to figure out if another filter
> > equal to the added one already exists), whereas for unfiltered events it's
> O(N).
> > But probably extensions that add many listeners to the same event are rare?
> > 
> > The other cost would be adding unnecessary entries into EventFilter, which
> means
> > that its URLMatcher would have to rebuild its internal data structures on
> every
> > add, but we do this for filters without events for code simplicity.
> 
> Could EventFilter skip rebuilding if there are no URL filters?

Yes, that could be done. It would require making the code more complex though
(which isn't to say it's not worth it.)

> 
> > I can investigate this path more today. If it proves feasible do you think I
> > should change this patch, or land this first and then remove the code that
> > handles unfiltered events? That approach seems more incremental to me.
> 
> Doesn't matter a whole lot to me. Though I think I disagree with your
definition
> of incremental :)

Heh yeah :-) Well, my thinking is that it's an incremental change in the sense
that only one API is translated over to the "new way", rather than all of them.
Having a unified attachment strategy is more complicated than either of the
Filtered / Unfiltered ones, but it is less complicated than Filtered +
Unfiltered.

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:157: url);
On 2012/06/14 20:05:11, battre wrote:
> nit: different formatting here, in line 137, 175 and 206

Done.

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.cc (right):

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:95: const std::string&
extension_id, const std::string& event_name) {
On 2012/06/14 20:05:11, battre wrote:
> nit: linebreak after ,

Done.

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:168: DictionaryValue* filter;
On 2012/06/14 20:05:11, battre wrote:
> initialize with NULL

Done here and above.

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:42: // |filter| represents a
generic filter structure that EventFilter knows how to
On 2012/06/14 20:05:11, battre wrote:
> nit: 80 characters

Done.

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:89: bool RemoveListener(const
EventListener& listener);
On 2012/06/14 20:05:11, battre wrote:
> nit: const EventListener* for symmetry to HasListener and AddListener?

Done.

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map_unittest.cc (right):

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:23:
scoped_ptr<DictionaryValue> HostSuffixFilter(const std::string& suffix) {
On 2012/06/14 20:05:11, battre wrote:
> Nit: CreateHostSuffixFilter

Done.

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:35:
scoped_ptr<ExtensionEvent> GetNamedEvent(const std::string& event_name) {
On 2012/06/14 20:05:11, battre wrote:
> Nit: CreateNamedEvent

Done.

http://codereview.chromium.org/10514013/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:40: const GURL& url) {
On 2012/06/14 20:05:11, battre wrote:
> nit: CreateEvent

Done.

[Matt Perry, 2012-06-15 00:10:39]

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10514013/diff/5001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:81: var
UnfilteredAttachmentStrategy = function(event) {
On 2012/06/15 00:07:14, koz wrote:
> On 2012/06/14 18:48:56, Matt Perry wrote:
> > On 2012/06/14 02:15:55, koz wrote:
> > > On 2012/06/13 01:24:27, Matt Perry wrote:
> > > > How much overhead would it be if we just treated unfiltered events as
> > filtered
> > > > events with a NULL filter? That would probably simplify the code a ton,
> yes?
> > > 
> > > Yeah, it would. I don't think the overhead would be extremely high... I
was
> > > originally reluctant to remove the unfiltered events "fast path" because
> > > filtered events are inherently less efficient.
> > > 
> > > The main consideration is that with filtered events it's O(N^2) to add N
> > > listeners on a single event (because we need to figure out if another
filter
> > > equal to the added one already exists), whereas for unfiltered events it's
> > O(N).
> > > But probably extensions that add many listeners to the same event are
rare?
> > > 
> > > The other cost would be adding unnecessary entries into EventFilter, which
> > means
> > > that its URLMatcher would have to rebuild its internal data structures on
> > every
> > > add, but we do this for filters without events for code simplicity.
> > 
> > Could EventFilter skip rebuilding if there are no URL filters?
> 
> Yes, that could be done. It would require making the code more complex though
> (which isn't to say it's not worth it.)
> 
> > 
> > > I can investigate this path more today. If it proves feasible do you think
I
> > > should change this patch, or land this first and then remove the code that
> > > handles unfiltered events? That approach seems more incremental to me.
> > 
> > Doesn't matter a whole lot to me. Though I think I disagree with your
> definition
> > of incremental :)
> 
> Heh yeah :-) Well, my thinking is that it's an incremental change in the sense
> that only one API is translated over to the "new way", rather than all of
them.
> Having a unified attachment strategy is more complicated than either of the
> Filtered / Unfiltered ones, but it is less complicated than Filtered +
> Unfiltered.

That's a good point. I'm convinced.

[Aaron Boodman, 2012-06-15 07:34:51]

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/accessibili...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/accessibili...
chrome/browser/accessibility/accessibility_extension_api.cc:167: event_name,
json_args, NULL, GURL(), extensions::EventFilteringInfo());
It's allowed style-wise to provide overloads that default params. Just sayin :).

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:41: struct EventListener {
I think this might be hard to change, but this looks more like an event
_registration_ than an event _listener_. A registration is an association
between a listener (e.g., a process) and an event (event_name).

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:93: std::set<const
EventListener*> GetEventTargets(
Why not GetEventListeners?

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:80: args.Set(2,
Value::CreateStringValue(info.AsJSONString()));
Can you juts pass this through as a Value? Serializing all the way to string is
kinda lame.

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:122: void
ExtensionEventRouter::OnListenerAdded(const EventListener* listener) {
Instead of the delegate interface can you just look at the return value from
AddListener()?

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:278: // Adds a filter to an event.
It's matt's new storage thingy ready for you to use yet?

http://codereview.chromium.org/10514013/diff/15060/chrome/common/extensions/v...
File chrome/common/extensions/value_set.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/common/extensions/v...
chrome/common/extensions/value_set.h:19: // Keeps a running count of Values,
like map<Value, int>. Adding / removing
Nit: The use of the term 'map' in this description is a bit confusing given the
class name. Is there a way to describe the functionality without resorting to
describing its internal storage?

http://codereview.chromium.org/10514013/diff/15060/chrome/common/extensions/v...
chrome/common/extensions/value_set.h:25: class ValueSet {
I guess maybe this is technically a MultiSet?

http://codereview.chromium.org/10514013/diff/15060/chrome/common/extensions/v...
chrome/common/extensions/value_set.h:34: int Add(const base::Value* value);
Chrome style would typically be to make this const & since it's an input param.

[Aaron Boodman, 2012-06-15 07:35:05]

(sorry for driveby)

[Aaron Boodman, 2012-06-15 07:38:06]

Oh, and most importantly -- this is changing the released API on all channels.
We need to figure out some way to not do that :|.

I know. I was supposed to do that.

[battre, 2012-06-15 07:45:58]

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/accessibili...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/accessibili...
chrome/browser/accessibility/accessibility_extension_api.cc:167: event_name,
json_args, NULL, GURL(), extensions::EventFilteringInfo());
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> It's allowed style-wise to provide overloads that default params. Just sayin
:).

are you sure?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

[Aaron Boodman, 2012-06-15 07:53:53]

On Fri, Jun 15, 2012 at 12:45 AM,  <battre@chromium.org> wrote:
>
>
http://codereview.chromium.org/10514013/diff/15060/chrome/browser/accessibili...
> File chrome/browser/accessibility/accessibility_extension_api.cc
> (right):
>
>
http://codereview.chromium.org/10514013/diff/15060/chrome/browser/accessibili...
> chrome/browser/accessibility/accessibility_extension_api.cc:167:
> event_name, json_args, NULL, GURL(), extensions::EventFilteringInfo());
> On 2012/06/15 07:34:51, Aaron Boodman wrote:
>>
>> It's allowed style-wise to provide overloads that default params. Just
>
> sayin :).
>
> are you sure?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

Terminology collision, I was referring to this rule:
google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Overloading#Function_Overloading

- a

[Matt Perry, 2012-06-15 18:59:34]

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:278: // Adds a filter to an event.
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> It's matt's new storage thingy ready for you to use yet?

It's ready in asynchronous form. Do we want to move events+filters to the
storage too? That means we'll have to block startup on loading the event
registrations out of the database.

[Aaron Boodman, 2012-06-15 22:37:31]

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:278: // Adds a filter to an event.
On 2012/06/15 18:59:34, Matt Perry wrote:
> On 2012/06/15 07:34:51, Aaron Boodman wrote:
> > It's matt's new storage thingy ready for you to use yet?
> 
> It's ready in asynchronous form. Do we want to move events+filters to the
> storage too? That means we'll have to block startup on loading the event
> registrations out of the database.

I think leaving this stuff in Preferences is kinda just as bad at this point.

I lean toward putting all our new state in level DB and blocking things that
need to be blocked when necessary.

[koz (OOO until 15th September), 2012-06-18 23:27:27]

I've addressed the comments, except for the one about using the new extensions
storage instead of preferences, which I'll work on now. Feel free to continue
reviewing the other parts.

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/accessibili...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/accessibili...
chrome/browser/accessibility/accessibility_extension_api.cc:167: event_name,
json_args, NULL, GURL(), extensions::EventFilteringInfo());
On 2012/06/15 07:45:59, battre wrote:
> On 2012/06/15 07:34:51, Aaron Boodman wrote:
> > It's allowed style-wise to provide overloads that default params. Just sayin
> :).
> 
> are you sure?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

B'oh! So wait, the takeaway is that you can have default arguments of a kind, if
you just implement them as function overloads? Oh well, the damage is done now.

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:41: struct EventListener {
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> I think this might be hard to change, but this looks more like an event
> _registration_ than an event _listener_. A registration is an association
> between a listener (e.g., a process) and an event (event_name).

Yeah, maybe. I think it's still valid to call these guys listeners, as they
implement the listener "pattern", just that dispatching is not merely calling
the listening object directly, it's a generalised dispatch step.

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:93: std::set<const
EventListener*> GetEventTargets(
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> Why not GetEventListeners?

Done.

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:80: args.Set(2,
Value::CreateStringValue(info.AsJSONString()));
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> Can you juts pass this through as a Value? Serializing all the way to string
is
> kinda lame.

Done.

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:122: void
ExtensionEventRouter::OnListenerAdded(const EventListener* listener) {
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> Instead of the delegate interface can you just look at the return value from
> AddListener()?

The delegate interface was requested by Matt, and I think it's good because it's
symmetrical with the OnListenerRemoved() code, which is called from a variety of
situations (eg: listener explicitly being removed, or a process dying).

That said, I do like the simplicity of return values over listeners, and it
would enforce an architecture that I agree with, namely ExtensionEventRouter
controls all info that goes into and out of EventListenerMap, which means that
EventListenerMap would never listen to notifications itself.

I'd probably rather stick with the delegate though. I'll change it if you still
think it's better as return values.

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:278: // Adds a filter to an event.
On 2012/06/15 22:37:31, Aaron Boodman wrote:
> On 2012/06/15 18:59:34, Matt Perry wrote:
> > On 2012/06/15 07:34:51, Aaron Boodman wrote:
> > > It's matt's new storage thingy ready for you to use yet?
> > 
> > It's ready in asynchronous form. Do we want to move events+filters to the
> > storage too? That means we'll have to block startup on loading the event
> > registrations out of the database.
> 
> I think leaving this stuff in Preferences is kinda just as bad at this point.
> 
> I lean toward putting all our new state in level DB and blocking things that
> need to be blocked when necessary.

Ok, I'll work on making that change in this patch then.

http://codereview.chromium.org/10514013/diff/15060/chrome/common/extensions/v...
File chrome/common/extensions/value_set.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/common/extensions/v...
chrome/common/extensions/value_set.h:19: // Keeps a running count of Values,
like map<Value, int>. Adding / removing
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> Nit: The use of the term 'map' in this description is a bit confusing given
the
> class name. Is there a way to describe the functionality without resorting to
> describing its internal storage?

Good point. How about ValueCounter?

http://codereview.chromium.org/10514013/diff/15060/chrome/common/extensions/v...
chrome/common/extensions/value_set.h:25: class ValueSet {
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> I guess maybe this is technically a MultiSet?

This is true, but I think ValueCounter captures the intent a bit better.

http://codereview.chromium.org/10514013/diff/15060/chrome/common/extensions/v...
chrome/common/extensions/value_set.h:34: int Add(const base::Value* value);
On 2012/06/15 07:34:51, Aaron Boodman wrote:
> Chrome style would typically be to make this const & since it's an input
param.

Done.

[Matt Perry, 2012-06-18 23:35:45]

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:278: // Adds a filter to an event.
On 2012/06/18 23:27:27, koz wrote:
> On 2012/06/15 22:37:31, Aaron Boodman wrote:
> > On 2012/06/15 18:59:34, Matt Perry wrote:
> > > On 2012/06/15 07:34:51, Aaron Boodman wrote:
> > > > It's matt's new storage thingy ready for you to use yet?
> > > 
> > > It's ready in asynchronous form. Do we want to move events+filters to the
> > > storage too? That means we'll have to block startup on loading the event
> > > registrations out of the database.
> > 
> > I think leaving this stuff in Preferences is kinda just as bad at this
point.
> > 
> > I lean toward putting all our new state in level DB and blocking things that
> > need to be blocked when necessary.
> 
> Ok, I'll work on making that change in this patch then.

It's more work than you might think. I would hold off for a future patch.

I'm not entirely convinced moving to level DB is a good idea. If something needs
to be read while blocking startup, it doesn't buy us anything to move it out of
Preferences (which blocks startup). That just adds 1 more file that blocks
startup.

[koz (OOO until 15th September), 2012-06-18 23:49:30]

Sure, I'm happy to do that. Are there any other outstanding issues with this
change?

[Matt Perry, 2012-06-19 00:02:54]

LGTM

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:80: args.Set(2,
Value::CreateStringValue(info.AsJSONString()));
On 2012/06/18 23:27:27, koz wrote:
> On 2012/06/15 07:34:51, Aaron Boodman wrote:
> > Can you juts pass this through as a Value? Serializing all the way to string
> is
> > kinda lame.
> 
> Done.

Doesn't look like it :)

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

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10514013/diff/15060/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:80: args.Set(2,
Value::CreateStringValue(info.AsJSONString()));
On 2012/06/19 00:02:55, Matt Perry wrote:
> On 2012/06/18 23:27:27, koz wrote:
> > On 2012/06/15 07:34:51, Aaron Boodman wrote:
> > > Can you juts pass this through as a Value? Serializing all the way to
string
> > is
> > > kinda lame.
> > 
> > Done.
> 
> Doesn't look like it :)

Oops, I think I forgot to upload this patch. I'm rebasing it right now and
there's a few conflicts with gdk's change along these lines, too.

Thanks for checking!

[battre, 2012-06-19 14:26:22]

My attention span is suffering (90 minutes code review). I am not sure whether
the review of chrome/common/extensions is very thorough.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:56: int id =
event_filter_.AddEventMatcher(listener->event_name,
proper typdef

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:82: it = listeners.erase(it);
why do you update it if you return in line 83 anyway?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:82: it = listeners.erase(it);
This is a vector, so erase(it) can be kind of expensive. - Swap with last index
and then call erase?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:85: it++;
move this into line 78?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:187: std::set<int> ids =
event_filter_.MatchEvent(event.event_name, event.info);
s/int/MatcherId/

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:188: for
(std::set<int>::iterator id = ids.begin(); id != ids.end(); id++) {
dito

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:63: int matcher_id;
nit: replace int with the correct typedef?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:86: // ExtensionEvent.
Return value?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:89: // Remove a listener that
.Equals() |listener|.
Return value?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:116: void
RemoveLazyListenersFor(const std::string& extension_id);
RemoveLazyListenersForExtension(...)? In the unit tests I was wondering what the
string parameter was.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:121: // RenderProcessHost*.
The meaning of |unfiltered| and |filtered| remains unclear to me.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:128: typedef
std::map<std::string, ListenerList> ListenerMap;
Can you add a comment that the key is an event name?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:140: std::map<int,
EventListener*> listeners_by_matcher_id_;
nit: replace int with typedef?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map_unittest.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:42: const GURL& url) {
nit: indent

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:58: "1", NULL,
scoped_ptr<DictionaryValue>())));
replace this "1" (and all others) with
const char kExt1[] = "extension_1";
and "event" with
const char kEventName[] = "event";
?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:135: event_name,
extension_id, process, scoped_ptr<DictionaryValue>()));
allocate the EventListener on the stack?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:202: event_name,
extension_id, NULL, scoped_ptr<DictionaryValue>()));
allocate on stack?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:245:
scoped_ptr<DictionaryValue>(filter.DeepCopy())));
allocate on stack?

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:297: scoped_ptr<Value>
event_args_value(Value::CreateStringValue(event_args));
on stack?
StringValue event_args_value(event_args);

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:337: scoped_ptr<Value>
event_args_value(Value::CreateStringValue(event_args));
same as 297

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:393:
MaybeLoadLazyBackgroundPage(
This function name does not reflect that this is about dispatching the event.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:405: extension_id);
nit: I think this fits into a single line.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:109: bool
remove_lazy_listener);
nit: indent

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:195: // Shared by
DispatchEvent*. If |extension_id| is empty, the event is
update comment (|extension_id| is renamed)

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:196: // broadcast. If
|process| is non-NULL, the event is only dispatched to that
can you also update this? there is no |process| any more.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:236: 
// Implementation of extensions::EventListenerMap::Delegate.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:982:
new_filtered_events.release());
What do you think of modifying the list instead of creating a deep copy? Same in
RemoveFilterFromEvent. You can use a ScopedExtensionPrefUpdate.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:290: const DictionaryValue*
GetFilteredEvents(const std::string& extension_id);
nit: make function const?

http://codereview.chromium.org/10514013/diff/23001/chrome/common/extensions/e...
File chrome/common/extensions/event_filter.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/common/extensions/e...
chrome/common/extensions/event_filter.h:76: return event_matcher_.get();
this is part of the other CL.

http://codereview.chromium.org/10514013/diff/23001/chrome/common/extensions/v...
File chrome/common/extensions/value_counter.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/common/extensions/v...
chrome/common/extensions/value_counter.h:49: int Decrement();
I think this does not match the style guide. (structs should not have functions)

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:9: #include "base/callback.h"
needed?

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:11: #include
"base/memory/scoped_ptr.h"
dupe of line 13

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:15: #include
"chrome/common/extensions/matcher/url_matcher.h"
needed?

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:192: 
nit: 2 newlines

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:198: base::DictionaryValue*
filter_dict;
initialize to NULL

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:201: v8::Context::GetCurrent());
you are leaking filter_value here

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:223:
g_filtered_listener_counts.Get()[extension_id].find(event_name);
assign "g_filtered_listener_counts.Get()[extension_id]" to a reference?

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:235: base::DictionaryValue* filter)
{
are you concerned that
g_filtered_listener_counts.Get()[extension_id][event_name] might be null?

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:245: v8::Handle<v8::Value>
DetachFilteredEvent(const v8::Arguments& args) {
what is the meaning of this return value?

[koz (OOO until 15th September), 2012-06-20 08:05:56]

Thanks for the thorough review, Dominic! I always feel like I learn something
from your reviews :-)

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:56: int id =
event_filter_.AddEventMatcher(listener->event_name,
On 2012/06/19 14:26:23, battre wrote:
> proper typdef

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:82: it = listeners.erase(it);
On 2012/06/19 14:26:23, battre wrote:
> why do you update it if you return in line 83 anyway?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:82: it = listeners.erase(it);
On 2012/06/19 14:26:23, battre wrote:
> This is a vector, so erase(it) can be kind of expensive. - Swap with last
index
> and then call erase?

Even better, swap to end and then pop_back()!

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:85: it++;
On 2012/06/19 14:26:23, battre wrote:
> move this into line 78?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:187: std::set<int> ids =
event_filter_.MatchEvent(event.event_name, event.info);
On 2012/06/19 14:26:23, battre wrote:
> s/int/MatcherId/

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.cc:188: for
(std::set<int>::iterator id = ids.begin(); id != ids.end(); id++) {
On 2012/06/19 14:26:23, battre wrote:
> dito

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:63: int matcher_id;
On 2012/06/19 14:26:23, battre wrote:
> nit: replace int with the correct typedef?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:86: // ExtensionEvent.
On 2012/06/19 14:26:23, battre wrote:
> Return value?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:89: // Remove a listener that
.Equals() |listener|.
On 2012/06/19 14:26:23, battre wrote:
> Return value?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:116: void
RemoveLazyListenersFor(const std::string& extension_id);
On 2012/06/19 14:26:23, battre wrote:
> RemoveLazyListenersForExtension(...)? In the unit tests I was wondering what
the
> string parameter was.

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:121: // RenderProcessHost*.
On 2012/06/19 14:26:23, battre wrote:
> The meaning of |unfiltered| and |filtered| remains unclear to me.

I've added a bit more explanation.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:128: typedef
std::map<std::string, ListenerList> ListenerMap;
On 2012/06/19 14:26:23, battre wrote:
> Can you add a comment that the key is an event name?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:140: std::map<int,
EventListener*> listeners_by_matcher_id_;
On 2012/06/19 14:26:23, battre wrote:
> nit: replace int with typedef?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map_unittest.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:42: const GURL& url) {
On 2012/06/19 14:26:23, battre wrote:
> nit: indent

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map_unittest.cc:58: "1", NULL,
scoped_ptr<DictionaryValue>())));
On 2012/06/19 14:26:23, battre wrote:
> replace this "1" (and all others) with
> const char kExt1[] = "extension_1";
> and "event" with
> const char kEventName[] = "event";
> ?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:135: event_name,
extension_id, process, scoped_ptr<DictionaryValue>()));
On 2012/06/19 14:26:23, battre wrote:
> allocate the EventListener on the stack?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:202: event_name,
extension_id, NULL, scoped_ptr<DictionaryValue>()));
On 2012/06/19 14:26:23, battre wrote:
> allocate on stack?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:245:
scoped_ptr<DictionaryValue>(filter.DeepCopy())));
On 2012/06/19 14:26:23, battre wrote:
> allocate on stack?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:297: scoped_ptr<Value>
event_args_value(Value::CreateStringValue(event_args));
On 2012/06/19 14:26:23, battre wrote:
> on stack?
> StringValue event_args_value(event_args);

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:337: scoped_ptr<Value>
event_args_value(Value::CreateStringValue(event_args));
On 2012/06/19 14:26:23, battre wrote:
> same as 297

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:393:
MaybeLoadLazyBackgroundPage(
On 2012/06/19 14:26:23, battre wrote:
> This function name does not reflect that this is about dispatching the event.

Changed to MaybeLoadLazyBackgroundPageToDispatchEvent().

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.cc:405: extension_id);
On 2012/06/19 14:26:23, battre wrote:
> nit: I think this fits into a single line.

It does! Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:109: bool
remove_lazy_listener);
On 2012/06/19 14:26:23, battre wrote:
> nit: indent

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:195: // Shared by
DispatchEvent*. If |extension_id| is empty, the event is
On 2012/06/19 14:26:23, battre wrote:
> update comment (|extension_id| is renamed)

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:196: // broadcast. If
|process| is non-NULL, the event is only dispatched to that
On 2012/06/19 14:26:23, battre wrote:
> can you also update this? there is no |process| any more.

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:236: 
On 2012/06/19 14:26:23, battre wrote:
> // Implementation of extensions::EventListenerMap::Delegate.

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:982:
new_filtered_events.release());
On 2012/06/19 14:26:23, battre wrote:
> What do you think of modifying the list instead of creating a deep copy? Same
in
> RemoveFilterFromEvent. You can use a ScopedExtensionPrefUpdate.

Sounds good! Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:290: const DictionaryValue*
GetFilteredEvents(const std::string& extension_id);
On 2012/06/19 14:26:23, battre wrote:
> nit: make function const?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/common/extensions/e...
File chrome/common/extensions/event_filter.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/common/extensions/e...
chrome/common/extensions/event_filter.h:76: return event_matcher_.get();
On 2012/06/19 14:26:23, battre wrote:
> this is part of the other CL.

Oops, I must have uploaded the union. Sorry about that.

http://codereview.chromium.org/10514013/diff/23001/chrome/common/extensions/v...
File chrome/common/extensions/value_counter.h (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/common/extensions/v...
chrome/common/extensions/value_counter.h:49: int Decrement();
On 2012/06/19 14:26:23, battre wrote:
> I think this does not match the style guide. (structs should not have
functions)

I've turned it into a class.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:9: #include "base/callback.h"
On 2012/06/19 14:26:23, battre wrote:
> needed?

Needed for base::Bind() below? Or is it base/bind.h that I need... I've changed
it to the latter.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:11: #include
"base/memory/scoped_ptr.h"
On 2012/06/19 14:26:23, battre wrote:
> dupe of line 13

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:15: #include
"chrome/common/extensions/matcher/url_matcher.h"
On 2012/06/19 14:26:23, battre wrote:
> needed?

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:192: 
On 2012/06/19 14:26:23, battre wrote:
> nit: 2 newlines

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:198: base::DictionaryValue*
filter_dict;
On 2012/06/19 14:26:23, battre wrote:
> initialize to NULL

Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:201: v8::Context::GetCurrent());
On 2012/06/19 14:26:23, battre wrote:
> you are leaking filter_value here

You mean in the case where the CHECK fails? The CHECK probably doesn't belong
here anyway because this input comes indirectly from the user. I've changed it
to a return, and delete filter_value in that case.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:223:
g_filtered_listener_counts.Get()[extension_id].find(event_name);
On 2012/06/19 14:26:23, battre wrote:
> assign "g_filtered_listener_counts.Get()[extension_id]" to a reference?

Yeah, good one. Done.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:235: base::DictionaryValue* filter)
{
On 2012/06/19 14:26:23, battre wrote:
> are you concerned that
> g_filtered_listener_counts.Get()[extension_id][event_name] might be null?

That is a valid concern, I've added a check for that.

http://codereview.chromium.org/10514013/diff/23001/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:245: v8::Handle<v8::Value>
DetachFilteredEvent(const v8::Arguments& args) {
On 2012/06/19 14:26:23, battre wrote:
> what is the meaning of this return value?

To be accessible from JS this function needs a return type of
v8::Handle<v8::Value> Func(const v8::Arguments& args);

(I also note that I was erroneously returning -1 below, when I meant to
unconditionally return undefined.)

[battre, 2012-06-20 09:50:45]

Great, I think we are almost there.

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/e...
File chrome/common/extensions/event_filtering_info.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/e...
chrome/common/extensions/event_filtering_info.cc:38: return
scoped_ptr<base::Value>(result.release());
nit:
return result.PassAs<base::Value>();

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
File chrome/common/extensions/value_counter.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
chrome/common/extensions/value_counter.cc:43: entries_.erase(it);
swap it with entries_.back() and pop_back()?

nit: no need for parenthesis

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
File chrome/common/extensions/value_counter.h (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
chrome/common/extensions/value_counter.h:55: linked_ptr<base::Value> value_;
please move private: before this line

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
File chrome/common/extensions/value_counter_unittest.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
chrome/common/extensions/value_counter_unittest.cc:15: scoped_ptr<base::Value>
value(new ListValue());
nit: you can allocate this and the other value* variables on the stack.

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/extensions...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:200: delete filter_value;
I think filter_value is still lost if you don't run into this branch. My
proposal would be to just make filter_value a scoped_ptr.

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:100: var i =
allAttachedEvents.indexOf(this.event_);
where is this declared?

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:227: listenerIDs =
MatchAgainstEventFilter(name, filteringInfo);
would it make sense to change this logic such that C++ does the (name,
filteringInfo) -> listenerIDs mapping before calling dispatchJSON and pass the
listenerIDs instead of filteringInfo? Would this save one round trip?

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:278:
allAttachedEvents[allAttachedEvents.length] = this;
Is my understanding correct, that this adds the Event to allAttachedEvents
regardless whether this is a filtered or unfiltered event? My reason for asking
is that only UnfilteredAttachmentStrategy.prototype.detach removes the Event
from the array. Maybe for symmetry reasons, adding and removing elements to/from
allAttachedEvents should happen in the same class.

[koz (OOO until 15th September), 2012-06-21 02:23:10]

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/e...
File chrome/common/extensions/event_filtering_info.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/e...
chrome/common/extensions/event_filtering_info.cc:38: return
scoped_ptr<base::Value>(result.release());
On 2012/06/20 09:50:46, battre wrote:
> nit:
> return result.PassAs<base::Value>();

Done.

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
File chrome/common/extensions/value_counter.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
chrome/common/extensions/value_counter.cc:43: entries_.erase(it);
On 2012/06/20 09:50:46, battre wrote:
> swap it with entries_.back() and pop_back()?
> 
> nit: no need for parenthesis

Done.

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
File chrome/common/extensions/value_counter.h (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
chrome/common/extensions/value_counter.h:55: linked_ptr<base::Value> value_;
On 2012/06/20 09:50:46, battre wrote:
> please move private: before this line

Done.

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
File chrome/common/extensions/value_counter_unittest.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/common/extensions/v...
chrome/common/extensions/value_counter_unittest.cc:15: scoped_ptr<base::Value>
value(new ListValue());
On 2012/06/20 09:50:46, battre wrote:
> nit: you can allocate this and the other value* variables on the stack.

Done.

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/extensions...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:200: delete filter_value;
On 2012/06/20 09:50:46, battre wrote:
> I think filter_value is still lost if you don't run into this branch. My
> proposal would be to just make filter_value a scoped_ptr.

Is it? I thought that filter_value->GetAsDictionary(&filter_dict) just sets
filter_dict to a new pointer to filter_value with a different type.  Below I
have "filter.reset(filter_dict)" which takes care of ownership of filter_dict
and therefore filter_value.

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:100: var i =
allAttachedEvents.indexOf(this.event_);
On 2012/06/20 09:50:46, battre wrote:
> where is this declared?

It was declared lower down, but I've moved above these classes so that its
declaration appears before its use.

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:227: listenerIDs =
MatchAgainstEventFilter(name, filteringInfo);
On 2012/06/20 09:50:46, battre wrote:
> would it make sense to change this logic such that C++ does the (name,
> filteringInfo) -> listenerIDs mapping before calling dispatchJSON and pass the
> listenerIDs instead of filteringInfo? Would this save one round trip?

Yes, that would work, but it wouldn't save a round trip to the browser, just to
C++, which is quite cheap as I understand it. This dispatch code is likely to
change a bit if we deprecate unfiltered events and handle them as filtered
events with an empty filter, so it might make sense to leave any changes like
this until then?

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:278:
allAttachedEvents[allAttachedEvents.length] = this;
On 2012/06/20 09:50:46, battre wrote:
> Is my understanding correct, that this adds the Event to allAttachedEvents
> regardless whether this is a filtered or unfiltered event? My reason for
asking
> is that only UnfilteredAttachmentStrategy.prototype.detach removes the Event
> from the array. Maybe for symmetry reasons, adding and removing elements
to/from
> allAttachedEvents should happen in the same class.

Nice spot, this code is really hairy. Done.

[battre, 2012-06-21 05:35:16]

Awesome. Thanks for your patience.

LGTM.

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/extensions...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/extensions...
chrome/renderer/extensions/event_bindings.cc:200: delete filter_value;
On 2012/06/21 02:23:11, koz wrote:
> On 2012/06/20 09:50:46, battre wrote:
> > I think filter_value is still lost if you don't run into this branch. My
> > proposal would be to just make filter_value a scoped_ptr.
> 
> Is it? I thought that filter_value->GetAsDictionary(&filter_dict) just sets
> filter_dict to a new pointer to filter_value with a different type.  Below I
> have "filter.reset(filter_dict)" which takes care of ownership of filter_dict
> and therefore filter_value.

ah, you are right. I missed the "As" in GetAsDictionary.

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10514013/diff/25005/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:227: listenerIDs =
MatchAgainstEventFilter(name, filteringInfo);
On 2012/06/21 02:23:11, koz wrote:
> On 2012/06/20 09:50:46, battre wrote:
> > would it make sense to change this logic such that C++ does the (name,
> > filteringInfo) -> listenerIDs mapping before calling dispatchJSON and pass
the
> > listenerIDs instead of filteringInfo? Would this save one round trip?
> 
> Yes, that would work, but it wouldn't save a round trip to the browser, just
to
> C++, which is quite cheap as I understand it. This dispatch code is likely to
> change a bit if we deprecate unfiltered events and handle them as filtered
> events with an empty filter, so it might make sense to leave any changes like
> this until then?

SGTM

[commit-bot: I haz the power, 2012-06-21 06:22:31]

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

[commit-bot: I haz the power, 2012-06-21 06:48:08]

Try job failure for 10514013-47002 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-06-21 07:02:08]

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

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

Try job failure for 10514013-51003 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-21 22:22:22]

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

[commit-bot: I haz the power, 2012-06-21 22:22:32]

Failed to apply patch for chrome/browser/download/download_extension_api.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/download/download_extension_api.cc
Hunk #1 FAILED at 1118.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/download/download_extension_api.cc.rej

[commit-bot: I haz the power, 2012-06-21 23:58:37]

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

[commit-bot: I haz the power, 2012-06-22 01:05:21]

Try job failure for 10514013-54001 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-22 06:21:09]

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

[commit-bot: I haz the power, 2012-06-22 07:16:01]

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

[commit-bot: I haz the power, 2012-06-22 07:43:36]

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

[commit-bot: I haz the power, 2012-06-22 08:36:05]

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

[commit-bot: I haz the power, 2012-06-25 06:44:56]

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

[commit-bot: I haz the power, 2012-06-25 07:45:22]

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

[commit-bot: I haz the power, 2012-06-25 09:18:21]

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

[commit-bot: I haz the power, 2012-06-25 11:39:02]

Change committed as 143896

[commit-bot: I haz the power, 2012-07-02 07:11:05]

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

[commit-bot: I haz the power, 2012-07-02 07:11:34]

Presubmit check for 10514013-84001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/extensions/extension_event_router.cc, line 544, 81 chars

Presubmit checks took 4.6s to calculate.

[commit-bot: I haz the power, 2012-07-02 07:18:09]

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

[commit-bot: I haz the power, 2012-07-02 07:38:36]

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

[commit-bot: I haz the power, 2012-07-02 12:55:23]

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

[commit-bot: I haz the power, 2012-07-02 13:41:41]

Try job failure for 10514013-90055 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, steps "interactive_ui_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-02 13:42:20]

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

[commit-bot: I haz the power, 2012-07-02 14:56:25]

Change committed as 145145

[commit-bot: I haz the power, 2012-07-04 23:52:11]

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

[commit-bot: I haz the power, 2012-07-05 01:05:00]

Change committed as 145484