[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 #12 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 248238
  --> https://bugs.webkit.org/attachment.cgi?id=248238
patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:217
> +    // The parser detects multiple style declarations in one line,
> +    // corrects invalid style declarations and adds missing line breaks

Style: WebKit style is for comments to end with periods. This comment applies to all comments in this patch.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:218
> +    _parseStyleDeclarations: function(string)

The name _parse implies that you are parsing the string into some data structure / representation. Here you are just returning a formatted string, so perhaps this should be named _formatStyleDeclarations. The comment above the function shouldn't be needed with a clear name, but if you want to keep it can go inside the function.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:230
> +                style = mode.token(stream, state);

Style: WebKit style for JavaScript is to just use var at the declaration point. You can move the `var`s from the for loop initializer section in front of the variables `stream` and `style`. Imagine them moving to `let` eventually.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:233
> +                if (style === "comment m-css") {

We typically test style.test(/\bcomment\b) to be resilient against additional styles getting added to the token.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:235
> +                    parsed.push(stream.current());
> +                // Otherwise parse line and consider it containing multiple declarations

Comment indentation here is confusing. You can add a `continue;` here, to reduce the `else if` to just an `if` with the comment.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:253
> +                    current = stream.current().trim();
> +                    // Detect opening comment
> +                    if (current.indexOf("/*") > -1) {
> +                        // Remove opening comment and whitespace
> +                        current = current.replace(/\s*\/\*\s*/, "");
> +                        state.isInlineComment = true;
> +                    }
> +                    // Detect ending comment
> +                    if (current.indexOf("*/") > -1) {
> +                        // Remove ending comment and whitespace
> +                        current = current.replace(/\s*\*\/\s*/, "");
> +                        state.isInlineComment = false;
> +                    }
> +                    // Add missing colon in case of a property without a value
> +                    // E.g.: "color;" turns into "color:;"
> +                    if (current.indexOf(":") === -1)
> +                        current += ":";

I see the logic here, but because we are using a CSS tokenizer we should take advantage of it.

stream.skipTo looks like it will just search characters, not tokens. So the ';' that it finds could be in the middle of a string or comment:

    /* foo; bar */
    div { background: url(some;thing);

Likewise for the comment searching.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150310/cb8f809c/attachment-0002.html>