[webkit-reviews] review denied: [Bug 92743] Web Inspector: test native memory instrumentation code with help of unittests : [Attachment 155510] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 92743: Web Inspector: test native memory instrumentation code with help of
unittests
https://bugs.webkit.org/show_bug.cgi?id=92743

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

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


> Source/WebCore/ChangeLog:11
> +	   was parked as private and addRootObject was introduced instead of
it.

typo: parked -> marked

> Source/WebCore/dom/MemoryInstrumentation.h:-57
> -    template <typename T> void addInstrumentedObject(const T& t)

I'd leave the old name as the object may well be referenced by several other
objects.

> Source/WebCore/dom/MemoryInstrumentation.h:219
> +    void MemoryInstrumentation::addObjectImpl(const OwnPtr<T>* const&
object, ObjectType objectType, OwningType owningType)

Wrong alignment.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:7
> + * 1.  Redistributions of source code must retain the above copyright

Please use license header like in
http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/ClipboardChro
miumTest.cpp, it is a bit different.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:26
> +

No empty line.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:44
> +    MemoryInstrumentationImpl(VisitedObjects& visitedObjects) :
m_visitedObjects(visitedObjects)

This way we have two implementations of MemoryInstrumentation which are almost
identical. Let's move the implementation into MemoryInstrumentation.cpp and add
a factory method to MemoryInstrumentation that would create its instance.