Bug 136500 – Web Inspector: Modify the type profiler runtime protocol to transfer some computation into the WebInspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[Saam Barati]

Currently, the displayTypeName property is computed inside Source/JavaScriptCore/runtime/TypeSet.cpp,
but, because this is a String that is displayed in the UI, it should be computed inside the WebInspector.
There is also more flexibility in changing the UI if it is computed in the Web Inspector instead of in JSC.

[Radar WebKit Bug Importer]

<rdar://problem/18220160>

[Saam Barati]

Created attachment 237900 [details]
patch

[Saam Barati]

Created attachment 237901 [details]
patch

(remove whitespace change from above patch).

[Joseph Pecoraro]

Comment on attachment 237901 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237901&action=review

I'd like to see some renames, but the patch itself looks good. r=me. Feel free to put up another if you want changes reviewed.

> Source/JavaScriptCore/inspector/protocol/Runtime.json:123
> +                { "name": "isInDictionaryMode", "type": "boolean", "optional": true, "description": "If true, it indicates that the fields in this StructureDescription may be inaccurate. I.e, there might have been fields that have been deleted before it was profiled or it has fields we haven't profiled." }

This field name is unclear to me. I wonder if there is an adjective that might better describe this state. "isInaccurate"? "isLossy"?  "isImprecise"? "isInexact"? "isUnstable"? "

> Source/JavaScriptCore/inspector/protocol/Runtime.json:127
> +            "id": "TypeSetBooleans",

How about just "TypeSet"? It is name spaced to not conflict with JSC::TypeSet. Also, labelling it "Booleans" might hamper future extensions.

> Source/JavaScriptCore/inspector/protocol/Runtime.json:137
> +                { "name": "isFunction", "type": "boolean", "optional": false, "description": "Indicates if this type description has been type Function." },
> +                { "name": "isUndefined", "type": "boolean", "optional": false, "description": "Indicates if this type description has been type Undefined." },
> +                { "name": "isNull", "type": "boolean", "optional": false, "description": "Indicates if this type description has been type Null." },
> +                { "name": "isBoolean", "type": "boolean", "optional": false, "description": "Indicates if this type description has been type Boolean." },
> +                { "name": "isInteger", "type": "boolean", "optional": false, "description": "Indicates if this type description has been type Integer." },
> +                { "name": "isNumber", "type": "boolean", "optional": false, "description": "Indicates if this type description has been type Number." },
> +                { "name": "isString", "type": "boolean", "optional": false, "description": "Indicates if this type description has been type String." },
> +                { "name": "isObject", "type": "boolean", "optional": false, "description": "Indicates if this type description has been type Object." }

You should leave off the  `"optional": false` part. When left off, we know it is required.

> Source/JavaScriptCore/inspector/protocol/Runtime.json:145
> +                { "name": "isValid", "type": "boolean", "optional": true, "description": "If true, we were able to correlate the offset successfuly with a program location. If false, the offset may be bogus or the offset may be from a CodeBlock that hasn't executed." },

Seems this property could be made non-optional. You always define it below.

> Source/JavaScriptCore/inspector/protocol/Runtime.json:146
> +                { "name": "leastCommonAncestor", "type": "string", "optional": true, "description": "Least common ancestor of all Constructors if the TypeDescription has seen any structures." },

It would help to know what this string is. Is it the constructor name? The display name of the constructor function?

> Source/JavaScriptCore/inspector/protocol/Runtime.json:147
> +                { "name": "typeSetBooleans", "$ref": "TypeSetBooleans", "optional": true, "description": "Set of booleans for determining the aggregate type of this type description." },

If you change the type name we can make this "typeSet".

> Source/JavaScriptCore/inspector/protocol/Runtime.json:153
> +                { "name": "isOverflown", "type": "boolean", "optional": true, "description": "If true, this indicates that no more structures are being profiled because some maximum threshold has been reached and profiling has stopped because of memory pressure." }

I feel like the name could be clearer. Overflow feels like loaded terminology. How about "isProfilingSuspended" or "isProfilingDisabled".

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:163
> +            if (scriptSyntaxTree.containsNonEmptyReturnStatement(node.body) 
> +                || (WebInspector.TypeSet.fromPayload(functionReturnType)).hasSeenTypesOtherThan(WebInspector.TypeSet.TypeBit.Undefined)) {

You can put this all on one line. And the extra ()s around the second expression can be removed.

> Source/WebInspectorUI/UserInterface/Models/TypeSet.js:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

You can also keep the Apple since a lot of this code stems from the JSC file with the Apple license, but I think you aren't employeed by Apple while writing this so you could include yourself:

    * Copyright (C) 2014 Saam Barati

> Source/WebInspectorUI/UserInterface/Models/TypeSet.js:74
> +        // This function checks if types in bitString is contained in the types described by the 'test' bitstring. (i.e we haven't seen more types than 'test').

Nit: "if types in bitString is contained" => "if types in bitString are contained"

> Source/WebInspectorUI/UserInterface/Models/TypeSet.js:97
> +        return ((~test) & this._bitString) > 0x0;

Hmm, could this just be:

    return this._bitString === test;

Or am I overlooking something?

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:61
> +        if (this.types.isOverflown)
> +            properties.push({name: "...", structure: null});

If this is shown in the UI we probably want the Unicode ellipsis character. "\u2026"

    …
    HORIZONTAL ELLIPSIS
    Unicode: U+2026, UTF-8: E2 80 A6

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:173
>              properties.push({name: "...", structure: null});

Ditto.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:174
> +        if (typeSet.isContainedIn(WebInspector.TypeSet.TypeBit.Function | WebInspector.TypeSet.TypeBit.Null | WebInspector.TypeSet.TypeBit.Undefined))

Since these are checked so frequently in pairs, it might be nice to have:

    WebInspector.TypeSet.NullOrUndefinedTypeBits = WebInspector.TypeSet.TypeBit.Null | WebInspector.TypeSet.TypeBit.Undefined;

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:190
> +        return "(many)";

I wonder if this one we should make a UIString. I think the others shouldn't be.

[Brian Burg]

Comment on attachment 237901 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237901&action=review

>> Source/JavaScriptCore/inspector/protocol/Runtime.json:123
>> +                { "name": "isInDictionaryMode", "type": "boolean", "optional": true, "description": "If true, it indicates that the fields in this StructureDescription may be inaccurate. I.e, there might have been fields that have been deleted before it was profiled or it has fields we haven't profiled." }
> 
> This field name is unclear to me. I wonder if there is an adjective that might better describe this state. "isInaccurate"? "isLossy"?  "isImprecise"? "isInexact"? "isUnstable"? "

Thinking ahead to explaining performance issues, we may want to think of this as a flag that the structure has been deoptimized because it is/was used like a dictionary.

>> Source/JavaScriptCore/inspector/protocol/Runtime.json:153
>> +                { "name": "isOverflown", "type": "boolean", "optional": true, "description": "If true, this indicates that no more structures are being profiled because some maximum threshold has been reached and profiling has stopped because of memory pressure." }
> 
> I feel like the name could be clearer. Overflow feels like loaded terminology. How about "isProfilingSuspended" or "isProfilingDisabled".

isTruncated?

>> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:190
>> +        return "(many)";
> 
> I wonder if this one we should make a UIString. I think the others shouldn't be.

This suggestion seems in line with previous distinctions between source-level items that should be verbatim and those that should be translated. I think (?) might also be worth translating. It might be worth it for someone at Apple to check with translators as to whether appending '?' to source-level items is going to make sense in other languages. If not, maybe use UIString("%s?") to parameterize it.

[Brian Burg]

Comment on attachment 237901 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237901&action=review

> Source/JavaScriptCore/runtime/TypeProfiler.cpp:-71
> -    if (!location)

I generally prefer building up protocol objects inside Inspector*Agent.cpp files using static methods, as long as the necessary data is available through a public API. That way, this function could just return the TypeLocation for the {descriptor, divot, sourceId}. The agent can separate the complexity/quirks of building protocol objects (which is likely to be improved in the near future) from the actual data sources.

[Saam Barati]

Created attachment 237922 [details]
patch

Took into account suggestions.

I'm going to leave the concern regarding question marks for another patch.

[Joseph Pecoraro]

Comment on attachment 237922 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237922&action=review

r=me!

> Source/JavaScriptCore/inspector/protocol/Runtime.json:148
> +                { "name": "primitiveTypeNames", "type": "array", "items": { "type": "string" }, "optional": true, "description": "Array of type names for primtive types (int, string, etc) seen at an instruction" },

Typo: "primtive" => "primitive"
Nit: Comment should end in a period.

[Saam Barati]

landed in: http://trac.webkit.org/changeset/173505