Bug 174741 – Web Inspector: Styles: Add a switch for Spreadsheet model style editor to experimental settings

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

It will show a blank page for "Style - Rules" at first.

[Radar WebKit Bug Importer]

<rdar://problem/33467954>

[Nikita Vasilyev]

Created attachment 316159 [details]
Patch

[Nikita Vasilyev]

Created attachment 316162 [details]
Patch

[Blaze Burg]

Comment on attachment 316162 [details]
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).

[Nikita Vasilyev]

I think it's acceptable to reload Web Inspector for experimental features to be enabled.
Turning an experimental feature on and off shouldn't be a very common action.

Chrome shows "WARNING: These experiments could be dangerous and may require restart.".
I'm not sure about this warning, but it would be nice to show "Reload" button below the experimental checkboxes once an experimental setting has been changed.

[Blaze Burg]

(In reply to Nikita Vasilyev from comment #5)
> I think it's acceptable to reload Web Inspector for experimental features to
> be enabled.
> Turning an experimental feature on and off shouldn't be a very common action.
> 
> Chrome shows "WARNING: These experiments could be dangerous and may require
> restart.".
> I'm not sure about this warning, but it would be nice to show "Reload"
> button below the experimental checkboxes once an experimental setting has
> been changed.

That approach fine, but the current UI is mysterious and not something I want to get further copied. Maybe you can add an "Apply Settings and Reload" button to the settings screen that shows up when this setting changes.

[Matt Baker]

(In reply to Nikita Vasilyev from comment #5)
> I think it's acceptable to reload Web Inspector for experimental features to
> be enabled.
> Turning an experimental feature on and off shouldn't be a very common action.

Agree.

> Chrome shows "WARNING: These experiments could be dangerous and may require
> restart.".
> I'm not sure about this warning, but it would be nice to show "Reload"
> button below the experimental checkboxes once an experimental setting has
> been changed.

A message explaining that a restart may be required after enabling features seems like a good idea. I don't think it needs to be as dire sounding as Chrome's warning.

[Nikita Vasilyev]

Created attachment 316390 [details]
Patch

[Nikita Vasilyev]

Created attachment 316391 [details]
[Animated GIF] Reopen Web Inspector button in Settings

[Blaze Burg]

Comment on attachment 316390 [details]
Patch

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

r=me please adjust the label text

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:275
> +        reopenInspectorButton.textContent = WebInspector.unlocalizedString("Reopen Web Inspector");

Should be "Reload Web Inspector". It never closes.

[Nikita Vasilyev]

Created attachment 316394 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316394 [details]
Patch

Clearing flags on attachment: 316394

Committed r219888: <http://trac.webkit.org/changeset/219888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.