Bug 232433 - Web Inspector: Add script to update CSSDocumentation.js
Summary: Web Inspector: Add script to update CSSDocumentation.js
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: 226883
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-28 05:08 PDT by Razvan Caliman
Modified: 2021-11-09 08:58 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.84 KB, patch)
2021-10-28 05:19 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2021-10-28 05:36 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
[fast-cq] Patch 2.0 (4.88 KB, patch)
2021-11-03 14:33 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
[fast-cq] Patch 2.1 (4.87 KB, patch)
2021-11-05 09:25 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
[fast-cq] Patch 2.2 (6.29 KB, patch)
2021-11-08 08:34 PST, 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-28 05:08:33 PDT
Download latest CSS data from upstream[1] data source and update the file at `Source/WebInspectorUI/UserInterface/External/CSSDocumentation/CSSDocumentation.js`


[1] https://github.com/microsoft/vscode-custom-data/blob/main/web-data/data/browsers.css-data.json
Comment 1 Radar WebKit Bug Importer 2021-10-28 05:09:11 PDT
<rdar://problem/84753008>
Comment 2 Razvan Caliman 2021-10-28 05:19:59 PDT
Created attachment 442692 [details]
Patch

Add script and README with instructions how to run and where the CSS data comes from
Comment 3 Razvan Caliman 2021-10-28 05:36:32 PDT
Created attachment 442695 [details]
Patch

Resolve Changelog conflict
Comment 4 Alexey Proskuryakov 2021-10-28 07:42:03 PDT
Comment on attachment 442695 [details]
Patch

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

> Source/WebInspectorUI/Scripts/update-CSSDocumentation.js:1
> +#!/usr/bin/env node

Can this be written in a language commonly used in WebKit tools, such as Python? Extending the number of tools and languages we need to support is very different desirable, we even talked about rewriting the large number of Ruby scripts used in JSC.
Comment 5 Razvan Caliman 2021-10-28 08:07:17 PDT
Comment on attachment 442695 [details]
Patch

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

>> Source/WebInspectorUI/Scripts/update-CSSDocumentation.js:1
>> +#!/usr/bin/env node
> 
> Can this be written in a language commonly used in WebKit tools, such as Python? Extending the number of tools and languages we need to support is very different desirable, we even talked about rewriting the large number of Ruby scripts used in JSC.

Python is not my strong point, but this script is simple enough. Rewriting shouldn't be a problem.
Comment 6 Patrick Angle 2021-10-28 09:10:56 PDT
Comment on attachment 442695 [details]
Patch

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

+1 for using python for this script. Also, since this isn't a build script, can we move it to Tools/Scripts/ so its on most folks path along with the other scripts, like `update-webkit-localizable-strings` et. al.?

I also left a few notes in Bug 231566 that I believe are issues with this version of the generator as is. They might not be an issue after rewriting in Python, though.

> Source/WebInspectorUI/UserInterface/External/CSSDocumentation/README:1
> +# How to update CSSDocumentation.js

I believe other generated source files will often have this information in a comment at the top of the generate file to make it easier to identify what script to run to regenerate the file as well as provide a warning not to manually update the file.
Comment 7 Razvan Caliman 2021-11-03 14:33:40 PDT
Created attachment 443240 [details]
[fast-cq] Patch 2.0

Rewrite script to Python. Address code review feedback
Comment 8 Razvan Caliman 2021-11-05 09:25:41 PDT
Created attachment 443404 [details]
[fast-cq] Patch 2.1

Add newline at end of generated script. Remove unnecessary semicolons
Comment 9 Patrick Angle 2021-11-05 09:45:33 PDT
Comment on attachment 443404 [details]
[fast-cq] Patch 2.1

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

> Tools/Scripts/update-css-documentation:36
> +VSCODE_CSS_DATA_JSON_URL = 'https://raw.githubusercontent.com/microsoft/vscode-custom-data/main/web-data/data/browsers.css-data.json'

IMO we should use double quotes around strings like we do in other parts of the code base, but I do realize that this rule is not consistent throughout other python scripts in the project. It appears currently the type of type of quotation mark is inconsistent throughout (e.g. Lines 39 and 40 use different quotation marks). It would be great if this were consistent.

> Tools/Scripts/update-css-documentation:37
> +ROOT_DIR = (pathlib.Path(__file__).parent / ".." / "..").resolve()

I found `Path` to be one of the things that was clearer to import with `from pathlib import Path` so that I could drop the `pathlib` part of creating a new Path throughout. Up to you tho.

> Tools/Scripts/update-css-documentation:46
> +    print('Download failed \n {0}'.format(err))

NIT: Unnecessary space around newline character.

> Tools/Scripts/update-css-documentation:53
> +    print('Parsing failed \n {0}'.format(err))

Ditto :46

> Tools/Scripts/update-css-documentation:67
> +        url = references[0].get('url')

Since our UI explicitly says that these URLs will take the user to MDN, it's probably a good idea for us to confirm that the reference's `name` is `MDN Reference` or the `url` is actually for MDN in case more links start going to other places some day.

> Tools/Scripts/update-css-documentation:71
> +    if (name.startswith('-moz-') or name.startswith('-ms-') or name.startswith('-o-')):
> +        continue

Could we flip this around to instead just skip properties starting with `-` unless it also starts with `-webkit-` in case we've missed a vendor prefix here? (Unlikely, but would be less brittle to do a check that a prefixed property is WebKit's instead of listing all the other vendor prefixes.)

> Tools/Scripts/update-css-documentation:99
> +    print('Writing failed \n {0}'.format(err))

Ditto :46
Comment 10 Patrick Angle 2021-11-05 09:47:17 PDT
Comment on attachment 443404 [details]
[fast-cq] Patch 2.1

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

> Tools/ChangeLog:13
> +        * Scripts/update-css-documentation: Added.

May I suggest `update-inspector-css-documentation` just to be clearer about what the script is for without having to open the script and read the header comments?
Comment 11 Devin Rousso 2021-11-05 10:42:25 PDT
Comment on attachment 443404 [details]
[fast-cq] Patch 2.1

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

>> Tools/ChangeLog:13
>> +        * Scripts/update-css-documentation: Added.
> 
> May I suggest `update-inspector-css-documentation` just to be clearer about what the script is for without having to open the script and read the header comments?

also, this should be in `Source/WebInspectorUI/Scripts` since this is really just for Web Inspector

> Tools/Scripts/update-css-documentation:40
> +PREAMBLE = "// This file is generated by update-css-documentation. Do not edit.\n"

I'd just inline this

>> Tools/Scripts/update-css-documentation:71
>> +        continue
> 
> Could we flip this around to instead just skip properties starting with `-` unless it also starts with `-webkit-` in case we've missed a vendor prefix here? (Unlikely, but would be less brittle to do a check that a prefixed property is WebKit's instead of listing all the other vendor prefixes.)

we might wanna also include stuff like `-apple-` or any things from other WebKit ports too tho, so maybe we should just match the behavior of `WI.CSSProperty` (and ideally include a comment in both places that links to the other)

> Tools/Scripts/update-css-documentation:79
> +    if description:

Style: we should either always or never have `(` `)` around the condition

> Tools/Scripts/update-css-documentation:89
> +    json_string = json.dumps(properties, indent=4, sort_keys=True)

why not inline this?
Comment 12 Joseph Pecoraro 2021-11-05 12:17:44 PDT
Comment on attachment 443404 [details]
[fast-cq] Patch 2.1

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

> Tools/Scripts/update-css-documentation:69
> +    # Skip properties with prefixes other than -webkit-

Normal WebKit style is that comments are sentences and should end with periods. Applies to all the comments in this script.
Comment 13 Joseph Pecoraro 2021-11-05 12:17:52 PDT
Neat!
Comment 14 Razvan Caliman 2021-11-08 08:31:20 PST
Comment on attachment 443404 [details]
[fast-cq] Patch 2.1

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

>>> Tools/ChangeLog:13
>>> +        * Scripts/update-css-documentation: Added.
>> 
>> May I suggest `update-inspector-css-documentation` just to be clearer about what the script is for without having to open the script and read the header comments?
> 
> also, this should be in `Source/WebInspectorUI/Scripts` since this is really just for Web Inspector

Renamed to `update-inspector-css-documentation` and moved to `Source/WebInspectorUI/Scripts`.

A previous review comment asked to move it to `Tools/Scripts` for convenience, but I agree it's contextual to `Source/WebInspectorUI` and should probably live here.

>> Tools/Scripts/update-css-documentation:36
>> +VSCODE_CSS_DATA_JSON_URL = 'https://raw.githubusercontent.com/microsoft/vscode-custom-data/main/web-data/data/browsers.css-data.json'
> 
> IMO we should use double quotes around strings like we do in other parts of the code base, but I do realize that this rule is not consistent throughout other python scripts in the project. It appears currently the type of type of quotation mark is inconsistent throughout (e.g. Lines 39 and 40 use different quotation marks). It would be great if this were consistent.

Updated to use double quotes everywhere.

Indeed, the style in scripts is inconsistent. I think I resorted to use the single-quote style because that's how the official Python documentation is written.

>> Tools/Scripts/update-css-documentation:37
>> +ROOT_DIR = (pathlib.Path(__file__).parent / ".." / "..").resolve()
> 
> I found `Path` to be one of the things that was clearer to import with `from pathlib import Path` so that I could drop the `pathlib` part of creating a new Path throughout. Up to you tho.

Done!

>> Tools/Scripts/update-css-documentation:40
>> +PREAMBLE = "// This file is generated by update-css-documentation. Do not edit.\n"
> 
> I'd just inline this

Done!

>> Tools/Scripts/update-css-documentation:46
>> +    print('Download failed \n {0}'.format(err))
> 
> NIT: Unnecessary space around newline character.

Done!

>> Tools/Scripts/update-css-documentation:53
>> +    print('Parsing failed \n {0}'.format(err))
> 
> Ditto :46

Done!

>> Tools/Scripts/update-css-documentation:67
>> +        url = references[0].get('url')
> 
> Since our UI explicitly says that these URLs will take the user to MDN, it's probably a good idea for us to confirm that the reference's `name` is `MDN Reference` or the `url` is actually for MDN in case more links start going to other places some day.

I'll add a check for the hostname and throw a warning if it differs.
If URLs change, checking the diff before landing the generated file should draw human attention too.

>> Tools/Scripts/update-css-documentation:69
>> +    # Skip properties with prefixes other than -webkit-
> 
> Normal WebKit style is that comments are sentences and should end with periods. Applies to all the comments in this script.

Done!

>>> Tools/Scripts/update-css-documentation:71
>>> +        continue
>> 
>> Could we flip this around to instead just skip properties starting with `-` unless it also starts with `-webkit-` in case we've missed a vendor prefix here? (Unlikely, but would be less brittle to do a check that a prefixed property is WebKit's instead of listing all the other vendor prefixes.)
> 
> we might wanna also include stuff like `-apple-` or any things from other WebKit ports too tho, so maybe we should just match the behavior of `WI.CSSProperty` (and ideally include a comment in both places that links to the other)

Ok! I'll reuse the allowlist of prefixes defined in `WI.CSSManager.canonicalNameForPropertyNamex(name)`: -webkit-|-khtml-|-apple-

>> Tools/Scripts/update-css-documentation:79
>> +    if description:
> 
> Style: we should either always or never have `(` `)` around the condition

Done! Always use parentheses.

>> Tools/Scripts/update-css-documentation:89
>> +    json_string = json.dumps(properties, indent=4, sort_keys=True)
> 
> why not inline this?

Done!

>> Tools/Scripts/update-css-documentation:99
>> +    print('Writing failed \n {0}'.format(err))
> 
> Ditto :46

Done!
Comment 15 Razvan Caliman 2021-11-08 08:34:53 PST
Created attachment 443550 [details]
[fast-cq] Patch 2.2

Address review feedback. Rename and move script to Source/WebInspectorUI/Scripts/update-inspector-css-documentation
Comment 16 Devin Rousso 2021-11-08 12:16:09 PST
Comment on attachment 443550 [details]
[fast-cq] Patch 2.2

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

r=me

Have we thought at all about what process we want to have in place for making sure this gets regularly updated?  Perhaps we can file bugs alongside the "create new protocol version" bugs that we already have to make sure that we at least update this script every time we save a protocol version too?

> Source/WebInspectorUI/Scripts/update-inspector-css-documentation:68
> +    if (url and not url.startswith("https://developer.mozilla.org")):

What about if there's a MDN link as the second reference?  Maybe we should iterate over `references` until we find a MDN link and then `break` after the first one?
Comment 17 Razvan Caliman 2021-11-09 08:54:36 PST
(In reply to Devin Rousso from comment #16)
> Comment on attachment 443550 [details]
> [fast-cq] Patch 2.2
> 
> r=me

Thanks for R+!

> Have we thought at all about what process we want to have in place for
> making sure this gets regularly updated?  Perhaps we can file bugs alongside
> the "create new protocol version" bugs that we already have to make sure
> that we at least update this script every time we save a protocol version
> too?

Filed <rdar://85204111> as a task to do at the appropriate time.

> > Source/WebInspectorUI/Scripts/update-inspector-css-documentation:68
> > +    if (url and not url.startswith("https://developer.mozilla.org")):
> 
> What about if there's a MDN link as the second reference?  Maybe we should
> iterate over `references` until we find a MDN link and then `break` after
> the first one?

We already catch any non-MDN URL and drop it with a warning to avoid pointing to unexpected URLs. 

Inspecting the generated file diff should alert us if something about URLs changed. We'll adapt the script accordingly when and if that happens.
Comment 18 EWS 2021-11-09 08:58:42 PST
Committed r285502 (244026@main): <https://commits.webkit.org/244026@main>

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