For #3439 - Add "report" action to crash notification #4087

rocketsroger · 2019-08-09T21:31:21Z

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

pocmo · 2019-08-12T13:21:03Z

components/lib/crash/src/main/java/mozilla/components/lib/crash/handler/CrashHandlerService.kt

+            if (skipPrompt) {
+                val notificationManager = getSystemService(Context.NOTIFICATION_SERVICE)
+                        as NotificationManager
+                notificationManager.cancelAll()


Cancelling all notification is a bit aggressive here.

I'll look into if I can just cancel the crash notification. Thanks,

pocmo · 2019-08-12T13:22:59Z

components/lib/crash/src/main/java/mozilla/components/lib/crash/handler/CrashHandlerService.kt

+                val notificationManager = getSystemService(Context.NOTIFICATION_SERVICE)
+                        as NotificationManager
+                notificationManager.cancelAll()
+                sendBroadcast(Intent(Intent.ACTION_CLOSE_SYSTEM_DIALOGS))


What's the reason for this broadcast here? 🤔

Once the report button is clicked, it retracts the notification drawer. I will agree if you think this is not needed.

pocmo · 2019-08-12T13:30:07Z

components/lib/crash/src/main/java/mozilla/components/lib/crash/handler/CrashHandlerService.kt

@@ -29,10 +31,18 @@ class CrashHandlerService : IntentService(WORKER_THREAD_NAME) {

        intent.extras?.let { extras ->
            val crash = Crash.NativeCodeCrash.fromBundle(extras)
+            val skipPrompt = extras.getBoolean(Crash.INTENT_EXTRA_SKIP_PROMPT, false)
+
+            if (skipPrompt) {


I'm concerned about reusing the CrashHandlerService class here: It's main purpose is to receive crashes from GeckoView (hence the Crash.NativeCodeCrash.fromBundle() above) and forward it to the CrashReporter instance.

With this patch it also receives crashes from the notification (which may not be Crash.NativeCodeCrash!). Since we kill this process immediately I wonder if we'd even be able to report any crashes (which may happen asynchronously).

We may want to consider using CrashReporter.submitReport() (like CrashReporterActivity does) and wait for it to completely. Not sure from where though? Maybe a foreground service? Or a workmanager worker (which would still need a service to schedule it?)? A workmanager worker would have the benefit that we could fix some of our other lib-crash issues (e.g. CrashReporterActivity blocking, CrashReporterActivity trying only once, ..). What do you think?

I see, I've misunderstood the role of CrashHandlerService then. I'll look into both the foreground service and the worker idea. Thanks,

pocmo

This looks already pretty great :)

components/lib/crash/src/main/AndroidManifest.xml

...nents/lib/crash/src/main/java/mozilla/components/lib/crash/notification/CrashNotification.kt

pocmo · 2019-08-13T14:36:08Z

...nents/lib/crash/src/main/java/mozilla/components/lib/crash/notification/CrashNotification.kt

+        } else {
+            PendingIntent.getService(
+                    context, REPORT_REQUEST_CODE, CrashPrompt.createReportIntent(context, crash), 0
+            )


nit: We could consider moving the creation of such a pending intent behind a helper. Maybe even in support-utils.

Had some issues moving this to support-util. Couldn't find a clean way because of the Crash class dependency.

opened #4113 to track this.

components/lib/crash/src/main/java/mozilla/components/lib/crash/prompt/CrashPrompt.kt

...nents/lib/crash/src/main/java/mozilla/components/lib/crash/service/SendCrashReportService.kt

pocmo

Looks great! :)

pocmo · 2019-08-14T12:16:38Z

...nents/lib/crash/src/main/java/mozilla/components/lib/crash/notification/CrashNotification.kt

 internal class CrashNotification(
    private val context: Context,
    private val crash: Crash,
    private val configuration: CrashReporter.PromptConfiguration
 ) {
    fun show() {
        val pendingIntent = PendingIntent.getActivity(
-            context, 0, CrashPrompt.createIntent(context, crash), 0
+                context, 0, CrashPrompt.createIntent(context, crash), 0


No need to change that in the commit. But this indentation seems to come from the default AS formatting that is a bit different than what ktlint does.

You can get Android Studio to follow ktlint (on Mac) like this:

brew install ktlint ktlint --apply-to-idea-project --android

After applying and restarting android studio. It still wants to do double intent for this line. I'll look into why. Thanks for the ktlint apply commands.

components/lib/crash/src/main/res/values/strings.xml

pocmo

Something looks not right here. There are now 29 commits in this PR.

rocketsroger · 2019-08-14T14:47:37Z

Something looks not right here. There are now 29 commits in this PR.

Yes, something went wrong when I reorder git history on my branch. Let me take a look. Thanks,

pocmo · 2019-08-14T16:12:10Z

Looks good now. Can you re-force-push your branch (or rebase) so that we do not get all those merge commits merged in? I just disabled the "update branch" button so that it's now gone :D

rocketsroger · 2019-08-14T18:36:24Z

Looks good now. Can you re-force-push your branch (or rebase) so that we do not get all those merge commits merged in? I just disabled the "update branch" button so that it's now gone :D

Done. Only 1 commit now for this change. Please let me know if its not clean. Thanks,

pocmo

bors r+

4087: For #3439 - Add "report" action to crash notification r=pocmo a=rocketsroger ### Pull Request checklist  - [x] **Quality**: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended) - [x] **Tests**: This PR includes thorough tests or an explanation of why it does not - [x] **Changelog**: This PR includes [a changelog entry](https://github.com/mozilla-mobile/android-components/blob/master/docs/changelog.md) or does not need one - [x] **Accessibility**: The code in this PR follows [accessibility best practices](https://github.com/mozilla-mobile/shared-docs/blob/master/android/accessibility_guide.md) or does not include any user facing features ### After merge - [ ] **Milestone**: Make sure issues closed by this pull request are added to the [milestone](https://github.com/mozilla-mobile/android-components/milestones) of the version currently in development. - [ ] **Breaking Changes**: If this is a breaking change, please push a draft PR on [Reference Browser](https://github.com/mozilla-mobile/reference-browser) to address the breaking issues. Co-authored-by: Roger Yang <[email protected]>

bors · 2019-08-15T17:03:03Z

Build succeeded

Taskcluster (push)

rocketsroger force-pushed the AC_3439 branch from 70e63e7 to 864f920 Compare August 9, 2019 21:54

rocketsroger marked this pull request as ready for review August 9, 2019 21:54

rocketsroger requested a review from a team as a code owner August 9, 2019 21:54

pocmo added the 🕵️‍♀️ needs review PRs that need to be reviewed label Aug 12, 2019

pocmo self-assigned this Aug 12, 2019

pocmo reviewed Aug 12, 2019

View reviewed changes

rocketsroger force-pushed the AC_3439 branch 2 times, most recently from b66a29e to a83d17b Compare August 12, 2019 22:58

pocmo reviewed Aug 13, 2019

View reviewed changes

rocketsroger force-pushed the AC_3439 branch from 43579c1 to 6eebb4b Compare August 13, 2019 18:48

pocmo approved these changes Aug 14, 2019

View reviewed changes

rocketsroger force-pushed the AC_3439 branch from c766015 to 5abcdf7 Compare August 14, 2019 14:40

rocketsroger requested a review from a team as a code owner August 14, 2019 14:40

pocmo suggested changes Aug 14, 2019

View reviewed changes

rocketsroger force-pushed the AC_3439 branch from 4f4c70c to 71d7a21 Compare August 14, 2019 14:59

Dexterp37 removed the request for review from a team August 14, 2019 15:07

rocketsroger requested a review from pocmo August 14, 2019 15:19

pocmo closed this Aug 14, 2019

pocmo reopened this Aug 14, 2019

For mozilla-mobile#3439 - Add "report" action to crash notification

3908909

rocketsroger force-pushed the AC_3439 branch from 7db02f8 to 3908909 Compare August 14, 2019 18:31

pocmo approved these changes Aug 15, 2019

View reviewed changes

bors bot merged commit 3908909 into mozilla-mobile:master Aug 15, 2019

rocketsroger deleted the AC_3439 branch August 15, 2019 17:12

rocketsroger added this to the 9.0.0 📷 milestone Aug 15, 2019

rocketsroger removed the 🕵️‍♀️ needs review PRs that need to be reviewed label Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #3439 - Add "report" action to crash notification #4087

For #3439 - Add "report" action to crash notification #4087

rocketsroger commented Aug 9, 2019 •

edited

Loading

pocmo Aug 12, 2019

rocketsroger Aug 12, 2019

pocmo Aug 12, 2019

rocketsroger Aug 12, 2019

pocmo Aug 12, 2019

rocketsroger Aug 12, 2019

pocmo left a comment

pocmo Aug 13, 2019

rocketsroger Aug 13, 2019

rocketsroger Aug 13, 2019

pocmo left a comment

pocmo Aug 14, 2019

rocketsroger Aug 14, 2019

pocmo left a comment

rocketsroger commented Aug 14, 2019

pocmo commented Aug 14, 2019

rocketsroger commented Aug 14, 2019

pocmo left a comment

bors bot commented Aug 15, 2019

For #3439 - Add "report" action to crash notification #4087

For #3439 - Add "report" action to crash notification #4087

Conversation

rocketsroger commented Aug 9, 2019 • edited Loading

Pull Request checklist

After merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocmo left a comment

Choose a reason for hiding this comment

rocketsroger commented Aug 14, 2019

pocmo commented Aug 14, 2019

rocketsroger commented Aug 14, 2019

pocmo left a comment

Choose a reason for hiding this comment

bors bot commented Aug 15, 2019

Build succeeded

rocketsroger commented Aug 9, 2019 •

edited

Loading