[webkit-reviews] review granted: [Bug 182523] Web Inspector: Styles: close unbalanced quotes and parenthesis when editing values : [Attachment 361363] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 182523: Web Inspector: Styles: close unbalanced quotes and parenthesis when
editing values
https://bugs.webkit.org/show_bug.cgi?id=182523

Attachment 361363: Patch

https://bugs.webkit.org/attachment.cgi?id=361363&action=review




--- Comment #8 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 361363
  --> https://bugs.webkit.org/attachment.cgi?id=361363
Patch

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

r=me, awesome work.  I'd love it if we could adapt this to work with any type
of string (<https://webkit.org/b/192102> would benefit from that).

> LayoutTests/inspector/unit-tests/css-completion.html:8
> +    let suite = InspectorTest.createSyncSuite("CSSCompletion");

NIT: this should probably be `CSSCompletions` to match the model class name.

> LayoutTests/inspector/unit-tests/css-completion.html:11
> +	   name: "fixUnbalancedCharacters",

NIT: please prefix the test case name with the suite name.

    CSSCompletions.fixUnbalancedCharacters.

> LayoutTests/inspector/unit-tests/css-completion.html:43
> +	       log(`('foo"`);
> +	       log(`('foo")`);
> +	       log(`("bar"')`);
> +	       log(`("bar")`);

As a sanity check, we should add tests for when a parenthesis is inside quotes.

    log (`"(`);
    log (`"(foo`);
    log (`'(`);
    log (`'(bar`);

> LayoutTests/inspector/unit-tests/css-completion.html:51
> +	       log(`"\\"`);
> +	       log(`'\\'`);
> +	       log(`(\\)`);

We should also test when there are multiple sequences of quotes/parenthesis in
a single value (e.g. `font-family`, `calc(...)`,
`linear-gradient(...)`/`radial-gradient(...)` with some color/`var`/`calc`
inside it).

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:174
> +    static fixUnbalancedCharacters(value)

I'm not a huge fan of this name.  Using `fix` implies to me that it will modify
`value` (which isn't possible if it's a string, as you'd create a new value
that wouldn't be reflected back on the caller argument).

How about `determineUnbalancedCharacters`?

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:177
> +	       Data: 0,

Is there a reason you skipped `1`?

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:180
> +	       SingleQuoteString: 2,
> +	       DoubleQuoteString: 3,
> +	       Comment: 4

Rather than keep these values as numbers, why not make them the actual
character value (e.g. a token), and replace `State.Data` with a null value?

    const Token = {
	SingleQuote: `'`,
	DoubleQuote: `"`,
	OpenParenthesis: `(`,
	ClosedParenthesis: `)`,
	Comment:  `/`,
	Asterisk: `*`,
	Backslash: `\\`,
    };

    let currentToken = null;

    ...

    switch (value[i]) {
    case Token.SingleQuote:
	if (!currentToken)
	    currentToken = Token.SingleQuote;
	else if (currentToken === Token.SingleQuote)
	    currentToken = null;
	...

    case Token.DoubleQuote:
	...

    case Token.OpenParenthesis:
	...

    case Token.ClosedParenthesis:
	...

    case Token.Comment:
	...

    case Token.Asterisk:
	...

    case Token.Backslash:
	...
    }

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:184
> +	   let parenthesisUnclosedCount = 0;

NIT: I'd flip this name to be `unclosedParenthesisCount`.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:210
> +		   if (state === State.Data && parenthesisUnclosedCount)

Aside: we'd still have an issue if I type `display: );`, so maybe we'd want to
add a version of this function that tries to calculate a prefix (AFAICT, it
would only add a leading open parenthesis or open comment, as everything else
can be handled by a suffix).  Just a thought :)

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:230
> +		       if (i + 1 < length && value[i + 1] === "/")

NIT: since you're just checking the value (e.g. not trying to call any
functions on the value), you can drop the `i + 1 < length` check.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:240
> +	       suffix = "\\";

NIT: I get that the string is empty until this point, but you're using `+=`
later, so I think we should also use `+=` here to be consistent.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:254
> +	   if (parenthesisUnclosedCount)

NIT: this `if` is unnecessary, as a `")".repeat(0)` will just return an empty
string.