RESOLVED INVALID 98500
Web Inspector: NMI fix String instrumentation the way it was discussed in WK97964 
https://bugs.webkit.org/show_bug.cgi?id=98500
Summary Web Inspector: NMI fix String instrumentation the way it was discussed in WK9...
Ilya Tikhonovsky
Reported 2012-10-05 01:58:57 PDT
Current instrumentation incorrectly covers the case when StringImpl is created via StringImpl::createWithTerminatingNullCharacter() Looks like the only way to detect the strings that were created from literals is to compare the addresses of buffer and stringImpl + 1.
Attachments
Patch (8.06 KB, patch)
2012-10-05 02:08 PDT, Ilya Tikhonovsky 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ilya Tikhonovsky
Comment 1 2012-10-05 02:08:01 PDT
Created attachment 167288 [details] Patch
Benjamin Poulain
Comment 2 2012-10-05 12:29:33 PDT
Comment on attachment 167288 [details] Patch I can't help but wonder why MemoryInstrumentationTest.cpp is in Chromium instead of being cross-port in WebKitTestAPI. Otherwise the change looks good.
Benjamin Poulain
Comment 3 2012-10-05 12:30:29 PDT
Why isn't memory instrumentation behind a feature flag? It looks like an experimental feature to me.
WebKit Review Bot
Comment 4 2012-10-06 02:26:19 PDT
Comment on attachment 167288 [details] Patch Clearing flags on attachment: 167288 Committed r130581: <http://trac.webkit.org/changeset/130581>
WebKit Review Bot
Comment 5 2012-10-06 02:26:23 PDT
All reviewed patches have been landed. Closing bug.
Ilya Tikhonovsky
Comment 6 2012-10-11 00:12:00 PDT
(In reply to comment #3) > Why isn't memory instrumentation behind a feature flag? It looks like an experimental feature to me. We decided to do the work without a flag because the feature doesn't affects UI, the rendering and the performance. It is quite challenging task by itself because we need to instrument a lot of classes for the better coverage and we would have additional work in case of being behind a flag because WebKit is constantly moving forward. Additional burden on the developers is small because the body of reportMemoryUsage member function for a typical class is very simple in almost all the cases and requires the attention only in the case of removing a member variable. We monitor the quality of the instrumentation with help of tcmalloc profiler. Also we will use clang plugin for the coverage check on the later stage.
Ilya Tikhonovsky
Comment 7 2012-10-11 00:55:34 PDT
(In reply to comment #2) > (From update of attachment 167288 [details]) > I can't help but wonder why MemoryInstrumentationTest.cpp is in Chromium instead of being cross-port in WebKitTestAPI. > > Otherwise the change looks good. Good point. I'll move it to to TestWebKitAPI.
Brian Burg
Comment 8 2014-12-12 13:40:51 PST
Closing as invalid, as this bug pertains to the old inspector UI and/or its tests. Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.
Note You need to log in before you can comment on or make changes to this bug.