Make DevTools support for the embedded SharedWorker.

content/browser/devtools/shared_worker_devtools_manager.h

Index: content/browser/devtools/shared_worker_devtools_manager.h
diff --git a/content/browser/devtools/worker_devtools_manager.h b/content/browser/devtools/shared_worker_devtools_manager.h
similarity index 14%
copy from content/browser/devtools/worker_devtools_manager.h
copy to content/browser/devtools/shared_worker_devtools_manager.h
index a70f1784b5b5e4b7842758d99eaa7e45f4f77be5..049feaff76f4228be2f8b58d3e789cd2293b21ee 100644
--- a/content/browser/devtools/worker_devtools_manager.h
+++ b/content/browser/devtools/shared_worker_devtools_manager.h
@@ -2,109 +2,79 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CONTENT_BROWSER_DEVTOOLS_WORKER_DEVTOOLS_MANAGER_H_
-#define CONTENT_BROWSER_DEVTOOLS_WORKER_DEVTOOLS_MANAGER_H_
-
-#include <list>
-#include <map>
-#include <string>
+#ifndef CONTENT_BROWSER_DEVTOOLS_SHARED_WORKER_DEVTOOLS_MANAGER_H_
+#define CONTENT_BROWSER_DEVTOOLS_SHARED_WORKER_DEVTOOLS_MANAGER_H_
 
 #include "base/basictypes.h"
+#include "base/containers/scoped_ptr_hash_map.h"
+#include "base/memory/scoped_vector.h"
 #include "base/memory/singleton.h"
-#include "content/browser/worker_host/worker_process_host.h"
-#include "content/common/content_export.h"
+#include "base/strings/string16.h"
 
-namespace content {
+class GURL;
 
+namespace content {
 class DevToolsAgentHost;
+class SharedWorkerInstance;
 
-// All methods are supposed to be called on the IO thread.
-class WorkerDevToolsManager {
+// SharedWorkerDevToolsManager is used instead of WorkerDevToolsManager when
+// "enable-embedded-shared-worker" flag is set.
+// This class lives on UI thread.
+class SharedWorkerDevToolsManager {
  public:
   typedef std::pair<int, int> WorkerId;
-  class WorkerDevToolsAgentHost;
+  class SharedWorkerDevToolsAgentHost;
 
-  // Returns the WorkerDevToolsManager singleton.
-  static WorkerDevToolsManager* GetInstance();
+  // Returns the SharedWorkerDevToolsManager singleton.
+  static SharedWorkerDevToolsManager* GetInstance();
 
-  // Called on the UI thread.
-  static DevToolsAgentHost* GetDevToolsAgentHostForWorker(
-      int worker_process_id,
-      int worker_route_id);
+  DevToolsAgentHost* GetDevToolsAgentHostForWorker(int worker_process_id,
+                                                   int worker_route_id);
 
-  void ForwardToDevToolsClient(int worker_process_id,
-                               int worker_route_id,
-                               const std::string& message);
-  void SaveAgentRuntimeState(int worker_process_id,
-                             int worker_route_id,
-                             const std::string& state);
-
-  // Called on the IO thread.
   // Returns true when the worker must be paused on start.
-  bool WorkerCreated(
-      WorkerProcessHost* process,
-      const WorkerProcessHost::WorkerInstance& instance);
-  void WorkerDestroyed(WorkerProcessHost* process, int worker_route_id);
-  void WorkerContextStarted(WorkerProcessHost* process, int worker_route_id);
+  bool WorkerCreated(int worker_process_id,
+                     int worker_route_id,
+                     const SharedWorkerInstance& instance);
+  void WorkerDestroyed(int worker_process_id, int worker_route_id);
+  void WorkerContextStarted(int worker_process_id, int worker_route_id);
 
  private:
-  friend struct DefaultSingletonTraits<WorkerDevToolsManager>;
-  class DetachedClientHosts;
-  struct InspectedWorker;
-  typedef std::list<InspectedWorker> InspectedWorkersList;
-
-  WorkerDevToolsManager();
-  virtual ~WorkerDevToolsManager();
-
-  void RemoveInspectedWorkerData(const WorkerId& id);
-  InspectedWorkersList::iterator FindInspectedWorker(int host_id, int route_id);
-
-  void ConnectDevToolsAgentHostToWorker(int worker_process_id,
-                                        int worker_route_id);
-  void ForwardToWorkerDevToolsAgent(int worker_process_host_id,
-                                    int worker_route_id,
-                                    const IPC::Message& message);
-  static void ForwardToDevToolsClientOnUIThread(
-      int worker_process_id,
-      int worker_route_id,
-      const std::string& message);
-  static void SaveAgentRuntimeStateOnUIThread(
-      int worker_process_id,
-      int worker_route_id,
-      const std::string& state);
-  static void NotifyConnectionFailedOnIOThread(int worker_process_id,
-                                               int worker_route_id);
-  static void NotifyConnectionFailedOnUIThread(int worker_process_id,
-                                               int worker_route_id);
-  static void SendResumeToWorker(const WorkerId& id);
-
-  InspectedWorkersList inspected_workers_;
-
-  struct TerminatedInspectedWorker;
-  typedef std::list<TerminatedInspectedWorker> TerminatedInspectedWorkers;
-  // List of terminated workers for which there may be a devtools client on
-  // the UI thread. Worker entry is added into this list when inspected worker
-  // is terminated and will be removed in one of two cases:
-  // - shared worker with the same URL and name is started(in wich case we will
+  friend struct DefaultSingletonTraits<SharedWorkerDevToolsManager>;
+
+  class WorkerInfo;
+  typedef base::ScopedPtrHashMap<WorkerId, WorkerInfo> WorkerInfoMap;
+
+  SharedWorkerDevToolsManager();
+  virtual ~SharedWorkerDevToolsManager();
+
+  void RemoveInspectedWorkerData(SharedWorkerDevToolsAgentHost* agent_host);
+
+  WorkerInfoMap::iterator FindTerminatedWorker(
+      const SharedWorkerInstance& instance);
+
+  WorkerInfoMap uninspected_workers_;
+  WorkerInfoMap inspected_workers_;
+
+  // Map of terminated workers for which there may be a devtools client. Worker
+  // entry is added into this map when inspected worker is terminated and will
+  // be removed in one of two cases:
+  // - shared worker with the same URL and name is started (in wich case we will
   // try to reattach existing DevTools client to the new worker).
-  // - DevTools client which was inspecting terminated worker is closed on the
-  // UI thread and and WorkerDevToolsManager is notified about that on the IO
-  // thread.
-  TerminatedInspectedWorkers terminated_workers_;
-
-  typedef std::map<WorkerId, WorkerId> PausedWorkers;
-  // Map from old to new worker id for the inspected workers that have been
-  // terminated and started again in paused state. Worker data will be removed
-  // from this list in one of two cases:
-  // - DevTools client is closed on the UI thread, WorkerDevToolsManager was
-  // notified about that on the IO thread and sent "resume" message to the
-  // worker.
+  // - DevTools client which was inspecting terminated worker is closed and
+  // WorkerDevToolsManager is notified about that.
+  WorkerInfoMap terminated_workers_;
+
+  // Map of the inspected workers that have been terminated and started again in
+  // paused state. WorkerId of WorkerInfo in this map is stil the old worker id.
+  // Worker data will be removed from this list in one of two cases:
+  // - DevTools client is closed and WorkerDevToolsManager was notified about
+  // and sent "resume" message to the worker.
   // - Existing DevTools client was reattached to the new worker.
-  PausedWorkers paused_workers_;
+  WorkerInfoMap paused_workers_;

[yurys, 2014/03/31 13:48:23]

All these worker WorkerInfo maps blow up the code and obscure the code making it
hard to track whether we always correctly dispose of WorkerInfo. My
understanding is that WorkerInfo should be created when a new worker is created
and destroyed when both following conditions are true:
1) The worker has been destroyed;
2) Corresponding ShareWorkerDevToolsAgentHost has been detroyed or never
existed.

The worker can be in one of the 4 states from the DevTools point of view:
uninspected (no SharedWorkerDevToolsAgentHost) -> inspected (there is
SharedWorkerDevToolsAgentHost for the worker) -> terminated (the worker was
destroyed while being inspected by a DevTools client) -> paused (terminated
worker was restarted and it should be reported to the DevTools client) ->
inspected.

Based on these observations it might be easier to have a single map worker id ->
WorkerInfo and have a flag stored in field on WorkerInfo which would reflect one
of the 4 DevTools states. Along with a pointer to SharedWorkerDevToolsAgentHost
that you have added this should cover all use cases be more readable. Is it
worth doing?

Sorry for such a long review, I understand that this code was already very
involved but as far as I remember that was mostly caused by jumping between the
UI and IO threads. Now that it works on a single thread I'd love to have it less
complicated.

[horo, 2014/04/01 08:34:09]

Done.
Yes that sounds good to me.
I introduced WorkerState enum.

 
-  DISALLOW_COPY_AND_ASSIGN(WorkerDevToolsManager);
+  DISALLOW_COPY_AND_ASSIGN(SharedWorkerDevToolsManager);
 };
 
 }  // namespace content
 
-#endif  // CONTENT_BROWSER_DEVTOOLS_WORKER_DEVTOOLS_MANAGER_H_
+#endif  // CONTENT_BROWSER_DEVTOOLS_SHARED_WORKER_DEVTOOLS_MANAGER_H_