Bug 80890 – Web Inspector: Introduce SnippetsScriptMapping.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

Patch to follow.
Tests for this mapping will be added once snippet could be evaluated.

[Vsevolod Vlasov]

Created attachment 131421 [details]
Patch

[Pavel Podivilov]

Comment on attachment 131421 [details]
Patch

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

> Source/WebCore/inspector/front-end/SnippetsModel.js:227
> +    WebInspector.snippetsModel.addEventListener(WebInspector.SnippetsModel.EventTypes.SnippetDeleted, this._snippetDeleted.bind(this));

SnippetRemoved would be more consistent with other code.

> Source/WebCore/inspector/front-end/SnippetsModel.js:237
> +        var snippet = this._snippetForScriptId[rawLocation.scriptId];

Could you only store the last script in _snippetForScriptId and delete stale scripts (_snippetForLastScriptId)? There should be no need to access snippet for stale scripts.

> Source/WebCore/inspector/front-end/SnippetsModel.js:242
> +        if (!uiSourceCode)

It's guaranteed that addScript was already called for this script, so uiSourceCode should always exist here.

> Source/WebCore/inspector/front-end/SnippetsModel.js:261
> +        if (!lastScript)

There's no script for this UISourceCode. How could there be a lastScript for the snippet?

> Source/WebCore/inspector/front-end/SnippetsModel.js:262
> +            return null;

Please make sure all clients will check for null.

> Source/WebCore/inspector/front-end/SnippetsModel.js:274
> +            result.push(snippet.uiSourceCode);

Wouldn't there be duplicates?

> Source/WebCore/inspector/front-end/SnippetsModel.js:289
> +        var uiSourceCode = new WebInspector.UISourceCode(script.sourceURL, script.sourceURL, new WebInspector.ScriptContentProvider(script));

You should probably bind this script to existing snippet.uiSourceCode instead of creating a new one.

> Source/WebCore/inspector/front-end/SnippetsModel.js:294
> +        if (!snippet)

Is it possible? Maybe preventing the snippet from being deleted while it's evaluated would make the code easier to understand.

> Source/WebCore/inspector/front-end/SnippetsModel.js:310
> +        uiSourceCode.isSnippet = true;

uiSourceCode.isEditable = true?

> Source/WebCore/inspector/front-end/SnippetsModel.js:350
> +        this._lastScriptForSnippet = new Map();

Clear other maps as well?

[Vsevolod Vlasov]

Comment on attachment 131421 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SnippetsModel.js:227
>> +    WebInspector.snippetsModel.addEventListener(WebInspector.SnippetsModel.EventTypes.SnippetDeleted, this._snippetDeleted.bind(this));
> 
> SnippetRemoved would be more consistent with other code.

Done.

>> Source/WebCore/inspector/front-end/SnippetsModel.js:237
>> +        var snippet = this._snippetForScriptId[rawLocation.scriptId];
> 
> Could you only store the last script in _snippetForScriptId and delete stale scripts (_snippetForLastScriptId)? There should be no need to access snippet for stale scripts.

Done.

>> Source/WebCore/inspector/front-end/SnippetsModel.js:242
>> +        if (!uiSourceCode)
> 
> It's guaranteed that addScript was already called for this script, so uiSourceCode should always exist here.

Done.

>> Source/WebCore/inspector/front-end/SnippetsModel.js:261
>> +        if (!lastScript)
> 
> There's no script for this UISourceCode. How could there be a lastScript for the snippet?

I have removed _lastScriptForSnippet map at all and rewritten this piece of code as we discussed offline.

>> Source/WebCore/inspector/front-end/SnippetsModel.js:289
>> +        var uiSourceCode = new WebInspector.UISourceCode(script.sourceURL, script.sourceURL, new WebInspector.ScriptContentProvider(script));
> 
> You should probably bind this script to existing snippet.uiSourceCode instead of creating a new one.

Done.

>> Source/WebCore/inspector/front-end/SnippetsModel.js:310
>> +        uiSourceCode.isSnippet = true;
> 
> uiSourceCode.isEditable = true?

Done.

>> Source/WebCore/inspector/front-end/SnippetsModel.js:350
>> +        this._lastScriptForSnippet = new Map();
> 
> Clear other maps as well?

No need to clear snippet<->uiSourceCode maps, since snippet are not removed upon navigation.

[Vsevolod Vlasov]

Created attachment 131614 [details]
Patch

[Vsevolod Vlasov]

Created attachment 131669 [details]
Patch

[Vsevolod Vlasov]

Created attachment 131686 [details]
Patch

[Pavel Podivilov]

Comment on attachment 131614 [details]
Patch

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

looks good!

> Source/WebCore/inspector/front-end/ScriptMapping.js:178
> +            if (WebInspector.snippetsModel.snippetForSourceURL(script.sourceURL))

There could be no snippet at this moment. Please check that script.sourceURL starts with "snippet://" instead.

> Source/WebCore/inspector/front-end/SnippetsModel.js:391
> +        this._uiSourceCodeForScriptId = {};

Do you need to dispatch removed events here?

[Pavel Podivilov]

Comment on attachment 131686 [details]
Patch

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

> Source/WebCore/inspector/front-end/SnippetsModel.js:307
> +    _uiSourceCodesForScripts: function()

Not uiSourceCodesForScripts actually, maybe releasedUISourceCodes?

[Vsevolod Vlasov]

Created attachment 131808 [details]
Patch

[Vsevolod Vlasov]

Committed r110681: <http://trac.webkit.org/changeset/110681>