Bug 135142 – Web Inspector: Create a UI for displaying JavaScript type information

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Saam Barati]

JavaSciptCore now gathers more type information. This patch moves the inspector in the direction of showing more information.

[Radar WebKit Bug Importer]

<rdar://problem/17757293>

[Saam Barati]

Created attachment 235263 [details]
Work In Progress

This is my work in progress towards integrating type information in the web inspector.

[Saam Barati]

Created attachment 235651 [details]
Work In Progress

This is a continued work in progress of the patch.

[Saam Barati]

Created attachment 235916 [details]
work in progress



This changes a few things from the last work in progress. 
This also can't be landed until JSC merges in the ftlopt branch.
Any feedback until then is greatly appreciated.

[Saam Barati]

Created attachment 235980 [details]
work in progress patch

This adds some optimizations to the previous patch. Specifically, we don't update type tokens while CodeMirror is being scrolled.
Also, we only show and update tokens for the viewable range in CodeMirror. We no longer traverse/annotate the entire AST because this is
not performant for sufficiently large JavaScript files.

[Brian Burg]

Comment on attachment 235980 [details]
work in progress patch

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.

[Timothy Hatcher]

I agree this should be split into multiple patches. Model + Tests patch and a UI patch.

I would call the AST classes, vars, types, etc.: "ScriptSyntaxTree". We don't use acronyms in types in the Web Inspector code.

[Saam Barati]

(In reply to comment #6)
> (From update of attachment 235980 [details])
> 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:

Thanks for all the feedback, I've made a bunch of changes b/c of it and have a few questions below.


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

1. BackForwardEntry.show() fix was actually landed in a separate patch, I accidentally left it in this diff.
2. I've created a new bug for the AST portion.
3. I'm leaving this bug open for the User Interface portions of this feature.


> For (2) I think it would be very beneficial to have some test coverage, especially for the special JSC modes.

My questions are mainly about testing. How should I test the protocol portion of this? 
I think it's also a good idea to test the AST, which can be done pretty easily.


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

No, but I've limited the AST nodes we request type information to approximately the nodes that are visible from within the text editor, so this should never take long.


> > 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.

So should I use RuntimeManager as an interface to RuntimeAgent.X where X are the different protocol methods I've defined? If so, how do I test these new protocol methods?


> > 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.

I do some intelligent caching on the JSC side of things based on text offsets. I don't think we can safely cache inside the inspector because type information for a variable at a given source code location can change at anytime.


Thanks for all the input. I've changed a bunch of because of it. I'll have another patch up today (for this bug and for the AST: https://bugs.webkit.org/show_bug.cgi?id=135763).

[Saam Barati]

Created attachment 236298 [details]
work in progress

Brian's comments taken into account. A few other updates since the last patch, which are mostly performance optimizations.

[Saam Barati]

Comment on attachment 236298 [details]
work in progress

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

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:173
> +        var start; 

This should be set to null.

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

I think the correct thing to do here is not to recompile all JS functions. Calling this function will potentially recompile everything. I think the correct action to take is to manipulate the setting, and when the Inspector closes, it should check this setting, and if it's false, it should call disableHighFidelityTypeProfiling.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1040
> +        else if (this._typeTokenAnnotator && this._typeTokenAnnotator.isActive)

Maybe this shouldn't be a mode that overrides other modes? Because this will almost certainly always be enabled when viewing script resources.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1197
> +        var offset;

Also set to null.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1247
> +        //this._popover = this._popover || new WebInspector.Popover(this);

I've had little luck with this line producing the correct results. In my testing, the nicest UI is simply to create a new popover every time.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1502
> +    _prettyPrint: function(pretty)

Is this okay to override?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1524
> +        if (this._isTrackingScrollEventsForTypeTokenAnnotator)

I still need to create a way to remove these listeners, I've just been lazy about it.

[Saam Barati]

Created attachment 236493 [details]
work in progress

I think this is pretty close to a patch I want to land, feedback is greatly appreciated.

[Brian Burg]

Comment on attachment 236493 [details]
work in progress

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

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:30
> +    console.assert(sourceCodeTextEditor && sourceCodeTextEditor instanceof WebInspector.SourceCodeTextEditor && script);

I prefer assertions to deal with one argument at a time if it's routine stuff like this.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:148
> +        var self = this;

you can pass the thisObject as the last parameter to forEach and avoid this capture.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:155
> +                    param._attachments._typePopoverTitle = "Type information for variable: " + param.name;

WebInspector.UIString (here and in other popovers, tooltips, etc)

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:160
> +            if (!node._attachments._typeToken && node._attachments._returnTypes && node._attachments._returnTypes.displayTypeName && (scriptSyntaxTree.containsNonEmptyReturnStatement(node.body) || node._attachments._returnTypes.displayTypeName !== "Undefined")) {

This condition is kinda unwieldy. It would be better to split it into some bools or have early exits:

var functionReturnType = node._attachments._returnTypes;
if (node._attachments._typeToken || !functionReturnType)
    break;

if (functionReturnType.displayTypeName && (functionReturnType.displayTypeName !== "Undefined" || ...) {
    ...
}

break;

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:183
> +            start = { line: location.lineNumber, ch: location.columnNumber };

Nit: house style is {line: ..., ch: ...}

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:189
> +        if (isReturn) {

I had to read through a lot of code to figure out why it's called isReturn and what's different. I would recommend something like shouldInsertAfterParameterList or something more wordy. In the comment below, maybe illustrate the adjustment that happens. As I imagine it, it's something like

function bar(a, b, c) [type-bubble] {
...
}

or 

foo: get function() [type-bubble] { return ... }

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:195
> +        // Note: CodeMirror bookmarks render to the left of the character they're being displayed next to; that is why right margin checks the current offset.

Is this true in general (i.e., for RTL case too)? I don't think this is an issue in the current usage, but you might want to fix the comment.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:197
> +        var shouldHaveLeftMargin = offset !== 0 && !isSpaceRegexp.test(sourceString[offset - 1]);

nice.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:208
> +        var Types = WebInspector.ScriptSyntaxTree.NodeType;

I would inline this into case labels.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:240
> +    _getNewOffsetForReturnStatement: function(node, offset, sourceString)

following from above, this could use a better name, like findOffsetAfterParameters or something.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:244
> +        // All this code is just a way to find this parenthesis while ignoring comments.

I don't remember much about the esprima API, but ostensibly we have to do this because it doesn't track positions for concrete syntax (like parens). Does it keep track of comments?

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:249
> +        while ((sourceString[offset] !== ")" || shouldIgnore) && offset < sourceString.length) {

I would use 'i' instead of offset, unless it's something like a unicode codepoint offset (where the chars and bytes are different things).

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:262
> +            WebInspector.showJavaScriptTypeInformationSetting.value = true;

you could update this outside of the branches.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:266
> +            // to call it here, we would hava JavaScriptCore compile out all the necesessary type profiilng information,

typos.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:282
> +        titleElement.className = "title";

Eventually, should convert to CSS class name constants.

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

2014

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:43
> +        var structures = this.types.globalStructures ? this.types.globalStructures : this.types.localStructures;

you could do: (applies various places)

var structures = this.types.globalStructures || this.types.localStructures;

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:160
> +                name: this.property.structure.prototypeStructure.constructorName + " (Prototype)",

UIString?

[Saam Barati]

Comment on attachment 236493 [details]
work in progress

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

>> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:155
>> +                    param._attachments._typePopoverTitle = "Type information for variable: " + param.name;
> 
> WebInspector.UIString (here and in other popovers, tooltips, etc)

I need to create a more abstracted way of forming popover titles here. (Applies for all the other places I inline titles).

>> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:160
>> +            if (!node._attachments._typeToken && node._attachments._returnTypes && node._attachments._returnTypes.displayTypeName && (scriptSyntaxTree.containsNonEmptyReturnStatement(node.body) || node._attachments._returnTypes.displayTypeName !== "Undefined")) {
> 
> This condition is kinda unwieldy. It would be better to split it into some bools or have early exits:
> 
> var functionReturnType = node._attachments._returnTypes;
> if (node._attachments._typeToken || !functionReturnType)
>     break;
> 
> if (functionReturnType.displayTypeName && (functionReturnType.displayTypeName !== "Undefined" || ...) {
>     ...
> }
> 
> break;

Yeah. I'm going to do a variation of this.

>> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:189
>> +        if (isReturn) {
> 
> I had to read through a lot of code to figure out why it's called isReturn and what's different. I would recommend something like shouldInsertAfterParameterList or something more wordy. In the comment below, maybe illustrate the adjustment that happens. As I imagine it, it's something like
> 
> function bar(a, b, c) [type-bubble] {
> ...
> }
> 
> or 
> 
> foo: get function() [type-bubble] { return ... }

I agree, a comment like this is a good addition. 
Renaming is also apt.

>> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:244
>> +        // All this code is just a way to find this parenthesis while ignoring comments.
> 
> I don't remember much about the esprima API, but ostensibly we have to do this because it doesn't track positions for concrete syntax (like parens). Does it keep track of comments?

Yes, you can keep track of comments in esprima by passing it an option when parsing. But I don't think it's worth us doing this unless we need it for another reason.
I'm not even sure if having comments on the AST will help us here, because I remember looking at this option and I think it just tacks comments onto different AST nodes, I'm not sure how in depth it tracks those comments' ranges.

>> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:249
>> +        while ((sourceString[offset] !== ")" || shouldIgnore) && offset < sourceString.length) {
> 
> I would use 'i' instead of offset, unless it's something like a unicode codepoint offset (where the chars and bytes are different things).

It's not for unicode characters, but I kind of like using offset here, why do you prefer 'i'? To me, it's more obvious by reading it that we're dealing with a text offset in the source code.

[Saam Barati]

Created attachment 236719 [details]
patch

Patch w/ Brian's suggestions.

[Joseph Pecoraro]

Comment on attachment 236719 [details]
patch

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

Mostly style comments so far.

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:53
>      JavaScriptExpression: "javascript-expression",
> -    MarkedTokens: "marked-tokens"
> +    MarkedTokens: "marked-tokens",
> +    JavaScriptTypeInformation: "javascript-type-information"

Nit, put the two JavaScript* ones next to eachother.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:98
> +        var scriptSyntaxTree = this._script._scriptSyntaxTree;

Nit: This should not reach into scriptSyntaxTree directly. You should add a public getter or always use the request version.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:99
> +        var sourceID = this._script.id;

Nit: This is not used until later on. I'd move it down with the other variables.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:115
> +        var codeMirror = this._sourceCodeTextEditor._codeMirror;
> +        var visibleRange = codeMirror.getViewport();

This does a lot of CodeMirror access. Perhaps it should be refactored.

Most of the other classes that make heavy use of CodeMirror we keep inside of TextEditor, so if we do have to replace the 3rd party editor, CodeMirror objects / APIs doesn't leak much outside of it. For example, we have a number of CodeMirror* classes, which TextEditor creates internally. This could probably be another of of them. The tricky bit is getting the CodeMirror editor.

    CodeMirrorTypeAnnotator which just needs:
      - CodeMirror (passed directly instead of reaching into TextEditor)
      - SourceCodeTextEditor (because we need a SourceCode)

This would be the first time a SourceCodeTextEditor touches "CodeMirror" but it would be limited to just getting the object. SourceCodeTextEditor could create as needed.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:127
> +        if (sourceMap) {
> +            startOffset = sourceMap.formattedToOriginalOffset(Math.max(visibleRange.from - 1, 0), 0);
> +            endOffset = sourceMap.formattedToOriginalOffset(visibleRange.to - 1, 0);
> +        } else {
> +            startOffset = codeMirror.getDoc().indexFromPos({line: visibleRange.from, ch: 0});
> +            endOffset = codeMirror.getDoc().indexFromPos({line: visibleRange.to, ch: 0});
> +        }
> +

Maybe we should add a few lines of padding to the visibleRange. Would that improve the UI while scrolling?

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:131
> +            // Because this is an asynchronous call, we could have been deactivated before the callback function is called.

Nit: The first half of the comment is unnecessary.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:136
> +            allNodesInRange.forEach(this._insertTypeTokensForEachNode, this);
> +            allNodesInRange.forEach(this._updateTypeTokensForEachNode);

Heh, lets pass "this" to the update function as well, least we forget in the future. It has no cost.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:157
> +                if (!param._attachments._typeToken && param._attachments._types && param._attachments._types.displayTypeName)
> +                    this._insertToken(param.range[0], param, false, WebInspector.TypeTokenView.TitleType.variable, param.name);

Now that I see _attachments properties used, I think we should modify the property names.

    // These are attachments created by ScriptSyntaxTree itself, they don't need underscores.
    param._attachments._types => param.attachments.types
    param._attachments._returnTypes => param.attachments.returnTypes

    // These are attachments created by objects outside of ScriptSyntaxTree, double underscore them.
    param._attachments._typeToken => param.attachments.__typeToken

This is a somewhat uncommon style we use, but I think it provides clarity.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:183
> +        var start = null; 

Declare "var offset" here. Otherwise you are leaking "offset" into the global scope!

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:184
> +        var location; 

Style: inline the var where used.

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:246
> +            if (isSingleLineComment && (sourceString[offset] === "\n" || (sourceString[offset] === "\r" && sourceString[offset + 1] === "\n"))) {
> +                if (sourceString[offset] === "\r")
> +                    offset++;
> +                isSingleLineComment = false;
> +                shouldIgnore = false;

I think this may be slightly incorrect but could be simplified.

Looking at the ES5 Grammar:
<http://www.ecma-international.org/ecma-262/5.1/#sec-7.3>
<http://www.ecma-international.org/ecma-262/5.1/#sec-7.4>

>    SingleLineComment ::
>        // SingleLineCommentChars?
>
>    SingleLineCommentChars ::
>        SingleLineCommentChar SingleLineCommentChars?
>
>    SingleLineCommentChar ::
>        SourceCharacter but not LineTerminator
>
>    LineTerminator ::
>        <LF>
>        <CR>
>        <LS>
>        <PS>

>    Table 3 — Line Terminator Characters
>    \u000A	Line Feed         	<LF>
>    \u000D	Carriage Return    	<CR>
>    \u2028	Line separator  	<LS>
>    \u2029	Paragraph separator	<PS>

So instead of looking for <LF> || <CR><LF> we should just look for <LF> || <CR> || <LS> || <PS> and move on.

    function isLineTerminator(c) {
        return c === "\n" || c === "\r" || c === "\u2028" || c === "\u2028";
    }

    if (isSingleLineComment && isLineTerminator(sourceString[offset])) {
        isSingleLineComment = false;
        shouldIgnore = false;
    } ...

You don't need to special case anything after the terminator since it will just naturally not be ")".

> Source/WebInspectorUI/UserInterface/Images/NavigationItemTypes.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 15 15">

The other button is 13x13 this is 15x15, seems  bit wide. You may be able to adjust appropriately and make a thinner T.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:112
> +        if (this._typeTokenAnnotator)
> +            this._typeTokenAnnotator.resume();

How about start / stop instead of resume / pause?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:420
> +        if (this._codeMirror.getMode().name === "javascript")
> +            this._makeTypeTokenAnnotator();

This should not check this._codeMirror. That is a layering violation.

Looking at the implementation of this function, just call it and it will return if there are no scripts.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1218
> +        if (this._formatterSourceMap)
> +            offset = this._formatterSourceMap.formattedToOriginalOffset(range.start.line, range.start.ch);
> +        else
> +            offset = this.tokenTrackingController._codeMirror.getDoc().indexFromPos({line: range.start.line, ch: range.start.ch});

This should not touch _codeMirror. That is a layering violation.

We should just put some method on TextEditor to get the offset given a (line, ch) pair:

    offsetForPosition: function(pos)
    {
        return this._codeMirror.getDoc().indexFromPos(pos);
    }

That you can call from here:

    offset = this.offsetForPosition(range.start);

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1513
> +    _prettyPrint: function(pretty)
> +    {
> +        WebInspector.TextEditor.prototype._prettyPrint.call(this, pretty);

We should rename this to "prettyPrint" and put it in a "Protected" section now that we are overriding it.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1522
> +        if (!this._sourceCode.scripts.length)
> +            return;

This should return immediately if the feature is not supported by the backend.

    if (!RuntimeAgent.getRuntimeTypesForVariablesAtOffsets)
        return;

For example, _typeTokenAnnotator should always be null when inspecting iOS 6 / 7 which does not support this feature. Otherwise when we got to a RuntimeAgent call we would throw an exception.

In fact, ScriptSyntaxTree.prototype.updateTypes should assert that RuntimeAgent.getRuntimeTypesForVariablesAtOffsets exists.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1524
> +        this._typeTokenAnnotator = new WebInspector.TypeTokenAnnotator(this, this._sourceCode.scripts[0]);

We should add a FIXME about making this work with a Resource with multiple scripts. E.g. HTML with multiple <script> blocks.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1543
> +    _enableScrollEventsForTypeTokenAnnotator: function()
> +    {
> +        // Pause updating type tokens while scrolling to prevent frame loss.
> +        var scrollHandler = this._makeTypeTokenScrollEventHandler();
> +        this._currentTypeTokenScrollHandler = scrollHandler;
> +        this._codeMirror.on("scroll", scrollHandler);
> +    },
> +
> +    _disableScrollEventsForTypeTokenAnnotator:function()
> +    {
> +        this._codeMirror.off("scroll", this._currentTypeTokenScrollHandler);
> +        this._currentTypeTokenScrollHandler = null;
> +    },

You should be able to put all of this scroll timer logic entirely in the TypeAnnotator (which has a reference to the CodeMirror object). Start/Stop can add/remove the event listener detection.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:57
> +    this._showTypesButtonNavigationItem = new WebInspector.ActivateButtonNavigationItem("show-types", toolTipTypes, activatedToolTipTypes, "Images/NavigationItemTypes.svg", 16, 16);

How does this look in Legacy (e.g. Mavericks)? I think it is probably incorrect to have 16x16 here despite the image being 15x15.

To test legacy, bump the ImageUtilities global number and set "isLegacyMac" to true in Platform.js. Recompile + test.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:61
> +    WebInspector.showJavaScriptTypeInformationSetting.addEventListener(WebInspector.Setting.Event.Changed, this._showJavaScriptTypeInformationSettingChanged, this);

You should duplicate all of this code for Views/ScriptContentView.js and probably also Views/TextContentView.js. Even if the buttons are always disabled, at least they will be consistent for now.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:211
> +        this._showTypesButtonNavigationItem.enabled = !!this._textEditor._typeTokenAnnotator;

Instead of reaching in and checking the private ivar, it would be nice to call a function like "canBeFormatted". How about "canAnnotateTypes" or "canShowTypeAnnotations". The implementation of that can just check if !!typeTokenAnnotator.

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:86
> +    if (a.indexOf("__proto__") !== -1)
> +        return 1;
> +    if (b.indexOf("__proto__") !== -1)
> +        return -1;

Why the indexOf proto check? What would be an example of two strings being compared in here?

Likewise, I think we could write a String.prototype.endsWith in Utilities.js that may be more efficient than indexOf if this really is an ends with check.

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:91
> +    // if used elsewhere make sure to
> +    //  - convert a and b to strings (not needed here, properties are all strings)

I suppose you can remove this comment.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.css:34
> +    font-size: 92%;

This is unusual. We would typically provide an explicit px size if possible. I assume this is snapping to a particular size anyways.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:35
> +    span.style.marginRight = (shouldHaveRightMargin ? "5px" : "0px");
> +    span.style.marginLeft = (shouldHaveLeftMargin ? "5px" : "0px");

These should be CSS classes.

    .left-spacing {
        margin: 5px;
    }

    .right-spacing {
        margin: 5px;
    }

Then on the JS side:

    if (shouldHaveRightMargin)
        span.classList.add("right-spacing");
    if (shouldHaveLeftMargin)
        span.classList.add("left-spacing");

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:51
> +        titleString = WebInspector.UIString("Type information for variable") + ": " + functionOrVariableName;

This should be a full format string so it can be localized for Left-to-Right:

    WebInspector.UIString("Type information for variable: %s").format(functionOrVariableName);

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:55
> +        titleString = WebInspector.UIString("Return type information for function");
> +        if (functionOrVariableName)
> +            titleString += ": " + functionOrVariableName;

Likewise. This one could be two strings.

    WebInspector.UIString("Return type information for function: %s").format(functionOrVariableName);
    WebInspector.UIString("Return type information for anonymous function")

If any of these titles are too long we can come up with shorter names.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:64
> +WebInspector.TypeTokenView.TitleType = {
> +    variable: "title-type-variable",
> +    returnStatement: "title-type-return-statement"
> +};

Style: "Variable" and "ReturnStatement".

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:73
> +WebInspector.TypeTokenView.Colors = {
> +    purple: "#cc3bbf",
> +    blue: "#3b70e2",
> +    red: "#df352e",
> +    green: "#76ae1d",
> +    gray: "#7e7e7e",
> +    orange: "#ed8400"
> +};

Ideally these would be meaningful class names in CSS and rgb(...) colors.

    .fallback {
        background-color: rgb(118, 174, 29)
    }

    .number {
        color: rgb(59, 112, 226)
    }
    ...

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:85
> +WebInspector.TypeTokenView.ColorForType = {
> +    "String": WebInspector.TypeTokenView.Colors.red,
> +    "Function": WebInspector.TypeTokenView.Colors.purple, 
> +    "Number": WebInspector.TypeTokenView.Colors.blue,
> +    "Integer": WebInspector.TypeTokenView.Colors.blue,
> +    "Undefined": WebInspector.TypeTokenView.Colors.gray,
> +    "Null": WebInspector.TypeTokenView.Colors.gray,
> +    "(?)": WebInspector.TypeTokenView.Colors.gray,
> +    "Boolean": WebInspector.TypeTokenView.Colors.purple,
> +    "(many)": WebInspector.TypeTokenView.Colors.orange
> +};

NOTE: This is the only place in the inspector where we have a list matching what the backend produces. We may want a comment here saying this matches the types produced by JavaScriptCore.

And instead of referencing color values here, these would be class names.

    WebInspector.TypeTokenView.ClassForType = {
        "String": "string",
        "Function": "function",
        "Number": "number",
        "Integer": "number",
        ...
    }

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:99
> +        this.element.style.backgroundColor = WebInspector.TypeTokenView.ColorForType[hashString] || WebInspector.TypeTokenView.Colors.green;

And this would be setting the element's class.

    var className = WebInspector.TypeTokenView.ClassForType[hashString] || "fallback";
    this.element.classList.add(className);

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:106
> +        var self = this;

Style: You could avoid self = this with bind.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:109
> +        this.element.onmouseover = function() 

Style: Where possible we use addEventListener.

    this.element.addEventListener("mouseover", function(event) {
        ...
    });

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:121
> +                timeoutID = setTimeout(showPopoverAfterDelay, 350);

Might be nice to make this a constant. Most of our timeouts are.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:124
> +        this.element.onmouseout = function() 

Ditto. RE: addEventListener.

[Saam Barati]

Comment on attachment 236719 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:127
>> +
> 
> Maybe we should add a few lines of padding to the visibleRange. Would that improve the UI while scrolling?

CodeMirror already does this padding for us. Maybe we can add more padding on top of that, but I don't think it's necessary.

>> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:183
>> +        var start = null; 
> 
> Declare "var offset" here. Otherwise you are leaking "offset" into the global scope!

It's passed in as a parameter to the function.

>> Source/WebInspectorUI/UserInterface/Images/NavigationItemTypes.svg:3
>> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 15 15">
> 
> The other button is 13x13 this is 15x15, seems  bit wide. You may be able to adjust appropriately and make a thinner T.

Yeah I agree.

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:112
>> +            this._typeTokenAnnotator.resume();
> 
> How about start / stop instead of resume / pause?

I personally like resume/pause more than start/stop because to me, it implies I'm not getting rid of data, but just pausing the UI, where start/stop to me imply that some backing data is being destroyed, which isn't the case.

>> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:86
>> +        return -1;
> 
> Why the indexOf proto check? What would be an example of two strings being compared in here?
> 
> Likewise, I think we could write a String.prototype.endsWith in Utilities.js that may be more efficient than indexOf if this really is an ends with check.

Actually. We don't need this at all. We're never passed an __proto__ property, unless a user specifies that one exists, and even then, I think it would just be converted in the passed in Object from JSC to prototypeStructure.

>> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:99
>> +        this.element.style.backgroundColor = WebInspector.TypeTokenView.ColorForType[hashString] || WebInspector.TypeTokenView.Colors.green;
> 
> And this would be setting the element's class.
> 
>     var className = WebInspector.TypeTokenView.ClassForType[hashString] || "fallback";
>     this.element.classList.add(className);

Right. The annoying thing here, though, is that we'd have to make sure to always remove the old class too.

[Saam Barati]

> > Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:246
> > +            if (isSingleLineComment && (sourceString[offset] === "\n" || (sourceString[offset] === "\r" && sourceString[offset + 1] === "\n"))) {
> > +                if (sourceString[offset] === "\r")
> > +                    offset++;
> > +                isSingleLineComment = false;
> > +                shouldIgnore = false;
> 
> I think this may be slightly incorrect but could be simplified.
> 
> Looking at the ES5 Grammar:
> <http://www.ecma-international.org/ecma-262/5.1/#sec-7.3>
> <http://www.ecma-international.org/ecma-262/5.1/#sec-7.4>
> 
> >    SingleLineComment ::
> >        // SingleLineCommentChars?
> >
> >    SingleLineCommentChars ::
> >        SingleLineCommentChar SingleLineCommentChars?
> >
> >    SingleLineCommentChar ::
> >        SourceCharacter but not LineTerminator
> >
> >    LineTerminator ::
> >        <LF>
> >        <CR>
> >        <LS>
> >        <PS>
> 
> >    Table 3 — Line Terminator Characters
> >    \u000A	Line Feed         	<LF>
> >    \u000D	Carriage Return    	<CR>
> >    \u2028	Line separator  	<LS>
> >    \u2029	Paragraph separator	<PS>
> 
> So instead of looking for <LF> || <CR><LF> we should just look for <LF> || <CR> || <LS> || <PS> and move on.
> 
>     function isLineTerminator(c) {
>         return c === "\n" || c === "\r" || c === "\u2028" || c === "\u2028";
>     }
> 
>     if (isSingleLineComment && isLineTerminator(sourceString[offset])) {
>         isSingleLineComment = false;
>         shouldIgnore = false;
>     } ...
> 
> You don't need to special case anything after the terminator since it will just naturally not be ")".

Yeah, this is much nicer. Thanks for looking into the spec.

[Jonathan Wells]

Comment on attachment 236719 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:77
> +    "Function": WebInspector.TypeTokenView.Colors.purple, 

Beware of trailing whitespace. There is much of it in this patch.

[Saam Barati]

(In reply to comment #16)

> Actually. We don't need this at all. We're never passed an __proto__ property, unless a user specifies that one exists, and even then, I think it would just be converted in the passed in Object from JSC to prototypeStructure.

Scratch that, __proto__ will show up in the properties for Object

[Timothy Hatcher]

Those color names should be Title-case. And any other named enum like it.

[Saam Barati]

Created attachment 236942 [details]
patch

Another WIP patch, but this should be closer to what will land

[Saam Barati]

Created attachment 236951 [details]
patch

Fixed a bug from previous patch, and more work for getting closer to landing the patch.

[Joseph Pecoraro]

Comment on attachment 236951 [details]
patch

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

This patch looks great. One more with a ChangeLog should probably be good enough to land! Are there any remaining issues you are aware of?

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:67
> +    this._showTypesButtonNavigationItem = new WebInspector.ActivateButtonNavigationItem("show-types", toolTipTypes, 
> +        activatedToolTipTypes, "Images/NavigationItemTypes.svg", showTypesImageSize, showTypesImageSize);

Style: One line.

Also, still missing this in TextContentView.js.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1519
> +        // Make sure JavaScriptCore supports type profiling.

Nit: This comment is unnecessary. And it isn't so much "JavaScriptCore" as whatever backend the inspector is talking to.

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:673
> +    visibleRangeStartOffset: function()
> +    {
> +        var startOffset = null;
> +        var visibleRange = this._codeMirror.getViewport();
> +
> +        if (this._formatterSourceMap)
> +            startOffset = this._formatterSourceMap.formattedToOriginalOffset(Math.max(visibleRange.from - 1, 0), 0);
> +        else
> +            startOffset = this._codeMirror.getDoc().indexFromPos({line: visibleRange.from, ch: 0});
> +
> +        return startOffset;
> +    },
> +
> +    visibleRangeEndOffset: function()
> +    {
> +        var endOffset = null;
> +        var visibleRange = this._codeMirror.getViewport();
> +
> +        if (this._formatterSourceMap)
> +            endOffset = this._formatterSourceMap.formattedToOriginalOffset(visibleRange.to - 1, 0);
> +        else
> +            endOffset = this._codeMirror.getDoc().indexFromPos({line: visibleRange.to, ch: 0});
> +
> +        return endOffset;
> +    },

Seems like we could replace this with one function that gets both the start and end offset at once to reduce the duplicated logic.

    visibleRangeOffsets: function()
    {
        ...
        return {startOffset: startOffset, endOffset: endOffset};
    }

The user could use destructuring assignment when calling this:

    var {startOffset, endOffset} = this._sourceCodeTextEditor.visibleRangeOffsets();

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:717
> +        this._codeMirrorScrollHandler = handler;

I don't think this instance variable is needed. I think the caller should pass the handler in both add and remove. Similar to addEventListener.

Otherwise it looks like this API can only be used to register 1 handler, but it could be made to support multiple since under the hood this is just using addEventListener.

    var on = CodeMirror.on = function(emitter, type, f) {
        if (emitter.addEventListener)
            emitter.addEventListener(type, f, false);
        ...
    }

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:59
> +    this._showTypesButtonNavigationItem = new WebInspector.ActivateButtonNavigationItem("show-types", toolTipTypes, 
> +        activatedToolTipTypes, "Images/NavigationItemTypes.svg", showTypesImageSize, showTypesImageSize);

Style: One line.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.css:47
> +    /* Purple */

Nit: All these comments are unnecessary.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.css:73
> +    /* Green */
> +    background-color: rgb(237, 132, 0);

For instance this comment is incorrect. The color is clearly is not green with values RED:237 GREEN:132 BLUE:0.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:44
> +
> +

Style: Duplicate blank line.

> Source/WebInspectorUI/UserInterface/Views/TypeTokenView.js:96
> +        var hashString = title[title.length - 1] === "?" ? title.slice(0, title.length - 1) : title;

Looking at this code, I wonder if we should make the "displayTypeName" a frontend decision and not a backend one.

Currently, the Runtime.TypeDescription protocol type includes the displayTypeName. The backend has determined we should show "String?", "(many)", or "(?)" etc.

If we wanted to change this in the future, older backends would either not get the new convention/style or the frontend would need to include a translation.

Maybe we should send the equivalent of "TypeSet" bits as a "typeSet" property and optional "commonConstructor". But lets tackle that in the future (a separate patch) if we want to go down that road.

[Jonathan Wells]

Comment on attachment 236951 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:186
> +        var bookmark = this._sourceCodeTextEditor.setUniqueBookmark(tokenPosition, typeToken.element);

Is bookmark what we want to call this? I know this is CodeMirror's name for it, but we might just refer to them as tokens or source tokens. I also think the setUniqueBookmark function in our API could just be addToken, or addBookmark, or addWhatever.

[Saam Barati]

(In reply to comment #24)
> (From update of attachment 236951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236951&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:186
> > +        var bookmark = this._sourceCodeTextEditor.setUniqueBookmark(tokenPosition, typeToken.element);
> 
> Is bookmark what we want to call this? I know this is CodeMirror's name for it, but we might just refer to them as tokens or source tokens. I also think the setUniqueBookmark function in our API could just be addToken, or addBookmark, or addWhatever.

Joe and I discussed this, and I think it's nice, in some ways, to stay consistent with CodeMirror, even though I think "bookmark" is a horrible name for this thing. I think it allows us to more easily reason about this code and other code that uses CodeMirror directly. That said, I'm not entirely opposed to changing the name of this away from "bookmark" to something like "token" or "inlineView" or something, but I think if we decide to do that, we should rename the other places that we use setUniqueBookmark on CodeMirror directly.

[Joseph Pecoraro]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 236951 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=236951&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:186
> > > +        var bookmark = this._sourceCodeTextEditor.setUniqueBookmark(tokenPosition, typeToken.element);
> > 
> > Is bookmark what we want to call this? [snip]
> 
> [snip]That said, I'm not entirely opposed to changing the name of this away from "bookmark" to something like "token" or "inlineView" or something [snip]

"setInlineWidget"?

[Saam Barati]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (From update of attachment 236951 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=236951&action=review
> > > 
> > > > Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:186
> > > > +        var bookmark = this._sourceCodeTextEditor.setUniqueBookmark(tokenPosition, typeToken.element);
> > > 
> > > Is bookmark what we want to call this? [snip]
> > 
> > [snip]That said, I'm not entirely opposed to changing the name of this away from "bookmark" to something like "token" or "inlineView" or something [snip]
> 
> "setInlineWidget"?

I like that. I can open a patch to have subclasses of TextEditor use its API rather than _codeMirror directly.

[Saam Barati]

Created attachment 237185 [details]
patch

Jono and Joe's considerations taken into account.

[Saam Barati]

Created attachment 237186 [details]
patch

grr, above patch left in an assert removal from CodeMirrorTokenTrackingController

[Joseph Pecoraro]

Comment on attachment 237186 [details]
patch

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

r=me. A few questions worth considering.

> Source/WebInspectorUI/ChangeLog:26
> +        This class is responsible for annotaing producing the inline

Typo: "annotaing" => "annotating"

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:63
> +    var showTypesImageSize = (WebInspector.Platform.isLegacyMacOS ? 15 : 16);

Style: No need for the ()s.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:155
> +        if (this._typeTokenScrollHandler)
> +            this._disableScrollEventsForTypeTokenAnnotator();

Is this necessary? I would expect that after "close" is called everything will get garbage collected.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1278
> -        this._popover = this._popover || new WebInspector.Popover(this);
> +        content.classList.add(WebInspector.SourceCodeTextEditor.PopoverDebuggerContentStyleClassName);
> +
> +        if (this._popover)
> +            this._dismissPopover();
> +        this._popover = new WebInspector.Popover(this);

Is the dismiss and new-assignment needed? Or can we stick with the old code and just reuse the popover?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1544
> +        this._typeTokenScrollHandler = this._makeTypeTokenScrollEventHandler();

Nit: console.assert(!this._typeTokenScrollHandler);

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1550
> +        this.removeScrollHandler(this._typeTokenScrollHandler);

Nit: console.assert(this._typeTokenScrollHandler);

[Saam Barati]

Created attachment 237407 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/WebInspectorUI/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/ChangeLog:128:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 17 files


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

[Joseph Pecoraro]

Comment on attachment 237407 [details]
patch

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

r=me

> Source/WebInspectorUI/ChangeLog:129
> +		
> +

Nit: Double empty line. And some tabs in the ChangeLog apparently.

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:-196
>      _startTracking: function()
>      {
> -        console.assert(!this._tracking);

This landed separately right?

> Source/WebInspectorUI/UserInterface/Views/TextContentView.js:58
> +    this._showTypesButtonNavigationItem.addEventListener(WebInspector.ButtonNavigationItem.Event.Clicked, function() {}, this);

If you are registering an empty event listener, just don't register an event listener!

[Saam Barati]

landed in: http://trac.webkit.org/changeset/173180