[webkit-reviews] review denied: [Bug 105739] Web Inspector: [Resources] Make grid columns set configurable. : [Attachment 180797] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Eugene Klyuchnikov
<eustas at chromium.org>'s request for review:
Bug 105739: Web Inspector: [Resources] Make grid columns set configurable.
https://bugs.webkit.org/show_bug.cgi?id=105739

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

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


> Source/WebCore/inspector/front-end/NetworkPanel.js:158
> +	   var hiddenColumns =
WebInspector.settings.hiddenNetworkLogColumns.get();

This setting should be defined in the panel.

> Source/WebCore/inspector/front-end/NetworkPanel.js:161
> +	       if (this._detailedViewColumnsSet[hiddenColumns[i]])

At some point, we will have more columns and only some of them will be visible
by default. So I don't think you need this._detailedViewColumnsSet preset. It
is jus that the setting should have such value by default.

> Source/WebCore/inspector/front-end/Settings.js:118
> +    this.hiddenNetworkLogColumns =
this.createSetting("hiddenNetworkLogColumns", []);

Move to NetworkPanel.js and have a default value for it.

> Source/WebCore/inspector/front-end/networkLogView.css:561
> +#network-container .hide-method-column .method-column,

Now this stops being scaleable. Adding a column would require adding to this
stupid rule... This (and other styles where we enumerate column names) could be
addressed in another patch though.