[Webkit-unassigned] [Bug 228548] Replace -webkit-*-* logical properties by standard logical properties in WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=228548

Devin Rousso <drousso at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #434601|review?                     |review+
              Flags|                            |

--- 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)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210730/abf3abb9/attachment-0001.htm>