Bug 174741 - Web Inspector: Styles: Add a switch for Spreadsheet model style editor to experimental settings
Summary: Web Inspector: Styles: Add a switch for Spreadsheet model style editor to exp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks: 145982
  Show dependency treegraph
 
Reported: 2017-07-21 19:01 PDT by Nikita Vasilyev
Modified: 2017-07-25 14:43 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.45 KB, patch)
2017-07-21 19:14 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (6.67 KB, patch)
2017-07-21 19:24 PDT, Nikita Vasilyev
bburg: review-
Details | Formatted Diff | Diff
Patch (12.09 KB, patch)
2017-07-25 14:13 PDT, Nikita Vasilyev
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
[Animated GIF] Reopen Web Inspector button in Settings (33.87 KB, image/gif)
2017-07-25 14:14 PDT, Nikita Vasilyev
no flags Details
Patch (12.08 KB, patch)
2017-07-25 14:27 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-07-21 19:01:59 PDT
It will show a blank page for "Style - Rules" at first.
Comment 1 Radar WebKit Bug Importer 2017-07-21 19:02:43 PDT
<rdar://problem/33467954>
Comment 2 Nikita Vasilyev 2017-07-21 19:14:40 PDT
Created attachment 316159 [details]
Patch
Comment 3 Nikita Vasilyev 2017-07-21 19:24:08 PDT
Created attachment 316162 [details]
Patch
Comment 4 BJ Burg 2017-07-24 09:40:32 PDT
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).
Comment 5 Nikita Vasilyev 2017-07-24 16:38:07 PDT
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.
Comment 6 BJ Burg 2017-07-24 17:12:51 PDT
(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.
Comment 7 Matt Baker 2017-07-24 17:14:31 PDT
(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.
Comment 8 Nikita Vasilyev 2017-07-25 14:13:04 PDT
Created attachment 316390 [details]
Patch
Comment 9 Nikita Vasilyev 2017-07-25 14:14:00 PDT
Created attachment 316391 [details]
[Animated GIF] Reopen Web Inspector button in Settings
Comment 10 BJ Burg 2017-07-25 14:20:09 PDT
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.
Comment 11 Nikita Vasilyev 2017-07-25 14:27:46 PDT
Created attachment 316394 [details]
Patch
Comment 12 WebKit Commit Bot 2017-07-25 14:43:17 PDT
Comment on attachment 316394 [details]
Patch

Clearing flags on attachment: 316394

Committed r219888: <http://trac.webkit.org/changeset/219888>
Comment 13 WebKit Commit Bot 2017-07-25 14:43:19 PDT
All reviewed patches have been landed.  Closing bug.