[webkit-reviews] review denied: [Bug 108183] Web Inspector: Create a container class for SidebarPane instances : [Attachment 185772] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 31 10:05:42 PST 2013
Pavel Feldman <pfeldman at chromium.org> has denied Vladislav Kaznacheev
<kaznacheev at chromium.org>'s request for review:
Bug 108183: Web Inspector: Create a container class for SidebarPane instances
https://bugs.webkit.org/show_bug.cgi?id=108183
Attachment 185772: Patch
https://bugs.webkit.org/attachment.cgi?id=185772&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=185772&action=review
Overall looks good. We'll need to go through the sidebars to check whether it
regressed anything UI-wise.
> Source/WebCore/ChangeLog:9
> + SidebarPane's are inserted into DOM lazily and can belong to more
than one container.
SidebarPanes are...
> Source/WebCore/inspector/front-end/ElementsPanel.js:89
> + this.sidebarPanes.styles.setShowCallback(this.updateStyles.bind(this));
Might be a good time to introduce WebInspector.View.Events.WasShown event.
Could be a part of separate change though.
> Source/WebCore/inspector/front-end/SidebarPane.js:38
> +// this.titleElement = document.createElement("div");
Remove this line.
> Source/WebCore/inspector/front-end/SidebarPane.js:39
> + this.titleElement = document.createDocumentFragment();
this._auxTitleFragment. Btw, what is the point of using fragment here?
> Source/WebCore/inspector/front-end/SidebarPane.js:43
> + this._expanded = false;
It is weird that we still have this._expanded.
> Source/WebCore/inspector/front-end/SidebarPane.js:91
> + setExpandCallback: function(callback)
Please annotate these
> Source/WebCore/inspector/front-end/SidebarPane.js:132
> + addPane: function(pane)
Here and below: please annotate.
> Source/WebCore/inspector/front-end/inspector.css:1788
> +.pane-title + .pane-title, .pane:not(.visible) + .pane-title,
.pane-title:first-of-type {
We'll need to test this thoroughly since there is a lot of minor features
associated with the titles.
More information about the webkit-reviews
mailing list