Bug 92743 – Web Inspector: test native memory instrumentation code with help of unittests

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

There is unittests framework in WebKit
It is a part of chromium port.
The instrumentation code is a bit tricky and I want to test it.

[Ilya Tikhonovsky]

Created attachment 155507 [details]
Patch

[Build Bot]

Comment on attachment 155507 [details]
Patch

Attachment 155507 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13390739

[Ilya Tikhonovsky]

Created attachment 155510 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 155510 [details]
Patch

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/ClipboardChromiumTest.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.

[Ilya Tikhonovsky]

Created attachment 155793 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #4)
> (From update of attachment 155510 [details])
> 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

done

> 
> > 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.

It is the same situationas in js snapshot. Many objects could have links to the window object but that does not prevent it to be the root object.

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

done

> 
> > 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/ClipboardChromiumTest.cpp, it is a bit different.

done

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

done

> 
> > 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.

landed as r124306

[Yury Semikhatsky]

Comment on attachment 155793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155793&action=review

> Source/WebCore/dom/MemoryInstrumentation.h:59
>          OwningTraits<T>::addInstrumentedObject(this, t);

addInstrumentedObject(this, t)

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:48
> +class MemoryInstrumentationImpl : public MemoryInstrumentation {

Please reuse existing MemoryInstrumentationImpl.

[Ilya Tikhonovsky]

Created attachment 155799 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 155799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155799&action=review

> Source/WebCore/inspector/MemoryInstrumentationImpl.h:54
> +    size_t reportedSize() const

reportedSizeForAllTypes?

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

Please remove 1 blank like

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:85
> +TEST(MemoryInstrumentationTest, ptrVsRef)

Split this into two tests.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:90
> +        Instrumented* instrumented = new Instrumented;

You may need adoptPtr(new Instrumented).leakPtr() to make some scripts happy.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:100
> +        impl.addRootObject(instrumented);

If it is a root object, its size shouldn't be reported yet.

[Ilya Tikhonovsky]

Committed r124334: <http://trac.webkit.org/changeset/124334>

[WebKit Review Bot]

Re-opened since this is blocked by 92879

[Ilya Tikhonovsky]

Committed r124412: <http://trac.webkit.org/changeset/124412>