Remove InterfaceRegistry/Provider from service workers

Remove InterfaceRegistry/Provider from service workers

This patch is to remove unnecessary InterfaceProvider usage. Events, who are
currently the only user of InterfaceProvider in EWInstance, will be triggered
via InterfacePtr which is passed at the starting up.

BUG=664019
Committed: https://crrev.com/317dd7b50bd2f138715fac044dcc44ce070ccbfc
Cr-Commit-Position: refs/heads/master@{#434434}

[shimazu, 2016-11-10 05:36:57]

shimazu@chromium.org changed reviewers:
+ horo@chromium.org

[shimazu, 2016-11-10 05:36:58]

ptal

[shimazu, 2016-11-10 06:58:02]

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

[commit-bot: I haz the power, 2016-11-10 06:58:17]

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, 2016-11-10 08:27:04]

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

[commit-bot: I haz the power, 2016-11-10 08:27:05]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-11 02:17:27]

Overall looks good.

https://codereview.chromium.org/2490623005/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2490623005/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:459: void
EmbeddedWorkerInstance::Start(
I think this approach is a bit confusing.
"EmbeddedWorkerStartParams = EmbeddedWorkerStartParamsInternal +
                             mojom::ServiceWorkerEventDispatcherRequest"

How about adding mojom::ServiceWorkerEventDispatcherRequest to the arguments of
EmbeddedWorkerInstance::Start()?
  void EmbeddedWorkerInstance::Start(
      std::unique_ptr<EmbeddedWorkerStartParams> params,
      mojom::ServiceWorkerEventDispatcherRequest dispatcher_request,
      const StatusCallback& callback) {}

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:197:
mojo::Binding<mojom::ServiceWorkerEventDispatcher> binding;
event_dispatcher_binding;?

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:509:
Bind(std::move(pending_dispatcher_request_));
BindEventDispatcher()

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:1254: void
ServiceWorkerContextClient::Bind(
BindEventDispatcher()

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.h:102: void
Bind(mojom::ServiceWorkerEventDispatcherRequest request);
BindEventDispatcher()

[shimazu, 2016-11-17 22:23:38]

thanks, updated.
ptal again:)

https://codereview.chromium.org/2490623005/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2490623005/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:459: void
EmbeddedWorkerInstance::Start(
On 2016/11/11 02:17:27, horo wrote:
> I think this approach is a bit confusing.
> "EmbeddedWorkerStartParams = EmbeddedWorkerStartParamsInternal +
>                              mojom::ServiceWorkerEventDispatcherRequest"
> 
> How about adding mojom::ServiceWorkerEventDispatcherRequest to the arguments
of
> EmbeddedWorkerInstance::Start()?
>   void EmbeddedWorkerInstance::Start(
>       std::unique_ptr<EmbeddedWorkerStartParams> params,
>       mojom::ServiceWorkerEventDispatcherRequest dispatcher_request,
>       const StatusCallback& callback) {}

Done.

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:197:
mojo::Binding<mojom::ServiceWorkerEventDispatcher> binding;
On 2016/11/11 02:17:27, horo wrote:
> event_dispatcher_binding;?

Done.

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:509:
Bind(std::move(pending_dispatcher_request_));
On 2016/11/11 02:17:27, horo wrote:
> BindEventDispatcher()

Done.

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:1254: void
ServiceWorkerContextClient::Bind(
On 2016/11/11 02:17:27, horo wrote:
> BindEventDispatcher()

Done.

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2490623005/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.h:102: void
Bind(mojom::ServiceWorkerEventDispatcherRequest request);
On 2016/11/11 02:17:27, horo wrote:
> BindEventDispatcher()

Done.

[horo, 2016-11-18 07:14:50]

lgtm

https://codereview.chromium.org/2490623005/diff/20001/content/common/service_...
File content/common/service_worker/embedded_worker_start_params.cc (right):

https://codereview.chromium.org/2490623005/diff/20001/content/common/service_...
content/common/service_worker/embedded_worker_start_params.cc:9:
EmbeddedWorkerStartParams::EmbeddedWorkerStartParams() {};
nit:
remove semicolon.

https://codereview.chromium.org/2490623005/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2490623005/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:556:
it.GetCurrentValue()->Run(SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
SERVICE_WORKER_ERROR_ABORT

[shimazu, 2016-11-18 19:51:34]

https://codereview.chromium.org/2490623005/diff/20001/content/common/service_...
File content/common/service_worker/embedded_worker_start_params.cc (right):

https://codereview.chromium.org/2490623005/diff/20001/content/common/service_...
content/common/service_worker/embedded_worker_start_params.cc:9:
EmbeddedWorkerStartParams::EmbeddedWorkerStartParams() {};
On 2016/11/18 07:14:49, horo wrote:
> nit:
> remove semicolon.

Done.

https://codereview.chromium.org/2490623005/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2490623005/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:556:
it.GetCurrentValue()->Run(SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
On 2016/11/18 07:14:50, horo wrote:
> SERVICE_WORKER_ERROR_ABORT

Done.

[shimazu, 2016-11-18 19:51:47]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-11-18 19:51:48]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2490623005/#ps40001
(title: "review")

[commit-bot: I haz the power, 2016-11-18 19:52:35]

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, 2016-11-18 21:01:15]

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

[commit-bot: I haz the power, 2016-11-18 21:01:17]

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...)

[shimazu, 2016-11-18 22:47:40]

shimazu@chromium.org changed reviewers:
+ ben@chromium.org, jkarlin@chromium.org, mkwst@chromium.org

[shimazu, 2016-11-18 22:47:41]

jkarlin@: PTAL at background_sync

mkwst@: PTAL at mojoms

ben@: PTAL at mainly render_thread_impl and could you quickly scan?

[dcheng, 2016-11-22 07:59:03]

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

[dcheng, 2016-11-22 07:59:04]

Nice cleanup! Just a few nits (and most of the comments are just for followup
requests)

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:109: new
EmbeddedWorkerStartParams);
Nit: base::MakeUnique

https://codereview.chromium.org/2490623005/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2490623005/diff/40001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:25:
DispatchSyncEvent(string tag, blink.mojom.BackgroundSyncEventLastChance
last_chance)
In a followup, can we add some comments about the different interfaces (where
they live, what they do) and the interface methods (what are the arguments,
return values, what do they do)? For example, I'm not really sure what |tag| is
here.

Nit: also 80 chars

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:881:
mojo::ConvertTo<blink::WebServiceWorkerContextProxy::LastChanceOption>(
Is it possible to typemap this?

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:222: void
DispatchFetchEvent(int fetch_event_id,
Nit: we should probably use int32_t consistently here to match the mojo types
(but this is OK to do in a followup)

[jkarlin, 2016-11-22 14:13:46]

background_sync lgtm, thanks for this!

[Ben Goodger (Google), 2016-11-22 18:48:47]

render_thread_impl lgtm

[Mike West, 2016-11-23 13:06:05]

Sorry, I was out sick; this change LGTM. That said, I agree with dcheng's
comments. If you clean those up, and he's happy, yay!

[falken, 2016-11-23 14:32:30]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2016-11-23 14:32:31]

drive by nits (you don't need my lg)

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:295: //
|dispatcher| is owned by |version_|. So it is safe to pass the unretained
nit: there is no longer a |dispatcher|

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.h:272: // after StopWorker
or OnDetached is called.
nit: the "should be" makes it unclear whether this comment is normative or
descriptive. Maybe s/should be/is/. But what does the function do when called
when it's invalid? Does it DCHECK in line 274? If so maybe the comment should
just say "This function can only be called when the worker is RUNNING." similar
to DispatchEvent below.

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:237: //
mojom::ServiceWorkerEventDispatcher
nit: DispatchFetchEvent and DispatchSyncEvent should be grouped together with a
comment "mojom::ServiceWorkerEventDispatcher overrides."
"When you derive from a base class, group any overriding functions in your
header file in one labeled section. Use the override specifier on all these
functions."
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:285: // This
should be bound on the worker thread.
"should be" is unclear... "This is bound on the worker thread" ?

[shimazu, 2016-11-24 06:47:32]

Thanks for your reviews! Fixed the nits.

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:109: new
EmbeddedWorkerStartParams);
On 2016/11/22 07:59:04, dcheng wrote:
> Nit: base::MakeUnique

Done.

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:295: //
|dispatcher| is owned by |version_|. So it is safe to pass the unretained
On 2016/11/23 14:32:31, falken (ooo 23 Nov in Japan) wrote:
> nit: there is no longer a |dispatcher|

Done.

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2490623005/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.h:272: // after StopWorker
or OnDetached is called.
On 2016/11/23 14:32:31, falken (ooo 23 Nov in Japan) wrote:
> nit: the "should be" makes it unclear whether this comment is normative or
> descriptive. Maybe s/should be/is/. But what does the function do when called
> when it's invalid? Does it DCHECK in line 274? If so maybe the comment should
> just say "This function can only be called when the worker is RUNNING."
similar
> to DispatchEvent below.

Thanks, that's right.
Updated.

https://codereview.chromium.org/2490623005/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2490623005/diff/40001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:25:
DispatchSyncEvent(string tag, blink.mojom.BackgroundSyncEventLastChance
last_chance)
On 2016/11/22 07:59:04, dcheng wrote:
> In a followup, can we add some comments about the different interfaces (where
> they live, what they do) and the interface methods (what are the arguments,
> return values, what do they do)? For example, I'm not really sure what |tag|
is
> here.
> 
> Nit: also 80 chars

Done.

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:881:
mojo::ConvertTo<blink::WebServiceWorkerContextProxy::LastChanceOption>(
On 2016/11/22 07:59:04, dcheng wrote:
> Is it possible to typemap this?

I don't think so because blink types aren't available in content/browser. I'll
do that when I move these functions to blink-side.

Added TODO for now.

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:222: void
DispatchFetchEvent(int fetch_event_id,
On 2016/11/22 07:59:04, dcheng wrote:
> Nit: we should probably use int32_t consistently here to match the mojo types
> (but this is OK to do in a followup)

Thanks, I'll do it when I move all of events to SWEventDispatcher.

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:237: //
mojom::ServiceWorkerEventDispatcher
On 2016/11/23 14:32:31, falken wrote:
> nit: DispatchFetchEvent and DispatchSyncEvent should be grouped together with
a
> comment "mojom::ServiceWorkerEventDispatcher overrides."
> "When you derive from a base class, group any overriding functions in your
> header file in one labeled section. Use the override specifier on all these
> functions."
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

Done.

https://codereview.chromium.org/2490623005/diff/40001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:285: // This
should be bound on the worker thread.
On 2016/11/23 14:32:31, falken wrote:
> "should be" is unclear... "This is bound on the worker thread" ?

Thanks, fixed it:)

[shimazu, 2016-11-24 06:47:43]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-11-24 06:47:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkarlin@chromium.org, mkwst@chromium.org, horo@chromium.org, ben@chromium.org
Link to the patchset: https://codereview.chromium.org/2490623005/#ps60001
(title: "review")

[commit-bot: I haz the power, 2016-11-24 06:48:04]

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, 2016-11-24 08:49:01]

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

[commit-bot: I haz the power, 2016-11-24 08:49:02]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[shimazu, 2016-11-24 23:56:44]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-11-24 23:56:57]

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, 2016-11-25 01:00:10]

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

[commit-bot: I haz the power, 2016-11-25 01:00:12]

Failed to apply patch for content/common/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: content/common/BUILD.gn:571
  error: content/common/BUILD.gn: patch does not apply

Patch:       content/common/BUILD.gn
Index: content/common/BUILD.gn
diff --git a/content/common/BUILD.gn b/content/common/BUILD.gn
index
ed827610ea9280eb4fc8b4d5c747fb8bd441afe9..e917c776c94b893f6aa7ec169dd56af6b4aae54b
100644
--- a/content/common/BUILD.gn
+++ b/content/common/BUILD.gn
@@ -571,8 +571,8 @@ mojom("mojo_bindings") {
     "renderer.mojom",
     "service_worker/embedded_worker.mojom",
     "service_worker/embedded_worker_setup.mojom",
-    "service_worker/fetch_event_dispatcher.mojom",
     "service_worker/service_worker.mojom",
+    "service_worker/service_worker_event_dispatcher.mojom",
     "service_worker/service_worker_types.mojom",
     "storage_partition_service.mojom",
     "url_loader.mojom",

[shimazu, 2016-11-25 01:44:00]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-11-25 01:44:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkarlin@chromium.org, mkwst@chromium.org, horo@chromium.org, ben@chromium.org
Link to the patchset: https://codereview.chromium.org/2490623005/#ps80001
(title: "rebase/fix comment")

[commit-bot: I haz the power, 2016-11-25 01:44:10]

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

[shimazu, 2016-11-25 03:17:44]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2016-11-25 03:17:50]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-11-25 03:17:57]

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, 2016-11-25 04:55:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480043870547410,
"parent_rev": "2e58279c828c0536e94399ba435dcbb064682f09", "commit_rev":
"b8d8b007523bdda2b9f7594da2a9def79d4e242a"}

[commit-bot: I haz the power, 2016-11-25 04:56:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-25 04:58:53]

Description was changed from

==========
Remove InterfaceRegistry/Provider from service workers

This patch is to remove unnecessary InterfaceProvider usage. Events, who are
currently the only user of InterfaceProvider in EWInstance, will be triggered
via InterfacePtr which is passed at the starting up.

BUG=664019
==========

to

==========
Remove InterfaceRegistry/Provider from service workers

This patch is to remove unnecessary InterfaceProvider usage. Events, who are
currently the only user of InterfaceProvider in EWInstance, will be triggered
via InterfacePtr which is passed at the starting up.

BUG=664019

Committed: https://crrev.com/317dd7b50bd2f138715fac044dcc44ce070ccbfc
Cr-Commit-Position: refs/heads/master@{#434434}
==========

[commit-bot: I haz the power, 2016-11-25 04:58:56]

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