Bug 139426 – Web Inspector: Abstract common functions from TypeTokenAnnotator into a parent class and introduce an AnnotatorManager

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

[Saam Barati]

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.

[Radar WebKit Bug Importer]

<rdar://problem/19186398>

[Saam Barati]

Created attachment 242948 [details]
path

[Saam Barati]

Created attachment 242969 [details]
patch

Fix whitespace alteration in previous patch.

[Joseph Pecoraro]

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

[Saam Barati]

Created attachment 243566 [details]
patch

Made recommended changes.

[Saam Barati]

Created attachment 243568 [details]
patch

Move TextEditor argument back to SourceCodeTextEditor.

[Timothy Hatcher]

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

[Saam Barati]

Created attachment 244145 [details]
patch

[WebKit Commit Bot]

Comment on attachment 244145 [details]
patch

Clearing flags on attachment: 244145

Committed r178030: <http://trac.webkit.org/changeset/178030>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 244145 [details]
patch

Nit: 2015