[webkit-reviews] review denied: [Bug 107552] Web Inspector: Show element style panes in a tabbed pane when they are below the DOM tree pane. : [Attachment 186803] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vladislav Kaznacheev
<kaznacheev at chromium.org>'s request for review:
Bug 107552: Web Inspector: Show element style panes in a tabbed pane when they
are below the DOM tree pane.
https://bugs.webkit.org/show_bug.cgi?id=107552

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186803&action=review


> Source/WebCore/inspector/front-end/ElementsPanel.js:99
> +    this.sidebarPaneView = new WebInspector.SidebarPaneGroupFlippable();

Should it accept a setting name for persistence?

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1112
> +	   this.sidebarPaneView.populateContextMenu(contextMenu);

This will add it into all the panels. You should also check if the current
panel is scripts panel -> then it'll appear on all UISourceCodes on that panel.


> Source/WebCore/inspector/front-end/Settings.js:-218
> -    this.elementsPanelSingleColumn =
this._createExperiment("elementsPanelSingleColumn", "Split Elements sidebar
horizontally when it is too narrow");

We still do want some setting to persist user preference.

> Source/WebCore/inspector/front-end/SidebarPane.js:143
> +		   if (this._panes[i]._expandPending)

I don't think you need this async complexity. Last expanded is the one that was
last to successfully expand.

> Source/WebCore/inspector/front-end/SidebarPane.js:172
> +	   pane.titleElement.removeSelf();

What is happening here?

> Source/WebCore/inspector/front-end/SidebarPane.js:260
> +	   for (var i = 0; i < this._panes.length; i++) {

No need for {} around single line block. Why do we do this lazily?

> Source/WebCore/inspector/front-end/SidebarPane.js:300
> +	   this._panes[id].prepareContent(function() {});

It is a shame you need this subtle method to have pane actually render itself.
It would be great if it was doing it upon its own wasShown + had a way to warm
up upon prepareContent

> Source/WebCore/inspector/front-end/SidebarPane.js:335
> +		   this._stackView._panes[selected].expand();

You should call the stackView's expand method here like you do with the tabs
below. expand() is just a convenience method here we might want to get rid of
in the future.

> Source/WebCore/inspector/front-end/SidebarPane.js:345
> +    addPane: function(pane)

Please annotate.

> Source/WebCore/inspector/front-end/SidebarPane.js:347
> +	   this._stackView.addPane(pane);

I would expect you to only add it into the current container.

> Source/WebCore/inspector/front-end/SidebarPane.js:359
> +WebInspector.SidebarPaneGroupFlippable = function()

What is the point of having non-flippable one? I think you should just merge
them.

> Source/WebCore/inspector/front-end/SidebarPane.js:374
> +	   var splitDirectionSettingName = panel.name +
"PanelSplitHorizontally";

Aha!

> Source/WebCore/inspector/front-end/SidebarPane.js:385
> +    populateContextMenu: function(contextMenu)

Please annotate

> Source/WebCore/inspector/front-end/SidebarPane.js:394
> +    _contextMenuEventFired: function(event)

ditto

> Source/WebCore/inspector/front-end/inspector.css:1838
> +.pane > * {

This is dangerous. Please rename .pane to something safe.

> Source/WebCore/inspector/front-end/inspector.css:1910
> +.pane {

This looks like a very dangerous selector.