Make the tool rename identifiers declared in (generated) ComputedStyleBase.h

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}
Committed: https://chromium.googlesource.com/chromium/src/+/1eb45fbe3b9ef87e457f3bdb044a9f19a5e20543

[Łukasz Anforowicz, 2017-02-02 01:06:15]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2017-02-02 01:06:16]

dcheng@, can you PTAL?

https://codereview.chromium.org/2667273003/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2667273003/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:516: //
(third_party/WebKit/Source/core/frame/Settings.h).
This TODO is probably not that important.  Things work as-is, so I don't want to
rock the boat too much.

[dcheng, 2017-02-02 05:19:58]

I'm probably missing something, but why do we want to change the generated file?
Doesn't the next generation pass just blow away the results anyway?

[Łukasz Anforowicz, 2017-02-02 17:12:44]

On 2017/02/02 05:19:58, dcheng wrote:
> I'm probably missing something, but why do we want to change the generated
file?
> Doesn't the next generation pass just blow away the results anyway?

If we want to avoid splitting G2 (to generate different code for
Case-1 and Case-2, then we need to make sure that identifiers in
Case-1 and Case-2 follow the same naming convention.  This CL
achieves this by making sure that renaming applied by the clang tool
will be consistently applied to both Case-1 and Case-2.  I've tested
that this approach works (i.e. avoids build errors after the big
rename).  I think this approach is reasonable (e.g. it means that
the big rename will go a little bit farther than previously expected
in unifying blink and chromium styles - i.e. this approach covers a
bit more of generated code which seems like a desirable thing in the
long term).  I think this approach is more efficient than
alternatives (e.g. I've tried to see what it would take to preland
changes to G2 and G1 and gave up).

[Łukasz Anforowicz, 2017-02-02 17:13:17]

On 2017/02/02 17:12:44, Łukasz Anforowicz wrote:
> On 2017/02/02 05:19:58, dcheng wrote:
> > I'm probably missing something, but why do we want to change the generated
> file?
> > Doesn't the next generation pass just blow away the results anyway?
> 
> If we want to avoid splitting G2 (to generate different code for
> Case-1 and Case-2, then we need to make sure that identifiers in
> Case-1 and Case-2 follow the same naming convention.  This CL
> achieves this by making sure that renaming applied by the clang tool
> will be consistently applied to both Case-1 and Case-2.  I've tested
> that this approach works (i.e. avoids build errors after the big
> rename).  I think this approach is reasonable (e.g. it means that
> the big rename will go a little bit farther than previously expected
> in unifying blink and chromium styles - i.e. this approach covers a
> bit more of generated code which seems like a desirable thing in the
> long term).  I think this approach is more efficient than
> alternatives (e.g. I've tried to see what it would take to preland
> changes to G2 and G1 and gave up).

Ooops.  Please ignore.  Copy& paste error apparently... :-(

[Łukasz Anforowicz, 2017-02-02 17:13:30]

On 2017/02/02 05:19:58, dcheng wrote:
> I'm probably missing something, but why do we want to change the generated
file?
> Doesn't the next generation pass just blow away the results anyway?

This is not a problem, because we will also change the generator
(make_computed_style_base.py + ComputedStyleBase*.tmpl - let's call
this generator G1).

   Side note: This will be ensured by covering generator G1 changes
   in https://crrev.com/2329463004 (referenced from "step-by-step
   doc" in section 5.1);  BTW - I encourage you to take a look at
   this other CL (of stashed manual changes to apply together with
   the big automatic rename) - this other CL has to touch more than
   one generator (and more generators than this one and the DOM
   bindings one).

The problem and the reason for the bug and this CL is that the
*other* generator (make_style_builder.py +
StyleBuilderFunctions*.tmpl - let's call this generator G2) uses a
single template that is used for generating code referencing to
*both* Case-1) identifiers declared in generated code (code
generated by G1 - i.e.  gen/blink/core/ComputedStyleBase.h) and
Case-2) identifiers declared in non-generated code (i.e.
third_party/WebKit/Source/core/style/SVGComputedStyle.h and
third_party/WebKit/Source/core/style/ComputedStyle.h).

If we want to avoid splitting G2 (to generate different code for
Case-1 and Case-2, then we need to make sure that identifiers in
Case-1 and Case-2 follow the same naming convention.  This CL
achieves this by making sure that renaming applied by the clang tool
will be consistently applied to both Case-1 and Case-2.  I've tested
that this approach works (i.e. avoids build errors after the big
rename).  I think this approach is reasonable (e.g. it means that
the big rename will go a little bit farther than previously expected
in unifying blink and chromium styles - i.e. this approach covers a
bit more of generated code which seems like a desirable thing in the
long term).  I think this approach is more efficient than
alternatives (e.g. I've tried to see what it would take to preland
changes to G2 and G1 and gave up).

[dcheng, 2017-02-08 19:42:22]

LGTM, but update the CL description with more details about the motivation.
Something along the lines of:

Some style properties are manually defined, while others are defined
in a generated file. To make sure everything is consistently
renamed, explicitly allow the rename tool to process style
identifiers, even if they live in a generated file.

[Łukasz Anforowicz, 2017-02-08 20:05:47]

Description was changed from

==========
Make the tool rename identifiers declared in (generated) ComputedStyleBase.h

BUG=685405
==========

to

==========
Make the tool rename identifiers declared in (generated) ComputedStyleBase.h

Some style properties are manually defined, while others are defined in
a generated file. To make sure everything is consistently renamed,
explicitly allow the rename tool to process style identifiers, even if
they live in a generated file.  For a more detailed explanation please
also see https://crrev.com/2667273003/#msg6.

BUG=685405
==========

[Łukasz Anforowicz, 2017-02-08 20:06:11]

On 2017/02/08 19:42:22, dcheng wrote:
> LGTM, but update the CL description with more details about the motivation.
> Something along the lines of:
> 
> Some style properties are manually defined, while others are defined
> in a generated file. To make sure everything is consistently
> renamed, explicitly allow the rename tool to process style
> identifiers, even if they live in a generated file.

Good point - done.

[Łukasz Anforowicz, 2017-02-08 21:06:54]

Description was changed from

==========
Make the tool rename identifiers declared in (generated) ComputedStyleBase.h

Some style properties are manually defined, while others are defined in
a generated file. To make sure everything is consistently renamed,
explicitly allow the rename tool to process style identifiers, even if
they live in a generated file.  For a more detailed explanation please
also see https://crrev.com/2667273003/#msg6.

BUG=685405
==========

to

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

[Łukasz Anforowicz, 2017-02-08 21:08:51]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2017-02-08 21:10:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 21:48:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 21:48:47]

Failed to apply patch for
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:
While running git apply --index -p1;
  error: patch failed:
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:497
  error: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp: patch
does not apply

Patch:       tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
Index: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
index
025fbdcc3d0389ad6ef5603228142beee8ec1841..3f44b500b524a064725f763f76d2448ec0907b92
100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -51,6 +51,8 @@ namespace {
 const char kBlinkFieldPrefix[] = "m_";
 const char kBlinkStaticMemberPrefix[] = "s_";
 const char kGeneratedFileRegex[] = "^gen/|/gen/";
+const char kGeneratedFileExclusionRegex[] =
+    "(^gen/|/gen/).*/ComputedStyleBase\\.h$";
 const char kGMockMethodNamePrefix[] = "gmock_";
 const char kMethodBlocklistParamName[] = "method-blocklist";
 
@@ -497,6 +499,41 @@ AST_MATCHER(clang::VarDecl, isKnownTraitName) {
   return IsKnownTraitName(Node.getName());
 }
 
+AST_MATCHER(clang::Decl, isDeclInGeneratedFile) {
+  // This matcher mimics the built-in isExpansionInFileMatching matcher from
+  // llvm/tools/clang/include/clang/ASTMatchers/ASTMatchers.h, except:
+  // - It special cases some files (e.g. doesn't skip renaming of identifiers
+  //   from gen/blink/core/ComputedStyleBase.h)
+
+  const clang::SourceManager& source_manager =
+      Node.getASTContext().getSourceManager();
+
+  // TODO(lukasza): Consider using getSpellingLoc below.
+  // The built-in isExpansionInFileMatching matcher uses getExpansionLoc below.
+  // We could consider using getSpellingLoc (which properly handles things like
+  // SETTINGS_GETTERS_AND_SETTERS macro which is defined in generated code
+  // (gen/blink/core/SettingsMacros.h), but expanded in non-generated code
+  // (third_party/WebKit/Source/core/frame/Settings.h).
+  clang::SourceLocation loc =
+      source_manager.getExpansionLoc(Node.getLocStart());
+
+  // TODO(lukasza): jump out of scratch space if token concatenation was used.
+  if (loc.isInvalid())
+    return false;
+
+  const clang::FileEntry* file_entry =
+      source_manager.getFileEntryForID(source_manager.getFileID(loc));
+  if (!file_entry)
+    return false;
+
+  static llvm::Regex exclusion_regex(kGeneratedFileExclusionRegex);
+  if (exclusion_regex.match(file_entry->getName()))
+    return false;
+
+  static llvm::Regex generated_file_regex(kGeneratedFileRegex);
+  return generated_file_regex.match(file_entry->getName());
+}
+
 // Helper to convert from a camelCaseName to camel_case_name. It uses some
 // heuristics to try to handle acronyms in camel case names correctly.
 std::string CamelCaseToUnderscoreCase(StringRef input) {
@@ -1469,7 +1506,7 @@ int main(int argc, const char* argv[]) {
   auto in_blink_namespace = decl(
       anyOf(decl_under_blink_namespace, decl_has_qualifier_to_blink_namespace,
             hasAncestor(decl_has_qualifier_to_blink_namespace)),
-     
unless(hasCanonicalDecl(isExpansionInFileMatching(kGeneratedFileRegex))));
+      unless(hasCanonicalDecl(isDeclInGeneratedFile())));
 
   // Field, variable, and enum declarations ========
   // Given

[Łukasz Anforowicz, 2017-02-09 01:05:08]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-02-09 01:05:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2667273003/#ps20001
(title: "Rebasing...")

[commit-bot: I haz the power, 2017-02-09 01:05:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 01:21:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486602308186550,
"parent_rev": "6f274770ac90254c94f1fe41e4b3e3d9187db221", "commit_rev":
"1eb45fbe3b9ef87e457f3bdb044a9f19a5e20543"}

[commit-bot: I haz the power, 2017-02-09 01:22:35]

Description was changed from

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

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/1eb45fbe3b9ef87e457f3bdb044a...
==========

[commit-bot: I haz the power, 2017-02-09 01:22:37]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/1eb45fbe3b9ef87e457f3bdb044a...