DevTools: Added service workers to chrome://inspect/#devices

DevTools: Added service workers to chrome://inspect/#devices

BUG=389454

[vkuzkokov, 2014-06-30 16:25:54]

I will remove code that ended up being unused.

[pfeldman, 2014-06-30 16:37:21]

https://codereview.chromium.org/349033009/diff/1/content/browser/devtools/emb...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/349033009/diff/1/content/browser/devtools/emb...
content/browser/devtools/embedded_worker_devtools_manager.cc:181: public:
We can't implement DevToolsTarget in content. It is defined in content to be
implemented by embedder. You should instead serialize right into JSON
preferrably in devtools_http_handler_impl.

https://codereview.chromium.org/349033009/diff/1/content/browser/devtools/emb...
content/browser/devtools/embedded_worker_devtools_manager.cc:468: //if (it ==
workers_.end());
Cleanup.

[vkuzkokov, 2014-07-04 16:06:48]

https://codereview.chromium.org/349033009/diff/1/content/browser/devtools/emb...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/349033009/diff/1/content/browser/devtools/emb...
content/browser/devtools/embedded_worker_devtools_manager.cc:181: public:
On 2014/06/30 16:37:21, pfeldman wrote:
> We can't implement DevToolsTarget in content. It is defined in content to be
> implemented by embedder. You should instead serialize right into JSON
> preferrably in devtools_http_handler_impl.  

That would require a new interface which would mirror method by method
DevToolsTarget with 2 implementations: this one and the one that delegates to
actual DevToolsTarget. Seems useless.

https://codereview.chromium.org/349033009/diff/1/content/browser/devtools/emb...
content/browser/devtools/embedded_worker_devtools_manager.cc:468: //if (it ==
workers_.end());
On 2014/06/30 16:37:21, pfeldman wrote:
> Cleanup.

Uncommented and finished

[pfeldman, 2014-07-05 20:17:09]

https://codereview.chromium.org/349033009/diff/1/content/browser/devtools/emb...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/349033009/diff/1/content/browser/devtools/emb...
content/browser/devtools/embedded_worker_devtools_manager.cc:181: public:
Can we migrate to DTAH for everything instead?

[dominicc (has gone to gerrit), 2014-07-08 02:35:54]

Looks good to me, but +horo since he implemented the
chrome://inspect/#service-workers stuff and is in OWNERs for
embedded_worker_instance.cc.

[horo, 2014-07-08 08:14:31]

https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:321:
ServiceWorkerVersion* version = it->second.service_worker_context_->
ServiceWorkerIdentifier's service_worker_context_ was intended to be used only
for the identification of the workers.
service_worker_context_(ServiceWorkerContextCore) could have been deleted even
if it is in workers_(WorkerInfoMap).

If the inspector window is opened, when the worker is stopped the WorkerInfo of
the worker is still stored in WorkerInfoMap.
And at this situation, the ServiceWorkerContextCore could be deleted which is
owend by StoragePartition.

So insteadof calling the method of service_worker_context_ please get the
ServiceWorkerContext using BrowserContext::ForEachStoragePartition() like
ServiceWorkerInternalsUI::GetAllRegistrations().
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...

[vkuzkokov, 2014-07-08 09:38:49]

https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:321:
ServiceWorkerVersion* version = it->second.service_worker_context_->
On 2014/07/08 08:14:31, horo wrote:
> ServiceWorkerIdentifier's service_worker_context_ was intended to be used only
> for the identification of the workers.
> service_worker_context_(ServiceWorkerContextCore) could have been deleted even
> if it is in workers_(WorkerInfoMap).
> 
> If the inspector window is opened, when the worker is stopped the WorkerInfo
of
> the worker is still stored in WorkerInfoMap.
> And at this situation, the ServiceWorkerContextCore could be deleted which is
> owend by StoragePartition.
> 
> So insteadof calling the method of service_worker_context_ please get the
> ServiceWorkerContext using BrowserContext::ForEachStoragePartition() like
> ServiceWorkerInternalsUI::GetAllRegistrations().
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...

ForEachStoragePartition requires passing instance of BrowserContext. Which, I
guess, means that I have to somehow get all live instances of BrowserContext
first.

[horo, 2014-07-09 02:30:44]

https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:321:
ServiceWorkerVersion* version = it->second.service_worker_context_->
On 2014/07/08 09:38:48, vkuzkokov wrote:
> On 2014/07/08 08:14:31, horo wrote:
> > ServiceWorkerIdentifier's service_worker_context_ was intended to be used
only
> > for the identification of the workers.
> > service_worker_context_(ServiceWorkerContextCore) could have been deleted
even
> > if it is in workers_(WorkerInfoMap).
> > 
> > If the inspector window is opened, when the worker is stopped the WorkerInfo
> of
> > the worker is still stored in WorkerInfoMap.
> > And at this situation, the ServiceWorkerContextCore could be deleted which
is
> > owend by StoragePartition.
> > 
> > So insteadof calling the method of service_worker_context_ please get the
> > ServiceWorkerContext using BrowserContext::ForEachStoragePartition() like
> > ServiceWorkerInternalsUI::GetAllRegistrations().
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> 
> ForEachStoragePartition requires passing instance of BrowserContext. Which, I
> guess, means that I have to somehow get all live instances of BrowserContext
> first.

Humm..
Doesn't DevToolsHttpHandler have a way to get the current profile?

[vkuzkokov, 2014-07-09 07:48:52]

On 2014/07/09 02:30:44, horo wrote:
>
https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
> File content/browser/devtools/embedded_worker_devtools_manager.cc (right):
> 
>
https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
> content/browser/devtools/embedded_worker_devtools_manager.cc:321:
> ServiceWorkerVersion* version = it->second.service_worker_context_->
> On 2014/07/08 09:38:48, vkuzkokov wrote:
> > On 2014/07/08 08:14:31, horo wrote:
> > > ServiceWorkerIdentifier's service_worker_context_ was intended to be used
> only
> > > for the identification of the workers.
> > > service_worker_context_(ServiceWorkerContextCore) could have been deleted
> even
> > > if it is in workers_(WorkerInfoMap).
> > > 
> > > If the inspector window is opened, when the worker is stopped the
WorkerInfo
> > of
> > > the worker is still stored in WorkerInfoMap.
> > > And at this situation, the ServiceWorkerContextCore could be deleted which
> is
> > > owend by StoragePartition.
> > > 
> > > So insteadof calling the method of service_worker_context_ please get the
> > > ServiceWorkerContext using BrowserContext::ForEachStoragePartition() like
> > > ServiceWorkerInternalsUI::GetAllRegistrations().
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> > 
> > ForEachStoragePartition requires passing instance of BrowserContext. Which,
I
> > guess, means that I have to somehow get all live instances of BrowserContext
> > first.
> 
> Humm..
> Doesn't DevToolsHttpHandler have a way to get the current profile?

It doesn't have current profile neither it should. It returns all targets
irrespectible of profile.

[horo, 2014-07-09 11:58:20]

On 2014/07/09 07:48:52, vkuzkokov wrote:
> On 2014/07/09 02:30:44, horo wrote:
> >
>
https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
> > File content/browser/devtools/embedded_worker_devtools_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
> > content/browser/devtools/embedded_worker_devtools_manager.cc:321:
> > ServiceWorkerVersion* version = it->second.service_worker_context_->
> > On 2014/07/08 09:38:48, vkuzkokov wrote:
> > > On 2014/07/08 08:14:31, horo wrote:
> > > > ServiceWorkerIdentifier's service_worker_context_ was intended to be
used
> > only
> > > > for the identification of the workers.
> > > > service_worker_context_(ServiceWorkerContextCore) could have been
deleted
> > even
> > > > if it is in workers_(WorkerInfoMap).
> > > > 
> > > > If the inspector window is opened, when the worker is stopped the
> WorkerInfo
> > > of
> > > > the worker is still stored in WorkerInfoMap.
> > > > And at this situation, the ServiceWorkerContextCore could be deleted
which
> > is
> > > > owend by StoragePartition.
> > > > 
> > > > So insteadof calling the method of service_worker_context_ please get
the
> > > > ServiceWorkerContext using BrowserContext::ForEachStoragePartition()
like
> > > > ServiceWorkerInternalsUI::GetAllRegistrations().
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> > > 
> > > ForEachStoragePartition requires passing instance of BrowserContext.
Which,
> I
> > > guess, means that I have to somehow get all live instances of
BrowserContext
> > > first.
> > 
> > Humm..
> > Doesn't DevToolsHttpHandler have a way to get the current profile?
> 
> It doesn't have current profile neither it should. It returns all targets
> irrespectible of profile.

OK then, please check WorkerState is not WORKER_TERMINATED before calling
methods of service_worker_context.

[vkuzkokov, 2014-07-09 12:12:25]

On 2014/07/09 11:58:20, horo wrote:
> On 2014/07/09 07:48:52, vkuzkokov wrote:
> > On 2014/07/09 02:30:44, horo wrote:
> > >
> >
>
https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
> > > File content/browser/devtools/embedded_worker_devtools_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
> > > content/browser/devtools/embedded_worker_devtools_manager.cc:321:
> > > ServiceWorkerVersion* version = it->second.service_worker_context_->
> > > On 2014/07/08 09:38:48, vkuzkokov wrote:
> > > > On 2014/07/08 08:14:31, horo wrote:
> > > > > ServiceWorkerIdentifier's service_worker_context_ was intended to be
> used
> > > only
> > > > > for the identification of the workers.
> > > > > service_worker_context_(ServiceWorkerContextCore) could have been
> deleted
> > > even
> > > > > if it is in workers_(WorkerInfoMap).
> > > > > 
> > > > > If the inspector window is opened, when the worker is stopped the
> > WorkerInfo
> > > > of
> > > > > the worker is still stored in WorkerInfoMap.
> > > > > And at this situation, the ServiceWorkerContextCore could be deleted
> which
> > > is
> > > > > owend by StoragePartition.
> > > > > 
> > > > > So insteadof calling the method of service_worker_context_ please get
> the
> > > > > ServiceWorkerContext using BrowserContext::ForEachStoragePartition()
> like
> > > > > ServiceWorkerInternalsUI::GetAllRegistrations().
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> > > > 
> > > > ForEachStoragePartition requires passing instance of BrowserContext.
> Which,
> > I
> > > > guess, means that I have to somehow get all live instances of
> BrowserContext
> > > > first.
> > > 
> > > Humm..
> > > Doesn't DevToolsHttpHandler have a way to get the current profile?
> > 
> > It doesn't have current profile neither it should. It returns all targets
> > irrespectible of profile.
> 
> OK then, please check WorkerState is not WORKER_TERMINATED before calling
> methods of service_worker_context.

This will cause race condition: WorkerState is changed on UI, methods of
service_worker_context are called on IO.

[horo, 2014-07-09 12:30:44]

On 2014/07/09 12:12:25, vkuzkokov wrote:
> On 2014/07/09 11:58:20, horo wrote:
> > On 2014/07/09 07:48:52, vkuzkokov wrote:
> > > On 2014/07/09 02:30:44, horo wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
> > > > File content/browser/devtools/embedded_worker_devtools_manager.cc
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/349033009/diff/40001/content/browser/devtools...
> > > > content/browser/devtools/embedded_worker_devtools_manager.cc:321:
> > > > ServiceWorkerVersion* version = it->second.service_worker_context_->
> > > > On 2014/07/08 09:38:48, vkuzkokov wrote:
> > > > > On 2014/07/08 08:14:31, horo wrote:
> > > > > > ServiceWorkerIdentifier's service_worker_context_ was intended to be
> > used
> > > > only
> > > > > > for the identification of the workers.
> > > > > > service_worker_context_(ServiceWorkerContextCore) could have been
> > deleted
> > > > even
> > > > > > if it is in workers_(WorkerInfoMap).
> > > > > > 
> > > > > > If the inspector window is opened, when the worker is stopped the
> > > WorkerInfo
> > > > > of
> > > > > > the worker is still stored in WorkerInfoMap.
> > > > > > And at this situation, the ServiceWorkerContextCore could be deleted
> > which
> > > > is
> > > > > > owend by StoragePartition.
> > > > > > 
> > > > > > So insteadof calling the method of service_worker_context_ please
get
> > the
> > > > > > ServiceWorkerContext using BrowserContext::ForEachStoragePartition()
> > like
> > > > > > ServiceWorkerInternalsUI::GetAllRegistrations().
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> > > > > 
> > > > > ForEachStoragePartition requires passing instance of BrowserContext.
> > Which,
> > > I
> > > > > guess, means that I have to somehow get all live instances of
> > BrowserContext
> > > > > first.
> > > > 
> > > > Humm..
> > > > Doesn't DevToolsHttpHandler have a way to get the current profile?
> > > 
> > > It doesn't have current profile neither it should. It returns all targets
> > > irrespectible of profile.
> > 
> > OK then, please check WorkerState is not WORKER_TERMINATED before calling
> > methods of service_worker_context.
> 
> This will cause race condition: WorkerState is changed on UI, methods of
> service_worker_context are called on IO.

Oh yes.
Can't we have a way to get the all profiles?

[dgozman, 2014-07-14 11:17:25]

https://codereview.chromium.org/349033009/diff/120001/android_webview/native/...
File android_webview/native/aw_dev_tools_server.cc (right):

https://codereview.chromium.org/349033009/diff/120001/android_webview/native/...
android_webview/native/aw_dev_tools_server.cc:74: if (RenderViewHost* rvh =
agent_host_->GetRenderViewHost()) {
Targets without RVH should get informative title (probably, from the type of
DevToolsAgentHost instance).
Same goes for all other embedders using DevToolsAgentHost::GetOrCreateAllHosts.

https://codereview.chromium.org/349033009/diff/120001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_target_impl.cc (right):

https://codereview.chromium.org/349033009/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/devtools_target_impl.cc:381: static void
CollectWorkers(const DevToolsTargetImpl::Callback& callback,
I'd rename this one to CollectAllTargets and it's callback to
CollectAllTargetsCallback/OnAllTargetsCollected.

https://codereview.chromium.org/349033009/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/devtools_target_impl.cc:394:
base::Bind(&DevToolsTargetImpl::EnumerateWorkerTargets,
This method should be called EnumerateSharedWorkerTargets.

https://codereview.chromium.org/349033009/diff/120001/content/browser/devtool...
File content/browser/devtools/worker_devtools_manager.cc (right):

https://codereview.chromium.org/349033009/diff/120001/content/browser/devtool...
content/browser/devtools/worker_devtools_manager.cc:81: void
DevToolsAgentHost::GetOrCreateAllHosts(
I think, this code should be in devtools_agent_host_impl.cc

https://codereview.chromium.org/349033009/diff/120001/content/public/browser/...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/349033009/diff/120001/content/public/browser/...
content/public/browser/devtools_agent_host.h:49: typedef
base::Callback<void(const List&)> Callback;
AllHostsCallback

https://codereview.chromium.org/349033009/diff/120001/content/public/browser/...
content/public/browser/devtools_agent_host.h:62: static
std::vector<RenderViewHost*> GetValidRenderViewHosts();
This one should go away.

https://codereview.chromium.org/349033009/diff/120001/content/public/browser/...
content/public/browser/devtools_agent_host.h:84: virtual bool IsWorker() const =
0;
You need to remove this method, and instead add GetType().

[vkuzkokov, 2014-07-14 15:31:33]

https://codereview.chromium.org/349033009/diff/120001/android_webview/native/...
File android_webview/native/aw_dev_tools_server.cc (right):

https://codereview.chromium.org/349033009/diff/120001/android_webview/native/...
android_webview/native/aw_dev_tools_server.cc:74: if (RenderViewHost* rvh =
agent_host_->GetRenderViewHost()) {
On 2014/07/14 11:17:24, dgozman wrote:
> Targets without RVH should get informative title (probably, from the type of
> DevToolsAgentHost instance).
> Same goes for all other embedders using
DevToolsAgentHost::GetOrCreateAllHosts.

This should be done on client.

https://codereview.chromium.org/349033009/diff/120001/content/browser/devtool...
File content/browser/devtools/worker_devtools_manager.cc (right):

https://codereview.chromium.org/349033009/diff/120001/content/browser/devtool...
content/browser/devtools/worker_devtools_manager.cc:81: void
DevToolsAgentHost::GetOrCreateAllHosts(
On 2014/07/14 11:17:24, dgozman wrote:
> I think, this code should be in devtools_agent_host_impl.cc

Done.

https://codereview.chromium.org/349033009/diff/120001/content/public/browser/...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/349033009/diff/120001/content/public/browser/...
content/public/browser/devtools_agent_host.h:49: typedef
base::Callback<void(const List&)> Callback;
On 2014/07/14 11:17:24, dgozman wrote:
> AllHostsCallback

ListCallback

https://codereview.chromium.org/349033009/diff/120001/content/public/browser/...
content/public/browser/devtools_agent_host.h:62: static
std::vector<RenderViewHost*> GetValidRenderViewHosts();
On 2014/07/14 11:17:25, dgozman wrote:
> This one should go away.

Done.

[dominicc (has gone to gerrit), 2014-07-17 04:03:17]

(FYI horo is out of office today.)

[horo, 2014-07-18 02:28:12]

LGTM in terms of EmbeddedWorkerDevToolsManager.

[pfeldman, 2014-08-12 11:45:00]

It seems like this change is doing more than one thing at a time. Mind splitting
it?