[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 #24 from Joseph Pecoraro <joepeck at webkit.org> ---
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

You mentioned you had tests for this as well?

> Source/WebInspectorUI/UserInterface/Controllers/FormatterContentBuilder.js:189
> +    _undo()
>      {
>          var removed = this._formattedContent.pop();
>          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.

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

> 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 =)

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:439
> +        var isLastComment = /\bcomment\b/.test(lastToken);
> +        var isLastSemicolon = lastContent === ";" && lastToken === null;
> +        var isSemicolon = content === ";" && token === null;
> +        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.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:472
> +        var isRegularProperty = /\bproperty\b/.test(token) && !(/\bmeta\b/.test(lastToken));
> +        var isPrefixedProperty = /\bmeta\b/.test(token) && lastContent !== ":";
> +        var isInvalidAfterComment = /\batom\b/.test(token) && /\bcomment\b/.test(lastToken);
> +        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.

-- 
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/f83502c9/attachment.html>