[webkit-reviews] review denied: [Bug 19207] Inspector should remember the size of sidebars set by the user : [Attachment 60691] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 19207: Inspector should remember the size of sidebars set by the user
https://bugs.webkit.org/show_bug.cgi?id=19207

Attachment 60691: Patch
https://bugs.webkit.org/attachment.cgi?id=60691&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
WebCore/inspector/front-end/Panel.js:35
 +	this._name = name;
Nit: it is easy for ancestors to hide it. I'd rename it to something more
specific (_panelName)

WebCore/inspector/front-end/Panel.js:42
 +	get toolbarItemClass()
No need to expose it, just use the name in get toolbarItem below.

WebCore/inspector/front-end/Panel.js:327
 +	get _sidebarWidthSettingName()
I'd not use getters for private fields (use function instead).

WebCore/inspector/front-end/Panel.js:392
 +	    WebInspector.log("saving " + this._sidebarWidthSettingName + " = "
+ this.sidebarElement.offsetWidth);
Remove logging.

WebCore/inspector/front-end/Settings.js:61
 +     
WebInspector.applicationSettings._installSetting("elementsSidebarWidth",
"elements-sidebar-width", undefined);
I think panel constructors should register theses instead - otherwise hack. r-
for this. I'd make installSetting public and allow its invocation before the
load takes place. I'd do the same thing as now + initialize store with default
values. Then I'd populate existing settings from JSON.parse(settingsString) in
_load.