[Webkit-unassigned] [Bug 141692] Web Inspector: Styles sidebar editing with incomplete property looks poor in UI

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=141692

--- Comment #25 from tobi+bugzilla at basecode.de ---
Comment on attachment 250617
  --> https://bugs.webkit.org/attachment.cgi?id=250617
patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/FormatterContentBuilder.js:189
>>          this._formattedContentLength -= removed.length;
> 
> The next line in this function pops from formattedLineEndings. However you call undo from two places, removeLastNewline and removeLastWhitespace.
> 
> Seems like this should be:
> 
>     if (removed === "\n")
>         this._formattedLineEndings.pop()
> 
> Or, perhaps better, move the pop up into removeLastNewline so there is no if check at all.

Good catch!

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:829
>> +            var editor = this._codeMirror;
> 
> This shouldn't be necessary. We prefer using the member variable directly instead of storing into a local. This code is not particularly hot so performance shouldn't be a concern. Likewise this is a common enough idiom that I would imagine the engine optimizes it efficiently.

Okay

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:881
>> +                    var to = {line: lineNumber};
> 
> Nice, you realized dropping the "ch" would get you to the end of the line =)

I thought it's a bug. How can this be considered as convenient API? :D

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:439
>> +        return isComment || isLastComment || isLastSemicolon || isSemicolon;
> 
> I'd prefer these be early returns to avoid unnecessary work. In pretty printing these functions are called very frequently.

Done.

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:472
>> +        return isComment || isPrefixedProperty || isRegularProperty || isInvalidAfterComment;
> 
> For example in this each check is for a distinct type of token. If you are one of those types, you shouldn't need to run the others.
> 
> Likewise it gives an opportunity to provide comments describing the case. I don't actually know what "isPrefixedProperty" means.

Done.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150414/478c76a4/attachment.html>