DevTools: prepare workers for DevToolsManager::Attach(Detach)ClientHost removal.

DevTools: prepare workers for DevToolsManager::Attach(Detach)ClientHost removal.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174392

[yurys, 2012-12-20 11:45:40]

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

https://codereview.chromium.org/11633035/diff/1/content/browser/devtools/work...
content/browser/devtools/worker_devtools_manager.cc:370:
NotifyWorkerDestroyedOnIOThread(worker_process_id, worker_route_id);
Now that the notification is removed who will take care of notifying
WorkerDevToolsAgentHost that the worker has gone?

https://codereview.chromium.org/11633035/diff/1/content/browser/devtools/work...
File content/browser/devtools/worker_devtools_manager.cc (right):

https://codereview.chromium.org/11633035/diff/1/content/browser/devtools/work...
content/browser/devtools/worker_devtools_manager.cc:99: bool attached() { return
attached_; }
Could you pick another name to avoid confusion with attachment to client?

https://codereview.chromium.org/11633035/diff/1/content/browser/devtools/work...
content/browser/devtools/worker_devtools_manager.cc:156: return;
The message object will leak here.

[pfeldman, 2012-12-20 16:53:53]

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

https://codereview.chromium.org/11633035/diff/1/content/browser/devtools/work...
content/browser/devtools/worker_devtools_manager.cc:370:
NotifyWorkerDestroyedOnIOThread(worker_process_id, worker_route_id);
On 2012/12/20 11:45:40, Yury Semikhatsky wrote:
> Now that the notification is removed who will take care of notifying
> WorkerDevToolsAgentHost that the worker has gone?

Done.

https://codereview.chromium.org/11633035/diff/1/content/browser/devtools/work...
File content/browser/devtools/worker_devtools_manager.cc (right):

https://codereview.chromium.org/11633035/diff/1/content/browser/devtools/work...
content/browser/devtools/worker_devtools_manager.cc:99: bool attached() { return
attached_; }
On 2012/12/20 11:45:40, Yury Semikhatsky wrote:
> Could you pick another name to avoid confusion with attachment to client?

Done.

https://codereview.chromium.org/11633035/diff/1/content/browser/devtools/work...
content/browser/devtools/worker_devtools_manager.cc:156: return;
On 2012/12/20 11:45:40, Yury Semikhatsky wrote:
> The message object will leak here.

Done.

[yurys, 2012-12-21 10:13:15]

https://codereview.chromium.org/11633035/diff/1006/content/browser/devtools/w...
File content/browser/devtools/worker_devtools_manager.cc (left):

https://codereview.chromium.org/11633035/diff/1006/content/browser/devtools/w...
content/browser/devtools/worker_devtools_manager.cc:57: if (!instance_)
This refactoring seems unrelated to the rest of the change, would be nice to
extract it into a separate patch.

https://codereview.chromium.org/11633035/diff/1006/content/browser/devtools/w...
content/browser/devtools/worker_devtools_manager.cc:108: delete this;
You need to keep this delete for the case when the agent is not attached to a
client and DevToolsManager doesn't know about it and hence won't call Detach.

https://codereview.chromium.org/11633035/diff/1006/content/browser/devtools/w...
File content/browser/devtools/worker_devtools_manager.cc (right):

https://codereview.chromium.org/11633035/diff/1006/content/browser/devtools/w...
content/browser/devtools/worker_devtools_manager.cc:42:
base::LazyInstance<AgentHosts>::Leaky g_orphan_map = LAZY_INSTANCE_INITIALIZER;
Should be a member of  WorkerDevToolsManager::DetachedClientHosts

https://codereview.chromium.org/11633035/diff/1006/content/browser/devtools/w...
content/browser/devtools/worker_devtools_manager.cc:100:
g_agent_map.Get().erase(worker_id_);
Should it be deleted from g_orphan_map as well?

https://codereview.chromium.org/11633035/diff/1006/content/browser/devtools/w...
content/browser/devtools/worker_devtools_manager.cc:181: DetachedClientHosts() {
}
I think you can omit these two since now all members are static.

[yurys, 2012-12-21 13:48:02]

lgtm

https://chromiumcodereview.appspot.com/11633035/diff/15001/content/browser/de...
File content/browser/devtools/worker_devtools_manager.cc (right):

https://chromiumcodereview.appspot.com/11633035/diff/15001/content/browser/de...
content/browser/devtools/worker_devtools_manager.cc:184: agent->SelfDestruct();
RemovePendingWorkerData is missing

[commit-bot: I haz the power, 2012-12-21 13:51:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pfeldman@chromium.org/11633035/4004

[commit-bot: I haz the power, 2012-12-21 13:58:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.