Clear an extension's filtered events when a context is destroyed.

Clear an extension's filtered events when a context is destroyed.

In crrev.com/325156 I moved event invalidation logic from JavaScript into C++,
but I missed out filtered events. This CL also does some cleanup.

BUG=503913
R=rdevlin.cronin@chromium.org

Committed: https://crrev.com/bd9f618ebc1989cade688046a7c0908a6bf82bb5
Cr-Commit-Position: refs/heads/master@{#338403}

[not at google - send to devlin, 2015-07-09 22:10:14]

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:41: // A map of (extension ID, event name)
pairs to the filtered listener counts
Having the type:
  map<string<map<string, linked_ptr<ValueCounter>>>>
was making me feel nauseous, so I changed it to:
  ScopedPtrMap<pair<string, string>, ValueCounter>
but, not really related to this change.

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:322: filter.Pass(),
context()->GetRenderFrame()->GetRoutingID()));
And again, not strictly related to this change, but having a DeepCopy() here was
silly, so I fixed it.

[Devlin, 2015-07-10 17:57:30]

https://codereview.chromium.org/1227093008/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/events_apitest.cc (right):

https://codereview.chromium.org/1227093008/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/events_apitest.cc:57: // A filtered event.
This comment seems incomplete.

https://codereview.chromium.org/1227093008/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/events_are_unregistered/page1.js
(right):

https://codereview.chromium.org/1227093008/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/events_are_unregistered/page1.js:24:
chrome.runtime.onSuspend.addListener(function() {});
why some with singleListener and some with function() {}?

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:54: // TODO(kalman): Just use pointer
equality..?
either ellipsis (...) or period (.), not undecided. :P

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:110:
g_filtered_listener_counts.Get().find(key);
getting the lazy instance is cheap, but not 100% free.  Hang on to a reference?
(also below)

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:129: return
counts->second->Remove(*filter) == 0;
Won't this be incorrect in the case of trying to remove the last value twice? 
You can fix it by removing the entry in the map after it's empty.  (This was
also a problem before)

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:68: void DetachFilteredEvent(int event_id,
bool is_manual);
|matcher_id|?

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:78: // The set of attached events and
filtered events. Maintain this so that we
nit: "maintain these"?

[not at google - send to devlin, 2015-07-10 20:02:27]

Patchset #2 (id:20001) has been deleted

[not at google - send to devlin, 2015-07-10 20:39:17]

https://codereview.chromium.org/1227093008/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/events_apitest.cc (right):

https://codereview.chromium.org/1227093008/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/events_apitest.cc:57: // A filtered event.
On 2015/07/10 17:57:30, Devlin wrote:
> This comment seems incomplete.

Done.

https://codereview.chromium.org/1227093008/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/events_are_unregistered/page1.js
(right):

https://codereview.chromium.org/1227093008/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/events_are_unregistered/page1.js:24:
chrome.runtime.onSuspend.addListener(function() {});
On 2015/07/10 17:57:30, Devlin wrote:
> why some with singleListener and some with function() {}?

Listener identity comes from pointer identity. If you pass in (function(){})
then every time that's a new pointer, and therefore a new listener. If you
assign (function(){}) to a variable and then use it more than once, it's the
same listener.

Note that the old code was subtly "wrong" because of this (the assertions were
still correct, just not testing what it was supposed to be testing).

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:54: // TODO(kalman): Just use pointer
equality..?
On 2015/07/10 17:57:30, Devlin wrote:
> either ellipsis (...) or period (.), not undecided. :P

Done.

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:110:
g_filtered_listener_counts.Get().find(key);
On 2015/07/10 17:57:30, Devlin wrote:
> getting the lazy instance is cheap, but not 100% free.  Hang on to a
reference?
> (also below)

Alright, fine. It does set up a lock I suppose.

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:129: return
counts->second->Remove(*filter) == 0;
On 2015/07/10 17:57:30, Devlin wrote:
> Won't this be incorrect in the case of trying to remove the last value twice? 
> You can fix it by removing the entry in the map after it's empty.  (This was
> also a problem before)

Done.

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:68: void DetachFilteredEvent(int event_id,
bool is_manual);
On 2015/07/10 17:57:30, Devlin wrote:
> |matcher_id|?

Done.

https://codereview.chromium.org/1227093008/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:78: // The set of attached events and
filtered events. Maintain this so that we
On 2015/07/10 17:57:30, Devlin wrote:
> nit: "maintain these"?

Done.

[Devlin, 2015-07-10 20:44:34]

lgtm

https://codereview.chromium.org/1227093008/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1227093008/diff/40001/extensions/renderer/eve...
extensions/renderer/event_bindings.h:64: // identified by an ID, not its name.
nitty nit: "not their names" or "a filtered event is"

[not at google - send to devlin, 2015-07-10 20:53:39]

https://codereview.chromium.org/1227093008/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1227093008/diff/40001/extensions/renderer/eve...
extensions/renderer/event_bindings.h:64: // identified by an ID, not its name.
On 2015/07/10 20:44:34, Devlin wrote:
> nitty nit: "not their names" or "a filtered event is"

Done.

[not at google - send to devlin, 2015-07-10 20:53:42]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-07-10 20:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1227093008/#ps60001
(title: "comment")

[not at google - send to devlin, 2015-07-10 20:53:54]

The CQ bit was unchecked by kalman@chromium.org

[not at google - send to devlin, 2015-07-10 21:55:59]

Patchset #4 (id:80001) has been deleted

[not at google - send to devlin, 2015-07-10 21:56:41]

Fixed bug causing the trybots to fail, and rewrote ValueCounter to be sane.

[Devlin, 2015-07-10 22:07:42]

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
File extensions/common/value_counter.cc (right):

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
extensions/common/value_counter.cc:13: struct ValueCounter::Entry {
Can we make a constructor (passing in a value, initing count to 1)?

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
extensions/common/value_counter.cc:45: return false;
These are right, but I feel like adding
    return true;  // Removed the last entry.
  }
  return false;  // Removed, but not the last entry.
 }
}
return false;  // Nothing to remove.

Makes it a little more clear.

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
extensions/common/value_counter.cc:51: bool ValueCounter::IsEmpty() const {
Could probably move this to be is_empty() in the header file.

[not at google - send to devlin, 2015-07-10 22:41:54]

Patchset #5 (id:120001) has been deleted

[not at google - send to devlin, 2015-07-10 22:42:43]

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
File extensions/common/value_counter.cc (right):

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
extensions/common/value_counter.cc:13: struct ValueCounter::Entry {
On 2015/07/10 22:07:42, Devlin wrote:
> Can we make a constructor (passing in a value, initing count to 1)?

Done.

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
extensions/common/value_counter.cc:45: return false;
On 2015/07/10 22:07:42, Devlin wrote:
> These are right, but I feel like adding
>     return true;  // Removed the last entry.
>   }
>   return false;  // Removed, but not the last entry.
>  }
> }
> return false;  // Nothing to remove.
> 
> Makes it a little more clear.

Done.

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
extensions/common/value_counter.cc:51: bool ValueCounter::IsEmpty() const {
On 2015/07/10 22:07:42, Devlin wrote:
> Could probably move this to be is_empty() in the header file.

Alright. I don't mind it being IsEmpty, it's consistent with Add/Remove.

[not at google - send to devlin, 2015-07-10 22:42:56]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-07-10 22:42:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1227093008/#ps140001
(title: "moar")

[commit-bot: I haz the power, 2015-07-10 22:43:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1227093008/140001

[Devlin, 2015-07-10 22:45:22]

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
File extensions/common/value_counter.cc (right):

https://codereview.chromium.org/1227093008/diff/100001/extensions/common/valu...
extensions/common/value_counter.cc:51: bool ValueCounter::IsEmpty() const {
On 2015/07/10 22:42:43, kalman wrote:
> On 2015/07/10 22:07:42, Devlin wrote:
> > Could probably move this to be is_empty() in the header file.
> 
> Alright. I don't mind it being IsEmpty, it's consistent with Add/Remove.

But those do Work, and this doesn't. :)

[commit-bot: I haz the power, 2015-07-11 00:01:43]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-07-11 00:03:54]

Patchset 5 (id:??) landed as
https://crrev.com/bd9f618ebc1989cade688046a7c0908a6bf82bb5
Cr-Commit-Position: refs/heads/master@{#338403}