[webkit-reviews] review granted: [Bug 136500] Web Inspector: Modify the type profiler runtime protocol to transfer some computation into the WebInspector : [Attachment 237901] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Saam Barati
<saambarati1 at gmail.com>'s request for review:
Bug 136500: Web Inspector: Modify the type profiler runtime protocol to
transfer some computation into the WebInspector
https://bugs.webkit.org/show_bug.cgi?id=136500

Attachment 237901: patch
https://bugs.webkit.org/attachment.cgi?id=237901&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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(We
bInspector.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.