http://codereview.chromium.org/50007/diff/1/5 File src/debug-agent.cc (right):
http://codereview.chromium.org/50007/diff/1/5#newcode52 Line 52: // If an error occoured wait a bit before retrying. On 2009/03/19 13:17:13, Erik Corry wrote: > More comments? Done. http://codereview.chromium.org/50007/diff/1/5#newcode73 Line 73: terminate_now_->Wait(kOneSecondInMicros); On 2009/03/19 13:17:13, Erik Corry wrote: > I think unless we have a concrete situation where we know it's good to wait then > we should probably not have a sleep here. Agree, removed the sleep. http://codereview.chromium.org/50007/diff/1/5#newcode79 Line 79: void DebuggerAgent::Shutdown() { On 2009/03/19 13:17:13, Erik Corry wrote: > Is it possible to assert we have the lock here? We don't need the lock here as we are not looking as session_. CloseSession will take the lock when terminating the session. Added a Join() after server_->Shutdown() to make sure that the listener thread is fully terminated before moving on. Otherwise we could get a new session created just after CloseSession(). http://codereview.chromium.org/50007/diff/1/2 File test/cctest/test-debug.cc (right): http://codereview.chromium.org/50007/diff/1/2#newcode3829 Line 3829: const int port = 5858; On 2009/03/19 13:17:13, Erik Corry wrote: > You already used port 5858 in this file. In the interest of being able to > parallelize the tests reliably you should use a different number. Good point. Used different ports for different tests, and added a comment in the tests about this. http://codereview.chromium.org/50007 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
