[webkit-reviews] review denied: [Bug 174741] Web Inspector: Styles: Add a switch for Spreadsheet model style editor to experimental settings : [Attachment 316162] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Nikita Vasilyev <nvasilyev at apple.com>'s
request for review:
Bug 174741: Web Inspector: Styles: Add a switch for Spreadsheet model style
editor to experimental settings
https://bugs.webkit.org/show_bug.cgi?id=174741

Attachment 316162: Patch

https://bugs.webkit.org/attachment.cgi?id=316162&action=review




--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 316162
  --> https://bugs.webkit.org/attachment.cgi?id=316162
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316162&action=review

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:36
> +	   if (WebInspector.settings.experimentalSpreadsheetStyleEditor.value)

You need to listen for the setting change event and swap which instance is
being used. Otherwise you'd need to reopen the inspector to test whether it
works. The current setup within this class doesn't allow for that to happen
easily.

 It's probably easiest to store both panels in instance variables and convert
this._panels into a getter. The getter can dynamically decide which of the rule
sidebars to show. You'll probably need to invalidate the view hierarchy or
something to force sidebar visibility to get synced correctly.

There are various other places that access the rules sidebar directly, so you
could turn this._rulesStyleDetailsPanel into a getter as well so that clients
are oblivious to which implementation is being used. As for the navigationInfo,
if you use the same identifier for both panels then it should just work (as
long as it goes through the getter).