Bug 231432 - Web Inspector: Move CSS longhand and shorthand mapping away from WI.CSSCompletions
Summary: Web Inspector: Move CSS longhand and shorthand mapping away from WI.CSSComple...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Razvan Caliman
URL:
Keywords: InRadar
Depends on:
Blocks: 231511 230351
  Show dependency treegraph
 
Reported: 2021-10-08 08:41 PDT by Razvan Caliman
Modified: 2021-10-11 09:13 PDT (History)
5 users (show)

See Also:


Attachments
Patch v1.0 (8.25 KB, patch)
2021-10-08 09:00 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch v1.0 (8.25 KB, patch)
2021-10-11 06:39 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Razvan Caliman 2021-10-08 08:41:59 PDT
`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.
Comment 1 Radar WebKit Bug Importer 2021-10-08 08:42:57 PDT
<rdar://problem/84029471>
Comment 2 Razvan Caliman 2021-10-08 09:00:24 PDT
Created attachment 440624 [details]
Patch v1.0

Moving longhand-to-shorthand mapping and checks away from WI.CSSCompletions
Comment 3 Devin Rousso 2021-10-08 11:36:33 PDT
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`)
Comment 4 Razvan Caliman 2021-10-11 06:30:58 PDT
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.
Comment 5 Razvan Caliman 2021-10-11 06:30:59 PDT
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.
Comment 6 Razvan Caliman 2021-10-11 06:31:00 PDT
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.
Comment 7 Razvan Caliman 2021-10-11 06:31:01 PDT
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.
Comment 8 Razvan Caliman 2021-10-11 06:39:05 PDT
Created attachment 440783 [details]
Patch v1.0

Carry over R+; Land patch as is and follow-up in Bug 231511
Comment 9 EWS 2021-10-11 09:13:37 PDT
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].