548667 - rewrite CrashSubmit.jsm to use FormData

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

sicking implemented the FormData API in bug 546528, we should use it instead of submitting a <form>.

Comment 1

[(not currently active) Ted Mielczarek]

From bug 574357, dolske notes:
(In reply to comment #6)
> (From update of attachment 453935 [details] [diff] [review])
> >-  submit: function CrashSubmit_submit(id, element, submitSuccess, submitError)
> >+  submit: function CrashSubmit_submit(id, element, submitSuccess, submitError,
> >+                                      noThrottle)
> 
> It's kind of unfortunate that this is a static method (in the C++ sense)... It
> would be much better to be able to do:
> 
>   let submission = new CrashSubmit(id, element);
>   submission.onSuccess = callback; // optional
>   submission.newoption = false;    // also optional
> 
> There's a bit of risk in the current code with getting arguments in the wrong
> order... Because existing callers have not been changed to explicitly pass in
> noThrottle==false, the next time an argument is added someone will just append
> it to one of these existing calls and it'll be passed in as noThrottle.
> 
> Maybe spin this out to a new bug? It's ok for now.

If someone fixes this, they should refactor the interface while they're there.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Submit crash reports via FormData.

I know this doesn't refactor the interface, but I wasn't really feeling like it. If you insist, I'll do it.

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 543261 [details] [diff] [review]
Submit crash reports via FormData.

Derp, try tests aren't passing even though they were locally.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Submit crash reports via FormData.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Future-proof CrashSubmit.submit against future argument additions.

I decided to play with the interface during a 10 hour car ride from New York to Toronto. How's this look?

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 543570 [details] [diff] [review]
Submit crash reports via FormData.

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

Lovely cleanup. Thanks!

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +244,5 @@
> +    let xhr = new this.window.XMLHttpRequest();
> +    xhr.open("POST", reportData.ServerURL, true);
> +    delete reportData.ServerURL;
> +
> +    let formData = new this.window.FormData();

Do the constructors for these not exist in JS Modules? That's unfortunate. Is there a bug filed on that?

Also, since the only thing you need is |window| now, can you change the interface + callers to just pass in a window instead?

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 543839 [details] [diff] [review]
Future-proof CrashSubmit.submit against future argument additions.

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

Nice, thanks a lot!

Comment 8

[Alfonso Martinez]

(In reply to comment #6)
> ::: toolkit/crashreporter/CrashSubmit.jsm
> @@ +244,5 @@
> > +    let xhr = new this.window.XMLHttpRequest();
> > +    xhr.open("POST", reportData.ServerURL, true);
> > +    delete reportData.ServerURL;
> > +
> > +    let formData = new this.window.FormData();
> 
> Do the constructors for these not exist in JS Modules? That's unfortunate.
> Is there a bug filed on that?

For XHR it's documented here:
https://developer.mozilla.org/En/Using_XMLHttpRequest#Using_XMLHttpRequest_from_JavaScript_modules_.2f_XPCOM.c2.a0components

I don't know any way to get a FormData object in a JS Module (that's what I'm searching now), this bug request the feature: https://bugzilla.mozilla.org/show_bug.cgi?id=672690

According to https://developer.mozilla.org/en/Extensions/Using_the_DOM_File_API_in_chrome_code , since Gecko 6.0 it should be possible to use File(path) from chrome code, but the File() isn't available in a module

Comment 9

[Josh Matthews [:jdm]]

Attachment: Submit crash reports via FormData.

Comment 10

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/03e942f652c1
http://hg.mozilla.org/integration/mozilla-inbound/rev/5cabd0a02373

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/5cabd0a02373
http://hg.mozilla.org/mozilla-central/rev/03e942f652c1