WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
80863
Web Inspector: Add snippets model.
https://bugs.webkit.org/show_bug.cgi?id=80863
Summary
Web Inspector: Add snippets model.
Vsevolod Vlasov
Reported
2012-03-12 11:43:54 PDT
Add snippets model.
Attachments
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Vsevolod Vlasov
Comment 1
2012-03-12 13:09:58 PDT
Created
attachment 131389
[details]
Patch
Yury Semikhatsky
Comment 2
2012-03-13 01:56:32 PDT
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
Comment 3
2012-03-13 03:09:07 PDT
Created
attachment 131579
[details]
Patch
Alexander Pavlov (apavlov)
Comment 4
2012-03-13 03:39:49 PDT
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
Comment 5
2012-03-13 05:18:12 PDT
Created
attachment 131594
[details]
Patch
Vsevolod Vlasov
Comment 6
2012-03-13 09:38:21 PDT
Committed
r110576
: <
http://trac.webkit.org/changeset/110576
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug