http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h File include/v8-profiler.h (right):
http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode442 include/v8-profiler.h:442: class ExternalResourceVisitor { You need to put V8EXPORT before the class name. http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode445 include/v8-profiler.h:445: virtual ~ExternalResourceVisitor() {} Destructor must be the first. http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode452 include/v8-profiler.h:452: static void VisitExternalResources(ExternalResourceVisitor* visitor); I'm not sure HeapProfiler class is a good place for this method -- it doesn't relate to HeapSnapshots at all, as it iterates live heap. Why not to add it into the V8 interface class? http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.cc File src/heap-profiler.cc (right): http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.cc#newcode112 src/heap-profiler.cc:112: if (!obj->IsString()) Why not obj->IsExternalString()? http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h File src/heap-profiler.h (right): http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h#newcode76 src/heap-profiler.h:76: class ExternalResourceVisitor { Do you really need this class? What's wrong with using the one you have defined in the API? http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h#newcode81 src/heap-profiler.h:81: static void VisitExternalResources(ExternalResourceVisitor* visitor); The same is for the implementation. Perhaps, put this into heap.cc? http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler.cc File test/cctest/test-heap-profiler.cc (right): http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler.cc#newcode860 test/cctest/test-heap-profiler.cc:860: static const size_t max_length = 100; kMaxLength http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler.cc#newcode890 test/cctest/test-heap-profiler.cc:890: if (resource1_ == resource) Perhaps, just use boolean flags? Modifying references looks too hackish. http://codereview.chromium.org/9139018/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
