Bug 233369 – Web Inspector: Extract a specialized CSSNameCompletions from CSSCompletions

• Component: Web Inspector
• Status: RESOLVED FIXED

[Razvan Caliman]

<rdar://83206520>

[Razvan Caliman]

Created attachment 444826 [details]
Patch 1.0

Depends on Bug 230351

[Devin Rousso]

Comment on attachment 444826 [details]
Patch 1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:52
> +        this.cssNameCompletions = null;

Style: this should either be declared as a static property on `WI.CSSManager` or made into a private `_cssNameCompletions` with a corresponding `get cssNameCompletions() { return this._cssNameCompletions; }`

NIT: It's also a bit wordy/redundant to write `WI.cssManager.cssNameCompletions`.  How about `propertyNameCompletions` so it's clearer and doesn't repeat "css" twice?

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:63
> +    initializeCSSCompletions(target)

I think we should rename this to `initializeCSSNameCompletions` to be more accurate.

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:70
> +        function propertiesCallback(error, cssProperties) {

I realize that this is mostly copied code, but I think we could/should update it while we're at it.

NIT: I'd just inline these functions

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:83
> +            var propertyNamesForCodeMirror = {};

`let`

ditto (below)

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:93
> +            function collectPropertyNameForCodeMirror(propertyName) {

NIT: I'd inline this

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:106
> +                for (var i = 0; i < keywords.length; ++i) {

```
    for (let keyword of keywords) {
```

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:114
> +            WI.CSSKeywordCompletions._colors.forEach(function(colorName) {

```
    for (let color of WI.CSSKeywordCompletions._colors)
```

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:36
> +    constructor(values, acceptEmptyPrefix = false)

The `= false` is unnecessary since omitting it will result in `undefined`, which is also a falsy value.  I'd just add `!!acceptEmptyPrefix` below.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:42
> +        if (values.length && typeof values[0] === "string")

You can drop the `values.length` check since that's implicitly checked by `typeof values[0] === "string"`.

> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:4
> + * Copyright (C) 2010 Nikita Vasilyev. All rights reserved.
> + * Copyright (C) 2010 Joseph Pecoraro. All rights reserved.
> + * Copyright (C) 2010 Google Inc. All rights reserved.

Since this is a new file, I dont think these should be here.

Also, is this the right copyright header?  It looks different.

> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:36
> +    constructor(properties, acceptEmptyPrefix)

If you're gonna do this then I'd probably make this (and the corresponding one for `WI.CSSCompletions`) into an `options = {}` so that if new values are added to `WI.CSSCompletions` in the future then you don't have to worry about them in subclasses (unless it affects the subclass).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:303
> +            let propertyNameIsValid = WI.cssManager.cssNameCompletions?.isValidPropertyName(this._property.name);

I'd just inline this

[Razvan Caliman]

Comment on attachment 444826 [details]
Patch 1.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:52
>> +        this.cssNameCompletions = null;
> 
> Style: this should either be declared as a static property on `WI.CSSManager` or made into a private `_cssNameCompletions` with a corresponding `get cssNameCompletions() { return this._cssNameCompletions; }`
> 
> NIT: It's also a bit wordy/redundant to write `WI.cssManager.cssNameCompletions`.  How about `propertyNameCompletions` so it's clearer and doesn't repeat "css" twice?

>Style: this should either be declared as a static property on `WI.CSSManager` or made into a private `_cssNameCompletions` with a corresponding `get cssNameCompletions() { return this._cssNameCompletions; }`
Why? It's meant to be publicly accessible.

> How about `propertyNameCompletions` so it's clearer and doesn't repeat "css" twice?
Sure we can use that as an accessor to reduce wordiness for consumers, but I'd prefer to keep "CSS" in the name of the class and of the file (`CSSPropertyNameCompletions`) so it stays co-located with other CSS-prefixed files.

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:63
>> +    initializeCSSCompletions(target)
> 
> I think we should rename this to `initializeCSSNameCompletions` to be more accurate.

Renamed to `initializeCSSPropertyNameCompletions` to match the new name of the class.

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:70
>> +        function propertiesCallback(error, cssProperties) {
> 
> I realize that this is mostly copied code, but I think we could/should update it while we're at it.
> 
> NIT: I'd just inline these functions

Done

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:83
>> +            var propertyNamesForCodeMirror = {};
> 
> `let`
> 
> ditto (below)

Done

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:93
>> +            function collectPropertyNameForCodeMirror(propertyName) {
> 
> NIT: I'd inline this

Done

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:106
>> +                for (var i = 0; i < keywords.length; ++i) {
> 
> ```
>     for (let keyword of keywords) {
> ```

Done

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:114
>> +            WI.CSSKeywordCompletions._colors.forEach(function(colorName) {
> 
> ```
>     for (let color of WI.CSSKeywordCompletions._colors)
> ```

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:36
>> +    constructor(values, acceptEmptyPrefix = false)
> 
> The `= false` is unnecessary since omitting it will result in `undefined`, which is also a falsy value.  I'd just add `!!acceptEmptyPrefix` below.

Done. Though I find it more self-explanatory when the boolean fallback is declared up-front rather than coerced with `!!`

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:42
>> +        if (values.length && typeof values[0] === "string")
> 
> You can drop the `values.length` check since that's implicitly checked by `typeof values[0] === "string"`.

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:4
>> + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> Since this is a new file, I dont think these should be here.
> 
> Also, is this the right copyright header?  It looks different.

Done. Replaced with the right copyright.

>> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:36
>> +    constructor(properties, acceptEmptyPrefix)
> 
> If you're gonna do this then I'd probably make this (and the corresponding one for `WI.CSSCompletions`) into an `options = {}` so that if new values are added to `WI.CSSCompletions` in the future then you don't have to worry about them in subclasses (unless it affects the subclass).

Done

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:303
>> +            let propertyNameIsValid = WI.cssManager.cssNameCompletions?.isValidPropertyName(this._property.name);
> 
> I'd just inline this

Done

[Razvan Caliman]

Created attachment 446418 [details]
Patch 1.1

Address code review feedback:
- renamed CSSNameCompletions to CSSPropertyNameCompletions
- added accessor WI.cssManager.propertyNameCompletions
- updated callsites
- updated contents of initializeCSSPropertyNameCompletions(target) to use contemporary JS
- updated signature of CSSCompletions to expect an object instead of a list of arguments

[Devin Rousso]

Comment on attachment 446418 [details]
Patch 1.1

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

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:93
> +            function collectPropertyNameForCodeMirror(propertyName) {

NIT: I'd inline this

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:335
> +    get propertyNameCompletions()

Style: this is a simple `get`, so you can make this into one line `get propertyNameCompletions() { return this._propertyNameCompletions; }` just below the `// Public` section :)

> Source/WebInspectorUI/UserInterface/Main.html:399
> +    <script src="Models/CSSPropertyNameCompletions.js"></script>
>      <script src="Models/CSSProperty.js"></script>

Style: these are ordered wrong

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:38
> +        console.assert(Array.isArray(values), values);

If you're asserting that `values` is always an `Array`, then it's not really an optional parameter.  As such, it shouldn't be in an `options` struct (especially since it has a default `= {}`).  We should only be putting optional things inside `options = {}`.
```
    constructor(values, {acceptEmptyPrefix} = {})
```

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:42
> +        if (typeof values[0] === "string")

Is it ever the case that the items in `values` are not all `String`?  If so, perhaps we should just make this into a `console.assert` and always do the initialization of `this._values`?

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:43
> +            this._values.pushAll(values);

NIT: Can we just `this._values = values;`?  Or is it necessary to do the copy (and if so, can we just `this._values = values.slice()` instead)?

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:44
> +        return {prefix: text, completions: WI.cssManager.propertyNameCompletions.values};

I wonder if it's actually safe for us to access `WI.cssManager.propertyNameCompletions` unchecked here? 🤔

ditto (below)

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:230
> +    const acceptEmptyPrefix = true;

Since this is now inside an `options` object, you can inline it instead of creating a `const` variable.
```
    return new WI.CSSCompletions(suggestions, {acceptEmptyPrefix: true});
```

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:28
> +    constructor({properties, acceptEmptyPrefix} = {})

ditto (Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:38)

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:31
> +        console.assert(properties[0].name, properties, "Expected an array of objects with `name` key");

NIT: I'd put the description first so that it appears first in the Web Inspector Console (i.e. context before data).

[Razvan Caliman]

Comment on attachment 446418 [details]
Patch 1.1

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

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:93
>> +            function collectPropertyNameForCodeMirror(propertyName) {
> 
> NIT: I'd inline this

Done

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:335
>> +    get propertyNameCompletions()
> 
> Style: this is a simple `get`, so you can make this into one line `get propertyNameCompletions() { return this._propertyNameCompletions; }` just below the `// Public` section :)

Done. :|

>> Source/WebInspectorUI/UserInterface/Main.html:399
>>      <script src="Models/CSSProperty.js"></script>
> 
> Style: these are ordered wrong

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:38
>> +        console.assert(Array.isArray(values), values);
> 
> If you're asserting that `values` is always an `Array`, then it's not really an optional parameter.  As such, it shouldn't be in an `options` struct (especially since it has a default `= {}`).  We should only be putting optional things inside `options = {}`.
> ```
>     constructor(values, {acceptEmptyPrefix} = {})
> ```

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:42
>> +        if (typeof values[0] === "string")
> 
> Is it ever the case that the items in `values` are not all `String`?  If so, perhaps we should just make this into a `console.assert` and always do the initialization of `this._values`?

Always expected as an array of strings. Done.

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:43
>> +            this._values.pushAll(values);
> 
> NIT: Can we just `this._values = values;`?  Or is it necessary to do the copy (and if so, can we just `this._values = values.slice()` instead)?

I'll make this `this._values = values.slice()` because we're mutating the array in-place with sorting on the next line.

Currently, this isn't a problem because all callers pass in an array that's not used elsewhere. But if this will not be the case in the future, sorting will cause subtle side-effects. Cloning is safer.

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:44
>> +        return {prefix: text, completions: WI.cssManager.propertyNameCompletions.values};
> 
> I wonder if it's actually safe for us to access `WI.cssManager.propertyNameCompletions` unchecked here? 🤔
> 
> ditto (below)

Yes. To get to a point where CSS completions are requested (user input), the `propertyNameCompletions` will have already been set when initializing with the target in `WI.CSSManager.initializeCSSPropertyNameCompletions()`

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:230
>> +    const acceptEmptyPrefix = true;
> 
> Since this is now inside an `options` object, you can inline it instead of creating a `const` variable.
> ```
>     return new WI.CSSCompletions(suggestions, {acceptEmptyPrefix: true});
> ```

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:28
>> +    constructor({properties, acceptEmptyPrefix} = {})
> 
> ditto (Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:38)

Done

[Razvan Caliman]

Created attachment 446530 [details]
Patch 1.2

Address code review feedback

[Devin Rousso]

Comment on attachment 446530 [details]
Patch 1.2

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

r=me, neato :)

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:28
> +    constructor(properties, {acceptEmptyPrefix} = {})

NIT: since `acceptEmptyPrefix` isn't used here, i'd make this into an `options = {}` so that if new things are added to `WI.CSSCompletions` in the future you don't have to also update this

[Razvan Caliman]

Created attachment 446702 [details]
Patch 1.3

[Razvan Caliman]

Carried over R+
Address final NIT.

[EWS]

Committed r286844 (245077@main): <https://commits.webkit.org/245077@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446702 [details].