[extension SW]: Support event listener registration and event dispatching.

[extension SW]: Support event listener registration and event dispatching.

Initial implementation, will only send events to an already running
  extension Service Worker. Subsequent CLs will add dispatching
  events to potentially stopped workers.
Ref count while dispatching an event is missing in the CL, so is
  ExtensionHostMsg_EventAck IPC.
Event filters are completely left out in this CL.
Currently EventListeners have an added field called "worker_thread_id", its
  value being 0 (kNonWorkerThreadId) means these are not SW event
  listeners. Otherwise similar to lazy and non-lazy events, we will have
  SW events with worker_thread_id filled in.
EventRouter's event registration methods are a bit verbose, e.g. existing
  methods:
    Add/RemoveListener
    Add/RemoveLazyListener [1]
  have their service worker counterpart versions:
    Add/RemoveServiceWorkerListener
    Add/RemoveServiceWorkerLazyListener [2]
  The difference of these [2] with the previous ones [1] are just an added
  |worker_thread_id| param. Exposing only these and removing the previous
  ones would also work, but that would require non service worker
  events/listeners to explicitly specify kNonWorkerThreadId as the
  value of |worker_thread_id| param. Since the consumers are not just
  within extensions core code, it's better to leave the change out in this
  CL.

Test fixture:
1. Open an extension SW that registers tabs.onUpdated listener.
2. Do something (page.js) to dispatch tabs.onUpdated event(s).
3. Expect extension SW to receive the event(s).

Note that there's no guarantee that the service worker is running during
step 2 and onwards. In worst case, the might become flaky, but it should
be highly unlikely.

BUG=721147
Review-Url: https://codereview.chromium.org/2886923002
Cr-Commit-Position: refs/heads/master@{#477787}
Committed: https://chromium.googlesource.com/chromium/src/+/e784724ab7bdebd696a585003d28c835352ddc75

[lazyboy, 2017-05-16 20:58:48]

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

[commit-bot: I haz the power, 2017-05-16 21:00:26]

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-05-16 21:27:03]

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

[commit-bot: I haz the power, 2017-05-16 21:27:04]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lazyboy, 2017-05-17 00:37:12]

Description was changed from

==========
DRAFT: Event listener registration and event dispatch for a running ESW.

BUG=
==========

to

==========
[extension SW]: Support event listener registration and event dispatching.

Initial implementation, will only send events to an already running
  extension Service Worker. Subsequent CLs will add dispatching
  events to potentially stopped workers.
Ref count while dispatching an event is missing in the CL, so is
  ExtensionHostMsg_EventAck IPC.
Event filters are completely left out in this CL.
Currently EventListeners have an added field called "worker_thread_id", its
  value being 0 (kNonWorkerThreadId) means these are not SW event
  listeners. Otherwise similar to lazy and non-lazy events, we will have
  SW events with worker_thread_id filled in.
EventRouter's event registration methods are a bit verbose, e.g. existing
  methods:
    Add/RemoveListener
    Add/RemoveLazyListener [1]
  have their service worker counterpart versions:
    Add/RemoveServiceWorkerListener
    Add/RemoveServiceWorkerLazyListener [2]
  The difference of these [2] with the previous ones [1] are just an added
  |worker_thread_id| param. Exposing only these and removing the previous
  ones would also work, but that would require non service worker
  events/listeners to explicitly specify kNonWorkerThreadId as the
  value of |worker_thread_id| param. Since the consumers are not just
  within extensions core code, it's better to leave the change out in this
  CL.

Test fixture:
1. Open an extension SW that registers tabs.onUpdated listener.
2. Do something (page.js) to dispatch tabs.onUpdated event(s).
3. Expect extension SW to receive the event(s).

Note that there's no guarantee that the service worker is running during
step 2 and onwards. In worst case, the might become flaky, but it should
be highly unlikely.

BUG=721147
==========

[lazyboy, 2017-05-17 00:40:15]

Hi Devlin,
This is the CL to register events from workers and dispatch events to them,
without the logic to send events to stopped workers (as I was pointing out in
our mtg). Hopefully, I've removed all the debug code and this should sane to
review. Thanks!

[lazyboy, 2017-05-17 00:41:13]

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

[lazyboy, 2017-05-17 00:41:14]

Hi Devlin,
This is the CL to register events from workers and dispatch events to them,
without the logic to send events to stopped workers (as I was pointing out in
our mtg). Hopefully, I've removed all the debug code and this should be sane to
review. Thanks!

[lazyboy, 2017-05-17 16:35:12]

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

[commit-bot: I haz the power, 2017-05-17 16:36:04]

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-05-17 17:44:28]

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

[commit-bot: I haz the power, 2017-05-17 17:44:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-05-24 17:58:25]

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
File extensions/browser/event_listener_map.cc (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_listener_map.cc:95: DCHECK(worker_thread_id_ ==
kNonWorkerThreadId);
nit: DCHECK_EQ

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_router.cc:180: std::unique_ptr<DictionaryValue>()));
nit: could just pass nullptr

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_router.cc:319: GetRegisteredEvents(extension_id,
kRegisteredLazyEvents);
Do we need to check is_for_service_worker here?

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_router.cc:543: std::make_tuple(browser_context_,
extension_id, kNonWorkerThreadId));
It's arguable we wouldn't need this.  We could also just dispatch the event to
the extension process and let the renderer figure it out.  Not sure if one is
preferable, so I'll leave it up to you.

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_router.h:245: const char* pref_key) const;
nit: can we pass an enum rather than a const char *?

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:253: bool WorkerMightHandleMessage(const
IPC::Message& message) {
Do we need this when the WorkerThreadDispatcher also checks message type?

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/ex...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/ex...
extensions/renderer/extension_frame_helper.cc:152: bool
ExtensionFrameHelper::IsContextForSW(const ScriptContext& context) {
We already have context_type() on ScriptContext; do we need this?  If so, we
should move it to ScriptContext, since it's very weird to query the frame
helper, when, by definition, service workers don't run in a frame.

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/wo...
File extensions/renderer/worker_thread_dispatcher.h (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/wo...
extensions/renderer/worker_thread_dispatcher.h:54: static void ForwardIPC(int
worker_thread_id, const IPC::Message& message);
Do we need to make this public?

[lazyboy, 2017-05-25 00:11:33]

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

[commit-bot: I haz the power, 2017-05-25 00:12:13]

Dry run: CQ is trying da patch.

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

[lazyboy, 2017-05-25 01:33:43]

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
File extensions/browser/event_listener_map.cc (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_listener_map.cc:95: DCHECK(worker_thread_id_ ==
kNonWorkerThreadId);
On 2017/05/24 17:58:25, Devlin (catching up) wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_router.cc:180: std::unique_ptr<DictionaryValue>()));
On 2017/05/24 17:58:25, Devlin (catching up) wrote:
> nit: could just pass nullptr

Done.

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_router.cc:319: GetRegisteredEvents(extension_id,
kRegisteredLazyEvents);
On 2017/05/24 17:58:25, Devlin (catching up) wrote:
> Do we need to check is_for_service_worker here?

Done.

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_router.cc:543: std::make_tuple(browser_context_,
extension_id, kNonWorkerThreadId));
On 2017/05/24 17:58:25, Devlin (catching up) wrote:
> It's arguable we wouldn't need this.  We could also just dispatch the event to
> the extension process and let the renderer figure it out.  Not sure if one is
> preferable, so I'll leave it up to you.

Right now there isn't a way to send events to *all* the workers in an extension
process. Unfortunately this means we will send multiple IPCs in this case.

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/browser/eve...
extensions/browser/event_router.h:245: const char* pref_key) const;
On 2017/05/24 17:58:25, Devlin (catching up) wrote:
> nit: can we pass an enum rather than a const char *?

Done.

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:253: bool WorkerMightHandleMessage(const
IPC::Message& message) {
On 2017/05/24 17:58:25, Devlin (catching up) wrote:
> Do we need this when the WorkerThreadDispatcher also checks message type?

Removed.

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/ex...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/ex...
extensions/renderer/extension_frame_helper.cc:152: bool
ExtensionFrameHelper::IsContextForSW(const ScriptContext& context) {
On 2017/05/24 17:58:25, Devlin (catching up) wrote:
> We already have context_type() on ScriptContext; do we need this?  If so, we
> should move it to ScriptContext, since it's very weird to query the frame
> helper, when, by definition, service workers don't run in a frame.

context_type() should be fine, thanks.
Done.

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/wo...
File extensions/renderer/worker_thread_dispatcher.h (right):

https://codereview.chromium.org/2886923002/diff/100001/extensions/renderer/wo...
extensions/renderer/worker_thread_dispatcher.h:54: static void ForwardIPC(int
worker_thread_id, const IPC::Message& message);
On 2017/05/24 17:58:25, Devlin (catching up) wrote:
> Do we need to make this public?

Done.

[Devlin, 2017-06-01 04:54:50]

https://codereview.chromium.org/2886923002/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/events/page.js (right):

https://codereview.chromium.org/2886923002/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/events/page.js:13:
console.error('data: ' + data);
probably remove this or replace with chrome.test.log; at least replace with
console.log so it doesn't look like an error.

https://codereview.chromium.org/2886923002/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/events/sw.js (right):

https://codereview.chromium.org/2886923002/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/events/sw.js:40: var getURL
= chrome.extension.getURL;
nit: probably not worth caching this for the one usage below.

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
File extensions/browser/event_listener_map.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
extensions/browser/event_listener_map.cc:48: new EventListener(event_name, "",
url::Origin(listener_url).GetURL(),
while we're here, prefer std::string()

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
extensions/browser/event_router.cc:273: void
EventRouter::AddLazyEventListenerImpl(const std::string& event_name,
Where possible, definition order should match declaration order

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/ext...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/ext...
extensions/browser/extension_message_filter.cc:157:
GetEventRouter()->AddServiceWorkerEventListener(
nit: maybe cache EventRouter, here and below.  It's not more efficient (each
branch is only one lookup), but I think it improves readability.  I don't feel
super strongly though.

https://codereview.chromium.org/2886923002/diff/160001/extensions/common/cons...
File extensions/common/constants.h (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/common/cons...
extensions/common/constants.h:118: // This is the default thread id used for non
service worker extension events.
nit: s/non service worker extension events/extension event listeners registered
from a non-service worker context
or similar

https://codereview.chromium.org/2886923002/diff/160001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:211: if (is_service_worker_context) {
This is a bit painful.  Can we add an IPCSender* and a thread id as members and
use those inline to avoid having two calls in each place?

https://codereview.chromium.org/2886923002/diff/160001/extensions/renderer/se...
File extensions/renderer/service_worker_request_sender.h (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/renderer/se...
extensions/renderer/service_worker_request_sender.h:30: void
SendIPC(IPC::Message* ipc);
Would it make more sense to expose the WorkerThreadDispatcher rather than the
RequestSender, since the former already has the Send() method?

[lazyboy, 2017-06-01 23:33:29]

https://codereview.chromium.org/2886923002/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/events/page.js (right):

https://codereview.chromium.org/2886923002/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/events/page.js:13:
console.error('data: ' + data);
On 2017/06/01 04:54:49, Devlin wrote:
> probably remove this or replace with chrome.test.log; at least replace with
> console.log so it doesn't look like an error.

Removed.

https://codereview.chromium.org/2886923002/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/events/sw.js (right):

https://codereview.chromium.org/2886923002/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/events/sw.js:40: var getURL
= chrome.extension.getURL;
On 2017/06/01 04:54:49, Devlin wrote:
> nit: probably not worth caching this for the one usage below.

Done.

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
File extensions/browser/event_listener_map.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
extensions/browser/event_listener_map.cc:48: new EventListener(event_name, "",
url::Origin(listener_url).GetURL(),
On 2017/06/01 04:54:49, Devlin wrote:
> while we're here, prefer std::string()

Done.

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
extensions/browser/event_router.cc:273: void
EventRouter::AddLazyEventListenerImpl(const std::string& event_name,
On 2017/06/01 04:54:49, Devlin wrote:
> Where possible, definition order should match declaration order

I've intentionally kept it this way, otherwise the diff becomes harder to
review. I'll wait to do that pass later.

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/ext...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/ext...
extensions/browser/extension_message_filter.cc:157:
GetEventRouter()->AddServiceWorkerEventListener(
On 2017/06/01 04:54:50, Devlin wrote:
> nit: maybe cache EventRouter, here and below.  It's not more efficient (each
> branch is only one lookup), but I think it improves readability.  I don't feel
> super strongly though.

Done.

https://codereview.chromium.org/2886923002/diff/160001/extensions/common/cons...
File extensions/common/constants.h (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/common/cons...
extensions/common/constants.h:118: // This is the default thread id used for non
service worker extension events.
On 2017/06/01 04:54:50, Devlin wrote:
> nit: s/non service worker extension events/extension event listeners
registered
> from a non-service worker context
> or similar

Done.

https://codereview.chromium.org/2886923002/diff/160001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:211: if (is_service_worker_context) {
On 2017/06/01 04:54:50, Devlin wrote:
> This is a bit painful.  Can we add an IPCSender* and a thread id as members
and
> use those inline to avoid having two calls in each place?


I've reworked the code a bit to remove duplicate IPCs. Let me know if that
works.

https://codereview.chromium.org/2886923002/diff/160001/extensions/renderer/se...
File extensions/renderer/service_worker_request_sender.h (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/renderer/se...
extensions/renderer/service_worker_request_sender.h:30: void
SendIPC(IPC::Message* ipc);
On 2017/06/01 04:54:50, Devlin wrote:
> Would it make more sense to expose the WorkerThreadDispatcher rather than the
> RequestSender, since the former already has the Send() method?

Removed, no longer needed as I'm sending IPC directly.

[Devlin, 2017-06-02 14:53:08]

lgtm

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:220: const int worker_thread_id =
content::WorkerThread::GetCurrentId();
hmm... this alone won't work, right?  Because if this is a worker thread, we'll
need to use a different IPC sender.  For now, I'm fine with this being a TODO.

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:211: IPC::Sender* sender =
Maybe make a method GetIPCSender() that does this?

IPC::Sender* GetIPCSender() {
  bool is_service_worker_context =
      context()->context_type() == Feature::SERVICE_WORKER_CONTEXT;
  DCHECK_EQ(is_service_worker_context,
            content::WorkerThread::GetCurrentId() != kNonWorkerThreadId);
  return is_service_worker_context ?
      WorkerThreadDispatcher::Get() :
      content::RenderThread::Get();
}

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/se...
File extensions/renderer/service_worker_request_sender.h (right):

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/se...
extensions/renderer/service_worker_request_sender.h:9: #include
"ipc/ipc_message.h"
needed?

[lazyboy, 2017-06-02 17:58:58]

lazyboy@chromium.org changed reviewers:
+ dcheng@chromium.org

[lazyboy, 2017-06-02 17:59:00]

+dcheng for
extensions/common/extension_messages.h

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2886923002/diff/160001/extensions/browser/eve...
extensions/browser/event_router.cc:273: void
EventRouter::AddLazyEventListenerImpl(const std::string& event_name,
On 2017/06/01 23:33:29, lazyboy wrote:
> On 2017/06/01 04:54:49, Devlin wrote:
> > Where possible, definition order should match declaration order
> 
> I've intentionally kept it this way, otherwise the diff becomes harder to
> review. I'll wait to do that pass later.

Moved this after OnExtensionUnloaded to match .h in patch set #11

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:220: const int worker_thread_id =
content::WorkerThread::GetCurrentId();
On 2017/06/02 14:53:08, Devlin wrote:
> hmm... this alone won't work, right?  Because if this is a worker thread,
we'll
> need to use a different IPC sender.  For now, I'm fine with this being a TODO.

Yes, added a TODO.

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:211: IPC::Sender* sender =
On 2017/06/02 14:53:08, Devlin wrote:
> Maybe make a method GetIPCSender() that does this?
> 
> IPC::Sender* GetIPCSender() {
>   bool is_service_worker_context =
>       context()->context_type() == Feature::SERVICE_WORKER_CONTEXT;
>   DCHECK_EQ(is_service_worker_context,
>             content::WorkerThread::GetCurrentId() != kNonWorkerThreadId);
>   return is_service_worker_context ?
>       WorkerThreadDispatcher::Get() :
>       content::RenderThread::Get();
> }

Done.

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/se...
File extensions/renderer/service_worker_request_sender.h (right):

https://codereview.chromium.org/2886923002/diff/180001/extensions/renderer/se...
extensions/renderer/service_worker_request_sender.h:9: #include
"ipc/ipc_message.h"
On 2017/06/02 14:53:08, Devlin wrote:
> needed?

Removed.

[lazyboy, 2017-06-02 18:09:59]

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

[commit-bot: I haz the power, 2017-06-02 18:10:32]

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-02 18:58:55]

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

[commit-bot: I haz the power, 2017-06-02 18:58:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lazyboy, 2017-06-02 20:34:19]

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

[commit-bot: I haz the power, 2017-06-02 20:34:47]

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-02 22:33:33]

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

[commit-bot: I haz the power, 2017-06-02 22:33:34]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-05 20:23:15]

One other request (maybe not for this CL) is more documentation/comments on this
whole event registration/listening/tracking thing works. It's not always 100%
clear how the various parts of the system interact.

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:218: event_name, worker_thread_id));
Isn't this identical to line 210?

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
File extensions/renderer/worker_thread_dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
extensions/renderer/worker_thread_dispatcher.cc:106: if (!found ||
worker_thread_id <= 0)
Why would the worker_thread_id be < 0? Maybe just explicitly check for
worker_thread_id == kNonWorkerThreadId?

[lazyboy, 2017-06-05 21:12:06]

re: documentation, added note in event_router.h

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:218: event_name, worker_thread_id));
On 2017/06/05 20:23:15, dcheng wrote:
> Isn't this identical to line 210?

Yes! Thanks for catching this, it must have creeped in during rebase. Removed.

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
File extensions/renderer/worker_thread_dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
extensions/renderer/worker_thread_dispatcher.cc:106: if (!found ||
worker_thread_id <= 0)
On 2017/06/05 20:23:15, dcheng wrote:
> Why would the worker_thread_id be < 0? Maybe just explicitly check for
> worker_thread_id == kNonWorkerThreadId?

Done.

[lazyboy, 2017-06-05 21:54:27]

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

[commit-bot: I haz the power, 2017-06-05 21:55: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-06 02:50:03]

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

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

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-06 04:55:07]

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
File extensions/renderer/worker_thread_dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
extensions/renderer/worker_thread_dispatcher.cc:105: bool found =
base::PickleIterator(message).ReadInt(&worker_thread_id);
Btw this seems pretty hard to understand. We should be able to assume this field
always exists right? This was previously a CHECK. Maybe this should be a routed
IPC...?

(A followup is OK for trying to make this routed, but in the short-term, it
seems like DCHECK(found)/CHECK(found) should be a reasonable thing to do still)

[lazyboy, 2017-06-06 19:12:00]

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
File extensions/renderer/worker_thread_dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
extensions/renderer/worker_thread_dispatcher.cc:105: bool found =
base::PickleIterator(message).ReadInt(&worker_thread_id);
On 2017/06/06 04:55:07, dcheng wrote:
> Btw this seems pretty hard to understand. We should be able to assume this
field
> always exists right? 
Yes, restored CHECK.
This was previously a CHECK. Maybe this should be a routed
> IPC...?
Can we send a routed message to a child thread directly? This pattern is
borrowed from content/'s service worker code, e.g.
https://cs.chromium.org/chromium/src/content/child/worker_thread_message_filt...
We need to be able to pick the thread id to be able to dispatch the IPC to that
thread.
> 
> (A followup is OK for trying to make this routed, but in the short-term, it
> seems like DCHECK(found)/CHECK(found) should be a reasonable thing to do
still)

[dcheng, 2017-06-06 20:59:55]

ipc lgtm

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
File extensions/renderer/worker_thread_dispatcher.cc (right):

https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
extensions/renderer/worker_thread_dispatcher.cc:105: bool found =
base::PickleIterator(message).ReadInt(&worker_thread_id);
On 2017/06/06 19:12:00, lazyboy wrote:
> On 2017/06/06 04:55:07, dcheng wrote:
> > Btw this seems pretty hard to understand. We should be able to assume this
> field
> > always exists right? 
> Yes, restored CHECK.
> This was previously a CHECK. Maybe this should be a routed
> > IPC...?
> Can we send a routed message to a child thread directly? This pattern is
> borrowed from content/'s service worker code, e.g.
>
https://cs.chromium.org/chromium/src/content/child/worker_thread_message_filt...
> We need to be able to pick the thread id to be able to dispatch the IPC to
that
> thread.
> > 
> > (A followup is OK for trying to make this routed, but in the short-term, it
> > seems like DCHECK(found)/CHECK(found) should be a reasonable thing to do
> still)
> 

Something something mojo I think. But in principle, there's no reason it
shouldn't be possible--fundamentally routable messages are just about unpickling
a known routing ID and then dispatching based off that.

[lazyboy, 2017-06-07 00:25:28]

On 2017/06/06 20:59:55, dcheng wrote:
> ipc lgtm
> 
>
https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
> File extensions/renderer/worker_thread_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2886923002/diff/240001/extensions/renderer/wo...
> extensions/renderer/worker_thread_dispatcher.cc:105: bool found =
> base::PickleIterator(message).ReadInt(&worker_thread_id);
> On 2017/06/06 19:12:00, lazyboy wrote:
> > On 2017/06/06 04:55:07, dcheng wrote:
> > > Btw this seems pretty hard to understand. We should be able to assume this
> > field
> > > always exists right? 
> > Yes, restored CHECK.
> > This was previously a CHECK. Maybe this should be a routed
> > > IPC...?
> > Can we send a routed message to a child thread directly? This pattern is
> > borrowed from content/'s service worker code, e.g.
> >
>
https://cs.chromium.org/chromium/src/content/child/worker_thread_message_filt...
> > We need to be able to pick the thread id to be able to dispatch the IPC to
> that
> > thread.
> > > 
> > > (A followup is OK for trying to make this routed, but in the short-term,
it
> > > seems like DCHECK(found)/CHECK(found) should be a reasonable thing to do
> > still)
> > 
> 
> Something something mojo I think. But in principle, there's no reason it
> shouldn't be possible--fundamentally routable messages are just about
unpickling
> a known routing ID and then dispatching based off that.

OK, added a todo in worker_thread_dispatcher.cc

[lazyboy, 2017-06-07 00:31:06]

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

[commit-bot: I haz the power, 2017-06-07 00:31:35]

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-07 02:04:40]

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

[commit-bot: I haz the power, 2017-06-07 02:04:41]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-07 18:24:14]

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

[commit-bot: I haz the power, 2017-06-07 18:25:19]

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-07 20:13:57]

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

[commit-bot: I haz the power, 2017-06-07 20:13:59]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-07 21:29:45]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-06-07 21:29:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2886923002/#ps320001
(title: "sync")

[commit-bot: I haz the power, 2017-06-07 21:30:13]

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-07 23:29:21]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1496870985927580,
"parent_rev": "b1bf7d6b39cc77b38b433873a3db686dc3a0ca73", "commit_rev":
"e784724ab7bdebd696a585003d28c835352ddc75"}

[commit-bot: I haz the power, 2017-06-07 23:29:35]

Description was changed from

==========
[extension SW]: Support event listener registration and event dispatching.

Initial implementation, will only send events to an already running
  extension Service Worker. Subsequent CLs will add dispatching
  events to potentially stopped workers.
Ref count while dispatching an event is missing in the CL, so is
  ExtensionHostMsg_EventAck IPC.
Event filters are completely left out in this CL.
Currently EventListeners have an added field called "worker_thread_id", its
  value being 0 (kNonWorkerThreadId) means these are not SW event
  listeners. Otherwise similar to lazy and non-lazy events, we will have
  SW events with worker_thread_id filled in.
EventRouter's event registration methods are a bit verbose, e.g. existing
  methods:
    Add/RemoveListener
    Add/RemoveLazyListener [1]
  have their service worker counterpart versions:
    Add/RemoveServiceWorkerListener
    Add/RemoveServiceWorkerLazyListener [2]
  The difference of these [2] with the previous ones [1] are just an added
  |worker_thread_id| param. Exposing only these and removing the previous
  ones would also work, but that would require non service worker
  events/listeners to explicitly specify kNonWorkerThreadId as the
  value of |worker_thread_id| param. Since the consumers are not just
  within extensions core code, it's better to leave the change out in this
  CL.

Test fixture:
1. Open an extension SW that registers tabs.onUpdated listener.
2. Do something (page.js) to dispatch tabs.onUpdated event(s).
3. Expect extension SW to receive the event(s).

Note that there's no guarantee that the service worker is running during
step 2 and onwards. In worst case, the might become flaky, but it should
be highly unlikely.

BUG=721147
==========

to

==========
[extension SW]: Support event listener registration and event dispatching.

Initial implementation, will only send events to an already running
  extension Service Worker. Subsequent CLs will add dispatching
  events to potentially stopped workers.
Ref count while dispatching an event is missing in the CL, so is
  ExtensionHostMsg_EventAck IPC.
Event filters are completely left out in this CL.
Currently EventListeners have an added field called "worker_thread_id", its
  value being 0 (kNonWorkerThreadId) means these are not SW event
  listeners. Otherwise similar to lazy and non-lazy events, we will have
  SW events with worker_thread_id filled in.
EventRouter's event registration methods are a bit verbose, e.g. existing
  methods:
    Add/RemoveListener
    Add/RemoveLazyListener [1]
  have their service worker counterpart versions:
    Add/RemoveServiceWorkerListener
    Add/RemoveServiceWorkerLazyListener [2]
  The difference of these [2] with the previous ones [1] are just an added
  |worker_thread_id| param. Exposing only these and removing the previous
  ones would also work, but that would require non service worker
  events/listeners to explicitly specify kNonWorkerThreadId as the
  value of |worker_thread_id| param. Since the consumers are not just
  within extensions core code, it's better to leave the change out in this
  CL.

Test fixture:
1. Open an extension SW that registers tabs.onUpdated listener.
2. Do something (page.js) to dispatch tabs.onUpdated event(s).
3. Expect extension SW to receive the event(s).

Note that there's no guarantee that the service worker is running during
step 2 and onwards. In worst case, the might become flaky, but it should
be highly unlikely.

BUG=721147

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

[commit-bot: I haz the power, 2017-06-07 23:29:37]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/e784724ab7bdebd696a585003d28...