Bug 139449 – Web Inspector: Scope details sidebar should label objects with constructor names

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

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.

[Radar WebKit Bug Importer]

<rdar://problem/19192204>

[Joseph Pecoraro]

It seems these objects fall into this second class:

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

js> Bar = function() {}; new Bar
Object

[Joseph Pecoraro]

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!

[Joseph Pecoraro]

> I'll give it a shot!

It works! Now we should just share the code so it is all in one place.

[Joseph Pecoraro]

(In reply to comment #3)
> Sam

Auto-Uncorrected of course! Saam*

[Joseph Pecoraro]

Created attachment 246587 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 246594 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

Comment on attachment 246594 [details]
[PATCH] Proposed Fix

Looks good to me. Could probably use some JSC reviewer eyes.

[Filip Pizlo]

Comment on attachment 246594 [details]
[PATCH] Proposed Fix

LGTM.

[Saam Barati]

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?

[Joseph Pecoraro]

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.

[Saam Barati]

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

[Joseph Pecoraro]

Comment on attachment 246594 [details]
[PATCH] Proposed Fix

Tests passed.

[WebKit Commit Bot]

Comment on attachment 246594 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 246594

Committed r180173: <http://trac.webkit.org/changeset/180173>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.