User "NeilK" posted a comment on MediaWiki.r94627. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/94627#c22982 Commit summary:
Add FormData upload transport. If enabled this is used in Browsers that support FormData. Larger files are uploaded in chunks. Comment: * mw.ApiUploadFormDataHandler.js This is *very* similar to mw.ApiUploadHandler.js. I can understand not wanting to perturb any other code (it made it easier to defer this review...) but even if we accept this, we should mark it for refactoring. The differences: - instead of 'configureForm()' which adds fields to a form, it creates a 'formData' object which will be used to initialize the formData stuff. - instead of an IframeTransport, it makes a FormDataTransport I think I made mw.ApiUploadHandler with the intention that one day you could swap out different "transports", one of them being an AJAX transport that also used the API, as you do here. But if all transports use the API (with the exception of mocked-out transports) maybe the notion of "handler" is just a useless layer. Must rethink. JSON parsing: - $j.parseJSON is a bit safer, we don't use the JSON object directly anywhere else - UploadWizard is written around the mw.Api abstraction, which maybe (?) we can't use here, but be aware that the callback isn't expecting a responseText property. It is expecting a correct parsed JSON result or nothing. If you can't parse the JSON we should instead trigger the error handler with an appropriate result object which explains what went wrong in the 'error' property - We seem to throw away the error from the JSON parse here? - Seems very repetitive... geckoFormData: I assume geckoFormData was copied from somewhere else, or did you write this from scratch? I think the variable 'wait' is misnamed, maybe it should be something like 'chunksRemaining' Ian: just a note to you, despite what we thought, the geckoFormData is *not* reading everything in -- blob is a chunk, and a chunk is a result of file.slice(), and a slice is really just a specification for filereader.readAsBinaryString() or any other read method; it contains no data yet. So it is doing the right thing. Otherwise this looks good -- I have just read through the code, will do some more testing soon _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
