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

Issue 643779 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

rewrite_to_chrome_style: Header files included *only* from generated code are not getting rewritten

Project Member Reported by lukasza@chromium.org, Sep 2 2016

Issue description

This 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|.
    }
 
I have two hypothesis right now:

1. Something wrong about finding the right decl of |protocol| (i.e. it goes through RefPtr template - maybe we don't see that templates are also |in_blink_namespace|...).

2. Wrong invocation of the tool (i.e. maybe this header is not included from any of the things the tool has been run against).  This is how I run the tool:

tools/clang/scripts/run_tool.py rewrite_to_chrome_style --generate-compdb out/gn_no_asan

Please shout if you think this is a wrong way to invoke the tool (e.g. maybe I should use --all?  AFAIR I've run into some trouble when trying to use --all, but maybe I should try again).
Description: Show this description
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

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)
Cc: lukasza@chromium.org
 Issue 669937  has been merged into this issue.
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. 
Cc: brucedaw...@chromium.org scottmg@chromium.org
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?)
Summary: rewrite_to_chrome_style: Some headers are not listed as source files in BUILD.gn (was: rewrite_to_chrome_style: The tool misses things referring to blink namespace that are outside of blink namespace)
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
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.
Summary: rewrite_to_chrome_style: Header files included *only* from generated code are not getting rewritten (was: rewrite_to_chrome_style: Some headers are not listed as source files in BUILD.gn)
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
Cc: nasko@chromium.org
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.
Project Member

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

Status: Fixed (was: Untriaged)
This is no longer an issue after #c13 above + when running run_tool.py with --all switch.

Sign in to add a comment