[webkit-reviews] review granted: [Bug 227097] Web Inspector: Styles: Autocomplete should support function completions : [Attachment 432310] Patch v1.4 - Roll back changes to SpreadsheetTextField to handle in a future patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 227097: Web Inspector: Styles: Autocomplete should support function
completions
https://bugs.webkit.org/show_bug.cgi?id=227097

Attachment 432310: Patch v1.4 - Roll back changes to SpreadsheetTextField to
handle in a future patch

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




--- Comment #26 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 432310
  --> https://bugs.webkit.org/attachment.cgi?id=432310
Patch v1.4 - Roll back changes to SpreadsheetTextField to handle in a future
patch

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

r=me, nice.  I'd suggest living on this a bit before landing tho, as some of
the logic inside `WI.CSSKeywordCompletions.forPartialPropertyValue` is maybe
slightly scary and/or different behavior from what we had (though this is
probably desirable).

NIT: It's a bit odd that `margin: env(` shows a completion hint for `margin:
env(safe-area-inset-bottom;` WITHOUT a `)`.  Could we make it so that the `)`
is added when doing completions for functions, or is that a super intense
change (I have a feeling it is)?

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:40
> +    if (text.length !== caretPosition)

NIT: I'd flip this

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:48
> +    return {prefix: text, completions};

NIT: I'd restructure this to use early returns
```
    if (!text && allowEmptyPrefix)
	return {prefix: text, completions:
WI.CSSCompletions.cssNameCompletions.values};
    return {prefix: text, completions:
WI.CSSCompletions.cssNameCompletions.startsWith(text)};
```

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:51
> +WI.CSSKeywordCompletions.forPartialPropertyValue = function(text,
propertyName, {caretPosition} = {})

So is `caretPosition` here mainly so that you can test the behavior of this,
even if you can't actually use it in `WI.SpreadsheetStyleProperty`?  That seems
mostly fine, but it is a bit odd that
`WI.CSSKeywordCompletions.forPartialPropertyName` explicitly bails if
`caretPosition !== text.length`. I'd expect both of these functions to either
do or not do that, not one of each.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:76
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:81
> +    // If the current token is only whitespace characters, treat it as if we
are at the start of a new token.

I'd move this to be next to the comment below since that's where this logic
actually is.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:83
> +    let currentTokenIsEmpty = !currentTokenValue.length;

I'd just inline this.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:84
> +    let caretIsMidToken = caretPosition !== passedCharacters;

NIT: `caretIsInMiddleOfToken`

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:93
> +    if (tokenAtCaret.type && /\b(comment|string)\b/.test(tokenAtCaret.type))
> +	   return {prefix: "", completions: []};

Could you move this above `let currentTokenValue = ...` so that we avoid doing
any of that work in this case?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:913
> +    _valueCompletionDataProvider(text, {allowEmptyPrefix} = {})

It doesn't look like `allowEmptyPrefix` is used right now.

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:115
> +	   expectedCompletions: ["papayawhip"],

PAPAYAWHIP ������