Detailed heap snapshot usability improvement.

Detailed heap snapshot usability improvement.

The detailed heap snapshot has two groups of nodes. The first one
is the group for v8 heap nodes and the second one is the group for
native objects. At the moment we have two different sets of native
objects. There are 'Detached DOM trees' and 'Document DOM trees' type of objects.

I think it'd be nice to replace one group containing all native objects with
separate groups for different types of native objects.

BUG=none
TEST=HeapSnapshotRetainedObjectInfo

[mnaganov (inactive), 2012-01-27 09:53:12]

On 2012/01/27 07:01:51, loislo wrote:

Please fix typos in the description:

"group for a native objects" -> "group for native objects"
"It are Detached DOM Trees" -> "They are..."
also why detacheds' name is spaced, while documents' not?
"one group for all native objects" -> "one group containing all native objects"

[mnaganov (inactive), 2012-01-27 09:53:17]

https://chromiumcodereview.appspot.com/9223009/diff/1005/include/v8-profiler.h
File include/v8-profiler.h (right):

https://chromiumcodereview.appspot.com/9223009/diff/1005/include/v8-profiler....
include/v8-profiler.h:479: * Returns human-readable label. It must be a
NULL-terminated UTF-8
Let's change it to "null-terminated" then:
http://en.wikipedia.org/wiki/Null-terminated_string

https://chromiumcodereview.appspot.com/9223009/diff/1005/include/v8-profiler....
include/v8-profiler.h:486: * encoded string. V8 copies its contents during a
call to GetGroupLabel.
Please elaborate more on this -- what's the difference between a regular label
and a group label. Can either of them be NULL?

https://chromiumcodereview.appspot.com/9223009/diff/1005/src/profile-generato...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/9223009/diff/1005/src/profile-generato...
src/profile-generator.cc:2884: void
NativeObjectsExplorer::SetRootNativesRootReference() {
Perhaps, you need to rename this to SetRootNativeRootsReference, because now you
are adding multiple entries.

https://chromiumcodereview.appspot.com/9223009/diff/1005/src/profile-generato...
src/profile-generator.cc:2893: HeapGraphEdge::kElement,
I think you should use kInternal and SetNamedAutoIndexReference

https://chromiumcodereview.appspot.com/9223009/diff/1005/test/cctest/test-hea...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/9223009/diff/1005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:688: virtual const char* GetLabel() { return
label_; }
Perhaps you need to test usage of GetGroupLabel as well.

[loislo, 2012-01-27 10:19:35]

comments addressed

[mnaganov (inactive), 2012-01-27 10:26:38]

Please brush up the comment, otherwise LGTM.

https://chromiumcodereview.appspot.com/9223009/diff/5002/include/v8-profiler.h
File include/v8-profiler.h (right):

https://chromiumcodereview.appspot.com/9223009/diff/5002/include/v8-profiler....
include/v8-profiler.h:488: * top level entries with these names and attach the
objects to the
nit: remove "the" before plurals

https://chromiumcodereview.appspot.com/9223009/diff/5002/include/v8-profiler....
include/v8-profiler.h:489: * corresponding top level group objects. There is a
default implementation
Perhaps, rephrase as "the default implementation assumes each object comprises a
group on its own".

https://chromiumcodereview.appspot.com/9223009/diff/5002/include/v8-profiler....
include/v8-profiler.h:490: * which is required because embedders don't have
their own implementation yet.
nit: 80 chars