Incorrect constant naming due to idl bindings |
||
Issue descriptionSVG unit types are not well capitalized[1]: kSvgUnitTypeuserspaceonuse kSvgUnitTypeObjectboundingbox kSvgMarkerunitsUserspaceonuse SVG is too big to fail. Can we update the idl bindings[2] to not require this odd capitalization (i.e., kSvgUnitTypeUserSpaceOnUse)? [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/SVGUnitTypes.h [2] https://chromium.googlesource.com/chromium/src/+/f1615358fb40347ba0df1e5bf0923fc6a3b5bdd3
,
Mar 17 2017
I'm not sure what you mean by 'too big to fail' here, or what the desired outcome is. Are we trying to get back to using SGVUnitTypes::SVG_UNIT_TYPE_USERSPACEONUSE (a), or just to something more sensible, but still following the style guide, like kSVGUnitTypeUserSpaceOnUse (b)? These oddities, btw, come from the IDL definitions of the SVG contstants, for instance in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/SVGUnitTypes.idl the original constants are defined without inter-word delimeters, e.g. SVG_UNIT_TYPE_USERSPACEONUSE It would be easy enough to allow the 'SVG' term to remain all-caps, as an acronym, but that would just result in kSVGUnitTypeUserspaceonuse. If we're looking to get to (b), then a better solution would be to update the IDL to separate the words, but if that's not possible, then I think we could add "ImplementedAs" annotations to map it to a better blink-style-name in C++ code. (Auto-detecting word boundaries in runonidentifiers is probably not going to happen in Q1 :) )
,
Jan 4
To be clear, while we can change the definition in svg_unit_types.h, and correct all of the uses in non-generated code, with some work, the bigger issue is that these names are also assumed to be in the current format by generated bindings code, which does not have the information needed to infer the correct capitalization. See https://cs.chromium.org/chromium/src/out/Debug/gen/third_party/blink/renderer/bindings/core/v8/v8_svg_unit_types.cc?sq=package:chromium&dr=CSs&g=0&l=91 for one instance of a compile-time check that the enum is defined as expected by the IDL file. (Also, I suspect that this issue has languished for 21 months because I never bounced it back to the reporter; fixed that problem here as well :) ) |
||
►
Sign in to add a comment |
||
Comment 1 by schenney@chromium.org
, Mar 7 2017Labels: BugSource-Team PaintTeamTriaged-20170307
Status: Assigned (was: Untriaged)