New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 685405 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

gen/blink/core/StyleBuilderFunctions.cpp refers to some renamed and some skipped names

Project Member Reported by lukasza@chromium.org, Jan 25 2017

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?


 

Comment 1 by dcheng@chromium.org, Jan 25 2017

Cc: sashab@chromium.org
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.
Scratch that - I think I didn't see more things, because only the first 20 errors per compilation are shown... :-(

Comment 4 by sashab@chromium.org, Jan 27 2017

Cc: meade@chromium.org
#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
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).
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.

Comment 7 by sashab@chromium.org, 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.
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",
  ]
}
Owner: lukasza@chromium.org
Status: Started (was: Available)
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
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment