[webkit-reviews] review granted: [Bug 109554] Web Inspector: Native Memory Instrumentation: reportLeaf method doesn't report the leaf node properly. : [Attachment 187821] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has granted Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 109554: Web Inspector: Native Memory Instrumentation: reportLeaf method
doesn't report the leaf node properly.
https://bugs.webkit.org/show_bug.cgi?id=109554

Attachment 187821: Patch
https://bugs.webkit.org/attachment.cgi?id=187821&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187821&action=review


> Source/WebCore/ChangeLog:6
> +	   In some cases leafs have no pointer so with the old schema we can't
generate nodeId for them because we

leafs -> leaves

> Source/WebCore/inspector/HeapGraphSerializer.h:46
> +class HeapGraphSerializerClient {

Turn it into inner class?

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:346
> +    } frontendWrapper(m_frontend);

This way frontendWrapper will be destroyed before graphSerializer.

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:42
> +namespace {

This should be namespace TestWebKitAPI for consistency.

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:52
> +	   m_currentPointer = 0;

Move this into the initializers list.

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:57
> +	   m_heapSnapshotChunk = heapSnapshotChunk;

Do you expect more than one chunk? If not there should be an assert for this.

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:85
> +    void* addNode(const char* className, const char* name, bool isRoot)

Looks like this and next method should go into a separate class, can you
extract them?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:113
> +	   RefPtr<InspectorObject> chunk =
*reinterpret_cast<RefPtr<InspectorObject>*>(&m_heapSnapshotChunk);

Wouldn't RefPtr<InspectorObject> chunk = m_heapSnapshotChunk; work? Or probably
we can use m_heapSnapshotChunk directly?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:122
> +	   return chunkPart(partName)->toJSONString().replace("\"", "'");

Why do you need to change the " to ' ?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:141
> +    String node(unsigned nodeIndex)

nodeToString?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:154
> +    String edge(unsigned edgeOrdinal)

edgeToString?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:211
> +    EXPECT_EQ(String("['','weak','ownRef','countRef','unknown','Root']"),
receiver.dumpLastChunkStrings());

ownRef -> ownPtr, countRef -> refPtr?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:257
> +TEST(HeapGraphSerializerTest, hashSetWithTwoItems)

hashSetWithTwoItems -> hashSetWithTwoStrings