Bug 231432 – Web Inspector: Move CSS longhand and shorthand mapping away from WI.CSSCompletions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Razvan Caliman]

`WI.CSSCompletions` has scope creep. Beyond handling filtering for CSS completions, it holds logic to deal with CSS property name longhand-to-shorthand mapping and checks.

But `WI.CSSCompletions` already relies heavily on metadata about properties set on `WI.CSSKeywordCompletions`. 
Moreover, the one-time initialization from `WI.CSSCompletions.initializeCSSCompletions(target)` > `WI.CSSKeywordCompletions.addCustomCompletions(properties)` already puts most of the metadata about CSS properties on:

- `WI.CSSKeywordCompletions.LonghandNamesForShorthandProperty`
- `WI.CSSKeywordCompletions.PropertyNameForAlias`
- `WI.CSSKeywordCompletions.InheritedProperties`

It makes sense to also move the longhand-to-shorthand mapping from `WI.CSSCompletions` to `WI.CSSKeywordCompletions`. 
This keep mapping and checks co-located. 

This is prep work for gradually removing `WI.CSSCompletions.cssNameCompletions` as a needlessly specialized completions manager.

[Radar WebKit Bug Importer]

<rdar://problem/84029471>

[Razvan Caliman]

Created attachment 440624 [details]
Patch v1.0

Moving longhand-to-shorthand mapping and checks away from WI.CSSCompletions

[Devin Rousso]

Comment on attachment 440624 [details]
Patch v1.0

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

r=me

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:233
> +                let shorthands = WI.CSSKeywordCompletions.ShorthandNamesForLongHandProperty.getOrInitialize(longhand, []);
> +                shorthands.push(property.name);

NIT: you could instead use a `Multimap` to avoid the `getOrInitialize` (tho you'd need to either adjust the fallback locations to be a `new Set` or change `Multimap.prototype.get` to have a fallback)

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:262
> +WI.CSSKeywordCompletions.ShorthandNamesForLongHandProperty = new Map;

yknow, technically, it's a bit odd for info about properties to be on something related to value keywords

IMO it would be better to move these to `WI.CSSCompletions` (or create a new `WI.CSSPropertyCompletions`)

[Razvan Caliman]

Comment on attachment 440624 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:262
>> +WI.CSSKeywordCompletions.ShorthandNamesForLongHandProperty = new Map;
> 
> yknow, technically, it's a bit odd for info about properties to be on something related to value keywords
> 
> IMO it would be better to move these to `WI.CSSCompletions` (or create a new `WI.CSSPropertyCompletions`)

I agree. I followed precedent, but `WI.CSSKeywordCompletions` holds too much already.
A new `WI.CSSPropertyCompletions` as host for all property-related maps would be best. 
This would isolate logic away from `WI.CSSKeywordCompletions` and `WI.CSSCompletions`.

The task is not big, but it will yield a large code diff which would be best split into multiple patches.

In the interest of making progress on Bug 230351 this week, I'll land this patch as-is (thanks for R+) and I'll open follow-ups to restructure code into a new `WI.CSSPropertyCompletions` afterwards.

[Razvan Caliman]

Comment on attachment 440624 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:262
>> +WI.CSSKeywordCompletions.ShorthandNamesForLongHandProperty = new Map;
> 
> yknow, technically, it's a bit odd for info about properties to be on something related to value keywords
> 
> IMO it would be better to move these to `WI.CSSCompletions` (or create a new `WI.CSSPropertyCompletions`)

I agree. I followed precedent, but `WI.CSSKeywordCompletions` holds too much already.
A new `WI.CSSPropertyCompletions` as host for all property-related maps would be best. 
This would isolate logic away from `WI.CSSKeywordCompletions` and `WI.CSSCompletions`.

The task is not big, but it will yield a large code diff which would be best split into multiple patches.

In the interest of making progress on Bug 230351 this week, I'll land this patch as-is (thanks for R+) and I'll open follow-ups to restructure code into a new `WI.CSSPropertyCompletions` afterwards.

[Razvan Caliman]

Comment on attachment 440624 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:262
>> +WI.CSSKeywordCompletions.ShorthandNamesForLongHandProperty = new Map;
> 
> yknow, technically, it's a bit odd for info about properties to be on something related to value keywords
> 
> IMO it would be better to move these to `WI.CSSCompletions` (or create a new `WI.CSSPropertyCompletions`)

I agree. I followed precedent, but `WI.CSSKeywordCompletions` holds too much already.
A new `WI.CSSPropertyCompletions` as host for all property-related maps would be best. 
This would isolate logic away from `WI.CSSKeywordCompletions` and `WI.CSSCompletions`.

The task is not big, but it will yield a large code diff which would be best split into multiple patches.

In the interest of making progress on Bug 230351 this week, I'll land this patch as-is (thanks for R+) and I'll open follow-ups to restructure code into a new `WI.CSSPropertyCompletions` afterwards.

[Razvan Caliman]

Comment on attachment 440624 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:262
>> +WI.CSSKeywordCompletions.ShorthandNamesForLongHandProperty = new Map;
> 
> yknow, technically, it's a bit odd for info about properties to be on something related to value keywords
> 
> IMO it would be better to move these to `WI.CSSCompletions` (or create a new `WI.CSSPropertyCompletions`)

I agree. I followed precedent, but `WI.CSSKeywordCompletions` holds too much already.
A new `WI.CSSPropertyCompletions` as host for all property-related maps would be best. 
This would isolate logic away from `WI.CSSKeywordCompletions` and `WI.CSSCompletions`.

The task is not big, but it will yield a large code diff which would be best split into multiple patches.

In the interest of making progress on Bug 230351 this week, I'll land this patch as-is (thanks for R+) and I'll open follow-ups to restructure code into a new `WI.CSSPropertyCompletions` afterwards.

[Razvan Caliman]

Created attachment 440783 [details]
Patch v1.0

Carry over R+; Land patch as is and follow-up in Bug 231511

[EWS]

Committed r283899 (242775@main): <https://commits.webkit.org/242775@main>

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