<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:timothy@apple.com" title="Timothy Hatcher <timothy@apple.com>"> <span class="fn">Timothy Hatcher</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Pressing tab in the styles sidebar shouldn't insert a tab character"
href="https://bugs.webkit.org/show_bug.cgi?id=146189">bug 146189</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #255467 Flags</td>
<td>review?
</td>
<td>review+, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Pressing tab in the styles sidebar shouldn't insert a tab character"
href="https://bugs.webkit.org/show_bug.cgi?id=146189#c7">Comment # 7</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Pressing tab in the styles sidebar shouldn't insert a tab character"
href="https://bugs.webkit.org/show_bug.cgi?id=146189">bug 146189</a>
from <span class="vcard"><a class="email" href="mailto:timothy@apple.com" title="Timothy Hatcher <timothy@apple.com>"> <span class="fn">Timothy Hatcher</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=255467&action=diff" name="attach_255467" title="Patch">attachment 255467</a> <a href="attachment.cgi?id=255467&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=255467&action=review">https://bugs.webkit.org/attachment.cgi?id=255467&action=review</a>
<span class="quote">> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:42
> + this._noCSSEndingSemicolon = false;</span >
I don't think you need CSS here. It could be used by JS too.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:342
> + if (reverse && typeof this._delegate.cssStyleDeclarationSectionEditorPrevRule === "function")</span >
Nit: Spell out Previous.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:472
> + if (event.keyCode === 9) {</span >
Does event.keyIdentifier === "Tab" work? This should also be an early return to avoid indenting the whole function body.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:69
> + this._completionController._noCSSEndingSemicolon = true;</span >
This uses a private property from another file. A no-no for us. It should be exposed as a public property with no underscore if it needed used externally.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:350
> + if (!line || !line.trim().length)</span >
Put the trimmed string in a local var to avoid doing it twice.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:354
> + this._codeMirror.setSelection({line: 0, ch: 0}, {line: 0, ch: index < 0 ? line.trim().length : index});</span >
trim() will remove prefix space, so this could put the end of the selection at the wrong column. You should use trimRight().
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:363
> + this._codeMirror.setSelection({line, ch: colon ? colon.index + colon[0].length : 0}, {line, ch: lineText.trim().length - lineText.trim().endsWith(";")});</span >
You should put lineText.trim() in a local var to avoid doing it twice. Also, trimRight().
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:395
> + cursor.ch = line.trim().length;</span >
Put the trimmed in a local. trimRight()?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:419
> + var prevLine = codeMirror.getLine(cursor.line - 1);</span >
Nit: previousLine
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:470
> + var trimmedLength = line.trim().length;</span >
Put the trimmed in a local. trimRight()?</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>