Implement CrashAnnotator interface for 1P Android #4929
Conversation
hlwarriner
requested review from
zhongqiliang,
johnxwork and
haozheng-cobalt
hlwarriner
force-pushed
the
java-impl
branch
from
7ac3b33 to
9e0980c
Compare
hlwarriner
force-pushed
the
java-impl
branch
from
9e0980c to
f732efc
Compare
...id/apk/app/src/main/java/dev/cobalt/browser/crashannotator/CrashAnnotatorImplFirstParty.java
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/coat/StarboardBridge.java
Outdated
Show resolved
Hide resolved
Sounds good!
For unit testing, all I can really think of is a unit test for For integration/e2e testing, there's an existing coverage gap: YTS tests that key/value pairs passed to this web API are propagated to crash reports, but only for Evergreen platforms. I'll follow up with the YTS team about closing this gap.
From https://source.android.com/docs/core/tests/debug/understanding-logging#log-level-guidelines it sounds like INFO logs are always kept? Anyway, I think it's fine to remove this (and just did). |
cobalt/android/apk/app/src/app/java/dev/cobalt/app/MainActivity.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/app/java/dev/cobalt/app/MainActivity.java
Outdated
Show resolved
Hide resolved
|
I uploaded #4967 as an alternative approach that we can consider. I still feel it would be good to enforce the Java StarboardBridge class as a singleton but it sounds like it might take some time for us to verify that it's safe to do so. |
We ended up going with a different approach (see fourth commit in this current PR). #4967 will be closed. |
There was a problem hiding this comment.
LGTM. Might worth checking if CrashAnnotatorImplFirstParty and StarboardBridge(CobaltActivity) are triggered on the same thread. We have no guarantee so far that the JavaBridge is on the same thread as the activity(though it's entirely possible). If we need to deal with multi-thread, we could use some mutex.
An enum is used to enforce the singleton property of StarboardBridge. Based on discussion with haozheng@ and colinliang@ this Java class, like its C++ counterpart, probably should be treated like a singleton. And implementing it as a singleton makes it easier for Java callers to use it. b/383301493
colinliang@ suggested this approach so that we can avoid enforcing the singleton property for StarboardBridge at this time. A todo comment is added to consider doing that later, when we have more time to test it and ensure that it will not introduce any race conditions. If/when we do enforce the singleton property for StarboardBridge we can decide if we want to keep the separate CrashContext singleton or move the crash context map back into StarboardBridge.
That's a great point. I confirmed the clients of |
The crash context map is extracted from StarboardBridge to a new CrashContext singleton, which @zhongqiliang suggested so that we can avoid making StarboardBridge a singleton at this time. An enum is used to enforce the singleton property.
I still feel that we should enforce the singleton property for the StarboardBridge Java class, as we currently assume that it has a single instance and we enforce this property for its corresponding C++ class. I added a todo comment for us to consider doing this later.
b/383301493