Bug 139449

Summary: Web Inspector: Scope details sidebar should label objects with constructor names
Product: WebKit Reporter: Brian Burg <burg>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, jonowells, mark.lam, sbarati, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix none

Description Brian Burg 2014-12-09 10:51:32 PST
Everything is an [Object] right now, not too useful. The labels used by type profiling seem to be quite accurate in my experience, maybe we can pull constructor names from the same data.
Comment 1 Radar WebKit Bug Importer 2014-12-09 10:52:11 PST
<rdar://problem/19192204>
Comment 2 Joseph Pecoraro 2015-02-12 13:39:59 PST
It seems these objects fall into this second class:

js> function Foo() {}; new Foo
Foo

js> Bar = function() {}; new Bar
Object
Comment 3 Joseph Pecoraro 2015-02-14 00:33:08 PST
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!
Comment 4 Joseph Pecoraro 2015-02-14 00:49:14 PST
> I'll give it a shot!

It works! Now we should just share the code so it is all in one place.
Comment 5 Joseph Pecoraro 2015-02-14 00:49:53 PST
(In reply to comment #3)
> Sam

Auto-Uncorrected of course! Saam*
Comment 6 Joseph Pecoraro 2015-02-14 02:19:46 PST
Created attachment 246587 [details]
[PATCH] Proposed Fix
Comment 7 Joseph Pecoraro 2015-02-14 02:20:58 PST
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
Comment 8 Joseph Pecoraro 2015-02-14 09:51:19 PST
Created attachment 246594 [details]
[PATCH] Proposed Fix
Comment 9 Timothy Hatcher 2015-02-16 10:10:35 PST
Comment on attachment 246594 [details]
[PATCH] Proposed Fix

Looks good to me. Could probably use some JSC reviewer eyes.
Comment 10 Filip Pizlo 2015-02-16 11:01:57 PST
Comment on attachment 246594 [details]
[PATCH] Proposed Fix

LGTM.
Comment 11 Saam Barati 2015-02-16 11:13:12 PST
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 12 Joseph Pecoraro 2015-02-16 11:23:34 PST
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.
Comment 13 Saam Barati 2015-02-16 11:29:54 PST
(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 14 Joseph Pecoraro 2015-02-16 13:27:04 PST
Comment on attachment 246594 [details]
[PATCH] Proposed Fix

Tests passed.
Comment 15 WebKit Commit Bot 2015-02-16 14:10:24 PST
Comment on attachment 246594 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 246594

Committed r180173: <http://trac.webkit.org/changeset/180173>
Comment 16 WebKit Commit Bot 2015-02-16 14:10:29 PST
All reviewed patches have been landed.  Closing bug.