gen/blink/core/StyleBuilderFunctions.cpp refers to some renamed and some skipped names |
||||
Issue description
If I don't change anything in
- third_party/WebKit/Source/build/scripts/make_style_builder.py
- third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl
- third_party/WebKit/Source/core/css/CSSProperties.in
and in particular don't change the following make_style_builder.py lines...
set_if_none(property, 'getter',
lower_first(name) if simple_type_name != name
else 'get' + name)
set_if_none(property, 'setter', 'set' + name)
...
set_if_none(property, 'initial', 'initial' + name)
..., then after a dry-run of the Great Blink Rename I get compile errors like this:
gen/blink/core/StyleBuilderFunctions.cpp:33:79:
error: no member named 'fillRule' in 'blink::SVGComputedStyle'; did you mean 'FillRule'?
...
../../third_party/WebKit/Source/core/style/SVGComputedStyle.h:337:12: note: 'FillRule' declared here
gen/blink/core/StyleBuilderFunctions.cpp:37:35:
error: no member named 'setFillRule' in 'blink::SVGComputedStyle'; did you mean 'SetFillRule'?
...
../../third_party/WebKit/Source/core/style/SVGComputedStyle.h:140:8: note: 'SetFillRule' declared here
but if I change these make_style_builder.py lines to say "Get", "Set", "Initial" (and to avoid using |lower_first(name)| for the getter), then I get a different set of compiler errors:
gen/blink/core/StyleBuilderFunctions.cpp:1065:18:
error: no member named 'SetWritingMode' in 'blink::ComputedStyle'; did you mean 'setWritingMode'?
...
gen/blink/core/ComputedStyleBase.h:258:8: note: 'setWritingMode' declared here
gen/blink/core/StyleBuilderFunctions.cpp:1065:54:
error: no member named 'GetWritingMode' in 'blink::ComputedStyle'; did you mean 'getWritingMode'?
...
gen/blink/core/ComputedStyleBase.h:257:15: note: 'getWritingMode' declared here
Now - obviously I could try changing how gen/blink/core/ComputedStyleBase.h gets generated (by switching "set" to "Set", etc. in make_computed_style_base.h), but this would introduce yet another set of failures:
../../third_party/WebKit/Source/core/style/ComputedStyle.h:3142:37:
error: use of undeclared identifier 'getWritingMode'; did you mean 'GetWritingMode'?
...
gen/blink/core/ComputedStyleBase.h:257:15: note: 'GetWritingMode' declared here
So - maybe this is something that we should try to preland/prerename manually?
,
Jan 27 2017
Actually, there aren't that many CSS properties affected: - cx, setCx, initialCx - fillRule, setFillRule, initialFillRule - variantNumeric, setVariantNumeric, initialVariantNumeric - vectorEffect, setVectorEffect, initialVectorEffect - convertFontVariantNumeric So - I think there might be good alternatives here: Alternative #1: After (or rather during) the Great Blink Rename, we can tweak CSSProperties.in, to explicitly spell setter=SetFillRule, initial=InitialFillRule, etc. Alternative #2: We can blacklist renaming of affected methods using the --method-blocklist parameter. I am leaning toward #2 above.
,
Jan 27 2017
Scratch that - I think I didn't see more things, because only the first 20 errors per compilation are shown... :-(
,
Jan 27 2017
#1 is not a completely terrible idea, since I'd rather that than have a whole bunch of style stuff blacklisted. Your original plan seemed fine to me. Why did you stop at make_computed_style_base.py? We already do this in make_computed_style_base.py: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/build/scripts/make_computed_style_base.py?rcl=0&l=110 (you can change this to uppercase 'Get' if that's the style you're after) Maybe upload your partial patch and we can help debug it. That last error seems like you missed a rename -- not sure if you updated make_computed_style_base.py fully -- but that should be the last bit and then your rename should work. +meade who should know this is blocking your work
,
Jan 28 2017
RE: #c4: why not the original plan from the top of this bug The problem is that: 1) It is easy make_computed_style_base.py and make_style_builder.py *consistently* generate either "set" or "Set" prefix for setters 2) The clang tool today only rewrites references to methods declared in non-generated code (so some methods stay as "setFoo" and some are rewritten to "SetFoo"). This means that regardless of which way I flip make_computed_style_base.py and make_style_builder.py, there will be a bunch of non-rewritten references to the names (because some of the names will be declared in the generated ComputedStyleBase.h and some in non-generated SVGComputedStyle.h). The decision to postpone rewriting things declared in generated is not written in stone, but so far has served as well. I plan to see on Monday how much effort it would be to preland the changes (i.e. flip to "Set" and "Initial" and "Reset" and "Get" in the code generators and semi-manually fix the fallout).
,
Jan 28 2017
To try expand on #c5 - I think there are 2 promising approaches here: 1. Try to manually rename things in make_style_builder.py and make_computed_style_base.py, so that they generate calls to methods that are spelled in PascalCased style. This is most important for getters, "setFoo", "initialFoo" (and for consistency is probably also desirable for "resetFoo"). The potential risk/problem with this approach is that there can be quite a few places in non-generated code that would also have to be manually fixed to match. 2. Try to make sure that all declarations of getters, "setFoo", "initialFoo" (at least the ones referenced from gen/blink/core/StyleBuilderFunctions.cpp) are in generated code. The comment at the top of SVGComputedStyle.h sounds promising here - maybe it is possible to just generate (into ComputedStyleBase.h?) the methods that are currently declared in the non-generated SVGComputedStyle.h.
,
Jan 28 2017
"The potential risk/problem with this approach is that there can be quite a few places in non-generated code that would also have to be manually fixed to match." Could we update the rest with the clang tool in the same patch? Then we could write a partial patch to fix the generated code, send it to you/you tell us how to run the script, then we do the full rename.
,
Jan 29 2017
For #c6.1. we could use the clang tool, but I think it might be simpler and more efficient to try to
0) change the generator
1) compile with -ferror-limit=0 passed (*) to the clang compiler and with -k 29999 passed to ninja (to gather all the errors, not just the first few)
2) use the errors to fix up the sources - I hope that there is a way to automate applying clang's fixit suggestions (and if not, it should be fairly simple to massage the errors into sed commands).
If the above won't work for any reason, and we want to try using the clang tool, then
1) the docs for using the clang tool are at docs/clang_tool_refactoring.md
2) we would probably need to locally/temporarily tweak the tool so that
2a) it does take into account declarations from generated code (probably just deleting |unless(hasCanonicalDecl(isExpansionInFileMatching(kGeneratedFileRegex)))| in RewriteToChromeStyle.cpp) and
2b) it ignores everything other than methods in the affected classes.
- I am not sure if blanked renaming *all* methods if okay - we might want to only rename a subset of methods affected by generator tweaks.
- I am not sure how to do this exactly, but it is probably possible to tweak |method_decl_matcher| (it should automatically affect |method_ref_matcher| and |method_member_matcher|).
(*)
--- a/build/config/clang/BUILD.gn
+++ b/build/config/clang/BUILD.gn
@@ -58,5 +58,7 @@ config("extra_warnings") {
"-Wstring-conversion",
"-Wtautological-overlap-compare",
+
+ "-ferror-limit=0",
]
}
,
Feb 1 2017
,
Feb 2 2017
I am able to build after the Great Blink Rename, using a combination of: 1. Manual fixes stashed at https://crrev.com/2329463004 - in particular fixes for third_party/WebKit/Source/build/scripts/make_computed_style_base.py third_party/WebKit/Source/build/scripts/make_media_features.py third_party/WebKit/Source/build/scripts/make_style_builder.py This part is not landable right now (it needs to be done as part of the Great Blink Rename). 2. Tweaks to the renaming tool to A) continue skipping generated code, except B) rename identifiers declared in (generated) ComputedStyleBase.h This means that identifiers used from generators tweaked in 1 above will be consistently renamed, regardless if they are declared in the generated ComputedStyleBase.h or in non-generated SVGComputedStyle.h or ComputedStyle.h. The tweaks are at https://crrev.com/2667273003
,
Feb 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1eb45fbe3b9ef87e457f3bdb044a9f19a5e20543 commit 1eb45fbe3b9ef87e457f3bdb044a9f19a5e20543 Author: lukasza <lukasza@chromium.org> Date: Thu Feb 09 01:20:55 2017 Make the tool rename identifiers declared in (generated) ComputedStyleBase.h Some style properties are manually declared (e.g. SVGComputedStyle.h), while others are declared in a generated file (ComputedStyleBase.h). To make sure everything is consistently renamed, this CL explicitly allows the rename tool to process style identifiers, even if they live in the generated ComputedStyleBase.h file. The consistent renaming of style properties is needed to consistently/sanely adjust make_style_builder.py (which deals with both kinds of style properties [generated vs non-generated] in a unified way). For more explanation please see also https://crrev.com/2667273003/#msg6. BUG= 685405 Review-Url: https://codereview.chromium.org/2667273003 Cr-Commit-Position: refs/heads/master@{#449176} [modify] https://crrev.com/1eb45fbe3b9ef87e457f3bdb044a9f19a5e20543/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
,
Feb 10 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by dcheng@chromium.org
, Jan 25 2017