[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
bugzilla-daemon at webkit.org
Tue Jul 6 23:56:38 PDT 2010
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.
More information about the webkit-reviews
mailing list