[webkit-reviews] review denied: [Bug 98024] Web Inspector: Implement CSS reload upon related SASS resource saving : [Attachment 167108] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 98024: Web Inspector: Implement CSS reload upon related SASS resource
saving
https://bugs.webkit.org/show_bug.cgi?id=98024

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167108&action=review


> Source/WebCore/inspector/front-end/SASSSourceMapping.js:69
> +	   WebInspector.fileManager.save(uiSourceCode.url, event.data.content,
false);

I don't think we should call FileManager.save implicitly in this case.
We can remove this handler and rely upon usual way to start file saving.

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:76
> +	       var cssen = this._cssURLsForSASSURL[event.data];

What is cssen?

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:83
> +	   var timeout = WebInspector.settings.cssReloadTimeout.get();

I would use cssURL -> timeout map to clear timeouts when the same sass file is
saved several times in a row.

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:96
> +	   uiSourceCode.commitWorkingCopy(function() {});

Please replace
uiSourceCode.setWorkingCopy(newContent);
uiSourceCode.commitWorkingCopy(function() {});
with
uiSourceCode.addRevision(newContent);

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:169
> +    _registerImportedSASS: function(content, rawURL, url)

As discussed offline I am not sure we should do that at all and we should
certainly move this to a separate patch.

> Source/WebCore/inspector/front-end/SettingsScreen.js:138
> +    _createNumberSetting: function(name, setting, min, max)

I don't think we need such a complex input field for timeout value.
I would use something similar to what we have for Device Metrics instead.