[webkit-reviews] review denied: [Bug 189041] Web Inspector: generate CSSKeywordCompletions from backend values : [Attachment 348609] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 189041: Web Inspector: generate CSSKeywordCompletions from backend values
https://bugs.webkit.org/show_bug.cgi?id=189041

Attachment 348609: Patch

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




--- Comment #14 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 348609
  --> https://bugs.webkit.org/attachment.cgi?id=348609
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:142
> +WI.CSSKeywordCompletions.PropertyNameForAlias = {};
>  
> -    // iOS Properties
> -    "-webkit-overflow-scrolling", "-webkit-touch-callout",
"-webkit-tap-highlight-color"
> -].keySet();
> +WI.CSSKeywordCompletions.LonghandNamesForShorthandProperty = {};

Nit: Would be nice to include a comment that these are populated from
CSS.getSupportedCSSProperties

Also seems these two objects should probably be a `new Map`.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:145
> +    // Compatibility (iOS 12): `inherited` didn't exist on `CSSPropertyInfo`

I'd put this comment above the variable definition, not in the list.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:305
>  ].keySet();

I wonder if `keySet` would be better implemented as `new Set` now. I think they
are efficient and inline-able by JITs.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:307
>  WI.CSSKeywordCompletions._propertyKeywordMap = {

What changes were made to this list and how were the changes made?

> LayoutTests/inspector/css/getSupportedCSSProperties.html:24
> +	       for (let expectedProperty of expectedProperties) {

r- here because this should be testing all of the new backend additions. This
seems to cover `inherited` but doesn't cover `aliases`.