DevTools: remove the WorkerDevToolsManager base - implementations are diverging.

DevTools: remove the WorkerDevToolsManager base - implementations are diverging.

BUG=463892
Committed: https://crrev.com/2d516ac38398b2b95d2a33972d1ffc2c95e5dd7a
Cr-Commit-Position: refs/heads/master@{#319593}

[pfeldman, 2015-03-07 16:00:53]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[pfeldman, 2015-03-07 17:56:54]

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
File content/browser/devtools/worker_devtools_manager.cc (left):

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
content/browser/devtools/worker_devtools_manager.cc:17:
scoped_refptr<DevToolsAgentHost> DevToolsAgentHost::GetForWorker(
This method did not belong here, now it is where it was supposed to be.

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
content/browser/devtools/worker_devtools_manager.cc:30:
WorkerDevToolsManager::GetDevToolsAgentHostForWorker(int worker_process_id,
This one is two lines, but the clients need to cast to the derived.

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
content/browser/devtools/worker_devtools_manager.cc:37: void
WorkerDevToolsManager::AddAllAgentHosts(DevToolsAgentHost::List* result) {
ditto

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
content/browser/devtools/worker_devtools_manager.cc:51:
FOR_EACH_OBSERVER(Observer, observer_list_, WorkerDestroyed(it->second));
We only want observers in the service worker.

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
content/browser/devtools/worker_devtools_manager.cc:65: void
WorkerDevToolsManager::AddObserver(Observer* observer) {
ditto

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
content/browser/devtools/worker_devtools_manager.cc:69: void
WorkerDevToolsManager::RemoveObserver(Observer* observer) {
ditto

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
content/browser/devtools/worker_devtools_manager.cc:90:
FOR_EACH_OBSERVER(Observer, observer_list_, WorkerCreated(host));
ditto

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/wor...
content/browser/devtools/worker_devtools_manager.cc:95: WorkerDevToolsAgentHost*
agent_host = it->second;
Currently the logic is the same, but it will not be longer term. ServiceWorker
and SharedWorker have different workflows - different versioning, different
restart logic.

[dgozman, 2015-03-07 20:46:41]

I think it may be easier to merge ServiceWorkerDevToolsAgentHost and
ServiceWorkerDevToolsManager now instead of calling methods between them.
Same for shared workers. Maybe a follow-up?

lgtm

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
File content/browser/devtools/service_worker_devtools_manager.h (right):

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
content/browser/devtools/service_worker_devtools_manager.h:14: #include
"content/browser/shared_worker/shared_worker_instance.h"
nit: unnecessary include

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
content/browser/devtools/service_worker_devtools_manager.h:67:
DevToolsAgentHostImpl* GetDevToolsAgentHostForWorker(int worker_process_id,
return ServiceWorkerDevToolsAgentHost*?

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
content/browser/devtools/service_worker_devtools_manager.h:77: void
WorkerReadyForInspection(int worker_process_id, int worker_route_id);
Should all this methods be public? Perhaps, "friend class
ServiceWorkerDevToolsAgentHost" is enough to make some of them private?

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/sha...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/sha...
content/browser/devtools/shared_worker_devtools_manager.h:30:
DevToolsAgentHostImpl* GetDevToolsAgentHostForWorker(int worker_process_id,
SharedWorkerDevToolsAgentHost*

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/sha...
content/browser/devtools/shared_worker_devtools_manager.h:39: void
WorkerReadyForInspection(int worker_process_id, int worker_route_id);
Same comment regarding public vs. private.

[pfeldman, 2015-03-08 12:17:28]

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
File content/browser/devtools/service_worker_devtools_manager.h (right):

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
content/browser/devtools/service_worker_devtools_manager.h:14: #include
"content/browser/shared_worker/shared_worker_instance.h"
On 2015/03/07 20:46:40, dgozman wrote:
> nit: unnecessary include

Done.

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
content/browser/devtools/service_worker_devtools_manager.h:14: #include
"content/browser/shared_worker/shared_worker_instance.h"
On 2015/03/07 20:46:40, dgozman wrote:
> nit: unnecessary include

Done.

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
content/browser/devtools/service_worker_devtools_manager.h:67:
DevToolsAgentHostImpl* GetDevToolsAgentHostForWorker(int worker_process_id,
Clients don't need it to be that specific, you only force them to add more
includes.

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/ser...
content/browser/devtools/service_worker_devtools_manager.h:77: void
WorkerReadyForInspection(int worker_process_id, int worker_route_id);
On 2015/03/07 20:46:40, dgozman wrote:
> Should all this methods be public? Perhaps, "friend class
> ServiceWorkerDevToolsAgentHost" is enough to make some of them private?

It is the other way around, these are the instrumentation calls that get
delegated to the agent hosts.

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/sha...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/sha...
content/browser/devtools/shared_worker_devtools_manager.h:30:
DevToolsAgentHostImpl* GetDevToolsAgentHostForWorker(int worker_process_id,
On 2015/03/07 20:46:40, dgozman wrote:
> SharedWorkerDevToolsAgentHost*

Clients don't need it to be that specific, you only force them to add more
includes.

https://codereview.chromium.org/985343002/diff/1/content/browser/devtools/sha...
content/browser/devtools/shared_worker_devtools_manager.h:39: void
WorkerReadyForInspection(int worker_process_id, int worker_route_id);
On 2015/03/07 20:46:40, dgozman wrote:
> Same comment regarding public vs. private.

Ditto

[pfeldman, 2015-03-08 12:20:23]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2015-03-08 12:20:23]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/985343002/#ps20001 (title:
"comments addressed.")

[commit-bot: I haz the power, 2015-03-08 12:20:43]

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

[commit-bot: I haz the power, 2015-03-08 13:44:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-08 13:44:55]

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