Web Inspector: Add snippets model.

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

[Vsevolod Vlasov]

Add snippets model.

[Vsevolod Vlasov]

Created attachment 131389 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 131389 [details]
Patch

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

> Source/WebCore/inspector/front-end/SnippetsModel.js:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

> Source/WebCore/inspector/front-end/SnippetsModel.js:35
> +    this._snippetsCount = 0;

It is unused, remove it.

> Source/WebCore/inspector/front-end/SnippetsModel.js:63
> +            savedSnippet.id = this._snippets[i].id;

Can we generate snippet id on load?

> Source/WebCore/inspector/front-end/SnippetsModel.js:102
> +        var snippet = new WebInspector.Snippet(this, this._snippets.length);

Using this._snippets.length as snippet id is fragile. Please use a separate counter for it as discussed offline.

> Source/WebCore/inspector/front-end/SnippetsModel.js:139
> +    snippetForId: function(snippetId)

This will break if one of the snippets has been deleted. Please fix.

[Vsevolod Vlasov]

Created attachment 131579 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 131579 [details]
Patch

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

> Source/WebCore/inspector/compile-front-end.py:87
> +            "SnippetsModel.js",

I guess it's a good idea to maintain the alphabetical order (believe it should be there but it's not.)

> Source/WebCore/inspector/front-end/SnippetsModel.js:35
> +    this._snippets = [];

You have duplicate initialization (here and in this._loadSettings()). I suggest that you remove this one and only declare the field:

/**
 * @type {Array.<WebInspector.Snippet>}
 */
 this._snippets;

> Source/WebCore/inspector/front-end/SnippetsModel.js:63
> +            savedSnippet.id = this._snippets[i].id;

You could have Snippet.prototype.toObject() or something similar, since you know they will be stored as plain objects.

> Source/WebCore/inspector/front-end/SnippetsModel.js:77
> +            var snippet = new WebInspector.Snippet(this, savedSnippets[i].id, savedSnippets[i].name, savedSnippets[i].content);

Same thing: something like fromObject(object) (or parseObject) would make sense. See WebInspector.CSSRule.parsePayload() (and other instances in the same file.)

> Source/WebCore/inspector/front-end/SnippetsModel.js:85
> +    deleteSnippet: function(snippet)

Do you need the entire snippet as an argument? An ID seems to be sufficient here.

You can also return a boolean to indicate whether or not a snippet has actually been deleted (i.e. was present in the model.)

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

optional: This method might accept optional name and content of the snippet

> Source/WebCore/inspector/front-end/SnippetsModel.js:106
> +        this._saveSettings();

Should this live inside this._snippetAdded()? Meaning, do we always save the state of the model once its contents have changed?

> Source/WebCore/inspector/front-end/SnippetsModel.js:131
> +    _snippetRenamed: function(snippet)

You might want to have the old name of the snippet here as well (to dispatch in the event), but be reasonable and don't write provisional code :)

> Source/WebCore/inspector/front-end/SnippetsModel.js:139
> +     * @return {WebInspector.Snippet}

@return {WebInspector.Snippet|undefined}

> Source/WebCore/inspector/front-end/SnippetsModel.js:182
> +    set name(name)

Don't know if it's important or not, but most of our setters accept the "x" argument (presumably, to distinguish the new value from anything else - here you have three "name"-related entities.)

> Source/WebCore/inspector/front-end/SnippetsModel.js:184
> +        this._name = name;

Check for the actual name change before doing anything (especially notifying the model.)

> Source/WebCore/inspector/front-end/SnippetsModel.js:198
> +        this._content = content;

ditto

> LayoutTests/inspector/debugger/snippets-model.html:5
> +function test()

This does not cover much of the model functionality. E.g., event dispatching, snippetForId() etc.

> LayoutTests/inspector/debugger/snippets-model.html:27
> +    // FIXME: Remove once snippets are taked out of experiments.

taken

> LayoutTests/inspector/debugger/snippets-model.html:29
> +        WebInspector.snippetsModel = new WebInspector.SnippetsModel();

Actually, you could just create your own SnippetsModel instance without relying on WebInspector.snippetsModel.

[Vsevolod Vlasov]

Created attachment 131594 [details]
Patch

[Vsevolod Vlasov]

Committed r110576: <http://trac.webkit.org/changeset/110576>