DevTools: Merge WorkerInfo into EmbeddedWorkerDevToolsAgentHost

content/browser/devtools/embedded_worker_devtools_manager.cc

Index: content/browser/devtools/embedded_worker_devtools_manager.cc
diff --git a/content/browser/devtools/embedded_worker_devtools_manager.cc b/content/browser/devtools/embedded_worker_devtools_manager.cc
index 0d9b7e9cd34444aaa3c098531be5cd9daca1278d..1815498a92fc71c8b4de59f5f442b5d940574cce 100644
--- a/content/browser/devtools/embedded_worker_devtools_manager.cc
+++ b/content/browser/devtools/embedded_worker_devtools_manager.cc
@@ -62,44 +62,28 @@ bool EmbeddedWorkerDevToolsManager::ServiceWorkerIdentifier::Matches(
          service_worker_version_id_ == other.service_worker_version_id_;
 }
 
-EmbeddedWorkerDevToolsManager::WorkerInfo::WorkerInfo(
-    const SharedWorkerInstance& instance)
-    : shared_worker_instance_(new SharedWorkerInstance(instance)),
-      state_(WORKER_UNINSPECTED),
-      agent_host_(NULL) {
-}
-
-EmbeddedWorkerDevToolsManager::WorkerInfo::WorkerInfo(
-    const ServiceWorkerIdentifier& service_worker_id)
-    : service_worker_id_(new ServiceWorkerIdentifier(service_worker_id)),
-      state_(WORKER_UNINSPECTED),
-      agent_host_(NULL) {
-}
-
-bool EmbeddedWorkerDevToolsManager::WorkerInfo::Matches(
-    const SharedWorkerInstance& other) {
-  if (!shared_worker_instance_)
-    return false;
-  return shared_worker_instance_->Matches(other);
-}
-
-bool EmbeddedWorkerDevToolsManager::WorkerInfo::Matches(
-    const ServiceWorkerIdentifier& other) {
-  if (!service_worker_id_)
-    return false;
-  return service_worker_id_->Matches(other);
-}
-
-EmbeddedWorkerDevToolsManager::WorkerInfo::~WorkerInfo() {
-}
-
 class EmbeddedWorkerDevToolsManager::EmbeddedWorkerDevToolsAgentHost
     : public IPCDevToolsAgentHost,
       public IPC::Listener {
  public:
-  explicit EmbeddedWorkerDevToolsAgentHost(WorkerId worker_id)
-      : worker_id_(worker_id), worker_attached_(false) {
-    AttachToWorker();
+  EmbeddedWorkerDevToolsAgentHost(
+      WorkerId worker_id,
+      const SharedWorkerInstance& shared_worker)
+      : shared_worker_(new SharedWorkerInstance(shared_worker)),
+        state_(WORKER_UNINSPECTED),
+        worker_id_(worker_id),
+        worker_attached_(false) {
+    WorkerCreated();
+  }
+
+  EmbeddedWorkerDevToolsAgentHost(
+      WorkerId worker_id,
+      const ServiceWorkerIdentifier& service_worker)
+      : service_worker_(new ServiceWorkerIdentifier(service_worker)),
+        state_(WORKER_UNINSPECTED),
+        worker_id_(worker_id),
+        worker_attached_(false) {
+    WorkerCreated();
   }
 
   // DevToolsAgentHost override.
@@ -139,7 +123,7 @@ class EmbeddedWorkerDevToolsManager::EmbeddedWorkerDevToolsAgentHost
     if (!IsAttached())
       return;
     AttachToWorker();
-    Reattach(state_);
+    Reattach(saved_state_);
   }
 
   void DetachFromWorker() {
@@ -148,10 +132,28 @@ class EmbeddedWorkerDevToolsManager::EmbeddedWorkerDevToolsAgentHost
     worker_attached_ = false;
     if (RenderProcessHost* host = RenderProcessHost::FromID(worker_id_.first))
       host->RemoveRoute(worker_id_.second);
+  }
+
+  void WorkerCreated() {
+    AddRef();
+  }
+
+  void WorkerDestroyed() {
     Release();
   }
 
   WorkerId worker_id() const { return worker_id_; }
+  WorkerState state() const { return state_; }
+  void set_worker_id(WorkerId worker_id) { worker_id_ = worker_id; }
+  void set_state(WorkerState state) { state_ = state; };
+
+  bool Matches(const SharedWorkerInstance& other) {
+    return shared_worker_ && shared_worker_->Matches(other);
+  }
+
+  bool Matches(const ServiceWorkerIdentifier& other) {
+    return service_worker_ && service_worker_->Matches(other);
+  }
 
  private:
   virtual ~EmbeddedWorkerDevToolsAgentHost() {
@@ -165,20 +167,25 @@ class EmbeddedWorkerDevToolsManager::EmbeddedWorkerDevToolsAgentHost
                                                                     message);
   }
 
-  void OnSaveAgentRuntimeState(const std::string& state) { state_ = state; }
+  void OnSaveAgentRuntimeState(const std::string& state) {
+    saved_state_ = state;
+  }
 
   void AttachToWorker() {
     if (worker_attached_)
       return;
     worker_attached_ = true;
-    AddRef();
+    set_state(WORKER_INSPECTED);
     if (RenderProcessHost* host = RenderProcessHost::FromID(worker_id_.first))
       host->AddRoute(worker_id_.second, this);
   }
 
+  scoped_ptr<SharedWorkerInstance> shared_worker_;
+  scoped_ptr<ServiceWorkerIdentifier> service_worker_;
+  WorkerState state_;
   WorkerId worker_id_;
   bool worker_attached_;
-  std::string state_;
+  std::string saved_state_;

[dgozman, 2014/08/06 08:13:48]

saved_agent_state_

[vkuzkokov, 2014/08/06 09:07:39]

Done.

   DISALLOW_COPY_AND_ASSIGN(EmbeddedWorkerDevToolsAgentHost);
 };
 
@@ -191,23 +198,9 @@ EmbeddedWorkerDevToolsManager* EmbeddedWorkerDevToolsManager::GetInstance() {
 DevToolsAgentHost* EmbeddedWorkerDevToolsManager::GetDevToolsAgentHostForWorker(
     int worker_process_id,
     int worker_route_id) {
-  WorkerId id(worker_process_id, worker_route_id);
-
-  WorkerInfoMap::iterator it = workers_.find(id);
-  if (it == workers_.end())
-    return NULL;
-
-  WorkerInfo* info = it->second;
-  if (info->state() != WORKER_UNINSPECTED &&
-      info->state() != WORKER_PAUSED_FOR_DEBUG_ON_START) {
-    return info->agent_host();
-  }
-
-  EmbeddedWorkerDevToolsAgentHost* agent_host =
-      new EmbeddedWorkerDevToolsAgentHost(id);
-  info->set_agent_host(agent_host);
-  info->set_state(WORKER_INSPECTED);
-  return agent_host;
+  WorkerInfoMap::iterator it = workers_.find(
+      WorkerId(worker_process_id, worker_route_id));
+  return it == workers_.end() ? NULL : it->second;
 }
 
 DevToolsAgentHost*
@@ -234,11 +227,12 @@ bool EmbeddedWorkerDevToolsManager::SharedWorkerCreated(
   const WorkerId id(worker_process_id, worker_route_id);
   WorkerInfoMap::iterator it = FindExistingSharedWorkerInfo(instance);
   if (it == workers_.end()) {
-    scoped_ptr<WorkerInfo> info(new WorkerInfo(instance));
-    workers_.set(id, info.Pass());
+    EmbeddedWorkerDevToolsAgentHost* agent =
+        new EmbeddedWorkerDevToolsAgentHost(id, instance);
+    workers_[id] = agent;
     return false;
   }
-  MoveToPausedState(id, it);
+  WorkerRestarted(id, it);
   return true;
 }
 
@@ -250,13 +244,14 @@ bool EmbeddedWorkerDevToolsManager::ServiceWorkerCreated(
   const WorkerId id(worker_process_id, worker_route_id);
   WorkerInfoMap::iterator it = FindExistingServiceWorkerInfo(service_worker_id);
   if (it == workers_.end()) {
-    scoped_ptr<WorkerInfo> info(new WorkerInfo(service_worker_id));
+    EmbeddedWorkerDevToolsAgentHost* agent =
+        new EmbeddedWorkerDevToolsAgentHost(id, service_worker_id);
     if (debug_service_worker_on_start_)
-      info->set_state(WORKER_PAUSED_FOR_DEBUG_ON_START);
-    workers_.set(id, info.Pass());
+      agent->set_state(WORKER_PAUSED_FOR_DEBUG_ON_START);
+    workers_[id] = agent;
     return debug_service_worker_on_start_;
   }
-  MoveToPausedState(id, it);
+  WorkerRestarted(id, it);
   return true;
 }
 
@@ -266,40 +261,38 @@ void EmbeddedWorkerDevToolsManager::WorkerDestroyed(int worker_process_id,
   const WorkerId id(worker_process_id, worker_route_id);
   WorkerInfoMap::iterator it = workers_.find(id);
   DCHECK(it != workers_.end());
-  WorkerInfo* info = it->second;
-  switch (info->state()) {
+  EmbeddedWorkerDevToolsAgentHost* agent = it->second;

[dgozman, 2014/08/06 08:13:48]

I'd prefer agent_host.

[vkuzkokov, 2014/08/06 09:07:39]

Done.

+  switch (agent->state()) {
     case WORKER_UNINSPECTED:
     case WORKER_PAUSED_FOR_DEBUG_ON_START:
-      workers_.erase(it);
+      agent->set_state(WORKER_TERMINATED);

[dgozman, 2014/08/06 08:13:48]

You call set_state(WORKER_TERMINATED) in every case. Move it below the switch?

[vkuzkokov, 2014/08/06 09:07:39]

Done.

+      agent->DetachFromWorker();
       break;
     case WORKER_INSPECTED: {
-      EmbeddedWorkerDevToolsAgentHost* agent_host = info->agent_host();
-      info->set_state(WORKER_TERMINATED);
-      if (!agent_host->IsAttached()) {
-        agent_host->DetachFromWorker();
-        return;
+      agent->set_state(WORKER_TERMINATED);
+      if (agent->IsAttached()) {
+        // Client host is debugging this worker agent host.
+        std::string notification =
+            DevToolsProtocol::CreateNotification(
+                devtools::Worker::disconnectedFromWorker::kName, NULL)
+                ->Serialize();
+        DevToolsManagerImpl::GetInstance()->DispatchOnInspectorFrontend(
+            agent, notification);
       }
-      // Client host is debugging this worker agent host.
-      std::string notification =
-          DevToolsProtocol::CreateNotification(
-              devtools::Worker::disconnectedFromWorker::kName, NULL)
-              ->Serialize();
-      DevToolsManagerImpl::GetInstance()->DispatchOnInspectorFrontend(
-          agent_host, notification);
-      agent_host->DetachFromWorker();
+      agent->DetachFromWorker();
       break;
     }
     case WORKER_TERMINATED:
       NOTREACHED();
       break;
     case WORKER_PAUSED_FOR_REATTACH: {
-      scoped_ptr<WorkerInfo> worker_info = workers_.take_and_erase(it);
-      worker_info->set_state(WORKER_TERMINATED);
-      const WorkerId old_id = worker_info->agent_host()->worker_id();
-      workers_.set(old_id, worker_info.Pass());
+      workers_.erase(it);

[dgozman, 2014/08/06 08:13:48]

This could kill the agent host.

[vkuzkokov, 2014/08/06 09:07:38]

workers_ stores raw pointers. This shouldn't have any effect on agent hosts.

+      agent->set_state(WORKER_TERMINATED);
+      workers_[agent->worker_id()] = agent;
       break;
     }
   }
+  agent->WorkerDestroyed();
 }
 
 void EmbeddedWorkerDevToolsManager::WorkerContextStarted(int worker_process_id,
@@ -308,16 +301,13 @@ void EmbeddedWorkerDevToolsManager::WorkerContextStarted(int worker_process_id,
   const WorkerId id(worker_process_id, worker_route_id);
   WorkerInfoMap::iterator it = workers_.find(id);
   DCHECK(it != workers_.end());
-  WorkerInfo* info = it->second;
-  if (info->state() == WORKER_PAUSED_FOR_DEBUG_ON_START) {
+  EmbeddedWorkerDevToolsAgentHost* agent = it->second;
+  if (agent->state() == WORKER_PAUSED_FOR_DEBUG_ON_START) {
     RenderProcessHost* rph = RenderProcessHost::FromID(worker_process_id);
-    scoped_refptr<DevToolsAgentHost> agent_host(
-        GetDevToolsAgentHostForWorker(worker_process_id, worker_route_id));
-    DevToolsManagerImpl::GetInstance()->Inspect(rph->GetBrowserContext(),
-                                                agent_host.get());
-  } else if (info->state() == WORKER_PAUSED_FOR_REATTACH) {
-    info->agent_host()->ReattachToWorker(id);
-    info->set_state(WORKER_INSPECTED);
+    DevToolsManagerImpl::GetInstance()->Inspect(
+        rph->GetBrowserContext(), agent);
+  } else if (agent->state() == WORKER_PAUSED_FOR_REATTACH) {
+    agent->ReattachToWorker(id);
   }
 }
 
@@ -325,26 +315,20 @@ void EmbeddedWorkerDevToolsManager::RemoveInspectedWorkerData(
     EmbeddedWorkerDevToolsAgentHost* agent_host) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   const WorkerId id(agent_host->worker_id());
-  scoped_ptr<WorkerInfo> worker_info = workers_.take_and_erase(id);
-  if (worker_info) {
-    DCHECK_EQ(worker_info->agent_host(), agent_host);
-    if (worker_info->state() == WORKER_TERMINATED)
-      return;
-    DCHECK_EQ(worker_info->state(), WORKER_INSPECTED);
-    worker_info->set_agent_host(NULL);
-    worker_info->set_state(WORKER_UNINSPECTED);
-    workers_.set(id, worker_info.Pass());
+  WorkerInfoMap::iterator it = workers_.find(id);
+  if (it != workers_.end()) {

[dgozman, 2014/08/06 08:13:48]

How is it possible to not find agent host in the map?

[vkuzkokov, 2014/08/06 09:07:39]

It used to be possible (see case WORKER_PAUSED_FOR_REATTACH in WorkerDestroyed).
Now it's not the case.

+    DCHECK_EQ(it->second, agent_host);
+    workers_.erase(it);
     return;
   }
   for (WorkerInfoMap::iterator it = workers_.begin(); it != workers_.end();
        ++it) {
-    if (it->second->agent_host() == agent_host) {
+    if (it->second == agent_host) {
       DCHECK_EQ(WORKER_PAUSED_FOR_REATTACH, it->second->state());
       SendMessageToWorker(
           it->first,
           new DevToolsAgentMsg_ResumeWorkerContext(it->first.second));
-      it->second->set_agent_host(NULL);
-      it->second->set_state(WORKER_UNINSPECTED);
+      workers_.erase(it);
       return;
     }
   }
@@ -372,13 +356,16 @@ EmbeddedWorkerDevToolsManager::FindExistingServiceWorkerInfo(
   return it;
 }
 
-void EmbeddedWorkerDevToolsManager::MoveToPausedState(
+void EmbeddedWorkerDevToolsManager::WorkerRestarted(
     const WorkerId& id,
     const WorkerInfoMap::iterator& it) {
   DCHECK_EQ(WORKER_TERMINATED, it->second->state());
-  scoped_ptr<WorkerInfo> info = workers_.take_and_erase(it);
-  info->set_state(WORKER_PAUSED_FOR_REATTACH);
-  workers_.set(id, info.Pass());
+  EmbeddedWorkerDevToolsAgentHost* agent = it->second;
+  agent->set_state(WORKER_PAUSED_FOR_REATTACH);
+  agent->set_worker_id(id);
+  agent->WorkerCreated();
+  workers_.erase(it);
+  workers_[id] = agent;
 }
 
 void EmbeddedWorkerDevToolsManager::ResetForTesting() {