rewrite_to_chrome_style: Consistently decide if things are in blink.

rewrite_to_chrome_style: Consistently decide if things are in blink.

The matcher rules would check IsInBlink and then exclude stuff in gen/.
But the method-matcher is special and it has to look at overrides, and
it would look check IsInBlink but would not exclude gen/.

2ndly, the matcher rules would check that any ancestor namespace was
blink or WTF, but the method-matcher would only check the first
namespace.

This makes them consistent. IsInBlinkOrWTF will always return false for
things in gen/, both for the matcher rules and for the method-matcher.
And both will check any ancestor namespace, not just the next namespace.

R=dcheng
BUG=593446
Committed: https://crrev.com/e8821e2f26fadc019c3b82807854d03ae203d5de
Cr-Commit-Position: refs/heads/master@{#380720}

[danakj, 2016-03-10 00:39:17]

Added a test where we have a method in blink { nestednamespace {} }

and we override it. it should override.

[danakj, 2016-03-10 00:42:56]

Added a test for gen/ with methods also.

[danakj, 2016-03-10 00:43:10]

Patchset #2 (id:20001) has been deleted

[danakj, 2016-03-10 02:04:31]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 02:04:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785563002/40001

[commit-bot: I haz the power, 2016-03-10 02:48:24]

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

[commit-bot: I haz the power, 2016-03-10 02:48:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2016-03-10 19:10:31]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-03-10 19:11:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785563002/40001

[commit-bot: I haz the power, 2016-03-10 19:11:07]

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

[commit-bot: I haz the power, 2016-03-10 19:11:08]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[danakj, 2016-03-10 19:54:06]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 19:54:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785563002/40001

[commit-bot: I haz the power, 2016-03-10 20:34:30]

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

[commit-bot: I haz the power, 2016-03-10 20:34:31]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-03-11 09:03:57]

https://codereview.chromium.org/1785563002/diff/40001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/1785563002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:91: const
clang::SourceLocation& location,
Nit: pass by value.

https://codereview.chromium.org/1785563002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:139:
overrides_non_blink = true;
I wonder if we can express more of this logic in matcher form. I'm just hoping
that this will help keep the matching logic  located mostly in one place. The
reason this was split up is because I wasn't very comfortable writing custom
matchers, but now that we've added a few, I think it might make sense to
consolidate this.

What I'm thinking is:

1. Expand the in_blink_namespace we build to just exclude gen stuff, rather than
making that a separate matcher that we have to add into each thing we want to
match on.

2. Add a custom  matcher for matching overrides that uses control flow like we
have here, but makes it more generic. The custom matcher will take another
matcher, and run it through the overrides / method itself using this same logic.
I don't have a good name for it, but for strawman purposes, let's call it
overrideUltimately().

3. Then in method_decl_matcher, we do something like
overrideUltimately(in_blink_namespace).

As a bonus, we won't have to plumb SourceManager around either. WDYT?

[danakj, 2016-03-11 19:27:09]

PTAL

https://codereview.chromium.org/1785563002/diff/40001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/1785563002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:91: const
clang::SourceLocation& location,
On 2016/03/11 09:03:57, dcheng wrote:
> Nit: pass by value.

Done.

[dcheng, 2016-03-11 19:35:54]

lgtm

https://codereview.chromium.org/1785563002/diff/80001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/1785563002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:90: bool
IsDeclContextInBlinkOrWTF(const clang::DeclContext* decl_context,
We can make this just about wtf now I think.

https://codereview.chromium.org/1785563002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:124: // also one
ancestor method but not the other. If we rename one of the and
 Nit: no also

[danakj, 2016-03-11 19:58:06]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-03-11 19:58:06]

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/1785563002/#ps120001
(title: "rewrite-asserts: review")

[danakj, 2016-03-11 19:58:21]

https://codereview.chromium.org/1785563002/diff/80001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/1785563002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:90: bool
IsDeclContextInBlinkOrWTF(const clang::DeclContext* decl_context,
On 2016/03/11 19:35:54, dcheng wrote:
> We can make this just about wtf now I think.

So we can.

https://codereview.chromium.org/1785563002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:124: // also one
ancestor method but not the other. If we rename one of the and
On 2016/03/11 19:35:54, dcheng wrote:
>  Nit: no also

Done.

[commit-bot: I haz the power, 2016-03-11 19:58:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785563002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785563002/120001

[commit-bot: I haz the power, 2016-03-11 20:43:59]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-03-11 20:45:32]

Description was changed from

==========
rewrite_to_chrome_style: Consistently decide if things are in blink.

The matcher rules would check IsInBlink and then exclude stuff in gen/.
But the method-matcher is special and it has to look at overrides, and
it would look check IsInBlink but would not exclude gen/.

2ndly, the matcher rules would check that any ancestor namespace was
blink or WTF, but the method-matcher would only check the first
namespace.

This makes them consistent. IsInBlinkOrWTF will always return false for
things in gen/, both for the matcher rules and for the method-matcher.
And both will check any ancestor namespace, not just the next namespace.

R=dcheng
BUG=593446
==========

to

==========
rewrite_to_chrome_style: Consistently decide if things are in blink.

The matcher rules would check IsInBlink and then exclude stuff in gen/.
But the method-matcher is special and it has to look at overrides, and
it would look check IsInBlink but would not exclude gen/.

2ndly, the matcher rules would check that any ancestor namespace was
blink or WTF, but the method-matcher would only check the first
namespace.

This makes them consistent. IsInBlinkOrWTF will always return false for
things in gen/, both for the matcher rules and for the method-matcher.
And both will check any ancestor namespace, not just the next namespace.

R=dcheng
BUG=593446

Committed: https://crrev.com/e8821e2f26fadc019c3b82807854d03ae203d5de
Cr-Commit-Position: refs/heads/master@{#380720}
==========

[commit-bot: I haz the power, 2016-03-11 20:45:33]

Patchset 6 (id:??) landed as
https://crrev.com/e8821e2f26fadc019c3b82807854d03ae203d5de
Cr-Commit-Position: refs/heads/master@{#380720}