[webkit-reviews] review denied: [Bug 195777] Web Inspector: provide a way to reset all settings : [Attachment 364885] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195777: Web Inspector: provide a way to reset all settings
https://bugs.webkit.org/show_bug.cgi?id=195777

Attachment 364885: Patch

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




--- Comment #10 from Timothy Hatcher <timothy at apple.com> ---
Comment on attachment 364885
  --> https://bugs.webkit.org/attachment.cgi?id=364885
Patch

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

I think we need a dialog UI for Web Inspector — maybe WI.Modal is that but as
WI.ModalDialog. Blurring the entire window is not ideal and not something found
in other apps. This would be a dialog with Reset and Cancel buttons. Having one
button makes it hard to reason how to exit without resetting.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:261
> +	       resetModalButton.addEventListener("click", (event) => {

This handler is big for being inline. I would expect this to be moved to a
member function. It also is more Manager like than View like code. Main.js
would be a better place for this logic to be maintained.