Bug 137890 – Web Inspector: Add infrastructure for eslint based static analyzer

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jonathan Wells]

Need to add a model object class AnalyzerManager to store analyzer messages, and an analyzer message model object. Additionally a resource needs to be able to analyze its own source.

[Jonathan Wells]

Created attachment 240139 [details]
[PATCH] analyzer models.

You can test this functionality using the attached test site and the following code. Load the index.html file, then open the inspector and inspect that inspector. Enter into the console:

WebInspector.frameResourceManager.mainFrame.resources[3].addEventListener(WebInspector.Resource.Event.AnalyzerDidFinish, function(event) {console.log(event);});

WebInspector.frameResourceManager.mainFrame.resources[3].analyze();

The analyzer response object will show, and you can look at the event.data object and see the messages.

[Jonathan Wells]

Created attachment 240140 [details]
[TEST CASE] small site to test.

[Jonathan Wells]

Ignore this comment.

[Jonathan Wells]

Also ignore this comment.

[Jonathan Wells]

Ignore this too. (Testing).

[Jonathan Wells]

<rdar://problem/18715320>

[Jonathan Wells]

I'm not sure of the placement of all the analyzer code in Resource.js. It's there because Resource.js has all the type information, but perhaps some of the base analyzer logic should exist in SourceCode.js.

[Timothy Hatcher]

Comment on attachment 240139 [details]
[PATCH] analyzer models.

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

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:30
> +    this._config = {

this._eslintConfig

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:74
> +    // Public
> +    mapSourceCodeToAnalyzerMessages: function(sourceCode, analyzerMessages)

New line between the comment and the method.

Having this method should be a sign that there is a problem. The manager is very thin and more of the logic should be here than in Resource.js.

> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:63
> +    get sourceCode()
> +    {
> +        return this._sourceCode;
> +    },
> +
> +    get text()
> +    {
> +        return this._text;
> +    },
> +
> +    get lineNumber()
> +    {
> +        return this._lineNumber;
> +    },
> +
> +    get columnNumber()
> +    {
> +        return this._columnNumber;
> +    },

This should hold a sourceCodeLocation, not sourceCode, lineNumber and columnNumber. That way it will work with source maps.

> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:65
> +    get ruleId()

What is ruleId? I'd say ruleIdentifier.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:91
> +WebInspector.Resource.Type._typeAnalyzerMap = new Map;
> +WebInspector.Resource.Type._typeAnalyzerMap.set(WebInspector.Resource.Type.Script, eslint);

This should live in AnalyzerManager.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:492
> +    analyze: function()
> +    {
> +        if (!this._needsAnalysis) {
> +            var analyzerMessages = WebInspector.analyzerManager.analyzerMessagesForSourceCode(this);
> +            this._analysisComplete(analyzerMessages);
> +            return;
> +        }
> +
> +        var analyzer = WebInspector.Resource.Type._typeAnalyzerMap.get(this._type);
> +
> +        function retrieveAnalyzerMessages()
> +        {
> +            var analyzerMessages = [];
> +            var rawAnalyzerMessages = analyzer.verify(this.content, WebInspector.analyzerManager._config);
> +
> +            for (var rawAnalyzerMessage of rawAnalyzerMessages) {
> +                analyzerMessages.push(new WebInspector.AnalyzerMessage(this, rawAnalyzerMessage.message, rawAnalyzerMessage.line, rawAnalyzerMessage.column, rawAnalyzerMessage.ruleId));
> +            }
> +
> +            WebInspector.analyzerManager.mapSourceCodeToAnalyzerMessages(this, analyzerMessages);
> +            this._needsAnalysis = false;
> +
> +            this._analysisComplete(analyzerMessages);
> +        }
> +
> +        if (!this._contentReceived) {
> +            this.requestContent(retrieveAnalyzerMessages.bind(this));
> +            return;
> +        }
> +
> +        retrieveAnalyzerMessages.call(this);
> +    },

This should also live in AnalyzerManager. I don't think anything needs added to Resource.js.

Client of AnalyzerManager should be able to be asked for analyzerMessagesForSourceCode, and be async with a Promise return. It can then do the requestConent if needed, analyze if needed, and fulfill the Promise with the messages.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:764
> +    handleCurrentRevisionContentChange: function()
> +    {
> +        this._needsAnalysis = true;
> +    },

This can notify AnalyzerManager of a change. I would normally say AnalyzerManager could listen for the WebInspector.SourceCode.Event.ContentDidChange event, but that would hold a reference that isn't as easy to break.

[Jonathan Wells]

Comment on attachment 240139 [details]
[PATCH] analyzer models.

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

>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:74
>> +    mapSourceCodeToAnalyzerMessages: function(sourceCode, analyzerMessages)
> 
> New line between the comment and the method.
> 
> Having this method should be a sign that there is a problem. The manager is very thin and more of the logic should be here than in Resource.js.

Joe had suggested I think rightfully that this should look a lot like IssueManager.js which is similarly thin. This would grow also as different components of the inspector start asking for the information from this manager. However, moving the analyzer logic into this manager will help with a few problems. 1) how can I have a different analyzer per resource type, 2) how can we make it easy to change the linter implementations later and 3) how can we solve my problem of a Script and a Resource both needing to be analyzed in the same way.

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:492
>> +    },
> 
> This should also live in AnalyzerManager. I don't think anything needs added to Resource.js.
> 
> Client of AnalyzerManager should be able to be asked for analyzerMessagesForSourceCode, and be async with a Promise return. It can then do the requestConent if needed, analyze if needed, and fulfill the Promise with the messages.

Can you clarify this just a little bit? I think I follow and agree, but you are saying that analyzerMessagesForSourceCode will be in AnalyzerMessenger, have a promise return, call sourceCode.requestContent if needed, do the analysis, and then fulfill the promise once the messages are available, leaving Resource to be left alone.

[Jonathan Wells]

Created attachment 240642 [details]
[PATCH] Adjustments after comments.

[Jonathan Wells]

Bump!

[Timothy Hatcher]

Comment on attachment 240642 [details]
[PATCH] Adjustments after comments.

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

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:31
> +        "env": {

No need for the "".

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:35
> +        "globals": {

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:38
> +        "rules": {

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:66
> +WebInspector.AnalyzerManager.typeAnalyzerMap = new Map;
> +WebInspector.AnalyzerManager.typeAnalyzerMap.set(WebInspector.Resource.Type.Script, eslint);

These should be _typeAnalyzerMap since we don't want them used outside this file.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:69
> +    AnalyzerMessagesWereAdded: "analyzer-manager-analyzer-messages-were-added"

Not used.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:98
> +                for (var rawAnalyzerMessage of rawAnalyzerMessages) {
> +                    analyzerMessages.push(new WebInspector.AnalyzerMessage(new WebInspector.SourceCodeLocation(sourceCode, rawAnalyzerMessage.line - 1, rawAnalyzerMessage.column - 1), rawAnalyzerMessage.message, rawAnalyzerMessage.ruleId));
> +                }

No need for {}. Should add a comment about the line and column numbers being one-based but we expect zero-based.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:108
> +            if (!sourceCode._contentReceived) {
> +                sourceCode.requestContent(retrieveAnalyzerMessages.bind(analyzerManager));
> +            }

No need for {}. This uses a private property, _contentReceived, on sourceCode. sourceCode.requestContent is safe to call even if the content is already fetched. Just always call requestContent.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:110
> +            retrieveAnalyzerMessages.call(analyzerManager);

You will be calling this twice if !sourceCode._contentReceived, once here and once when requestContent fires.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:111
> +        });

Should just add ".bind(this)" here and not declare and use analyzerManager.

> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:38
> +    } else if (sourceCodeOrSourceCodeLocation instanceof WebInspector.SourceCode) {
> +        this._sourceCode = sourceCodeOrSourceCodeLocation;
> +    }

No need for the {}. Why is sourceCodeLocation allowed to be null?

> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:61
> +    get lineNumber()
> +    {
> +        return this._sourceCodeLocation.lineNumber;
> +    },

Odd to have lineNumber and not columnNumber. I'd force clients to use sourceCodeLocation for this. IssueMessage is a bad example.

Though sourceCodeLocation can be null based on the constructor. Why is that?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:74
> +    AnalyzerDidFinish: "resource-analyzer-did-finish"

Not used anymore.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:445
> +    get canBeAnalyzed()
> +    {
> +        if (this._type === WebInspector.Resource.Type.Script)
> +            return true;
> +        return false;
> +    },

This puts a question only the AnalyzerManager knows in Resource. You can move this to AnalyzerManager. Having it here adds a maintenance burden of keeping getAnalyzerMessagesForSourceCode and this in sync. We will likely forget about this.

> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:190
> +    set needsAnalysis(value)
> +    {
> +        this._needsAnalysis = value;
> +    },
> +
> +    get needsAnalysis()
> +    {
> +        return this._needsAnalysis;
> +    },

This property would make more sense as read-only with a function to reset it.

get needsAnalysis()
markAsAnalyzed: function()

Or AnalyzerManager can keep track of this state by listening for WebInspector.SourceCode.Event.ContentDidChange and marking/deleting the messages for the SourceCode. (You can listen like SourceCode.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, ...) to catch all source code changes. And also avoid reference cycles on individual instances.) Nothing needs to change in Resource.js or SourceCode.js.

[Jonathan Wells]

Comment on attachment 240642 [details]
[PATCH] Adjustments after comments.

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

>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:111
>> +        });
> 
> Should just add ".bind(this)" here and not declare and use analyzerManager.

This doesn't work I don't think. `this` would refer to the Promise.

>> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:38
>> +    }
> 
> No need for the {}. Why is sourceCodeLocation allowed to be null?

I'm anticipating a need to have AnalyzerMessages that do not have locations but instead speak of the entire document.

>> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:61
>> +    },
> 
> Odd to have lineNumber and not columnNumber. I'd force clients to use sourceCodeLocation for this. IssueMessage is a bad example.
> 
> Though sourceCodeLocation can be null based on the constructor. Why is that?

I'm fine with that. See above regarding the null value.

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:445
>> +    },
> 
> This puts a question only the AnalyzerManager knows in Resource. You can move this to AnalyzerManager. Having it here adds a maintenance burden of keeping getAnalyzerMessagesForSourceCode and this in sync. We will likely forget about this.

That makes perfect sense.

[Jonathan Wells]

Comment on attachment 240642 [details]
[PATCH] Adjustments after comments.

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

>>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:111
>>> +        });
>> 
>> Should just add ".bind(this)" here and not declare and use analyzerManager.
> 
> This doesn't work I don't think. `this` would refer to the Promise.

... unless I bind the Promise

[Timothy Hatcher]

Comment on attachment 240642 [details]
[PATCH] Adjustments after comments.

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

>>>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:111
>>>> +        });
>>> 
>>> Should just add ".bind(this)" here and not declare and use analyzerManager.
>> 
>> This doesn't work I don't think. `this` would refer to the Promise.
> 
> ... unless I bind the Promise

The bind is for the function, not the Promise. The Promise will call the bound function.

new Promise(function(resolve, reject) { ... }.bind(this));

[Jonathan Wells]

Comment on attachment 240642 [details]
[PATCH] Adjustments after comments.

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

>> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:190
>> +    },
> 
> This property would make more sense as read-only with a function to reset it.
> 
> get needsAnalysis()
> markAsAnalyzed: function()
> 
> Or AnalyzerManager can keep track of this state by listening for WebInspector.SourceCode.Event.ContentDidChange and marking/deleting the messages for the SourceCode. (You can listen like SourceCode.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, ...) to catch all source code changes. And also avoid reference cycles on individual instances.) Nothing needs to change in Resource.js or SourceCode.js.

I like the second option. I'm working on that path. A change in the sourceCode would I think just mark it as necessary for reanalysis in another map.

[Timothy Hatcher]

Comment on attachment 240642 [details]
[PATCH] Adjustments after comments.

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

>>> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:190
>>> +    },
>> 
>> This property would make more sense as read-only with a function to reset it.
>> 
>> get needsAnalysis()
>> markAsAnalyzed: function()
>> 
>> Or AnalyzerManager can keep track of this state by listening for WebInspector.SourceCode.Event.ContentDidChange and marking/deleting the messages for the SourceCode. (You can listen like SourceCode.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, ...) to catch all source code changes. And also avoid reference cycles on individual instances.) Nothing needs to change in Resource.js or SourceCode.js.
> 
> I like the second option. I'm working on that path. A change in the sourceCode would I think just mark it as necessary for reanalysis in another map.

Or just delete the messages from the existing map, then the next time it will request the new content and reanalyze.

[Jonathan Wells]

Comment on attachment 240642 [details]
[PATCH] Adjustments after comments.

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

>>>> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:190
>>>> +    },
>>> 
>>> This property would make more sense as read-only with a function to reset it.
>>> 
>>> get needsAnalysis()
>>> markAsAnalyzed: function()
>>> 
>>> Or AnalyzerManager can keep track of this state by listening for WebInspector.SourceCode.Event.ContentDidChange and marking/deleting the messages for the SourceCode. (You can listen like SourceCode.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, ...) to catch all source code changes. And also avoid reference cycles on individual instances.) Nothing needs to change in Resource.js or SourceCode.js.
>> 
>> I like the second option. I'm working on that path. A change in the sourceCode would I think just mark it as necessary for reanalysis in another map.
> 
> Or just delete the messages from the existing map, then the next time it will request the new content and reanalyze.

Yes. Well. That's much smarter.

[Jonathan Wells]

Comment on attachment 240642 [details]
[PATCH] Adjustments after comments.

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

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:116
> +    mapSourceCodeToAnalyzerMessages: function(sourceCode, analyzerMessages)

Looking at it again, I don't really need this. Feel like I just need this._analyzerSourceCodeMap.set(sourceCode, analyzerMessages);

[Jonathan Wells]

Created attachment 241012 [details]
[PATCH] Adjustments after comments 2.

[WebKit Commit Bot]

Attachment 241012 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:94:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:98:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:100:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:114:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:116:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:117:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:118:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:120:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:121:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:122:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:123:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 11 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Comment on attachment 241012 [details]
[PATCH] Adjustments after comments 2.

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:2
> - * Copyright (C) 2013 Apple Inc. All rights reserved.
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

Should be 2013-2014.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:77
> +            var analyzer = WebInspector.AnalyzerManager._typeAnalyzerMap.get(sourceCode._type);

This should use sourceCode.type not _type.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:85
> +                var analyzerMessages = this._sourceCodeMessagesMap.get(sourceCode);
> +                resolve(analyzerMessages);

Could be one line.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:95
> +				// Raw line and column numbers are one-based. SourceCodeLocation expects them to be zero-based so we subtract 1 from each.
> +                for (var rawAnalyzerMessage of rawAnalyzerMessages)

Whitespace indent is wrong for the comment.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:98
> +				this._sourceCodeMessagesMap.set(sourceCode, analyzerMessages);

Whitespace indent is wrong.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:100
> +				sourceCode.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, this._handleSourceCodeContentDidChange, this);

Whitespace is wrong. This also leaks the sourceCode by holding a strong reference by the singleton AnalyzerManager until content changes (which might not happen). The event does not need to be listened for each sourceCode that gets analyzed. It can be registered once on the SourceCode constructor, which will fire when any instance of SourceCode changes. That is fine, because all _handleSourceCodeContentDidChange does is delete from the map.

So this should move to the AnalyzerManager constructor or be lazy done once the first time getAnalyzerMessagesForSourceCode is called.

It should look like:

WebInspector.SourceCode.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, this._handleSourceCodeContentDidChange, this);

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:122
> +		var sourceCode = event.target;
> +
> +		// Since sourceCode has changed, remove it and its messages from the map so getAnalyzerMessagesForSourceCode will have to reanalyze the next time it is called.
> +		this._sourceCodeMessagesMap.delete(sourceCode);
> +		sourceCode.removeEventListener(WebInspector.SourceCode.Event.ContentDidChange, this._handleSourceCodeContentDidChange, this);

Whitespace indent is wrong. No need for removeEventListener anymore.

[Timothy Hatcher]

Comment on attachment 241012 [details]
[PATCH] Adjustments after comments 2.

r- for the leak and whitespace.

[Timothy Hatcher]

Comment on attachment 241012 [details]
[PATCH] Adjustments after comments 2.

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

>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:100
>> +				sourceCode.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, this._handleSourceCodeContentDidChange, this);
> 
> Whitespace is wrong. This also leaks the sourceCode by holding a strong reference by the singleton AnalyzerManager until content changes (which might not happen). The event does not need to be listened for each sourceCode that gets analyzed. It can be registered once on the SourceCode constructor, which will fire when any instance of SourceCode changes. That is fine, because all _handleSourceCodeContentDidChange does is delete from the map.
> 
> So this should move to the AnalyzerManager constructor or be lazy done once the first time getAnalyzerMessagesForSourceCode is called.
> 
> It should look like:
> 
> WebInspector.SourceCode.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, this._handleSourceCodeContentDidChange, this);

Oh, SourceCode.js is missing a call that will allow this to work. After the constructor in SourceCode.js, add this line:

WebInspector.Object.addConstructorFunctions(WebInspector.SourceCode);

See Resource.js for reference. Any class that has events should also have this call.

[Jonathan Wells]

Comment on attachment 241012 [details]
[PATCH] Adjustments after comments 2.

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:2
>> + * Copyright (C) 2014 Apple Inc. All rights reserved.
> 
> Should be 2013-2014.

Why is this?

[Jonathan Wells]

Created attachment 241036 [details]
[PATCH] Adjustments after comments 3.

[WebKit Commit Bot]

Comment on attachment 241036 [details]
[PATCH] Adjustments after comments 3.

Clearing flags on attachment: 241036

Committed r175628: <http://trac.webkit.org/changeset/175628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 241036 [details]
[PATCH] Adjustments after comments 3.

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

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:93
> +WebInspector.AnalyzerManager._typeAnalyzerMap = new Map;
> +WebInspector.AnalyzerManager._typeAnalyzerMap.set(WebInspector.Resource.Type.Script, eslint);
> +
> +WebInspector.AnalyzerManager.prototype = {
> +    constructor: WebInspector.AnalyzerManager,
> +    __proto__: WebInspector.Object.prototype,
> +
> +    // Public
> +
> +    getAnalyzerMessagesForSourceCode: function(sourceCode)
> +    {
> +        return new Promise(function(resolve, reject) {
> +            var analyzer = WebInspector.AnalyzerManager._typeAnalyzerMap.get(sourceCode.type);
> +            if (!analyzer) {
> +                reject(new Error("This resource type cannot be analyzed."));
> +                return;
> +            }
> +
> +            if (this._sourceCodeMessagesMap.has(sourceCode)) {
> +                resolve(this._sourceCodeMessagesMap.get(sourceCode));
> +                return;
> +            }
> +
> +            function retrieveAnalyzerMessages()
> +            {
> +                var analyzerMessages = [];
> +                var rawAnalyzerMessages = analyzer.verify(sourceCode.content, this._eslintConfig);

I think this could go one step further and be much nicer!

Currently the AnalyzerManager._typeAnalyzerMap has just one analyzer (eslint). The manager looks like it tries to have different resource type analyzers, but currently knows way too much about JavaScript, including holding an eslint config! E.g. the map is used like:

    var analyzer = map.get(type);
    var messages = analyzer.verify(content, eslintConfig);

That makes it hard to extend this to include a CSS / StyleSheet analyzer!

----

I think AnalyzerManager's map should hold WebInspector.Analyzer instances. With pseudocode.

    function WebInspector.Analyzer() {}
    WebInspector.Analyzer.prototype = {
        analyze: function(content) { /* subclass implements, returns a list of AnalyzerMessage objects */ }
    };

    function WebInspector.ScriptAnalyzer() {
        this._eslintConfig = {...};
    }
    WebInspector.Analyzer.prototype = {
        analyze: function(content) { eslint.verify(content, this._eslintConfig); ... }
    };

    function WebInspector.StylesheetAnalyzer() {
        this._otherConfig = {...};
    }
    WebInspector.Analyzer.prototype = {
        analyze: function(content) { ... }
    };


Then the AnalyzerManager knows nothing about how individual analyzers work, but it get get analyzer messages, using a simpler API:

    var analyzer = map.get(type);
    var messages = analyzer.analyze(content);

----

I'm a little skeptical of using just WebInspector.Resource.Type.Script as the key. I think it is clever, but I'm not sure that is enough information for an analyzer to work with. A WebInspector.Resource.Type.Script could be JavaScript, CoffeeScript, TypeScript, and more. However I think the eslint analyzer will only work with "text/javascript". So I think analyze might need to pass the content and mime type for the Analyzer to make a complete decision if we are going to have one Analyzer per Resource.Type.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 138451

[Timothy Hatcher]

Simon saw this in his console:

Main.js:9372:806: CONSOLE ERROR ReferenceError: Can't find variable: eslint
Main.html:38:28: CONSOLE ERROR TypeError: undefined is not a function (evaluating 'WebInspector.loaded()')

[Jonathan Wells]

Ok, looking into it.

[Timothy Hatcher]

It likely is an error when the resources are combined production/nightly builds.

copy-user-interface-resources.pl and copy-user-interface-resources-dryrun.rb should help.

[Jonathan Wells]

Yeah, this is the problem. The resulting Main.js:

    <script src="Main.js"></script>
    <script src="Esprima.js"></script>
    <script src="ESLint.js"></script>

Esprima and ESLint should be included first. I'll change copy-user-interface-resources.pl.

[Jonathan Wells]

I meant Main.html.

[Timothy Hatcher]

Comment on attachment 241036 [details]
[PATCH] Adjustments after comments 3.

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

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:68
> +WebInspector.AnalyzerManager._typeAnalyzerMap = new Map;
> +WebInspector.AnalyzerManager._typeAnalyzerMap.set(WebInspector.Resource.Type.Script, eslint);

Ah, I see. it is because of this global code. This really could move into the constructor and hang off of this, like _sourceCodeMessagesMap. There are no external users of this and WebInspector.AnalyzerManager is already a singleton. Less global code the better.

> Source/WebInspectorUI/UserInterface/Main.html:178
> +    <script src="External/ESLint/eslint.js"></script>

This likely needs

[Timothy Hatcher]

Comment on attachment 241036 [details]
[PATCH] Adjustments after comments 3.

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

>> Source/WebInspectorUI/UserInterface/Main.html:178
>> +    <script src="External/ESLint/eslint.js"></script>
> 
> This likely needs

Ignore this.

[Jonathan Wells]

Created attachment 241305 [details]
[PATCH] Patch with change to Main.html ordering.

Updated with only the file ordering changed.

[WebKit Commit Bot]

Comment on attachment 241305 [details]
[PATCH] Patch with change to Main.html ordering.

Clearing flags on attachment: 241305

Committed r175838: <http://trac.webkit.org/changeset/175838>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.