Summary: | Web Inspector: test native memory instrumentation code with help of unittests | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ilya Tikhonovsky <loislo> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Ilya Tikhonovsky <loislo> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, alph, apavlov, bweinstein, haraken, japhet, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 92879 | ||||||||||||
Bug Blocks: | 87262 | ||||||||||||
Attachments: |
|
Description
Ilya Tikhonovsky
2012-07-31 05:25:14 PDT
Created attachment 155507 [details]
Patch
Comment on attachment 155507 [details] Patch Attachment 155507 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13390739 Created attachment 155510 [details]
Patch
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. Created attachment 155793 [details]
Patch
(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 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. Created attachment 155799 [details]
Patch
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. Committed r124334: <http://trac.webkit.org/changeset/124334> Re-opened since this is blocked by 92879 Committed r124412: <http://trac.webkit.org/changeset/124412> |