There appears to be an incompatibility between JaCoCo and Android that manifests itself as a PG error: start with https://stackoverflow.com/a/50997230. We'll have to either work around it or upgrade to tooling that doesn't have the issue; this ticket tracks that work. This is the same issue as https://bugzilla.mozilla.org/show_bug.cgi?id=1490724 worked around. In addition, Bug 1486524 was backed out over this.

Permafail after this merge: https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=8540104bb0bd811f9fd45b160dbe38a67c9aa5cd Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=213062576&repo=mozilla-central&lineNumber=2114 Treeherder: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&searchStr=android-4-0-armv7-api16-ccov%2Cdebug%2Cbuild-android-api-16-ccov%2Fdebug%2C%28b%29&fromchange=8991d660f20e3eea652e060c30e17670b45a9257&tochange=8540104bb0bd811f9fd45b160dbe38a67c9aa5cd&selectedJob=213062576

https://github.com/luongvo/SurveysApp/commit/d7dd6614c09f3e64f2b0e05c20fef354605f05e4 is the work-around pattern. https://issuetracker.google.com/issues/109759777 is the upstream ticket, which is of course closed almost immediately (it is a Google issue tracker, after all).

My take-away from all of this is that this is an Android "not really Java 8" issue. It means that we can't use `default` with `interface` AND keep JaCoCo. I don't think we'd pass on JaCoCo, having gotten it working; but I don't make that call. The immediate work-around is to apply the pattern to https://searchfox.org/mozilla-central/rev/8f89901f2d69d9783f946a7458a6d7ee70635a94/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#2603.

snorp: can you direct traffic?

Ugh. I guess we could do things like 'public abstract class BaseContentDelegate' to work around the lack of default implementations, but I *really* don't want to. It's archaic. I'm inclined to turn of JaCoCo until it gets fixed there or upstream. Having Java code coverage is great, but I think it has dubious value on GeckoView where the vast majority of code that actually runs is native or JS.

The big drawback of abstract class over default impl is that you don't have multiple inheritance in Java. I think this significantly reduces the versatility of our API. IMHO we should prioritize having the best API over having code coverage.

If we make the interfaces public in the API adding methods to the interface is still a breaking change because some embedders might forgo the Base class and implement the interface directly (to avoid the multiple inheritance problem most likely). Also this decision might be hard to reverse in the future. Is there anyone at GeckoView actually looking at the JaCoCo report?

(In reply to Agi [:agi] from comment #7) > If we make the interfaces public in the API adding methods to the interface > is still a breaking change because some embedders might forgo the Base class > and implement the interface directly (to avoid the multiple inheritance > problem most likely). Also this decision might be hard to reverse in the > future. Is there anyone at GeckoView actually looking at the JaCoCo report? Not that I'm aware of, but Sylvestre might know otherwise. Sylvestre?

Marco is the expert here :)

JoCoCo chokes on default implementations in interfaces, so remove those for now.

Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/73385b831880 Remove default implementations in GeckoView interfaces r=nalexander

(In reply to Agi [:agi] from comment #7) > If we make the interfaces public in the API adding methods to the interface > is still a breaking change because some embedders might forgo the Base class > and implement the interface directly (to avoid the multiple inheritance > problem most likely). Also this decision might be hard to reverse in the > future. Is there anyone at GeckoView actually looking at the JaCoCo report? The reports are part of our overall coverage reports that contain C++ and JS too. We don't look at the Java coverage in particular, but only see the overall coverage numbers and how they change. They will also be used in the future for things like "mach try coverage" (a try selector to run all tests covering the code you modified) or to show coverage information on Phabricator at review phase. If we disable Java code coverage, these things will be unavailable to you and to release managers looking at your Java patches to assess if they are properly covered by tests. I don't like that you have to change the API because of coverage instrumentation either, but if you disable it this is what you would lose.