Bug 98024 – Web Inspector: Implement CSS reload upon related SASS resource saving

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 167108 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 167108 [details]
Patch

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.

[Alexander Pavlov (apavlov)]

Comment on attachment 167108 [details]
Patch

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.

OK

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

The plural of "css" auf Deutsch ("csss" or "csses" are equally ugly, and I have seen "cssen" around).

>> 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.

Done

>> Source/WebCore/inspector/front-end/SASSSourceMapping.js:96
>> +        uiSourceCode.commitWorkingCopy(function() {});
> 
> Please replace
> uiSourceCode.setWorkingCopy(newContent);
> uiSourceCode.commitWorkingCopy(function() {});
> with
> uiSourceCode.addRevision(newContent);

Done

>> 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.

Done

>> 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.

Have a look at their value change handler :) Simplified the builder a bit.

[Alexander Pavlov (apavlov)]

Created attachment 170093 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 170093 [details]
Patch

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

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:76
> +        var timer = this._timeoutForURL[sassURL];

I would reuse timeout :)

> Source/WebCore/inspector/front-end/Settings.js:108
> +    this.cssReloadTimeout = this.createSetting("cssReloadTimeout", 1000);

I don't think this should be enabled by default.

> Source/WebCore/inspector/front-end/SettingsScreen.js:312
> +        setting.className = "setting";

number-setting?

> Source/WebCore/inspector/front-end/helpScreen.css:214
> +.help-content p.setting {

Looks like we generally use fieldset for that.

[Alexander Pavlov (apavlov)]

Committed r132321: <http://trac.webkit.org/changeset/132321>

[Nikita Vasilyev]

Is it feasible to do the same for CoffeeScript? (or any other language with Source Maps support)