Bug 137890 - Web Inspector: Add infrastructure for eslint based static analyzer
Summary: Web Inspector: Add infrastructure for eslint based static analyzer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: jonowells
URL:
Keywords: InRadar
Depends on: 138451
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-20 12:26 PDT by jonowells
Modified: 2014-11-10 16:39 PST (History)
7 users (show)

See Also:


Attachments
[PATCH] analyzer models. (13.69 KB, patch)
2014-10-20 12:34 PDT, jonowells
timothy: review-
Details | Formatted Diff | Diff
[TEST CASE] small site to test. (3.30 KB, application/zip)
2014-10-20 12:35 PDT, jonowells
no flags Details
[PATCH] Adjustments after comments. (15.01 KB, patch)
2014-10-29 17:25 PDT, jonowells
timothy: review-
Details | Formatted Diff | Diff
[PATCH] Adjustments after comments 2. (11.36 KB, patch)
2014-11-05 01:34 PST, jonowells
timothy: review-
Details | Formatted Diff | Diff
[PATCH] Adjustments after comments 3. (11.94 KB, patch)
2014-11-05 09:43 PST, jonowells
no flags Details | Formatted Diff | Diff
[PATCH] Patch with change to Main.html ordering. (11.95 KB, patch)
2014-11-10 14:01 PST, jonowells
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jonowells 2014-10-20 12:26:50 PDT
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.
Comment 1 jonowells 2014-10-20 12:34:46 PDT
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.
Comment 2 jonowells 2014-10-20 12:35:50 PDT
Created attachment 240140 [details]
[TEST CASE] small site to test.
Comment 3 jonowells 2014-10-20 14:16:01 PDT
Ignore this comment.
Comment 4 jonowells 2014-10-20 14:23:27 PDT
Also ignore this comment.
Comment 5 jonowells 2014-10-20 16:31:30 PDT
Ignore this too. (Testing).
Comment 6 jonowells 2014-10-23 11:36:10 PDT
<rdar://problem/18715320>
Comment 7 jonowells 2014-10-23 12:14:23 PDT
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.
Comment 8 Timothy Hatcher 2014-10-23 12:40:55 PDT
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.
Comment 9 jonowells 2014-10-23 18:25:31 PDT
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.
Comment 10 jonowells 2014-10-29 17:25:32 PDT
Created attachment 240642 [details]
[PATCH] Adjustments after comments.
Comment 11 jonowells 2014-11-04 11:11:25 PST
Bump!
Comment 12 Timothy Hatcher 2014-11-04 14:54:38 PST
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.
Comment 13 jonowells 2014-11-04 15:33:33 PST
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.
Comment 14 jonowells 2014-11-04 15:36:16 PST
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
Comment 15 Timothy Hatcher 2014-11-04 15:58:34 PST
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));
Comment 16 jonowells 2014-11-04 18:10:42 PST
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.
Comment 17 Timothy Hatcher 2014-11-04 18:37:38 PST
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.
Comment 18 jonowells 2014-11-04 18:58:45 PST
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.
Comment 19 jonowells 2014-11-04 19:01:47 PST
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);
Comment 20 jonowells 2014-11-05 01:34:21 PST
Created attachment 241012 [details]
[PATCH] Adjustments after comments 2.
Comment 21 WebKit Commit Bot 2014-11-05 01:35:39 PST
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.
Comment 22 Timothy Hatcher 2014-11-05 08:16:57 PST
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.
Comment 23 Timothy Hatcher 2014-11-05 08:18:48 PST
Comment on attachment 241012 [details]
[PATCH] Adjustments after comments 2.

r- for the leak and whitespace.
Comment 24 Timothy Hatcher 2014-11-05 08:22:10 PST
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.
Comment 25 jonowells 2014-11-05 09:23:38 PST
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?
Comment 26 jonowells 2014-11-05 09:43:34 PST
Created attachment 241036 [details]
[PATCH] Adjustments after comments 3.
Comment 27 WebKit Commit Bot 2014-11-05 10:43:38 PST
Comment on attachment 241036 [details]
[PATCH] Adjustments after comments 3.

Clearing flags on attachment: 241036

Committed r175628: <http://trac.webkit.org/changeset/175628>
Comment 28 WebKit Commit Bot 2014-11-05 10:43:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Joseph Pecoraro 2014-11-05 11:27:32 PST
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.
Comment 30 WebKit Commit Bot 2014-11-05 20:02:39 PST
Re-opened since this is blocked by bug 138451
Comment 31 Timothy Hatcher 2014-11-06 12:37:31 PST
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()')
Comment 32 jonowells 2014-11-06 12:39:53 PST
Ok, looking into it.
Comment 33 Timothy Hatcher 2014-11-06 12:47:14 PST
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.
Comment 34 jonowells 2014-11-06 14:07:51 PST
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.
Comment 35 jonowells 2014-11-06 14:08:11 PST
I meant Main.html.
Comment 36 Timothy Hatcher 2014-11-06 15:08:41 PST
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
Comment 37 Timothy Hatcher 2014-11-06 15:10:36 PST
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.
Comment 38 jonowells 2014-11-10 14:01:43 PST
Created attachment 241305 [details]
[PATCH] Patch with change to Main.html ordering.

Updated with only the file ordering changed.
Comment 39 WebKit Commit Bot 2014-11-10 16:39:14 PST
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>
Comment 40 WebKit Commit Bot 2014-11-10 16:39:19 PST
All reviewed patches have been landed.  Closing bug.