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 { On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
You need to put V8EXPORT before the class name.
Done. http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode445 include/v8-profiler.h:445: virtual ~ExternalResourceVisitor() {} On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
Destructor must be the first.
Done. http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode452 include/v8-profiler.h:452: static void VisitExternalResources(ExternalResourceVisitor* visitor); On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
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?
Since it is used to analyze heap I decided to put it into the heap profiler. I don't mind moving it into V8 class if you feel it should rather go there. 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()) On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
Why not obj->IsExternalString()?
Done. 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 { On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
Do you really need this class? What's wrong with using the one you
have defined
in the API?
Done. I thought we try to work only with internal classes in the code under src/ http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h#newcode81 src/heap-profiler.h:81: static void VisitExternalResources(ExternalResourceVisitor* visitor); On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
The same is for the implementation. Perhaps, put this into heap.cc?
Done. 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; On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
kMaxLength
Done. http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler.cc#newcode890 test/cctest/test-heap-profiler.cc:890: if (resource1_ == resource) On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
Perhaps, just use boolean flags? Modifying references looks too
hackish. Done. http://codereview.chromium.org/9139018/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
