1088125 - Make CrashSubmit.submit return a Promise

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Biru [:poiru]]

See bug 1024677 comment 17.

Comment 1

[Biru [:poiru]]

Attachment: Part 1: Remove bool return value of CrashSubmit.submit

This particular return value is used in crashes.js, but it is not strictly
neccessary as the submitError callback is called in all cases.

Comment 2

[Biru [:poiru]]

Attachment: Part 2: Use arrow function instead of using separate 'self' variable

Comment 3

[Biru [:poiru]]

Attachment: Part 3: Make CrashSubmit.submit return a Promise

Sorry for the delay.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Thanks, i will take a look this week, but i can't tell yet when exactly.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Sorry for the long delay, we had a work-week and i didn't get to my request queue.

I think these patches would be much easier to look at if they were just one part (e.g. you stop returning a value from submit() in part 1, then add a return again in part 3).

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8543674 [details] [diff] [review]
Part 3: Make CrashSubmit.submit return a Promise

Review of attachment 8543674 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, only smaller issues here :)

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +206,5 @@
> +
> +  this.promise = new Promise((resolve, reject) => {
> +    this.resolvePromise = resolve;
> +    this.rejectPromise = reject;
> +  });

Lets use PromiseUtils.defer() here.

::: toolkit/crashreporter/content/crashes.js
@@ +20,3 @@
>  function submitPendingReport(event) {
>    var link = event.target;
>    var id = link.firstChild.textContent;

Lets make those |let| while we are here.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8543673 [details] [diff] [review]
Part 2: Use arrow function instead of using separate 'self' variable

Review of attachment 8543673 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good and is easy to review as a separate patch. For patch 1 and 3, i'd rather see them folded together though.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Do we have test coverage for CrashSubmit yet?
Have you checked whether all the tests run through?

Comment 9

[Biru [:poiru]]

Attachment: Part 1: Make CrashSubmit.submit return a Promise

Comment 5 and 6 addressed.

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Do we have test coverage for CrashSubmit yet?

We do, indirectly. E.g. the checks added in bug 1061821 ensure that submission through CrashSubmit works as expected.

> Have you checked whether all the tests run through?

Yep, try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbf0eef6960b

Comment 10

[(not currently active) Ted Mielczarek]

CrashSubmit.jsm gets tested by way of about:crashes here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js

(It submits to a JSM in httpd.js.)

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8558345 [details] [diff] [review]
Part 1: Make CrashSubmit.submit return a Promise

Review of attachment 8558345 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good.

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +411,2 @@
>      }
> +    return this.deferredSubmit.promise;

This seems redundant, lets just remove the first of the two returns here.

Comment 12

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6bfd511db87
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d91dfe3f657

https://treeherder.mozilla.org/#/jobs?repo=try&revision=855002dd12a6

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6bfd511db87
https://hg.mozilla.org/mozilla-central/rev/9d91dfe3f657