mozilla / nightlytt

Nightly Tester Tools
https://wiki.mozilla.org/Auto-tools/Automation_Development/Projects/Addons/NightlyTesterTools
Other
61 stars 37 forks source link

Why don't you use FormData instead of multipartFormData.js? #176

Closed KOLANICH closed 7 years ago

KOLANICH commented 9 years ago

Why don't you use FormData instead of multipartFormData.js?

xabolcs commented 9 years ago

Partially covered in PR #93.

whimboo commented 9 years ago

Which other places would need an update?

KOLANICH commented 9 years ago

Screenshot uploader, of course. PS you don't need to fix IH, I think we should replace it with imgur.

xabolcs commented 9 years ago

PS you don't need to fix IS, I think we should replace it with imgur.

See Issue #49 and the PR for that!

whimboo commented 9 years ago

Ok, so both cases when we make use of MultipartFormData are listed now. So once those issues are fixed we can also remove the multipartFormData.js file, right? If that is the case we can update the summary of this issue to reflect the removal of this module.

xabolcs commented 9 years ago

So once those issues are fixed we can also remove the multipartFormData.js file, right?

@whimboo, removing this module is equal to upping the <minVersion> of toolkit@mozilla.org and it's friends at least to 2.0, a.k.a. Issue #161.

whimboo commented 9 years ago

Then this issue also depends on #161, which can be done after the next release I think.

xabolcs commented 9 years ago

The migration of screenshot uploader isn't trivial.

xabolcs commented 9 years ago

The migration of screenshot uploader isn't trivial.

Because nsIWebNavigation.loadURI() needs nsIInputStream for postData and for headers and FormData doesn't know about those.

KOLANICH commented 9 years ago

Can we use Blob there?

xabolcs commented 9 years ago

Can we use Blob there?

No need for Blob, IMHO. Picture data are transmitted as base64, so FormData would be good at screenshot upload, but screenshot uploader currently uses webNavigation.loadURI() instead xhr.send().

Of course there is a way to rewrite screenshot uploader to use xhr to allow this multipartFormData FormData change, but I think this wouldn't be the correct way. This transition would shrink not just userbase (Gecko 2.0 and up), but the functionality too: FormData does support xhr.send() only. For example see this SO question: How to pass formData to postData in loadOneTab? and the referenced answer from @nmaier.

KOLANICH commented 9 years ago

Base64 is evil, let's send as binary.

KOLANICH commented 9 years ago

how-to-pass-formdata-to-postdata-in-loadonetab

why not just extract redirection url via getResponseHeader() and open tab for it? Or just use API?

xabolcs commented 9 years ago

why not just extract redirection url via getResponseHeader() and open tab for it?

Yeah, this is the "rewrite screenshot uploader to use xhr.send() to allow us use FormData" case I mentioned above. For screenshot uploader there is a provider.js, and it would be nice to add even more upload target, and I wouldn't like to limit them to being XHR compatible. So screenshot.js:submitScreenshot() should simply do a fileService.submitImageAndOpenLandingPage(postData); call or something like, instead of doing magic. Offtopic, but I would like to add something like for text upload (pastebin providers).

Or just use API?

API for what?

xabolcs commented 9 years ago

Base64 is evil, let's send as binary.

Should I file an issue for you, @KOLANICH ?

KOLANICH commented 9 years ago

No, I've no time for rewriting the addon.

xabolcs commented 9 years ago

To sum up this issue, I vote for wontfix to "use FormData instead of multipartFormData.js", because it doesn't support loadOneTab, loadURI and others.

Of course multipartFormData.js needs work:

KOLANICH commented 9 years ago

I still vote for enchancement and against wontfix.

whimboo commented 7 years ago

With the rewrite of the extension the pastebin feature is gone at the moment. We will have to re-implement it, and which is covered by issue #251.