[webkit-reviews] review granted: [Bug 105764] Web Inspector: Refactor SourceMapParser to enable range-based sourcemap : [Attachment 180786] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has granted Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 105764: Web Inspector: Refactor SourceMapParser to enable range-based
sourcemap
https://bugs.webkit.org/show_bug.cgi?id=105764

Attachment 180786: Patch
https://bugs.webkit.org/attachment.cgi?id=180786&action=review

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=180786&action=review


> Source/WebCore/inspector/front-end/SourceMap.js:60
> +	   var result = [];

return Object.keys(this._sources);

> Source/WebCore/inspector/front-end/SourceMap.js:311
> +	       if (map.sourceRoot)

We should do this only once in _parseMap

> Source/WebCore/inspector/front-end/SourceMap.js:340
> +	       strippedMappings.push(this._mappings[i]);

this._mappings.filter(filterOut...)

> Source/WebCore/inspector/front-end/SourceMap.js:367
> +		   if (nextEntry && nextEntry[0] === lineNumber && nextEntry[1]
=== columnNumber)

I would replace nextEntry check with index + 1 < this._mappings.length check
before reading from the array.
also you can reuse comparator here.

> Source/WebCore/inspector/front-end/SourceMap.js:409
> +WebInspector.SourceRange = function(url, startLine, startColumn, endLine,
endColumn)

I would call it WebInspector.RangeBasedSourceMap.SourceRange instead