Bug 98500 – Web Inspector: NMI fix String instrumentation the way it was discussed in WK97964

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

[Ilya Tikhonovsky]

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.

[Ilya Tikhonovsky]

Created attachment 167288 [details]
Patch

[Benjamin Poulain]

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]

Why isn't memory instrumentation behind a feature flag? It looks like an experimental feature to me.

[WebKit Review Bot]

Comment on attachment 167288 [details]
Patch

Clearing flags on attachment: 167288

Committed r130581: <http://trac.webkit.org/changeset/130581>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ilya Tikhonovsky]

(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]

(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]

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.