[Webkit-unassigned] [Bug 135142] Web Inspector: Begin showing type information

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135142





--- Comment #6 from Brian Burg <burg at cs.washington.edu>  2014-08-07 14:54:16 PST ---
(From update of attachment 235980)
View in context: https://bugs.webkit.org/attachment.cgi?id=235980&action=review

Fantastic work so far! I think this should be at least 3 patches:

1. BackForwardEntry.show() fix
2. Obtaining and converting the Script AST
3. Integration with the TextEditor and sidebar panels

For (2) I think it would be very beneficial to have some test coverage, especially for the special JSC modes.
I've sprinkled some smaller comments throughout, let me know if you have questions. brrian on IRC.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:28
> +    WebInspector.Object.call(this);

Add more argument assertions.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:41
> +

put __proto__ here.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:43
> +    get isActive()

Newline after // Public

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:71
> +        if (this.isActive) {

We usually prefer to always use the private member, if there is no material difference otherwise.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:92
> +            self._script._sourceCodeASTDidLoadCallback = function() {

Don't use |self| if |this| can be used instead. Also, this would be better if Script.waitForSourceCodeAST() (or similarly named) returned a promise that is fulfilled when SourceCodeAST has loaded. Then you can say 

if (!this._script.sourceCodeAST)
    this._script.waitForSourceCodeAST().then(this.insertAnnotations.bind(this));

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:96
> +            if (self._script.canRequestContentFromBackend())

I think this is a protected member. Try Script.prototype.requestContent().

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:104
> +        var startOffset;

We tend to use null as a sentinel when the value should always be assigned.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:116
> +        var startTime = (new Date()).getTime();

Date.now()

> Source/WebInspectorUI/UserInterface/Models/Script.js:128
> +            callback.apply(callback, Array.prototype.slice.call(arguments));

callback(...arguments)

> Source/WebInspectorUI/UserInterface/Models/Script.js:131
> +        DebuggerAgent.getScriptSource(this._id, callbackWrapper.bind(this));

DebuggerAgent.getScriptSource(this._id)
    .then(function(payload) {
        this._makeAST(payload.scriptSource);
    }).then(callback);

> Source/WebInspectorUI/UserInterface/Models/Script.js:195
> +        if (this._sourceCodeASTDidLoadCallback && typeof this._sourceCodeASTDidLoadCallback === "function") {

See above comment regarding requestContent on how to do this more cleanly.

> Source/WebInspectorUI/UserInterface/Models/SourceCodeAST.js:26
> +WebInspector.SourceCodeAST = function(sourceText, script) 

I prefer a name that explicitly mentions JavaScript or Script. WebInspector.SourceCode is the base class for all kinds of source files. I'm more ambivalent about spelling out AbstractSyntaxTree all over the place.. :)

> Source/WebInspectorUI/UserInterface/Models/SourceCodeAST.js:29
> +    this._script = script;

Lately we try to add instanceof assertions for arguments, especially in constructors. So,

console.assert(script && script instanceof WebInspector.Script, script);

The trailing argument will log the offending member if the assertion fails.

> Source/WebInspectorUI/UserInterface/Models/SourceCodeAST.js:35
> +        var verbose = false;

This doesn't make sense. The log statement is dead code.

> Source/WebInspectorUI/UserInterface/Models/SourceCodeAST.js:37
> +            console.log("Couldn't parse JavaScript File: \n" + e.message)

Use console.error

> Source/WebInspectorUI/UserInterface/Models/SourceCodeAST.js:181
> +        var startTime = (new Date()).getTime();

Date.now()

> Source/WebInspectorUI/UserInterface/Models/SourceCodeAST.js:227
> +            console.log("\n## RecurseTree time: " + "(" + scriptURL + "):" + ((new Date().getTime()) - startRecurseTreeTime) + "ms");

Does this code automatically split up work if it takes too long?

> Source/WebInspectorUI/UserInterface/Models/SourceCodeAST.js:492
> +        var ret;

result

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:-193
> -        // Hide the currently visible content view.

You should split this stuff into a separate patch.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:257
> +            RuntimeAgent.enableHighFidelityTypeProfiling();

This really shouldn't be directly called from the text editor, as it is unavailable from inspector tests. Maybe put it in DebuggerManager or RuntimeManager.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1082
> +

remove this extra newline

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1202
> +        RuntimeAgent.getRuntimeTypesForVariablesAtOffsets(allRequests, handler.bind(this));

Again, this should go through a Manager so we can test it and possibly cache repeatedly requested tokens.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1216
> +            //this.tokenTrackingController.highlightRange(candidate.expressionRange);

I know this is a WIP patch, but make sure to remove commented code eventually. :)

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:43
> +        for (i = 0; i < structures.length; i++) {

Use for/of here and throughout this function.

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:44
> +            var property = {};

This could be an object literal, right?

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:77
> +WebInspector.TypePropertiesSection.CompareProperties = function(propertyA, propertyB)

This should probably be called PropertyComparator, so we know where it's going to be used.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.