rewrite_to_chrome_style: Header files included *only* from generated code are not getting rewritten |
|||||||
Issue descriptionThis is a bit similar to issue 640688 , but seems separate. Example - no renames in third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h: #include "platform/weborigin/SecurityOrigin.h" #include "url/mojo/origin.mojom-blink.h" #include "wtf/text/WTFString.h" namespace mojo { template <> struct StructTraits<url::mojom::blink::Origin::DataView, RefPtr<::blink::SecurityOrigin>> { static WTF::String scheme(const RefPtr<::blink::SecurityOrigin>& origin) { return origin->protocol(); // <- HERE!!! This should say |Protocol|. }
,
Sep 2 2016
,
Sep 2 2016
Hmmm.... I am not able to repro with the test below (i.e. |origin->protocol()| *does* get renamed to |origin->Protocol()|, so I am thinking that I must be using the tool wrong when running against the code base...
Attempted test (no repro / test passes):
namespace blink {
namespace mojo_trait_test_support {
class RefCountedBase {
public:
void ref() const {};
};
template <typename T>
class RefCounted : public RefCountedBase {
public:
void deref() const {}
};
template <typename T>
class RefPtr {
public:
T* operator->() const { return nullptr; }
};
class SecurityOrigin : public RefCounted<SecurityOrigin> {
public:
int protocol() const { return 123; }
};
} // namespace mojo_trait_test_support
} // namespace blink
namespace mojo {
template <typename DataViewType, typename T>
struct StructTraits;
using RefPtrToSecurityOrigin = ::blink::mojo_trait_test_support::RefPtr<
::blink::mojo_trait_test_support::SecurityOrigin>;
template <>
struct StructTraits<int, RefPtrToSecurityOrigin> {
public:
static int scheme(const RefPtrToSecurityOrigin& origin) {
return origin->protocol();
}
};
} // namespace mojo
,
Sep 3 2016
FWIW, when running the tool with --all, I am getting quite a few errors like below:
Failed to process /usr/local/google/home/lukasza/src/chromium4/src/out/gn_no_asan/gen/services/ui/public/interfaces/accessibility_manager.mojom-blink.cc
warning: ../../third_party/llvm-build/Release+Asserts/bin/clang++: 'linker' input unused
error: error reading 'gen/services/ui/public/interfaces/accessibility_manager.mojom-blink.cc'
1 error generated.
Error while processing /usr/local/google/home/lukasza/src/chromium4/src/out/gn_no_asan/gen/services/ui/public/interfaces/accessibility_manager.mojom-blink.cc.
Repro steps (AFAIR) were:
1. git rebase-update
2. gclient sync
3. rm rf third_party/llvm-build
4. tools/clang/scripts/update.py
5. ninja -C out/gn_no_asan ... gn_all
6. tools/clang/scripts/update.py --bootstrap --force-local-build --without-android \
--tools blink_gc_plugin plugins rewrite_to_chrome_style
7. tools/clang/scripts/run_tool.py rewrite_to_chrome_style --generate-compdb out/gn_no_asan --all
(if I run without --all, then I get only 1 or 2 tool running failures in total;
right now I am at 4.89% and I already saw 9 failures)
,
Nov 30 2016
,
Nov 30 2016
Interestingly, I see that
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.h
third_party/WebKit/public/web/WindowFeaturesStructTraits.h
got some renames/rewrites applied by
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style --generate-compdb out/gn
OTOH, I see that there are other files that haven't been touched despite looking quite similar:
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
third_party/WebKit/Source/platform/mojo/KURLStructTraits.h
And the fact that something really straightforward (like the example below) wasn't rewritten, seems to support the hypothesis, that the problem is not *in* the rewrite_to_chrome_style tool, but rather in the *way* the tool is invoked (or not invoked). Simple example that should have been rewritten in SecurityOriginStructTraits.h:
// Invocation of create method below can't get much simpler, but it wasn't rewritten.
*out = ::blink::SecurityOrigin::create(scheme, host, data.port());
I'll just fold edits to the 2 files above into a manual-post-processing CL that I am stashing/accumulating locally, but I am somewhat hesitant to also add these edits to the CL shared at https://codereview.chromium.org/2329463004.
,
Dec 1 2016
It sounds like they aren't in the compile DB. A quick look at //third_party/WebKit/Source/platform/BUILD.gn shows that they are not listed there either. (Is there some way for GN to warn for unlisted source files?)
,
Dec 1 2016
,
Dec 2 2016
FWIW, I've tried adding the missing headers to BUILD.gn files ...
diff --git a/third_party/WebKit/Source/platform/BUILD.gn b/third_party/WebKit/Source/platform/BUILD.gn
index b562a1cf..4cf5a22 100644
--- a/third_party/WebKit/Source/platform/BUILD.gn
+++ b/third_party/WebKit/Source/platform/BUILD.gn
@@ -1120,8 +1120,12 @@ component("platform") {
"mhtml/MHTMLParser.cpp",
"mhtml/MHTMLParser.h",
"mojo/CommonCustomTypesStructTraits.cpp",
+ "mojo/CommonCustomTypesStructTraits.h",
"mojo/GeometryStructTraits.cpp",
+ "mojo/GeometryStructTraits.h",
+ "mojo/KURLStructTraits.h",
"mojo/MojoHelper.h",
+ "mojo/SecurityOriginStructTraits.h",
"network/ContentSecurityPolicyParsers.cpp",
"network/ContentSecurityPolicyParsers.h",
"network/ContentSecurityPolicyResponseHeaders.cpp",
diff --git a/third_party/WebKit/public/BUILD.gn b/third_party/WebKit/public/BUILD.gn
index 660d2fb..abd0b67 100644
--- a/third_party/WebKit/public/BUILD.gn
+++ b/third_party/WebKit/public/BUILD.gn
@@ -379,6 +379,7 @@ source_set("blink_headers") {
"platform/modules/screen_orientation/WebLockOrientationCallback.h",
"platform/modules/screen_orientation/WebLockOrientationError.h",
"platform/modules/screen_orientation/WebScreenOrientationClient.h",
+ "platform/modules/screen_orientation/WebScreenOrientationEnumTraits.h",
"platform/modules/screen_orientation/WebScreenOrientationLockType.h",
"platform/modules/screen_orientation/WebScreenOrientationType.h",
"platform/modules/serviceworker/WebNavigationPreloadState.h",
... and after running ...
$ gclient sync
$ gn gen out/gn
$ ninja -C out/gn … -k 9999 gn_all # some errors but nothing alarming to me...
$ tools/clang/scripts/update.py --bootstrap --force-local-build \
--without-android --tools blink_gc_plugin plugins rewrite_to_chrome_style
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style \
--generate-compdb out/gn # failure when processing media/cast/test/simulator.cc
... I still see no changes in third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
,
Dec 3 2016
1) Did you check if it appears in the compile DB? 2) I'm guessing it's not included from any non-generated file, so the tooling might never run over it.
,
Dec 14 2016
Yes - I think that #2 is what is happening here: - include.*NonElementParentNode.h has only hits under out/.../gen - include.*SecurityOriginStructTraits.h has only hits under out/.../gen
,
Dec 15 2016
One problem here is that run_tool.py uses the same set of arguments to control both 1) the set of input files that are "compiled" by the tool and 2) the set of files that the edits are applied to. Splitting the "generate" from "apply edits" phases ( issue 598138 ) will help. In the meantime, I plan tweak run_tool.py to stop filtering the set of files that the edits are applied to if --all is used.
,
Dec 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d857418db3173e922d97cefe2346724922c8acd6 commit d857418db3173e922d97cefe2346724922c8acd6 Author: lukasza <lukasza@chromium.org> Date: Fri Dec 16 17:34:55 2016 Fix filtering of where edits are applied when --all switch is used. Before this CL, when --all switch was used, then |filenames| 1) wouldn't include any header filenames (which should get rewritten) 2) would include generated files (which shouldn't get rewritten) This would lead to incorrect filtering of edits/files that are passed to _ApplyEdits function. The new filtering behavior helps to ensure that we apply edits to header files that are only included from generated files. Doing this requires 1) running the tool on generated files (this is what --all switch accomplishes even before this CL) and 2) ensuring that edits are applied to the right files (this is what this CL does). Examples of headers that are only included from generated files can be found in https://crbug.com/643779#c11 BUG= 643779 Review-Url: https://codereview.chromium.org/2583623002 Cr-Commit-Position: refs/heads/master@{#439136} [modify] https://crrev.com/d857418db3173e922d97cefe2346724922c8acd6/tools/clang/scripts/run_tool.py
,
Dec 28 2016
This is no longer an issue after #c13 above + when running run_tool.py with --all switch. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by lukasza@chromium.org
, Sep 2 2016