Ted, can you help triage please?

Adding java@

Should we just mandate that uniqueness of Java filenames is a thing?  If you have two tests files, should you have NetworkChangeNotifierIntegrationTest and NetworkChangeNotifierUnitTest?

Having two files with the same name is kind of annoying IMO for IDE users, so I would be fine saying these are legit problems and we should just fix them.

I second tedchoc@'s opinion here. It should be possible to give the classes more helpful names in these cases.

Thirded. Let's make it happen

Isn't it sufficient to mandate that fully qualified names are different?

I guess WebView can prefix our class with "Aw". We do that in other places, but it's kind of redundant given that it's under "com.android.webview.chromium." So I would argue that the fully qualified name is already helpful :)

Actually, specifically in WebView vs. Monochrome case, there's probably a real reason why the classes have the same name... +torne to make sure renaming the class isn't a problem for the stub.

There are a few cases where we define the same class in different file on purpose. E.g.:
LicenseContentProvider.java

We actually use a different implementation in webview vs monochrome. The same is also true for BuildHooksAndroidImpl.java. You need to create a class of that name by design to override the stub one.

Maybe we could just add a whitelist for the files that do this on purpose?

 Issue 778400  has been merged into this issue.

Re tests: I agree that junit vs instrumentation tests should be named differently.

Re the intentional exceptions: if it's a sufficiently short and stable list, then I'd be ok with a whitelist.

For the very specific cases that this is required, it sounds like the list shouldn't be too long, so whitelist SGTM2.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0d4c7f44cd703f0ddd444d725576be9c7ebb875

commit e0d4c7f44cd703f0ddd444d725576be9c7ebb875
Author: Rouslan Solomakhin <rouslan@chromium.org>
Date: Thu Oct 26 17:13:50 2017

[Payments] Deduplicate test name.

Bug:  778442 
Change-Id: I9732fd381b3a8f1081081e7db22e63f8b2937edd
Reviewed-on: https://chromium-review.googlesource.com/739301
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511858}
[modify] https://crrev.com/e0d4c7f44cd703f0ddd444d725576be9c7ebb875/chrome/android/java_sources.gni
[rename] https://crrev.com/e0d4c7f44cd703f0ddd444d725576be9c7ebb875/chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderUnitTest.java

Can we also fix the presubmit check to only warn if you're touching one of the files in question? As it is, I get spammed on every single upload for files that I don't care about--and presumably it's slowing down presubmit on every non-Android platform as well.

Hmm AFAICT checkdeps runs against the diff only, and I suppressed the warning messages to prevent this situation. https://chromium-review.googlesource.com/c/chromium/buildtools/+/727499#message-ed9caf6228c23c3cf2e119730fcb7747d46b3886 I don't get such warnings with my git cl upload. Does anyone see why this is still happening? Maybe gclient sync again to get the latest update on buildtools/?

Even if the warnings aren't emitted, doesn't that mean this checker is incorrectly running when it shouldn't?

I still get the warnings every time I upload. I synced this morning.

So DEPS include_rules work based on file paths, so deps_checker for java has to figure out which file each java import rule is referring to, and when it sees two matching paths, then it gets confused. It does look to not be very smart, and tries to load all java files rather than only ones it needs to look up. Perhaps that can be improved.

But an easy first step is to just skip deps_checker.CheckAddedJavaImports call if added_java_imports is empty, as it will be most of the time for developers who don't work on android.

Okay it's the prescan step that reads all the classes and build self._classmap to check java package dependency later against the diff. It's performed regardless of the changes made in the CL. Needs some work to optimize it.

Granted, I still need to figure out why the change in buildtools is not picked up.

buildtools is pinned on deps. If you change it, you need to roll it to pick up changes there.

Ah, should have rolled DEPS too. On it now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ac8827a09efedf36463e0977f622283e70216fc

commit 3ac8827a09efedf36463e0977f622283e70216fc
Author: Dominick Ng <dominickn@chromium.org>
Date: Tue Oct 31 01:46:45 2017

Deduplicate WebApkUpdateManager and GeolocationHeader test file names.

This CL also runs git cl format on the test files.

BUG= 778442 

Change-Id: If823c7f0dc2c26485102c74223d1c1c6a946f0bb
Reviewed-on: https://chromium-review.googlesource.com/743162
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512699}
[modify] https://crrev.com/3ac8827a09efedf36463e0977f622283e70216fc/chrome/android/java_sources.gni
[rename] https://crrev.com/3ac8827a09efedf36463e0977f622283e70216fc/chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderUnitTest.java
[rename] https://crrev.com/3ac8827a09efedf36463e0977f622283e70216fc/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerUnitTest.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5439c8f373c5f9fded9bad848a4727937b8370b6

commit 5439c8f373c5f9fded9bad848a4727937b8370b6
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Tue Oct 31 02:08:59 2017

Roll buildtools e043d81e91..3275a099f3

Picking up following change:

3275a099f3: Output warning only in verbose mode in Java deps checker

Test: gclient sync

TBR=dpranke@chromium.org

Bug:  778442 
Change-Id: I9a1e348823acd3e8173e2bb53814e2cc5ef461db
Reviewed-on: https://chromium-review.googlesource.com/745481
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512719}
[modify] https://crrev.com/5439c8f373c5f9fded9bad848a4727937b8370b6/DEPS

shenghuazhang@ Assigned to you in case you're interested, since I saw the script was updated by you to work properly. Feel free to assign it back to me if you're not. Thanks.

Sure I would like to work on this! The current logic checks java files even if it's a none-java file CL, this can be eliminated by skipping deps_checker.CheckAddedJavaImports as #17 suggested. I will start from here.

The prescan step of java_checker needs to loop through all java file names under base directories, because it's possible that the added imports are java files from any subdirectory in Chromium tree. So I haven't come up with a way as a good optimization. Welcome comments if you have idea :)

But I think the untouched files related 'multiple definition WARNINGs' could be avoided. That is checked at prescan step, and could let it warn only when conflicting with affected file classes instead of comparing whole tree file names. 

> But I think the untouched files related 'multiple definition WARNINGs' could be avoided. That is checked at prescan step, and could let it warn only when conflicting with affected file classes instead of comparing whole tree file names. 

Yeah that basically. Don't even process a files if it does not match a newly added import line.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/buildtools/+/b0f94d04e26f1cdaf4d2357e0f9eeaffa894b872

commit b0f94d04e26f1cdaf4d2357e0f9eeaffa894b872
Author: Shenghua Zhang <shenghuazhang@chromium.org>
Date: Fri Nov 03 18:03:03 2017

Java checker only warns multiple definitions of touched files

checkdeps calles JavaChecker even if no java file changing in CL. So
change the logic to skip JavaChecker if there is no java related
added_imports exists.
Also, java_checker _PrescanFile warns multiple definitions of all
java files under base subdirectory in verbose mode. Change the logic
to warn for touched files in none-verbose mode.

Bug:778442
Change-Id: I59b9bdc0c6c512a2aa5bfff193ff483a8fbbd746

[modify] https://crrev.com/b0f94d04e26f1cdaf4d2357e0f9eeaffa894b872/checkdeps/java_checker.py
[modify] https://crrev.com/b0f94d04e26f1cdaf4d2357e0f9eeaffa894b872/checkdeps/checkdeps.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/buildtools/+/7f134c70f0f86ed35c2ca1f6b834a48c4ba47d3e

commit 7f134c70f0f86ed35c2ca1f6b834a48c4ba47d3e
Author: Shenghua Zhang <shenghuazhang@chromium.org>
Date: Thu Nov 16 03:00:06 2017

Allow java file multiple definition for excluded paths in java checker

src/PRESUBMIT.py could whitelist java files to allow multiple definition
file names. This cl is the logic in java checker to not show multiple
definition warnings for those whitelisted files.

Bug:778442
Change-Id: I3f4369d611e45d71776183280f009d5522f2a743
[modify] https://crrev.com/7f134c70f0f86ed35c2ca1f6b834a48c4ba47d3e/checkdeps/java_checker.py
[modify] https://crrev.com/7f134c70f0f86ed35c2ca1f6b834a48c4ba47d3e/checkdeps/checkdeps.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bfaa38b87ab0e1105d38dc7551fafc2b573cb3c5

commit bfaa38b87ab0e1105d38dc7551fafc2b573cb3c5
Author: Shenghua Zhang <shenghuazhang@chromium.org>
Date: Thu Nov 16 21:58:02 2017

Add Java file whitelist to allow same name files passing java checker


Bug:  778442 
Change-Id: Ic4d924ff0a36b47d78b7af16bebbe010ce7094c2
Reviewed-on: https://chromium-review.googlesource.com/770612
Commit-Queue: Shenghua Zhang <shenghuazhang@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517202}
[modify] https://crrev.com/bfaa38b87ab0e1105d38dc7551fafc2b573cb3c5/PRESUBMIT.py

Now presubmit will output java multi-definition warnings for:
  1) in verbose mode - all files
  2) in non-verbose mode - only touched files in patch

re #9 > Now we have the whitelist to enable the same java class in different directories on purpose. The whitelist is in src/PRESUBMIT.py, and we can add java file paths into list '_JAVA_MULTIPLE_DEFINITION_EXCLUDED_PATHS' e.g.
https://codesearch.chromium.org/chromium/src/PRESUBMIT.py?l=447

Presubmit won't print multiple definition warnings for whitelist files, even if the files are touched in patch.

Mark this bug as fixed. Feel free to let me know any questions or requests. Thanks!