These changes are in preparation for the soon to be finished Basic Block Annotator. - This patch will create a parent class that will be shared by both TypeTokenAnnaotor and BasicBlockAnnotator. - This patch will also create an AnnotatorManager that is responsible for keeping track of a group annotators.
<rdar://problem/19186398>
Created attachment 242948 [details] path
Created attachment 242969 [details] patch Fix whitespace alteration in previous patch.
Comment on attachment 242969 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=242969&action=review I think this is good, but I want to see another patch with the changes mentioned here, and then I'll look more closely at the other details! > Source/WebInspectorUI/ChangeLog:15 > + This patch also introduces JavaScriptAnnotatorManager which controls a > + set of JavaScriptAnnotators and provides a single interface for > + controlling them. From the description it sounds like these objects have little that is JavaScript specific. The Annotators hold "SourceCode"TextEditors, it would be great to abstract this to be SourceCodes if we ever wanted to use something similar for CSS. > Source/WebInspectorUI/UserInterface/Controllers/JavaScriptAnnotator.js:31 > + console.assert(sourceCodeTextEditor && sourceCodeTextEditor instanceof WebInspector.SourceCodeTextEditor, sourceCodeTextEditor); Nit: The "sourceCodeTextEditor &&" part is implied by the second condition so it can be removed. > Source/WebInspectorUI/UserInterface/Controllers/JavaScriptAnnotator.js:32 > + console.assert(script, script); Nit: The second arguments to this asserts seems unnecessary, since it would almost certainly just be "null". That said, the Annotator doesn't need to hold the script/sourceCode object. The sourceCodeTextEditor already has it. No need for the argument / instance variable. > Source/WebInspectorUI/UserInterface/Controllers/JavaScriptAnnotatorManager.js:31 > + this._annotators = {}; Lets make this a "Map". <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map> this._annotators = new Map; this._annotators.set(key, annotator); return this._annotators.get(key) || null; > Source/WebInspectorUI/UserInterface/Controllers/JavaScriptAnnotatorManager.js:62 > + for (var annotator of this._getAnnotators()) If "this._annotators" is an array, then you can iterate it directly: // Key and value for (var [key, annotator] of this._annotators) { ... } // Just values (order or insertion I believe) for (var annotator of this._annotators.values()) { ... } Or, you can do the normal forEach. this._annotators.forEach(function(key, value) { ... }); > Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js:63 > this._script.requestScriptSyntaxTree(function(syntaxTree) { Now that this is a subclass we wouldn't want to access "this._script" directly. You could create: - Assert it is a script in the constructor: console.assert(this.sourceCodeTextEditor.sourceCode instanceof WebInspector.Script); - Add a convenience getter get script() { return this.sourceCodeTextEditor.sourceCode; } - And use "this.script" instead
Created attachment 243566 [details] patch Made recommended changes.
Created attachment 243568 [details] patch Move TextEditor argument back to SourceCodeTextEditor.
Comment on attachment 243568 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=243568&action=review > Source/WebInspectorUI/UserInterface/Controllers/AnnotatorManager.js:103 > + _getAnnotators: function() > + { > + return this._annotators.values(); > + } Not sure this adds any thing. this._getAnnotators() isn't much better than this._annotators.values().
Created attachment 244145 [details] patch
Comment on attachment 244145 [details] patch Clearing flags on attachment: 244145 Committed r178030: <http://trac.webkit.org/changeset/178030>
All reviewed patches have been landed. Closing bug.
Comment on attachment 244145 [details] patch Nit: 2015