[Webkit-unassigned] [Bug 146189] Web Inspector: Pressing tab in the styles sidebar shouldn't insert a tab character

[bugzilla-daemon at webkit.org]

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

Timothy Hatcher <timothy at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #255467|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #7 from Timothy Hatcher <timothy at apple.com> ---
Comment on attachment 255467
  --> https://bugs.webkit.org/attachment.cgi?id=255467
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:42
> +        this._noCSSEndingSemicolon = false;

I don't think you need CSS here. It could be used by JS too.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:342
> +        if (reverse && typeof this._delegate.cssStyleDeclarationSectionEditorPrevRule === "function")

Nit: Spell out Previous.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:472
> +        if (event.keyCode === 9) {

Does event.keyIdentifier === "Tab" work? This should also be an early return to avoid indenting the whole function body.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:69
> +        this._completionController._noCSSEndingSemicolon = true;

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.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:350
> +        if (!line || !line.trim().length)

Put the trimmed string in a local var to avoid doing it twice.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:354
> +        this._codeMirror.setSelection({line: 0, ch: 0}, {line: 0, ch: index < 0 ? line.trim().length : index});

trim() will remove prefix space, so this could put the end of the selection at the wrong column. You should use trimRight().

> 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(";")});

You should put lineText.trim() in a local var to avoid doing it twice. Also, trimRight().

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:395
> +        cursor.ch = line.trim().length;

Put the trimmed in a local. trimRight()?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:419
> +        var prevLine = codeMirror.getLine(cursor.line - 1);

Nit: previousLine

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:470
> +        var trimmedLength = line.trim().length;

Put the trimmed in a local. trimRight()?

-- 
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/20150624/132d16ed/attachment-0001.html>