[3/5 chromium] Shows the clients which are controlled by ServiceWorker in DevTools.

[3/5 chromium] Shows the clients which are controlled by ServiceWorker in DevTools.

This cl changes ServiceWorkerVersion::GetInfo() to fill the infromation about
the controllee in ServiceWorkerVersionInfo. And ServiceWorkerHandler sends it to
DevTools. DevTools will shows it in ServiceWorkersView after
https://codereview.chromium.org/1164583002/.

This cl changes ServiceWorkerHostMsg_ProviderCreated message to pass not only
the frame IDs of the SW controlled documents, but also the Shared Worker route
IDs of the SW controlled Shared Workers. The frame IDs and the Shared Worker
route IDs are used to get DevToolsAgentHost in ServiceWorkerHandler.

Screenshot: https://code.google.com/p/chromium/issues/detail?id=466871#c65

1/5 chromium: https://codereview.chromium.org/1160133002/
2/5 blink: https://codereview.chromium.org/1151993003/
3/5 chromium: This cl.
4/5 blink: https://codereview.chromium.org/1164583002/
5/5 chromium: https://codereview.chromium.org/1143363009/

BUG=466871
Committed: https://crrev.com/bcbc8a511277a134eda362b1961940819de827b2
Cr-Commit-Position: refs/heads/master@{#334981}

[horo, 2015-06-02 03:16:59]

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

[horo, 2015-06-02 03:17:00]

nhiroki@
Could you please review this?

[horo, 2015-06-03 08:32:02]

On 2015/06/02 03:17:00, horo wrote:
> nhiroki@
> Could you please review this?

Sorry.
I'm rewriting this cl to incorporate pfeldman's comment in
https://codereview.chromium.org/1151993003/.
So please wait to review.

[horo, 2015-06-05 02:35:00]

Patchset #2 (id:20001) has been deleted

[horo, 2015-06-05 05:52:48]

Patchset #3 (id:60001) has been deleted

[horo, 2015-06-05 05:57:31]

Patchset #2 (id:40001) has been deleted

[horo, 2015-06-10 07:51:46]

Patchset #7 (id:180001) has been deleted

[horo, 2015-06-10 07:51:54]

Patchset #7 (id:200001) has been deleted

[horo, 2015-06-10 07:53:33]

On 2015/06/03 08:32:02, horo wrote:
> On 2015/06/02 03:17:00, horo wrote:
> > nhiroki@
> > Could you please review this?
> 
> Sorry.
> I'm rewriting this cl to incorporate pfeldman's comment in
> https://codereview.chromium.org/1151993003/.
> So please wait to review.

I finished updating this CL.
Please review it again.

Thank you.

[horo, 2015-06-12 01:36:52]

On 2015/06/10 07:53:33, horo wrote:
> On 2015/06/03 08:32:02, horo wrote:
> > On 2015/06/02 03:17:00, horo wrote:
> > > nhiroki@
> > > Could you please review this?
> > 
> > Sorry.
> > I'm rewriting this cl to incorporate pfeldman's comment in
> > https://codereview.chromium.org/1151993003/.
> > So please wait to review.
> 
> I finished updating this CL.
> Please review it again.
> 
> Thank you.

ping?

[kinuko, 2015-06-12 02:06:35]

kinuko@chromium.org changed reviewers:
+ serviceworker-reviews@chromium.org

[kinuko, 2015-06-12 02:06:54]

On 2015/06/12 01:36:52, horo wrote:
> On 2015/06/10 07:53:33, horo wrote:
> > On 2015/06/03 08:32:02, horo wrote:
> > > On 2015/06/02 03:17:00, horo wrote:
> > > > nhiroki@
> > > > Could you please review this?
> > > 
> > > Sorry.
> > > I'm rewriting this cl to incorporate pfeldman's comment in
> > > https://codereview.chromium.org/1151993003/.
> > > So please wait to review.
> > 
> > I finished updating this CL.
> > Please review it again.
> > 
> > Thank you.
> 
> ping?

+serviceworker-reviews@, hiroki might be a bit overloaded.

[nhiroki, 2015-06-12 02:11:27]

Looks good overall.

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:99: }
nit: {} is not necessary

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:106: }
ditto.

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:447: // TODO(horo):
Remove this.
nit: bug link

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:460: }
nit: {}

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.h (right):

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.h:62: // TODO(horo):
Remove this.
nit: Can we have a bug link?

https://codereview.chromium.org/1149383004/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1149383004/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:94:
provider_type_ = SERVICE_WORKER_PROVIDER_FOR_SHARED_WORKER;
(I'm assuming that "shared_worker_route_id" is almost equal to
"render_frame_id")

How about merging shared_worker_route_id into render_frame_id (rename it to
route_id??), and keep provider_type argument? Identifying keeping multiple ids
could not scale and current provider_type could be more readable.

[nhiroki, 2015-06-12 02:33:21]

On 2015/06/12 02:11:27, nhiroki wrote:
>
https://codereview.chromium.org/1149383004/diff/220001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.cc:94:
> provider_type_ = SERVICE_WORKER_PROVIDER_FOR_SHARED_WORKER;
> (I'm assuming that "shared_worker_route_id" is almost equal to
> "render_frame_id")
> 
> How about merging shared_worker_route_id into render_frame_id (rename it to
> route_id??), and keep provider_type argument? Identifying keeping multiple ids
> could not scale and current provider_type could be more readable.

nit: please ignore "Identifying" :)

[horo, 2015-06-12 04:19:19]

Thank you

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:99: }
On 2015/06/12 02:11:27, nhiroki wrote:
> nit: {} is not necessary

Done.

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:106: }
On 2015/06/12 02:11:27, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:447: // TODO(horo):
Remove this.
On 2015/06/12 02:11:27, nhiroki wrote:
> nit: bug link

Done.

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:460: }
On 2015/06/12 02:11:27, nhiroki wrote:
> nit: {}

Done.

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.h (right):

https://codereview.chromium.org/1149383004/diff/220001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.h:62: // TODO(horo):
Remove this.
On 2015/06/12 02:11:27, nhiroki wrote:
> nit: Can we have a bug link?

Done.

https://codereview.chromium.org/1149383004/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1149383004/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:94:
provider_type_ = SERVICE_WORKER_PROVIDER_FOR_SHARED_WORKER;
On 2015/06/12 02:11:27, nhiroki wrote:
> (I'm assuming that "shared_worker_route_id" is almost equal to
> "render_frame_id")
> 
> How about merging shared_worker_route_id into render_frame_id (rename it to
> route_id??), and keep provider_type argument? Identifying keeping multiple ids
> could not scale and current provider_type could be more readable.

Done.

[nhiroki, 2015-06-12 05:48:02]

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_info.cc (right):

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_info.cc:12:
ServiceWorkerVersionInfo::ClientInfo::ClientInfo()
Is this ctor used?

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_info.cc:15:
shared_worker_route_id(MSG_ROUTING_NONE) {
How about using the delegated ctor?

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_info.h (right):

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_info.h:26: int
shared_worker_route_id;
Why don't you merge frame_id and shared_worker_route_id into route_id, and have
WebServiceWorkerClientType instead?

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:55: //
MSG_ROUTING_NONE, Whent this provider host is for a Document,
", Whent" -> ". When"
(comma) -> (dot)

[horo, 2015-06-12 08:45:43]

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_info.cc (right):

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_info.cc:12:
ServiceWorkerVersionInfo::ClientInfo::ClientInfo()
On 2015/06/12 05:48:01, nhiroki wrote:
> Is this ctor used?

We need it to use ClientInfo as a value type of map in
"std::map<std::string, ClientInfo> clients;".

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_info.cc:15:
shared_worker_route_id(MSG_ROUTING_NONE) {
On 2015/06/12 05:48:01, nhiroki wrote:
> How about using the delegated ctor?

Done.

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_info.h (right):

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_info.h:26: int
shared_worker_route_id;
On 2015/06/12 05:48:02, nhiroki wrote:
> Why don't you merge frame_id and shared_worker_route_id into route_id, and
have
> WebServiceWorkerClientType instead?

Done.

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/1149383004/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:55: //
MSG_ROUTING_NONE, Whent this provider host is for a Document,
On 2015/06/12 05:48:02, nhiroki wrote:
> ", Whent" -> ". When"
> (comma) -> (dot)

Done.

[nhiroki, 2015-06-12 09:07:47]

LGTM with nits

https://codereview.chromium.org/1149383004/diff/260001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/1149383004/diff/260001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:221: default:
This switch-case covers all types. Can we delete this "default" and move
NOTREACHED to 224?

https://codereview.chromium.org/1149383004/diff/260001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1149383004/diff/260001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:542: const
ServiceWorkerProviderHost* const host = controllee.second;
"const T* const foo" seems not common among our codebase... "const T* foo" could
be sufficient?

[horo, 2015-06-12 09:23:47]

https://codereview.chromium.org/1149383004/diff/260001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/1149383004/diff/260001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:221: default:
On 2015/06/12 09:07:47, nhiroki wrote:
> This switch-case covers all types. Can we delete this "default" and move
> NOTREACHED to 224?

Done.

https://codereview.chromium.org/1149383004/diff/260001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1149383004/diff/260001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:542: const
ServiceWorkerProviderHost* const host = controllee.second;
On 2015/06/12 09:07:47, nhiroki wrote:
> "const T* const foo" seems not common among our codebase... "const T* foo"
could
> be sufficient?

Done.

[horo, 2015-06-12 09:27:16]

horo@chromium.org changed reviewers:
+ dcheng@chromium.org, pfeldman@chromium.org

[horo, 2015-06-12 09:27:17]

pfeldman@
Please review content/browser/devtools/protocol/service_worker_handler.*.

dcheng@
Please review content/common/service_worker/service_worker_messages.h.

[pfeldman, 2015-06-13 06:53:23]

devtools lgtm

[horo, 2015-06-16 07:58:06]

dcheng@
Could you please review content/common/service_worker/service_worker_messages.h?

[dcheng, 2015-06-16 08:36:55]

Sorry I'll look at this first thing in the morning.

On Tue, Jun 16, 2015, 00:58  <horo@chromium.org> wrote:

> dcheng@
> Could you please review
> content/common/service_worker/service_worker_messages.h?
>
> https://codereview.chromium.org/1149383004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2015-06-17 02:57:31]

Also, can you elaborate the CL description? Is it the case that Devtools
couldn't inspect service workers at all? Also, please mention some specifics of
the plumbing details (blah blah change IPC routing we can track service workers
associated with Document/shared worker/etc)

https://codereview.chromium.org/1149383004/diff/300001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:223: return "";
Nit: return std::string() instead of return ""

https://codereview.chromium.org/1149383004/diff/300001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.h (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.h:66:
scoped_refptr<TargetInfo>* target_info);
Yuck =(

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:561: if
(!observer_list_.get())
No .get()

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:34:
ServiceWorkerClientInfo FocusOnUIThread(int render_process_id, int route_id) {
Some questions:
1) What should focus do if it's called on a service worker for a shared worker?
2) What happens if we fail to find a RenderFrameHost? Won't we just crash the
browser today?
3) Can a compromised renderer focus any RenderFrameHost it wants?

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:541: for (auto&
controllee : controllee_map_) {
const auto&?

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:544:
std::pair<std::string, ServiceWorkerVersionInfo::ClientInfo>(
std::make_pair

https://codereview.chromium.org/1149383004/diff/300001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/1149383004/diff/300001/content/common/service...
content/common/service_worker/service_worker_messages.h:153: // When the context
is a Document, |route_id| is the frame ID of it. When the
What does "context" mean here?

[horo, 2015-06-17 07:37:05]

> Also, can you elaborate the CL description? Is it the case that Devtools
> couldn't inspect service workers at all? Also, please mention some specifics
of
> the plumbing details (blah blah change IPC routing we can track service
workers
> associated with Document/shared worker/etc)

Done.

https://codereview.chromium.org/1149383004/diff/300001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.cc:223: return "";
On 2015/06/17 02:57:30, dcheng wrote:
> Nit: return std::string() instead of return ""

Done.

https://codereview.chromium.org/1149383004/diff/300001/content/browser/devtoo...
File content/browser/devtools/protocol/service_worker_handler.h (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/devtoo...
content/browser/devtools/protocol/service_worker_handler.h:66:
scoped_refptr<TargetInfo>* target_info);
On 2015/06/17 02:57:30, dcheng wrote:
> Yuck =(
The following code will be generated in devtools_protocol_dispatcher.cc by
devtools_protocol_handler_generator.py after this
(https://codereview.chromium.org/1164583002/diff/180001/Source/devtools/protoc...)
change.
So it must be "scoped_refptr<TargetInfo>* target_info".

  scoped_refptr<devtools::service_worker::TargetInfo> out_target_info;
  Response response = service_worker_handler_->GetTargetInfo(
      in_target_id,
      &out_target_info);

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:561: if
(!observer_list_.get())
On 2015/06/17 02:57:30, dcheng wrote:
> No .get()

Done.

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:34:
ServiceWorkerClientInfo FocusOnUIThread(int render_process_id, int route_id) {
On 2015/06/17 02:57:30, dcheng wrote:
> Some questions:
> 1) What should focus do if it's called on a service worker for a shared
worker?

We only support focusing windows.
WindowClient interface has focus() method, but Client interface doesn't have.
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#client
So if the controlled client is a shared worker, we can't call .focus().
I added a code to check |provider_type_| in ServiceWorkerProviderHost::Focus().

> 2) What happens if we fail to find a RenderFrameHost? Won't we just crash the
> browser today?

It doesn't crash.
It just returns an empty ServiceWorkerClientInfo.

> 3) Can a compromised renderer focus any RenderFrameHost it wants?

Only when the compromised renderer knows the UUID of the pages which are
controlled by ServiceWorkers, it can focus them.

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:541: for (auto&
controllee : controllee_map_) {
On 2015/06/17 02:57:30, dcheng wrote:
> const auto&?

Done.

https://codereview.chromium.org/1149383004/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:544:
std::pair<std::string, ServiceWorkerVersionInfo::ClientInfo>(
On 2015/06/17 02:57:30, dcheng wrote:
> std::make_pair

Done.

https://codereview.chromium.org/1149383004/diff/300001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/1149383004/diff/300001/content/common/service...
content/common/service_worker/service_worker_messages.h:153: // When the context
is a Document, |route_id| is the frame ID of it. When the
On 2015/06/17 02:57:30, dcheng wrote:
> What does "context" mean here?

"context" means "for what the provider was created".

[dcheng, 2015-06-17 23:21:13]

lgtm with a nit

https://codereview.chromium.org/1149383004/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1149383004/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:34:
ServiceWorkerClientInfo FocusOnUIThread(int render_process_id, int route_id) {
Nit: keep this named "render_frame_id"

[horo, 2015-06-17 23:51:07]

Thank you!

https://codereview.chromium.org/1149383004/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1149383004/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:34:
ServiceWorkerClientInfo FocusOnUIThread(int render_process_id, int route_id) {
On 2015/06/17 23:21:13, dcheng wrote:
> Nit: keep this named "render_frame_id"

Done.

[horo, 2015-06-18 00:02:16]

The CQ bit was checked by horo@chromium.org

[horo, 2015-06-18 00:02:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, pfeldman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1149383004/#ps340001
(title: "incorporated dcheng's comment")

[commit-bot: I haz the power, 2015-06-18 00:05:53]

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

[commit-bot: I haz the power, 2015-06-18 02:00:40]

Committed patchset #13 (id:340001)

[commit-bot: I haz the power, 2015-06-18 02:01:42]

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