Bug 232433

<rdar://problem/84753008>

Created attachment 442692 [details] Patch Add script and README with instructions how to run and where the CSS data comes from

Created attachment 442695 [details] Patch Resolve Changelog conflict

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

Created attachment 443240 [details] [fast-cq] Patch 2.0 Rewrite script to Python. Address code review feedback

Created attachment 443404 [details] [fast-cq] Patch 2.1 Add newline at end of generated script. Remove unnecessary semicolons

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

Neat!

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!

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

(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.

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].