Clang plugin: Support for Blink in web/.

Clang plugin: Detect Blink code by "blink" or "WTF" namespace

We used to detect Blink code by paths including third_party/WebKit, and it won't
work if we move the Blink code to elsewhere. This CL removes the patch check for
third_party/WebKit, and adds namespace checks instead.


BUG=622551
Review-Url: https://codereview.chromium.org/2749043004
Cr-Commit-Position: refs/heads/master@{#465175}
Committed: https://chromium.googlesource.com/chromium/src/+/f9774e4bbc3820cff53edeb95f5562ae69dc391a

[tkent, 2017-03-15 23:22:46]

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

[commit-bot: I haz the power, 2017-03-15 23:23:45]

Dry run: 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-03-15 23:53:56]

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

[commit-bot: I haz the power, 2017-03-15 23:53:57]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-03-15 23:59:04]

tkent@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org

[tkent, 2017-03-15 23:59:05]

avi@, dcheng@, would you review this please?

[tkent, 2017-03-16 00:00:04]

Description was changed from

==========
Clang plugin: Support for Blink in web/.

BUG=622551
==========

to

==========
Clang plugin: Support for Blink in web/.

Also, renaming enforce-in-thirdparty-webkit flag to enforce-in-blink.

BUG=622551
==========

[dcheng, 2017-03-16 00:05:19]

I actually have a CL in-progress right now to enable more checks in Blink. Would
you be OK with waiting for that CL and then rebasing on top of that?

[tkent, 2017-03-16 00:16:21]

On 2017/03/16 at 00:05:19, dcheng wrote:
> I actually have a CL in-progress right now to enable more checks in Blink.
Would you be OK with waiting for that CL and then rebasing on top of that?

Sure. I'll wait for it.

[Avi (use Gerrit), 2017-03-16 00:33:18]

This LGTM; do whatever you need to make Daniel happy.

https://codereview.chromium.org/2749043004/diff/1/tools/clang/plugins/ChromeC...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/2749043004/diff/1/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.cpp:245: }
To be clear, the first branch of the if() has an implicit "allow /web/" but
that's implied because it's top-level, and the second branch has an implicit
"ban /third_party/WebKit/" but that's also implied by the ban of /third_party/.?

[tkent, 2017-03-16 00:35:58]

https://codereview.chromium.org/2749043004/diff/1/tools/clang/plugins/ChromeC...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/2749043004/diff/1/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.cpp:245: }
On 2017/03/16 at 00:33:18, Avi wrote:
> To be clear, the first branch of the if() has an implicit "allow /web/" but
that's implied because it's top-level, and the second branch has an implicit
"ban /third_party/WebKit/" but that's also implied by the ban of /third_party/.?

That's right.
I'll add comments.

[tkent, 2017-03-28 00:39:27]

On 2017/03/16 at 00:16:21, tkent wrote:
> On 2017/03/16 at 00:05:19, dcheng wrote:
> > I actually have a CL in-progress right now to enable more checks in Blink.
Would you be OK with waiting for that CL and then rebasing on top of that?
> 
> Sure. I'll wait for it.

dcheng, did you finish your CL?

[dcheng, 2017-03-29 00:32:12]

On 2017/03/28 00:39:27, tkent wrote:
> On 2017/03/16 at 00:16:21, tkent wrote:
> > On 2017/03/16 at 00:05:19, dcheng wrote:
> > > I actually have a CL in-progress right now to enable more checks in Blink.
> Would you be OK with waiting for that CL and then rebasing on top of that?
> > 
> > Sure. I'll wait for it.
> 
> dcheng, did you finish your CL?

I was investigating a build time regression I thought I caused... but it turns
out I didn't. I need to do some more tests; I'll send out the CL tonight. Sorry
for the delay.

[tkent, 2017-04-12 23:26:01]

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

[tkent, 2017-04-12 23:26:52]

did rebase.  dcheng@, would you review this please?

[commit-bot: I haz the power, 2017-04-12 23:27:06]

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

[tkent, 2017-04-12 23:33:13]

Description was changed from

==========
Clang plugin: Support for Blink in web/.

Also, renaming enforce-in-thirdparty-webkit flag to enforce-in-blink.

BUG=622551
==========

to

==========
Clang plugin: Support for Blink in web/.

BUG=622551
==========

[dcheng, 2017-04-12 23:35:27]

https://codereview.chromium.org/2749043004/diff/40001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/2749043004/diff/40001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:237:
blink_directories_.emplace("/web/");
Is it possible that this will cause us to match things that it shouldn't? For
instance, we have files in the /web/ directory in the gen files directory:
https://cs.chromium.org/search/?q=file:/gen/+file:/web/+package:%5Echromium$&...

[commit-bot: I haz the power, 2017-04-12 23:40:42]

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

[commit-bot: I haz the power, 2017-04-12 23:40:43]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-04-13 00:18:00]

https://codereview.chromium.org/2749043004/diff/40001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/2749043004/diff/40001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:237:
blink_directories_.emplace("/web/");
On 2017/04/12 at 23:35:27, dcheng wrote:
> Is it possible that this will cause us to match things that it shouldn't? For
instance, we have files in the /web/ directory in the gen files directory:
https://cs.chromium.org/search/?q=file:/gen/+file:/web/+package:%5Echromium$&...

Oh, it's possible.  I also found src/ios/web/, which should not be
LocationType::kBlink.
Is there a way to get Chromium repository root in
ChromeClassTester::ClassifyLocation()?

[tkent, 2017-04-13 00:32:38]

https://codereview.chromium.org/2749043004/diff/40001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/2749043004/diff/40001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:237:
blink_directories_.emplace("/web/");
On 2017/04/13 at 00:18:00, tkent wrote:
> On 2017/04/12 at 23:35:27, dcheng wrote:
> > Is it possible that this will cause us to match things that it shouldn't?
For instance, we have files in the /web/ directory in the gen files directory:
https://cs.chromium.org/search/?q=file:/gen/+file:/web/+package:%5Echromium$&...
> 
> Oh, it's possible.  I also found src/ios/web/, which should not be
LocationType::kBlink.
> Is there a way to get Chromium repository root in
ChromeClassTester::ClassifyLocation()?

I think switching behavior by a plugin argument is more flexible.
   e.g. -Xclang -plugin-arg-find-bad-constructs -Xclang check-for-blink
What do you think?

[dcheng, 2017-04-13 02:49:32]

On 2017/04/13 00:32:38, tkent wrote:
>
https://codereview.chromium.org/2749043004/diff/40001/tools/clang/plugins/Chr...
> File tools/clang/plugins/ChromeClassTester.cpp (right):
> 
>
https://codereview.chromium.org/2749043004/diff/40001/tools/clang/plugins/Chr...
> tools/clang/plugins/ChromeClassTester.cpp:237:
> blink_directories_.emplace("/web/");
> On 2017/04/13 at 00:18:00, tkent wrote:
> > On 2017/04/12 at 23:35:27, dcheng wrote:
> > > Is it possible that this will cause us to match things that it shouldn't?
> For instance, we have files in the /web/ directory in the gen files directory:
>
https://cs.chromium.org/search/?q=file:/gen/+file:/web/+package:%5Echromium$&...
> > 
> > Oh, it's possible.  I also found src/ios/web/, which should not be
> LocationType::kBlink.
> > Is there a way to get Chromium repository root in
> ChromeClassTester::ClassifyLocation()?
> 
> I think switching behavior by a plugin argument is more flexible.
>    e.g. -Xclang -plugin-arg-find-bad-constructs -Xclang check-for-blink
> What do you think?

Hmm, how would the plugin argument help with the problem of determining if the
/web/ directory is the right one or not?
I guess the right thing to do is to try to normalize the path somehow...

[tkent, 2017-04-13 03:56:00]

> > > > Is it possible that this will cause us to match things that it
shouldn't?
> > For instance, we have files in the /web/ directory in the gen files
directory:
> >
https://cs.chromium.org/search/?q=file:/gen/+file:/web/+package:%5Echromium$&...
> > > 
> > > Oh, it's possible.  I also found src/ios/web/, which should not be
> > LocationType::kBlink.
> > > Is there a way to get Chromium repository root in
> > ChromeClassTester::ClassifyLocation()?
> > 
> > I think switching behavior by a plugin argument is more flexible.
> >    e.g. -Xclang -plugin-arg-find-bad-constructs -Xclang check-for-blink
> > What do you think?
> 
> Hmm, how would the plugin argument help with the problem of determining if the
/web/ directory is the right one or not?
> I guess the right thing to do is to try to normalize the path somehow...

If we can obtain Chromium repository root path, it's not difficult to classify
right /web/ in ClassifyLocation(). Otherwise, we need to add checks against
hard-coded paths.  e.g. Return kBlink if a path contains /web/ but no /ios/, no
/gen/ ...
If we have a plugin argument, we can just update cflags in
third_party/WebKit/Source/BUILD.gn, and do not need to have hard-coded path in
the plugin.  It work well even after we move third_party/WebKit to anywhere.

[tkent, 2017-04-17 00:44:43]

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

[commit-bot: I haz the power, 2017-04-17 00:44:49]

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

[tkent, 2017-04-17 00:46:32]

On 2017/04/13 at 03:56:00, tkent wrote:
> > > I think switching behavior by a plugin argument is more flexible.
> > >    e.g. -Xclang -plugin-arg-find-bad-constructs -Xclang check-for-blink
> > > What do you think?

I implemented it in Patch Set 4.

[tkent, 2017-04-17 00:48:33]

Description was changed from

==========
Clang plugin: Support for Blink in web/.

BUG=622551
==========

to

==========
Clang plugin: Add "check-for-blink" plugin argument.

It enables limited set of checks for Blink code. We should add the argument
in BUILD.gn for Blink code.

BUG=622551
==========

[commit-bot: I haz the power, 2017-04-17 00:54:42]

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

[commit-bot: I haz the power, 2017-04-17 00:54:43]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-17 02:41:00]

https://codereview.chromium.org/2749043004/diff/60001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/2749043004/diff/60001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:160: if (options_.check_for_blink)
Hmm, so the potential issue with this is that Blink files include other
third_party code--and we want those SourceLocations to still be treated as
kThirdParty. Otherwise, this could cause the style checker plugin to warn on
third-party code if a Blink file includes a header from third_party.

I've been thinking about it more, and I have two main ideas to fix this:
- Use a namespace check to determine if something should be Blink or not. e.g.
namespace blink == Blink, otherwise something else
- Specify "web/public", "web/Source" as the blink directory paths.

I kind of prefer using "namespace blink" as the main check if possible, though
that's a bit clunky to implement atm. I think once
https://chromium-review.googlesource.com/c/478907/ land, this should be pretty
easy to do though. WDYT?

[dcheng, 2017-04-17 03:11:07]

On 2017/04/17 02:41:00, dcheng wrote:
>
https://codereview.chromium.org/2749043004/diff/60001/tools/clang/plugins/Chr...
> File tools/clang/plugins/ChromeClassTester.cpp (right):
> 
>
https://codereview.chromium.org/2749043004/diff/60001/tools/clang/plugins/Chr...
> tools/clang/plugins/ChromeClassTester.cpp:160: if (options_.check_for_blink)
> Hmm, so the potential issue with this is that Blink files include other
> third_party code--and we want those SourceLocations to still be treated as
> kThirdParty. Otherwise, this could cause the style checker plugin to warn on
> third-party code if a Blink file includes a header from third_party.
> 
> I've been thinking about it more, and I have two main ideas to fix this:
> - Use a namespace check to determine if something should be Blink or not. e.g.
> namespace blink == Blink, otherwise something else
> - Specify "web/public", "web/Source" as the blink directory paths.
> 
> I kind of prefer using "namespace blink" as the main check if possible, though
> that's a bit clunky to implement atm. I think once
> https://chromium-review.googlesource.com/c/478907/ land, this should be pretty
> easy to do though. WDYT?

(Actually I think this is probably easy enough to do independent of that CL.)

[tkent, 2017-04-17 07:43:47]

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

[commit-bot: I haz the power, 2017-04-17 07:43:56]

Dry run: 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-04-17 07:51:58]

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

[commit-bot: I haz the power, 2017-04-17 07:51:59]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-04-17 08:00:23]

https://codereview.chromium.org/2749043004/diff/60001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/2749043004/diff/60001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:160: if (options_.check_for_blink)
On 2017/04/17 at 02:40:57, dcheng wrote:
> Hmm, so the potential issue with this is that Blink files include other
third_party code--and we want those SourceLocations to still be treated as
kThirdParty. Otherwise, this could cause the style checker plugin to warn on
third-party code if a Blink file includes a header from third_party.

Ah, I see.

> I've been thinking about it more, and I have two main ideas to fix this:
> - Use a namespace check to determine if something should be Blink or not. e.g.
namespace blink == Blink, otherwise something else

Patch Set 5 implemented this.

> - Specify "web/public", "web/Source" as the blink directory paths.

We'd like to omit "Source/". So, this would be "web/public, web/bindings,
web/core, web/devtools, web/modules, web/platform, web/web, or web/wtf".  It
wouldn't be robust for Blink-internal structure changes.  I'd like to assume
it's the last fallback option.

[dcheng, 2017-04-17 17:39:07]

LGTM, but please check two things:

1) How does this affect build times locally? It's not necessary to build
everything, but maybe just something like content_shell.

2) I assume you've confirmed that this doesn't break anything in a local build
(not sure if we have trybots for clang changes now? +hans, +thakis for insight
there... otherwise, you can just watch the clang fyi bots here:
https://build.chromium.org/p/chromium.fyi/console)

[Nico, 2017-04-17 17:42:19]

No trybot.

On Apr 17, 2017 1:39 PM, <dcheng@chromium.org> wrote:

> LGTM, but please check two things:
>
> 1) How does this affect build times locally? It's not necessary to build
> everything, but maybe just something like content_shell.
>
> 2) I assume you've confirmed that this doesn't break anything in a local
> build
> (not sure if we have trybots for clang changes now? +hans, +thakis for
> insight
> there... otherwise, you can just watch the clang fyi bots here:
> https://build.chromium.org/p/chromium.fyi/console)
>
> https://codereview.chromium.org/2749043004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tkent, 2017-04-18 08:09:09]

On 2017/04/17 at 17:39:07, dcheng wrote:
> LGTM, but please check two things:
> 
> 1) How does this affect build times locally? It's not necessary to build
everything, but maybe just something like content_shell.

Build with local clang:

Without this CL:
% time ninja -C out/ptest content_shell
ninja: Entering directory `out/ptest'
[20903/20903] LINK ./content_shell
ninja -C out/ptest content_shell  46980.70s user 1460.09s system 738% cpu
1:49:18.64 total

With this CL:
% time ninja -C out/ptest content_shell
ninja: Entering directory `out/ptest'
[20903/20903] LINK ./content_shell
ninja -C out/ptest content_shell  47006.66s user 1527.12s system 742% cpu
1:49:00.32 total

I observed no significant difference.

> 2) I assume you've confirmed that this doesn't break anything in a local build
(not sure if we have trybots for clang changes now? +hans, +thakis for insight
there... otherwise, you can just watch the clang fyi bots here:
https://build.chromium.org/p/chromium.fyi/console)

Yes, I confirmed.

[tkent, 2017-04-18 08:09:37]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-04-18 08:09:38]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2749043004/#ps80001
(title: "namespace approach")

[commit-bot: I haz the power, 2017-04-18 08:09:52]

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

[tkent, 2017-04-18 08:10:50]

The CQ bit was unchecked by tkent@chromium.org

[tkent, 2017-04-18 08:13:30]

Description was changed from

==========
Clang plugin: Add "check-for-blink" plugin argument.

It enables limited set of checks for Blink code. We should add the argument
in BUILD.gn for Blink code.

BUG=622551
==========

to

==========
Clang plugin: Detect Blink code by "blink" or "WTF" namespace

We used to detect Blink code by paths including third_party/WebKit, and it won't
work if we move the Blink code to elsewhere. This CL removes the patch check for
third_party/WebKit, and adds namespace checks instead.


BUG=622551
==========

[tkent, 2017-04-18 08:13:44]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-04-18 08:13:58]

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-04-18 08:22:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492503224806130,
"parent_rev": "db9cef9964f34067223f3d95f8a0206a3d07254b", "commit_rev":
"f9774e4bbc3820cff53edeb95f5562ae69dc391a"}

[commit-bot: I haz the power, 2017-04-18 08:22:34]

Description was changed from

==========
Clang plugin: Detect Blink code by "blink" or "WTF" namespace

We used to detect Blink code by paths including third_party/WebKit, and it won't
work if we move the Blink code to elsewhere. This CL removes the patch check for
third_party/WebKit, and adds namespace checks instead.


BUG=622551
==========

to

==========
Clang plugin: Detect Blink code by "blink" or "WTF" namespace

We used to detect Blink code by paths including third_party/WebKit, and it won't
work if we move the Blink code to elsewhere. This CL removes the patch check for
third_party/WebKit, and adds namespace checks instead.


BUG=622551

Review-Url: https://codereview.chromium.org/2749043004
Cr-Commit-Position: refs/heads/master@{#465175}
Committed:
https://chromium.googlesource.com/chromium/src/+/f9774e4bbc3820cff53edeb95f55...
==========

[commit-bot: I haz the power, 2017-04-18 08:22:36]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f9774e4bbc3820cff53edeb95f55...