<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Styles sidebar editing with incomplete property looks poor in UI"
href="https://bugs.webkit.org/show_bug.cgi?id=141692#c25">Comment # 25</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Styles sidebar editing with incomplete property looks poor in UI"
href="https://bugs.webkit.org/show_bug.cgi?id=141692">bug 141692</a>
from <span class="vcard"><a class="email" href="mailto:tobi+bugzilla@basecode.de" title="tobi+bugzilla@basecode.de">tobi+bugzilla@basecode.de</a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=250617&action=diff" name="attach_250617" title="patch">attachment 250617</a> <a href="attachment.cgi?id=250617&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=250617&action=review">https://bugs.webkit.org/attachment.cgi?id=250617&action=review</a>
<span class="quote">>> 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.</span >
Good catch!
<span class="quote">>> 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.</span >
Okay
<span class="quote">>> 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 =)</span >
I thought it's a bug. How can this be considered as convenient API? :D
<span class="quote">>> 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.</span >
Done.
<span class="quote">>> 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.</span >
Done.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>