-
Notifications
You must be signed in to change notification settings - Fork 578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update crashlytics global data collection #1434
Conversation
Coverage ReportAffected SDKsNo changes between base commit (f1e32c1) and head commit (462d9124). Test Logs
NotesHTML coverage reports can be produced locally with Head commit (462d9124) is created by Prow via merging commits: f1e32c1 e29abd6. |
The public api surface has changed for the subproject firebase-crashlytics: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (462d9124) is created by Prow via merging commits: f1e32c1 e29abd6. |
The public api surface has changed for the subproject firebase-crashlytics: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
The public api surface has changed for the subproject firebase-crashlytics: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, pls get an lgtm from the crashlytics team
crashlyticsDataCollectionEnabled = enabled; | ||
crashlyticsDataCollectionExplicitlySet = true; | ||
sharedPreferences.edit().putBoolean(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED, enabled).commit(); | ||
public synchronized void setCrashlyticsDataCollectionEnabled(Boolean enabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can help our future selves by adding some comments here :)
arbiter.setCrashlyticsDataCollectionEnabled(false); | ||
assertFalse(arbiter.isAutomaticDataCollectionEnabled()); | ||
|
||
when(mockPrefs.contains(PREFS_KEY)).thenReturn(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you split this up into separate tests that express the conditions they check?
...ics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java
Show resolved
Hide resolved
...ics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java
Outdated
Show resolved
Hide resolved
/test device-check-changed |
@@ -463,4 +463,23 @@ public boolean didCrashOnPreviousExecution() { | |||
public void setCrashlyticsCollectionEnabled(boolean enabled) { | |||
core.setCrashlyticsCollectionEnabled(enabled); | |||
} | |||
|
|||
/** | |||
* Enables/disables/clears automatic data collection config by Crashlytics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest replacing line 468 with:
"* Allows you to enable or disable the automatic data collection configuration in Crashlytics."
/** | ||
* Enables/disables/clears automatic data collection config by Crashlytics. | ||
* | ||
* <p>If this is set, it overrides the data collection settings provided by the Android Manifest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest replacing lines 470 and 471 with:
"* If not set, defaults to True
. When set to False
, disables automatic collection by overriding the data collections settings in the AndroidManifest.xml, as well as any Firebase-wide automatic data collection settings. When set to null
, clears the Crashlytics data collection configuration (in this case, enabling data collection becomes dependent on other settings)."
* disabled. Use {@link #deleteUnsentReports()} to delete any reports stored on the device without | ||
* sending them to Crashlytics. | ||
* | ||
* @param enabled whether to enable automatic data collection. The value of null would clear the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest replacing line 479 with:
"@param
Determines whether to enable or disable automatic data collection. If set to null
, clears the Crashlytics data collection configuration."
No description provided.