Bug 136515 – Web Inspector: JavaScript source text editor should have a linter

• Component: Web Inspector
• Status: NEW

[Jonathan Wells]

ESLint or similar should be used to provide feedback to user for serious issues and warnings. Feedback should be provided inline and update as the user adds to the source.

[Radar WebKit Bug Importer]

<rdar://problem/18222691>

[Jonathan Wells]

Created attachment 239233 [details]
[PATCH] Work in progress.

I'm posting this just for feedback. I'm working on the UI elements and proper coalescing of the edits to refresh the analyzer, but herein you will see the basic architecture and naming conventions, etc. Please leave feedback!

[Joseph Pecoraro]

Could you include some screenshots from this patch? Given that this introduces UI. It also makes it easier for reviewers to reason about things =)

[Saam Barati]

Comment on attachment 239233 [details]
[PATCH] Work in progress.

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

This looks pretty good to me. I'm excited to start using it with a UI, I think it's going to be really nice.

I have one question about the general design:
Why did you choose the singleton model for AnalyzerManager and IssueManager? It
seems a bit weird that there are all these different views and other classes that listen 
on Issue events and they all end up doing some boilerplate managing to make sure 
the URL associated w/ the issue matches some internal data structure.

It seems like these things might map pretty nicely to being 1:1 w/ a SourceCode. Maybe
I'm missing something?

Some specific comments below:

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

Nit: It seems that most enums use all hyphenated words, instead of camel case, so:
analyzer-manager-analyzer-messages-were-added

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:73
> +    initialize: function()

Seems like this method might not be needed, why not just put this in the constructor?
If you do keep it, it seems like it should be private, it seems like the constructor is the only place it's called.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:85
> +            sourceCode.requestContent(this.analyzerMessagesForSourceCode);

I think you probably need to bind the function to 'this' here.
Nit: No need for braces.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:88
> +        var analyzerMessages;

I think the inspector style for this type of thing, where you predeclare a variable that is guaranteed to be assigned to in the future, is to assign, at the point of declaration, to null.

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:115
> +        if (numberOfMessages === undefined || isNaN(numberOfMessages))

Why would this ever be undefined or NaN?

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:120
> +        this._previousResultButton.disabled = this._nextResultButton.disabled = (numberOfMessages <= 0);

It also seems slightly awkward to compare null with a number, even though the comparisons work out as expected.

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:156
> +            setTimeout(delayedWork.bind(this), 0);

Does this prevent or enable a transition? 
If it enables a transition, maybe it's better to use requestAnimationFrame?

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:191
> +        setTimeout(delayedWork.bind(this), 0);

Maybe requestAnimationFrame here too?

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:193
> +        this.dispatchEventToListeners(WebInspector.AnalyzerBanner.Event.DidShow);

Should this go inside delatedWork, since that is when it will actually show?

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:212
> +        if (this._delegate && typeof this._delegate.AnalyzerBannerRevealPreviousResult === "function")

Nit: It seems a bit weird that the names of these functions are capitalized.

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:218
> +        if (this._delegate && typeof this._delegate.AnalyzerBannerRevealNextResult === "function")

Ditto.

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:255
> +WebInspector.AnalyzerBanner.prototype.__proto__ = WebInspector.Object.prototype;

This can go above in the prototype object.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:421
> +        for (var i = 0; i < issues.length; ++i) {

You can use (for of ...) here

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:432
> +        WebInspector.analyzerManager.analyzerMessagesForSourceCode(this._sourceCode, this);

I don't think you nee the second argument here.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:466
> +        return !!this._analyzerIsActive;

I think it's nice to set these these fields like this in the constructor to their starting values. It's nice when reading the constructor to know that SourceCodeTextEditor has this field. 
This would also allow you to drop the !! here.

[Joseph Pecoraro]

Comment on attachment 239233 [details]
[PATCH] Work in progress.

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

Its tough to see what is implemented and what isn't implemented in this patch. You can use a ChangeLog, even for a Work in progress patch, to describe your design.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:31
> +WebInspector.AnalyzerManager = function()
> +{
> +    WebInspector.Object.call(this);
> +
> +    this._analyzer = eslint;
> +    this._config = {

AnalyzerManager sounds generic, but this seems very specific to JavaScript. We could potentially have CSS analyzers. So perhaps things can be broken out somewhere.

I'd expect a different kind of analyzer for different Resource types.

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:75
> +        this._analyzerSourceCodeMap = new WeakMap();

Style: new WeakMap;

> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:78
> +    analyzerMessagesForSourceCode: function(sourceCode)

I think we can do slightly better. Currently:

    analyzerMessagesForSourceCode on a source code
      - without content - requests content, analyzes, and triggers Event
      - already analyzed - triggers an Event (even if nothing was added/changed)
      - not analyzed - analyzes and triggers an Event

I think a better contract is decoupling the analysis from this method. This method just returns the current list of warnings.

   analyzeMessagesForSourceCode on a source code
      - always returns the current list of analyzer warnings for the source code

Analysis should happen elsewhere. For example:

  SourceCode.prototype.analyze([callback])
      - creates specific analyzer for the Resource type
      - runs analyzer if needed
      - if new warnings, fire Events so AnalyzerManager can collect all AnalyzerMessages and UIs can update
      - calls the optional callback when done analyzing. Useful if analysis produces no warnings.

>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:85
>> +            sourceCode.requestContent(this.analyzerMessagesForSourceCode);
> 
> I think you probably need to bind the function to 'this' here.
> Nit: No need for braces.

Also, this should return! Otherwise you do unnecessary work and potentially cause an exception.

>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:88
>> +        var analyzerMessages;
> 
> I think the inspector style for this type of thing, where you predeclare a variable that is guaranteed to be assigned to in the future, is to assign, at the point of declaration, to null.

Better yet, don't define the variable here. Just put the var inside the if and later on for the other variable.

> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:34
> +    this._sourceCode = sourceCode;
> +    this._text = text;
> +    this._line = line;
> +    this._column = column;
> +    this._ruleId = ruleId;

For those that are expected, lets do some asserts so nobody accidentally creates an object incorrectly.

    console.assert(sourceCode instanceof WebInspector.SourceCode);
    console.assert(typeof text === "string");
    console.assert(typeof line === "number");

Also, what is "ruleId"? Maybe it should be a "category" string or something?

Style: we tend to prefer "lineNumber" and "columnNumber" over "line"/"column".

> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:38
> +    constructor: WebInspector.AnalyzerMessage,

Style: __proto__ here instead of below.

> Source/WebInspectorUI/UserInterface/Models/AnalyzerMessage.js:39
> +

Nit: getter for this._sourceCode.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:439
> +    get canBeAnalyzed() {

Style: Opening brace should be on a newline.
Style: I don't think this needs to be a getter. For example, for pretty printing "canBeFormatted" is a function.

> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:137
> +        this._needsAnalysis = !!value;

Nit: Lets assume proper use and value is a boolean and just assign to value.

> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:91
> +        console.log('changed');

Nit: debug

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.css:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Nit: 2014.

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:26
> +WebInspector.AnalyzerBanner = function(delegate, element) {

A lot of this code is very similar to FindBanner. I wonder what can be shared.

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:29
> +    this._delegate = delegate || null; // For instance a content view.

I don't understand the comment. In this case, the delegate is a ContentBrowser, not a ContentView. Just out of date? I'd say you can remove the comment.

> Source/WebInspectorUI/UserInterface/Views/AnalyzerBanner.js:64
> +    this._numberOfResultsMessages;

Nit: seems this should be: (note the name change)

    this._numberOfResults = null;

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:473
> +            this._analyzerBanner.numberOfMessages = null;

I think I might prefer a clear() / reset() method instead of setting these properties to null.

> Source/WebInspectorUI/UserInterface/Views/LineMarker.js:24
> + */

Empty files? More to come later?

> Source/WebInspectorUI/UserInterface/Views/LineMessage.js:26
> +WebInspector.LineMessage = function(message, type)

It would be real nice if this type was an enum somewhere instead of raw strings.

> Source/WebInspectorUI/UserInterface/Views/LineMessage.js:44
> +WebInspector.LineMessage.ColorClassForType = {
> +    "warning": "line-message-warning",
> +    "error": "line-message-error"
> +};

Nit: StyleClassForType?

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:73
> +    // this._toggleAnalyzerButtonNavigationItem.addEventListener(WebInspector.ButtonNavigationItem.Event.Clicked, this._toggleAnalyzer, this);

You will want this. You can test scripts by doing a named eval.

    js> eval('//# sourceURL=test.js\n...');

Will create a "test.js" Script resource in the sidebar. Opening that is a ScriptContentView.

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:240
> +        console.log("ScriptContentView sez: about to toggle the analyzer");

Debug.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:285
> +            return this._analyzerIsActive = true;

Style: Split into two lines.

    this_analyzerIsActive = true;
    return;

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:289
> +        return this._analyzerIsActive = false;

Ditto. No return needed in this case then.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:815
> +        WebInspector.analyzerManager.removeEventListener(WebInspector.AnalyzerManager.Event.AnalyzerMessagesWereAdded, this._analyzerMessagesWereAdded, this);

Hmm, this sounds hacky to be adding and removing ourselves as an event listener based on operations. We should always be registered, always update model objects, and only update UI if needed.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1150
> +        // Ok no here's the deal: this is going to happen here. This is the text editor,
> +        // so it will know if it is actively doing stuff or not. Since the
> +        // text resource content view pretty much has access to this, WE
> +        // are going to add a few more events for the TextResourceContentView to handle.
> +        // OR we are going to have the TextResourceContentView just poll this source document for state
> +        // as necessary.
> +
> +        // So for instance this will know if we are currently in an analyzing state.
> +        // The sourceCode doesn't really need to know that about itself. Source just knows if its
> +        // current analysis data, should it exist on WebInspector.AnalyzerManager, is dirty or not.

I didn't quite follow these comments. Polling sounds bad, we should be able to use events.

There is an interesting discussion that can happen about analyzer messages. You can contrast AnalyzerMessages to "Issues" and "Formatting / Pretty Printing".

  - Issues
    - currently these are any errors the engine tells us about
    - these can be associated with a SourceCode, and if they are, they correspond to (typically) the original content of the SourceCode, as that is what the engine saw
    - how does this handle user editing the content?
      - I am not sure. I would expect issues disappear once you edit the document, as any line number may be broken.

  - Formatting (Pretty Printing)
    - this is just a re-skin of the current document's contents
    - this is implemented at the TextEditor level, because it is just a transformation of the current text
    - a mapping between the SourceCode's original text and the transformed text is kept on the SourceCode and TextEditor so anyone can map original -> display positions
    - how does this handle user editing the content?
      - the mapping goes away, and the pretty print button goes back to deactivated, because you can then format again if you want.

--

Now, how would you describe Analyzing/AnalyzerMessages?

Analyzing could purely be a pass over the contents of a TextEditor. It is just like Formatting in that sense, so if you make edits all warnings are void until you analyze again.

However, if we want to have the ability to run a report analyzing all Resources, then we can't rely on TextEditor. We don't want to have to open a TextEditor for every resource to get analyzer information.

I imagine it like this. If we run a global analyzer report, I think it should be over the content the engine saw. In this scenario, the SourceCode/AnalyzerManager stores the analyzer warnings for its original contents. If an unedited SourceCode is loaded in a TextEditor, the TextEditor can get this list of analyzer warnings or if no analysis has happened, then the first analyze pass can set the list of warnings on the SourceCode/AnalyzerManager. As soon as the user makes edits, they are out of luck. The TextEditor must ignore this special set of warnings against the original content. If we want, the TextEditor should offer a way to re-analyze the new contents (just like the pretty print button offers to re-pretty-print contents after edits). But that new list of warnings will only be tied to this new content.

Our editing story in general is not the strongest. Some support is there (Branches) but there are kinks. So I totally think nailing the un-edited story would be best for this first patch. When we focus on our editing story, we can improve all of this.

[Jonathan Wells]

Comment on attachment 239233 [details]
[PATCH] Work in progress.

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

>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:78
>> +    analyzerMessagesForSourceCode: function(sourceCode)
> 
> I think we can do slightly better. Currently:
> 
>     analyzerMessagesForSourceCode on a source code
>       - without content - requests content, analyzes, and triggers Event
>       - already analyzed - triggers an Event (even if nothing was added/changed)
>       - not analyzed - analyzes and triggers an Event
> 
> I think a better contract is decoupling the analysis from this method. This method just returns the current list of warnings.
> 
>    analyzeMessagesForSourceCode on a source code
>       - always returns the current list of analyzer warnings for the source code
> 
> Analysis should happen elsewhere. For example:
> 
>   SourceCode.prototype.analyze([callback])
>       - creates specific analyzer for the Resource type
>       - runs analyzer if needed
>       - if new warnings, fire Events so AnalyzerManager can collect all AnalyzerMessages and UIs can update
>       - calls the optional callback when done analyzing. Useful if analysis produces no warnings.

This makes sense.

>>> Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:85
>>> +            sourceCode.requestContent(this.analyzerMessagesForSourceCode);
>> 
>> I think you probably need to bind the function to 'this' here.
>> Nit: No need for braces.
> 
> Also, this should return! Otherwise you do unnecessary work and potentially cause an exception.

So I just realized I somehow posted an older diff! The actual function here looks like: 

    analyzerMessagesForSourceCode: function(sourceCode, callback)
    {
        function retrieveAnalyzerMessages()
        {
            var analyzerMessages;

            if (!sourceCode.needsAnalysis && this._analyzerSourceCodeMap.has(sourceCode)) {
                analyzerMessages = this._analyzerSourceCodeMap.get(sourceCode);
                callback(analyzerMessages);
                return;
            }

            analyzerMessages = [];
            var rawAnalyzerMessages = this._analyze(sourceCode.content);

            for (rawAnalyzerMessage of rawAnalyzerMessages) {
                analyzerMessages.push(new WebInspector.AnalyzerMessage(sourceCode, rawAnalyzerMessage.message, rawAnalyzerMessage.line, rawAnalyzerMessage.column, rawAnalyzerMessage.ruleId));
            }

            this._analyzerSourceCodeMap.set(sourceCode, analyzerMessages);
            sourceCode.needsAnalysis = false;

            callback(analyzerMessages);
        }

        if (!sourceCode.contentReceived) {
            sourceCode.requestContent(retrieveAnalyzerMessages.bind(this));
            return;
        }

        retrieveAnalyzerMessages.call(this);
    },

However, I'm going to take Joe's note above and decouple the manager and analysis work.

[Jonathan Wells]

Comment on attachment 239233 [details]
[PATCH] Work in progress.

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

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:439
>> +    get canBeAnalyzed() {
> 
> Style: Opening brace should be on a newline.
> Style: I don't think this needs to be a getter. For example, for pretty printing "canBeFormatted" is a function.

What is the rule for this? (getter vs function with return value)

>> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:91
>> +        console.log('changed');
> 
> Nit: debug

Yeah I had removed this, but then posted the older diff.

>> Source/WebInspectorUI/UserInterface/Views/LineMarker.js:24
>> + */
> 
> Empty files? More to come later?

More to come.

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:421
>> +        for (var i = 0; i < issues.length; ++i) {
> 
> You can use (for of ...) here

True, though this isn't part of my patch technically. I just moved some code around, though I could update that.

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:432
>> +        WebInspector.analyzerManager.analyzerMessagesForSourceCode(this._sourceCode, this);
> 
> I don't think you nee the second argument here.

So had I posted the correct diff (see above) this line would look like:

        WebInspector.analyzerManager.analyzerMessagesForSourceCode(this._sourceCode, this._analyzerMessagesWereAdded.bind(this));

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:815
>> +        WebInspector.analyzerManager.removeEventListener(WebInspector.AnalyzerManager.Event.AnalyzerMessagesWereAdded, this._analyzerMessagesWereAdded, this);
> 
> Hmm, this sounds hacky to be adding and removing ourselves as an event listener based on operations. We should always be registered, always update model objects, and only update UI if needed.

I actually changed this in favor of a callback model, then submitted the wrong diff here. Correct code is:

    _analyzerMessagesWereAdded: function(analyzerMessages)
    {
        for (analyzerMessage in analyzerMessages) {
            // TODO: Need to update the UI with widgets, etc.
        }
    },

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1150
>> +        // current analysis data, should it exist on WebInspector.AnalyzerManager, is dirty or not.
> 
> I didn't quite follow these comments. Polling sounds bad, we should be able to use events.
> 
> There is an interesting discussion that can happen about analyzer messages. You can contrast AnalyzerMessages to "Issues" and "Formatting / Pretty Printing".
> 
>   - Issues
>     - currently these are any errors the engine tells us about
>     - these can be associated with a SourceCode, and if they are, they correspond to (typically) the original content of the SourceCode, as that is what the engine saw
>     - how does this handle user editing the content?
>       - I am not sure. I would expect issues disappear once you edit the document, as any line number may be broken.
> 
>   - Formatting (Pretty Printing)
>     - this is just a re-skin of the current document's contents
>     - this is implemented at the TextEditor level, because it is just a transformation of the current text
>     - a mapping between the SourceCode's original text and the transformed text is kept on the SourceCode and TextEditor so anyone can map original -> display positions
>     - how does this handle user editing the content?
>       - the mapping goes away, and the pretty print button goes back to deactivated, because you can then format again if you want.
> 
> --
> 
> Now, how would you describe Analyzing/AnalyzerMessages?
> 
> Analyzing could purely be a pass over the contents of a TextEditor. It is just like Formatting in that sense, so if you make edits all warnings are void until you analyze again.
> 
> However, if we want to have the ability to run a report analyzing all Resources, then we can't rely on TextEditor. We don't want to have to open a TextEditor for every resource to get analyzer information.
> 
> I imagine it like this. If we run a global analyzer report, I think it should be over the content the engine saw. In this scenario, the SourceCode/AnalyzerManager stores the analyzer warnings for its original contents. If an unedited SourceCode is loaded in a TextEditor, the TextEditor can get this list of analyzer warnings or if no analysis has happened, then the first analyze pass can set the list of warnings on the SourceCode/AnalyzerManager. As soon as the user makes edits, they are out of luck. The TextEditor must ignore this special set of warnings against the original content. If we want, the TextEditor should offer a way to re-analyze the new contents (just like the pretty print button offers to re-pretty-print contents after edits). But that new list of warnings will only be tied to this new content.
> 
> Our editing story in general is not the strongest. Some support is there (Branches) but there are kinks. So I totally think nailing the un-edited story would be best for this first patch. When we focus on our editing story, we can improve all of this.

Definitely. Also that bit of rambling was left in by me and should have been replace with this:

    _sourceCodeContentDidChange: function(event)
    {
        // TODO: Coalesce changes and set a timer such that after a couple seconds if editing inactivity we will call the AnalyzerManager again.
    },

Based on your previous comment typing some changes then pausing for two seconds or so would rerun sourceCode.analyze which would then notify/update the AnalyzerManager and any other interested UI would update.

[Jonathan Wells]

Created attachment 239305 [details]
[PATCH] Work in progress, corrected patch.

This is what I should have posted, plus just the changes from the comments thus far.

[Blaze Burg]

<rdar://problem/19524548>