Bug 80863 - Web Inspector: Add snippets model.
Summary: Web Inspector: Add snippets model.
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
Depends on:
Blocks: 75094
  Show dependency treegraph
Reported: 2012-03-12 11:43 PDT by Vsevolod Vlasov
Modified: 2012-03-13 09:38 PDT (History)
11 users (show)

See Also:

Patch (18.11 KB, patch)
2012-03-12 13:09 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (17.36 KB, patch)
2012-03-13 03:09 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (16.87 KB, patch)
2012-03-13 05:18 PDT, Vsevolod Vlasov
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2012-03-12 11:43:54 PDT
Add snippets model.
Comment 1 Vsevolod Vlasov 2012-03-12 13:09:58 PDT
Created attachment 131389 [details]
Comment 2 Yury Semikhatsky 2012-03-13 01:56:32 PDT
Comment on attachment 131389 [details]

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.


> 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.
Comment 3 Vsevolod Vlasov 2012-03-13 03:09:07 PDT
Created attachment 131579 [details]
Comment 4 Alexander Pavlov (apavlov) 2012-03-13 03:39:49 PDT
Comment on attachment 131579 [details]

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>}

> 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;


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


> 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.
Comment 5 Vsevolod Vlasov 2012-03-13 05:18:12 PDT
Created attachment 131594 [details]
Comment 6 Vsevolod Vlasov 2012-03-13 09:38:21 PDT
Committed r110576: <http://trac.webkit.org/changeset/110576>