Bug 96405

Summary: Web Inspector: NMI move String* instrumentation to wtf
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web Inspector (Deprecated)Assignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: alph, apavlov, benjamin, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 96487    
Bug Blocks: 96367    
Attachments:
Description Flags
Patch
none
Patch yurys: review+, webkit.review.bot: commit-queue-

Description Ilya Tikhonovsky 2012-09-11 10:19:57 PDT 
It is hard to upstream instrumentation traits for string classes.
It is much simple to instrument string classes directly.
Also this instrumentation is solving the problem with SubStrings.
Comment 1 Ilya Tikhonovsky 2012-09-11 10:25:18 PDT 
Created attachment 163398 [details]
Patch
Comment 2 Yury Semikhatsky 2012-09-12 01:34:40 PDT 
Comment on attachment 163398 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.h:723
> +        // count size used by internal buffer but skip strings that were constructed from literals.

count -> Count

> Source/WTF/wtf/text/StringImpl.h:731
> +        typename MemoryObjectInfo::ClassInfo info(memoryObjectInfo, this, 0, selfSize);

You should be consistent in using either constant MemoryObjectType::Unknown or 0 everywhere.

> Source/WTF/wtf/text/StringImpl.h:735
> +        else if (m_hashAndFlags & s_hashFlagHas16BitShadow) // Substrings never have it's own shadow.

typo: Substring never has its own shadow.
Comment 3 Ilya Tikhonovsky 2012-09-12 01:46:50 PDT 
Created attachment 163546 [details]
Patch
Comment 4 Yury Semikhatsky 2012-09-12 01:52:57 PDT 
Comment on attachment 163546 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.h:735
> +        else if (m_hashAndFlags & s_hashFlagHas16BitShadow) // Substring never has it's own shadow.

it's -> its
Comment 5 Ilya Tikhonovsky 2012-09-12 01:57:46 PDT 
(In reply to comment #2)
> (From update of attachment 163398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163398&action=review
> 
> > Source/WTF/wtf/text/StringImpl.h:723
> > +        // count size used by internal buffer but skip strings that were constructed from literals.
> 
> count -> Count

Done

> 
> > Source/WTF/wtf/text/StringImpl.h:731
> > +        typename MemoryObjectInfo::ClassInfo info(memoryObjectInfo, this, 0, selfSize);
> 
> You should be consistent in using either constant MemoryObjectType::Unknown or 0 everywhere.

Ok I think we can use 0 everywhere otherwise I need to have a separate header file for generic memory types and cpp file for the value.

> 
> > Source/WTF/wtf/text/StringImpl.h:735
> > +        else if (m_hashAndFlags & s_hashFlagHas16BitShadow) // Substrings never have it's own shadow.
> 
> typo: Substring never has its own shadow.

Done.
Comment 6 Ilya Tikhonovsky 2012-09-12 02:13:06 PDT 
Committed r128279: <http://trac.webkit.org/changeset/128279>
Comment 7 WebKit Review Bot 2012-09-12 02:39:55 PDT 
Re-opened since this is blocked by 96487
Comment 8 WebKit Review Bot 2012-09-12 05:26:38 PDT 
Comment on attachment 163546 [details]
Patch

Attachment 163546 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13811919

New failing tests:
MemoryInstrumentationTest.visitFirstMemberInNonVirtualClass
MemoryInstrumentationTest.dataRef
MemoryInstrumentationTest.sizeOf
inspector/profiler/memory-instrumentation-cached-images.html
MemoryInstrumentationTest.ptrVsRef
MemoryInstrumentationTest.refPtr
MemoryInstrumentationTest.ownerTypePropagation
MemoryInstrumentationTest.ownPtr
MemoryInstrumentationTest.ownPtrNotInstrumented
MemoryInstrumentationTest.visitStrings
Comment 9 Ilya Tikhonovsky 2012-09-12 05:49:52 PDT 
Committed r128300: <http://trac.webkit.org/changeset/128300>