Summary: | Web Inspector: Scope details sidebar should label objects with constructor names | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, graouts, joepeck, jonowells, mark.lam, saam, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Brian Burg
2014-12-09 10:51:32 PST
It seems these objects fall into this second class: js> function Foo() {}; new Foo Foo js> Bar = function() {}; new Bar Object Sam pointed me in the right direction: Structure::toStructureShape is doing the same constructor function look up we were doing in JSC, but better. PassRefPtr<StructureShape> Structure::toStructureShape(JSValue value) { ... bool foundCtorName = false; if (JSObject* profilingVal = curValue.getObject()) { ExecState* exec = profilingVal->globalObject()->globalExec(); PropertySlot slot(storedPrototype()); PropertyName constructor(exec->propertyNames().constructor); if (profilingVal->getPropertySlot(exec, constructor, slot)) { if (slot.isValue()) { JSValue constructorValue = slot.getValue(exec, constructor); if (constructorValue.isCell()) { if (JSCell* constructorCell = constructorValue.asCell()) { if (JSObject* ctorObject = constructorCell->getObject()) { if (JSFunction* constructorFunction = jsDynamicCast<JSFunction*>(ctorObject)) { curShape->setConstructorName(constructorFunction->calculatedDisplayName(exec)); foundCtorName = true; } else if (InternalFunction* constructorFunction = jsDynamicCast<InternalFunction*>(ctorObject)) { curShape->setConstructorName(constructorFunction->calculatedDisplayName(exec)); foundCtorName = true; } } } } } } } if (!foundCtorName) curShape->setConstructorName(curStructure->classInfo()->className); ... } This looks like something we can just plug into: JSValue JSInjectedScriptHost::internalConstructorName(ExecState* exec) I'll give it a shot! > I'll give it a shot!
It works! Now we should just share the code so it is all in one place.
(In reply to comment #3) > Sam Auto-Uncorrected of course! Saam* Created attachment 246587 [details]
[PATCH] Proposed Fix
Comment on attachment 246587 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=246587&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:274 > + if (InternalFunction* constructorFunction = jsDynamicCast<InternalFunction*>(ctorObject)) Nit: else if Created attachment 246594 [details]
[PATCH] Proposed Fix
Comment on attachment 246594 [details]
[PATCH] Proposed Fix
Looks good to me. Could probably use some JSC reviewer eyes.
Comment on attachment 246594 [details]
[PATCH] Proposed Fix
LGTM.
Comment on attachment 246594 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=246594&action=review > Source/JavaScriptCore/runtime/Structure.cpp:1065 > + else This patch LGTM, just wondering about this one specific code path: Joe, does this pass all the type profiler tests that run as part of run-javascriptcore-tests? I'm wondering because this 'else' path will not only run if the curValue is not an object, which should almost never happen (except for a few cases and the bug that exists now where Symbol's go down this code path), instead of before when foundCtorName is false. I remember that the below code path caught some specific cases where the constructor name couldn't be found, but don't exactly remember what that set is. I think it was there for some set of native objects, I think DOM objects. I think your new function will handle this case of displaying DOM objects' names properly, but have you verified this? Comment on attachment 246594 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=246594&action=review >> Source/JavaScriptCore/runtime/Structure.cpp:1065 >> + else > > This patch LGTM, just wondering about this one specific code path: > > Joe, does this pass all the type profiler tests that run as part of run-javascriptcore-tests? > I'm wondering because this 'else' path will not only run if the curValue is not an object, which should almost never > happen (except for a few cases and the bug that exists now where Symbol's go down this code path), instead of before > when foundCtorName is false. I remember that the below code path caught some specific cases where the constructor name > couldn't be found, but don't exactly remember what that set is. I think it was there for some set of native objects, I think DOM > objects. I think your new function will handle this case of displaying DOM objects' names properly, but have you verified this? I think the Inspector tests covered this pretty well. I'll hold cq and run-javascriptcore-tests to verify though. The idea here is that if the value is an object, JSObject::calculatedClassName should get you the best name no matter what. So it tries 3 approaches: obj.prototype.constructor name, method table, and class info. That is actually 1 more approach than TypeSet originally tried. If anything I would expect better names for things like "Arguments" objects. The else here is for any value that was not an object so it should match the original behavior. (In reply to comment #12) > Comment on attachment 246594 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246594&action=review > > >> Source/JavaScriptCore/runtime/Structure.cpp:1065 > >> + else > > > > This patch LGTM, just wondering about this one specific code path: > > > > Joe, does this pass all the type profiler tests that run as part of run-javascriptcore-tests? > > I'm wondering because this 'else' path will not only run if the curValue is not an object, which should almost never > > happen (except for a few cases and the bug that exists now where Symbol's go down this code path), instead of before > > when foundCtorName is false. I remember that the below code path caught some specific cases where the constructor name > > couldn't be found, but don't exactly remember what that set is. I think it was there for some set of native objects, I think DOM > > objects. I think your new function will handle this case of displaying DOM objects' names properly, but have you verified this? > > I think the Inspector tests covered this pretty well. I'll hold cq and > run-javascriptcore-tests to verify though. > > The idea here is that if the value is an object, > JSObject::calculatedClassName should get you the best name no matter what. > So it tries 3 approaches: obj.prototype.constructor name, method table, and > class info. That is actually 1 more approach than TypeSet originally tried. > If anything I would expect better names for things like "Arguments" objects. > The else here is for any value that was not an object so it should match the > original behavior. Okay cool, I agree that this should be better in general. You probably don't have to run all the tests, if you edit the run-javascriptcore-tests file to just comment out everything that isn't "typeProfiler.yaml" it will only run those tests. Comment on attachment 246594 [details]
[PATCH] Proposed Fix
Tests passed.
Comment on attachment 246594 [details] [PATCH] Proposed Fix Clearing flags on attachment: 246594 Committed r180173: <http://trac.webkit.org/changeset/180173> All reviewed patches have been landed. Closing bug. |