Move lazy event dispatching code out of EventRouter.

Move lazy event dispatching code out of EventRouter.

Separate out lazy background page logic from EventRouter to
LazyEventDispatcher. This will facilitate introducing service worker
(another "lazy context") lazy events to extensions.

Also introduce LazyContextId to identify a lazy context. This is
currently used only for identifying event page context, but in future
will also identify extension service worker context.

BUG=721147, 723754
Review-Url: https://codereview.chromium.org/2940883007
Cr-Commit-Position: refs/heads/master@{#479833}
Committed: https://chromium.googlesource.com/chromium/src/+/e464732fb0179fa3514239419c804547a0f56b8d

[lazyboy, 2017-06-15 01:11:25]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2017-06-15 01:11:25]

Hi Devlin, I've extracted some event_router code from my service worker draft CL
https://codereview.chromium.org/2924213002/ here, would you be able to take a
look? This should be helpful for helping land the (larger) service worker events
CL.

[lazyboy, 2017-06-15 01:11:32]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 01:11:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-06-15 01:37:27]

Nice!

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:149: bool cross_incognito =
event.restrict_to_browser_context &&
nit: since we're here, maybe s/cross/crosses

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:450:
base::Bind(&EventRouter::DispatchPendingEvent, base::Unretained(this)));
This Unretained was just copied, but isn't inherently safe.  Can we switch to
weak ptrs?

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:107: // |event_args| with the proper arguments
to send, which may differ if
I realize this was copy-paste, but event_args don't exist anymore.  Can we
update the comment?

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
File extensions/browser/events/lazy_event_dispatcher.h (right):

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:37: using DispatchFunc =
nit: s/Func/Function (avoid less common abbreviations)

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:52: // Returns whether or not
a listener is queued for dispatch already.
nit: maybe s/a listener/an identical listener?

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:61: const
base::DictionaryValue* listener_filter);
nit: \n

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:67: const
base::DictionaryValue* listener_filter);
nit: \n (after the other functions, too)

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:76:
std::set<EventPageDispatchIdentifier> dispatched_ep_ids_;
s/ep/<something else - or nothing.  dispatched_ids_ is probably fine>

[commit-bot: I haz the power, 2017-06-15 02:35:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 02:35:15]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-15 04:39:23]

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:149: bool cross_incognito =
event.restrict_to_browser_context &&
On 2017/06/15 01:37:26, Devlin wrote:
> nit: since we're here, maybe s/cross/crosses

Done.

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:450:
base::Bind(&EventRouter::DispatchPendingEvent, base::Unretained(this)));
On 2017/06/15 01:37:26, Devlin wrote:
> This Unretained was just copied, but isn't inherently safe.  Can we switch to
> weak ptrs?

Good point, fixed.

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:107: // |event_args| with the proper arguments
to send, which may differ if
On 2017/06/15 01:37:26, Devlin wrote:
> I realize this was copy-paste, but event_args don't exist anymore.  Can we
> update the comment?

Done.

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
File extensions/browser/events/lazy_event_dispatcher.h (right):

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:37: using DispatchFunc =
On 2017/06/15 01:37:26, Devlin wrote:
> nit: s/Func/Function (avoid less common abbreviations)

Done.

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:52: // Returns whether or not
a listener is queued for dispatch already.
On 2017/06/15 01:37:26, Devlin wrote:
> nit: maybe s/a listener/an identical listener?

Done.

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:61: const
base::DictionaryValue* listener_filter);
On 2017/06/15 01:37:26, Devlin wrote:
> nit: \n

Done.

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:67: const
base::DictionaryValue* listener_filter);
On 2017/06/15 01:37:26, Devlin wrote:
> nit: \n (after the other functions, too)

Done.

https://codereview.chromium.org/2940883007/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:76:
std::set<EventPageDispatchIdentifier> dispatched_ep_ids_;
On 2017/06/15 01:37:26, Devlin wrote:
> s/ep/<something else - or nothing.  dispatched_ids_ is probably fine>

I want something other than plain dispatch_ids_ b/c I'll need to add another set
for service workers and I want to differentiate between the two clearly.

I'll just name it (verbose) dispatched_ids_for_event_page_;

[lazyboy, 2017-06-15 04:39:59]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 04:40:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 04:50:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 04:50:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lazyboy, 2017-06-15 04:57:14]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 04:57:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 06:43:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 06:43:22]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-15 15:09:50]

lgtm

https://codereview.chromium.org/2940883007/diff/40001/extensions/browser/even...
File extensions/browser/events/lazy_event_dispatcher.cc (right):

https://codereview.chromium.org/2940883007/diff/40001/extensions/browser/even...
extensions/browser/events/lazy_event_dispatcher.cc:56: // extension service
worker) to handle the event. The latter case
nit: with the addition of the "a lazy background page or an extension service
worker" piece, which "latter" this is referring to (either incognito context or
extension service worker) becomes ambiguous.  Maybe s/The latter case occurs/We
need to use the incognito context/?

[lazyboy, 2017-06-15 17:33:51]

https://codereview.chromium.org/2940883007/diff/40001/extensions/browser/even...
File extensions/browser/events/lazy_event_dispatcher.cc (right):

https://codereview.chromium.org/2940883007/diff/40001/extensions/browser/even...
extensions/browser/events/lazy_event_dispatcher.cc:56: // extension service
worker) to handle the event. The latter case
On 2017/06/15 15:09:50, Devlin wrote:
> nit: with the addition of the "a lazy background page or an extension service
> worker" piece, which "latter" this is referring to (either incognito context
or
> extension service worker) becomes ambiguous.  Maybe s/The latter case
occurs/We
> need to use the incognito context/?

Done.

[lazyboy, 2017-06-15 17:34:23]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 17:34:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 19:23:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 19:23:19]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-15 21:11:46]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-06-15 21:11:47]

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/2940883007/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2017-06-15 21:12:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 21:17:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497561106494210,
"parent_rev": "25149208ab460c968e74de1fe9b31f6f5cbfce7f", "commit_rev":
"e464732fb0179fa3514239419c804547a0f56b8d"}

[commit-bot: I haz the power, 2017-06-15 21:17:41]

Description was changed from

==========
Move lazy event dispatching code out of EventRouter.

Separate out lazy background page logic from EventRouter to
LazyEventDispatcher. This will facilitate introducing service worker
(another "lazy context") lazy events to extensions.

Also introduce LazyContextId to identify a lazy context. This is
currently used only for identifying event page context, but in future
will also identify extension service worker context.

BUG=721147, 723754
==========

to

==========
Move lazy event dispatching code out of EventRouter.

Separate out lazy background page logic from EventRouter to
LazyEventDispatcher. This will facilitate introducing service worker
(another "lazy context") lazy events to extensions.

Also introduce LazyContextId to identify a lazy context. This is
currently used only for identifying event page context, but in future
will also identify extension service worker context.

BUG=721147, 723754

Review-Url: https://codereview.chromium.org/2940883007
Cr-Commit-Position: refs/heads/master@{#479833}
Committed:
https://chromium.googlesource.com/chromium/src/+/e464732fb0179fa3514239419c80...
==========

[commit-bot: I haz the power, 2017-06-15 21:17:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e464732fb0179fa3514239419c80...