Bug 80863

Created attachment 131389 [details] Patch

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.

Created attachment 131579 [details] Patch

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.

Created attachment 131594 [details] Patch

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