http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h File include/v8-profiler.h (right):
http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h#newcode403 include/v8-profiler.h:403: * the given JavaScript wrapper object. We should document that its prohibited to enter V8 while the callback is running: only getters on the handle and GetPointerFromInternalField on the objects are allowed. http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h#newcode426 include/v8-profiler.h:426: /** Binds a callback to embedder's class ID. */ Move the "no class" constant here and document that it must not be used to define a class and that it could be used to reset a class of a persistent handle. http://codereview.chromium.org/6626043/diff/1/include/v8.h File include/v8.h (right): http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode400 include/v8.h:400: * Assigns a class ID to the handle. See RetainedObjectInfo "a class" -> "a wrapper class" http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2549 include/v8.h:2549: * Interface for providing information about embedder's objects Is there a good reason to keep this interface here and not in v8-profile.h? Also please mention in the very first sentence here that the interface is used by the heap profiler. http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2568 include/v8.h:2568: * Returned RetainedObjectInfo instances are kept alive only during Let's change the first sentence to "V8 takes ownership of RetainedObjectInfo instances passed to it and keeps them alive only during snapshot collection". http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2586 include/v8.h:2586: /** Returns human-readable label. */ Please document the expected encoding (and whether it should be NUL-terminated) and the ownership of the returned pointer. http://codereview.chromium.org/6626043/diff/1/src/global-handles.h File src/global-handles.h (right): http://codereview.chromium.org/6626043/diff/1/src/global-handles.h#newcode57 src/global-handles.h:57: v8::RetainedObjectInfo* info_; Needs DISALLOW_COPY_AND_ASSIGN to prevent accidental double dispose of "info". http://codereview.chromium.org/6626043/diff/1/src/global-handles.h#newcode113 src/global-handles.h:113: // Iterates over all handles that has embedder-assigned class ID. "has" -> "have" http://codereview.chromium.org/6626043/diff/1/src/heap.h File src/heap.h (right): http://codereview.chromium.org/6626043/diff/1/src/heap.h#newcode1144 src/heap.h:1144: static void call_global_gc_prologue_callback() { CallGlobalGCPrologueCallback() http://codereview.chromium.org/6626043/diff/1/src/heap.h#newcode1148 src/heap.h:1148: static void call_global_gc_epilogue_callback() { CallGlobalGCEpilogueCallback() http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcode1400 src/profile-generator.cc:1400: const uint64_t HeapObjectsMap::kInternalRootObjectId = 1; Document why these have to be odd. http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcode1684 src/profile-generator.cc:1684: reinterpret_cast<HeapObject*>(3); Shouldn't the constants defined above be reused here? http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcode2200 src/profile-generator.cc:2200: reinterpret_cast<HeapThing>(5); Same as above. http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcode2243 src/profile-generator.cc:2243: info->GetLabel(), What if the returned label's lifetime is tied to the retained object info's lifetime? http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h File src/profile-generator.h (right): http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode69 src/profile-generator.h:69: const char* PrintF(const char* format, ...); "PrintF" is a bad name. We don't actually print anything here. "StringPrintF" or "AddFormatted"? http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode843 src/profile-generator.h:843: const char* GetName(String* name) { return names_.GetName(name); } I think it'd be much better to expose the string storage object directly instead of adding its functions here. http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode1083 src/profile-generator.h:1083: ~NativeObjectsExplorer(); Please make the implicit "virtual" explicit. http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler.cc File test/cctest/test-heap-profiler.cc (right): http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler.cc#newcode1299 test/cctest/test-heap-profiler.cc:1299: return new TestRetainedObjectInfo(1, "aaa", 100); "bbb"? http://codereview.chromium.org/6626043/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
