[webkit-reviews] review granted: [Bug 228548] Replace -webkit-*-* logical properties by standard logical properties in WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js : [Attachment 434601] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Sonia Singla
<soniasingla.1812 at gmail.com>'s request for review:
Bug 228548: Replace -webkit-*-* logical properties by standard logical
properties in WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js
https://bugs.webkit.org/show_bug.cgi?id=228548

Attachment 434601: Patch

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




--- Comment #25 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 434601
  --> https://bugs.webkit.org/attachment.cgi?id=434601
Patch

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

r=me

>>>> Source/WebInspectorUI/ChangeLog:3
>>>> +	      Web Inspector: Add standard logical properties in
WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js
>>> 
>>> Shouldn't we also add update ContextualDocumentationDatabase.js to add the
unprefixed properties ?
>> 
>> And it seems the JetStream test is also using these -webkit-* values to test
WebInspector CSSKeywordCompletions, so this is related to the change here too.
>> I wonder whether you should call this "Web Inspector: Add standard logical
properties to CSS keyword completion".
>> 
>> PerformanceTests/JetStream2/code-load/inspector-payload.js
>> PerformanceTests/JetStream2/code-load/inspector-payload-minified.js
>>
Websites/browserbench.org/JetStream2.0/code-load/inspector-payload-minified.js
>> Websites/browserbench.org/JetStream2.0/code-load/inspector-payload.js
>> 
>> BTW, bugzilla's bug title should be updated too.
> 
> `ContextualDocumentationDatabase.js` is a file derived from information in
the `vscode-custom-data` open source project, so we shouldn't add anything
manually. Properties without documentation available are gracefully handled, so
the presence of an entry in that file is not required.

You definitely don't need to update any of the perf tests.  Those are not meant
to represent actual Web Inspector and I believe are just a way to test large JS
payloads.  They are also _extremely_ out of date.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:466
> +	   "auto", "intrinsic", "min-intrinsic", "min-content",
"-webkit-min-content", "max-content",
> +	   "-webkit-max-content", "-webkit-fill-available", "fit-content",
"-webkit-fit-content", "calc()"

Style: Please don't split this into two lines.	Either keep it all on one line
(preferred) or split every item onto it's own line.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:550
> +	   "auto", "intrinsic", "min-intrinsic", "min-content",
"-webkit-min-content",
> +	   "max-content", "-webkit-max-content", "-webkit-fill-available",
"fit-content", "-webkit-fit-content", "calc()"

ditto (:465)

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:559
> +	   "auto"

Style: missing trailing comma

(ditto with most of the other added lines in this patch too)