http://codereview.chromium.org/8536042/diff/1/src/api.cc File src/api.cc (right):
http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode505 src/api.cc:505: nit: Remove one blank line. http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode509 src/api.cc:509: reinterpret_cast<uint32_t>(extension)); I'm not sure it'll work with 64 bit pointers. I think you need something like static_cast<uint32_t>(reinterpret_cast<uintptr_t>(extension)). http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode517 src/api.cc:517: static Allocator default_allocator; Allocate the allocator with "new" to avoid issues with static destructors. http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode529 src/api.cc:529: entry = Lookup(extension, Hash(extension), true); Can't you simply return UNVISITED? http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode534 src/api.cc:534: reinterpret_cast<int>(entry->value)); Again, I think it has to be something like static_cast<ExtensionTraversalState>(reintepret_cast<intptr_t>(entry->value)). http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode542 src/api.cc:542: } nit: // namespace internal http://codereview.chromium.org/8536042/diff/1/src/api.h File src/api.h (right): http://codereview.chromium.org/8536042/diff/1/src/api.h#newcode164 src/api.h:164: class ExtensionStates : private HashMap { Use delegation instead of implementation inheritance. http://codereview.chromium.org/8536042/diff/1/src/api.h#newcode170 src/api.h:170: } nit: // namespace internal http://codereview.chromium.org/8536042/diff/1/src/isolate.h File src/isolate.h (right): http://codereview.chromium.org/8536042/diff/1/src/isolate.h#newcode814 src/isolate.h:814: ExtensionStates* extension_states() { return extension_states_; } It's really unfortunate that this is a part of the isolate state. Please explore a slightly different approach. Make the InstallExtension family of functions instance (instead of static) methods of Genesis and move all the extension state tracking code there. To support the assert in serialize.cc a simple bool flag in Bootstrapper (e.g. "have_installed_extensions") should be enough. http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc File test/cctest/test-lockers.cc (right): http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc#newcode662 test/cctest/test-lockers.cc:662: context.Dispose(); Dispose the isolate too? http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc#newcode669 test/cctest/test-lockers.cc:669: TEST(ExtensionsRegistration) { Please describe what this tests and add a link to the bug. http://codereview.chromium.org/8536042/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
