r94627 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r94626‎ | r94627 | r94628 >
Date:12:33, 16 August 2011
Author:j
Status:ok (Comments)
Tags:neilk 
Comment:
Add FormData upload transport.
If enabled this is used in Browsers that support FormData.
Larger files are uploaded in chunks.
Modified paths:
  • /trunk/extensions/UploadWizard/UploadWizard.config.php (modified) (history)
  • /trunk/extensions/UploadWizard/UploadWizardHooks.php (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.ApiUploadFormDataHandler.js (added) (history)
  • /trunk/extensions/UploadWizard/resources/mw.FormDataTransport.js (added) (history)
  • /trunk/extensions/UploadWizard/resources/mw.UploadWizard.js (modified) (history)

Diff [purge]

Index: trunk/extensions/UploadWizard/UploadWizard.config.php
——@@ -287,6 +287,9 @@
288288 // supports "passthrough" mode so that assets that don't need conversions behave very similar
289289 // to a normal XHR post.
290290 'enableFirefogg' => false,
 291+
 292+ // Check if we want to enable FormData upload
 293+ 'enableFormData' => false,
291294
292295 // Setup list of video extensions for recomending firefogg.
293296 'transcodeExtensionList' => array( 'avi','asf','asx','wmv','wmx','dv','rm','ra','3gp','mkv',
Index: trunk/extensions/UploadWizard/UploadWizardHooks.php
——@@ -76,7 +76,11 @@
7777 'resources/mw.Firefogg.js',
7878 'resources/mw.FirefoggHandler.js',
7979 'resources/mw.FirefoggTransport.js',
80 -
 80+
 81+ //upload using FormData, large files in chunks
 82+ 'resources/mw.FormDataTransport.js',
 83+ 'resources/mw.ApiUploadFormDataHandler.js',
 84+
8185 // interface libraries
8286 'resources/mw.GroupProgressBar.js',
8387
Index: trunk/extensions/UploadWizard/resources/mw.ApiUploadFormDataHandler.js
——@@ -0,0 +1,67 @@
 2+/**
 3+ * Represents an object which configures an html5 FormData object to upload.
 4+ * Large files are uploaded in chunks.
 5+ * @param an UploadInterface object, which contains a .form property which points to a real HTML form in the DOM
 6+ */
 7+mw.ApiUploadFormDataHandler = function( upload, api ) {
 8+ this.upload = upload;
 9+ this.api = api;
 10+ this.$form = $j( this.upload.ui.form );
 11+ this.formData = {
 12+ action: 'upload',
 13+ stash: 1,
 14+ format: 'json'
 15+ };
 16+
 17+ var _this = this;
 18+ this.transport = new mw.FormDataTransport(
 19+ this.$form,
 20+ this.formData,
 21+ function( fraction ) {
 22+ _this.upload.setTransportProgress( fraction );
 23+ },
 24+ function( result ) {
 25+ _this.upload.setTransported( result );
 26+ }
 27+ );
 28+
 29+};
 30+
 31+mw.ApiUploadFormDataHandler.prototype = {
 32+ /**
 33+ * Optain a fresh edit token.
 34+ * If successful, store token and call a callback.
 35+ * @param ok callback on success
 36+ * @param err callback on error
 37+ */
 38+ configureEditToken: function( callerOk, err ) {
 39+ var _this = this;
 40+
 41+ var ok = function( token ) {
 42+ _this.formData.token = token;
 43+ callerOk();
 44+ };
 45+
 46+ _this.api.getEditToken( ok, err );
 47+ },
 48+
 49+ /**
 50+ * Kick off the upload!
 51+ */
 52+ start: function() {
 53+ var _this = this;
 54+ var ok = function() {
 55+ _this.beginTime = ( new Date() ).getTime();
 56+ _this.upload.ui.setStatus( 'mwe-upwiz-transport-started' );
 57+ _this.upload.ui.showTransportProgress();
 58+ _this.transport.upload();
 59+ };
 60+ var err = function( code, info ) {
 61+ _this.upload.setError( code, info );
 62+ };
 63+ this.configureEditToken( ok, err );
 64+ }
 65+};
 66+
 67+
 68+
Index: trunk/extensions/UploadWizard/resources/mw.UploadWizard.js
——@@ -550,12 +550,18 @@
551551 */
552552 getUploadHandler: function(){
553553 if( !this.uploadHandler ){
554 - if( typeof( Firefogg ) != 'undefined'
 554+ if( mw.UploadWizard.config[ 'enableFirefogg' ]
555555 &&
556 - mw.UploadWizard.config[ 'enableFirefogg' ]
 556+ typeof( Firefogg ) != 'undefined'
557557 ) {
558558 mw.log("mw.UploadWizard::getUploadHandler> FirefoggHandler");
559559 this.uploadHandler = new mw.FirefoggHandler( this, this.api );
 560+ } else if( mw.UploadWizard.config[ 'enableFormData' ] &&
 561+ (($j.browser.mozilla && $j.browser.version >= '5.0') ||
 562+ ($j.browser.webkit && $j.browser.version >= '534.28'))
 563+ ) {
 564+ mw.log("mw.UploadWizard::getUploadHandler> ApiUploadFormDataHandler");
 565+ this.uploadHandler = new mw.ApiUploadFormDataHandler( this, this.api );
560566 } else {
561567 // By default use the apiUploadHandler
562568 mw.log("mw.UploadWizard::getUploadHandler> ApiUploadHandler");
Index: trunk/extensions/UploadWizard/resources/mw.FormDataTransport.js
——@@ -0,0 +1,275 @@
 2+/**
 3+ * Represents a "transport" for files to upload; using html5 FormData.
 4+ *
 5+ * @param form jQuery selector for HTML form with selected file
 6+ * @param formData object with additinal form fields required for upload api call
 7+ * @param progressCb callback to execute when we've started.
 8+ * @param transportedCb callback to execute when we've finished the upload
 9+ */
 10+
 11+
 12+mw.FormDataTransport = function( $form, formData, progressCb, transportedCb ) {
 13+ this.$form = $form;
 14+ this.formData = formData;
 15+ this.progressCb = progressCb;
 16+ this.transportedCb = transportedCb;
 17+
 18+ this.postUrl = this.$form[0].action;
 19+ this.chunkSize = 1 * 1024 * 1024; //1Mb
 20+ this.maxRetries = 2;
 21+ this.retries = 0;
 22+
 23+ // Workaround for Firefox < 7.0 sending an empty string
 24+ // as filename for Blobs in FormData requests, something PHP does not like
 25+ // https://bugzilla.mozilla.org/show_bug.cgi?id=649150
 26+ this.gecko = $j.browser.mozilla && $j.browser.version < "7.0";
 27+};
 28+
 29+mw.FormDataTransport.prototype = {
 30+ upload: function() {
 31+ var _this = this,
 32+ file = this.$form.find('input[name=file]')[0].files[0];
 33+ if(file.size > this.chunkSize) {
 34+ this.uploadChunk(0);
 35+ } else {
 36+ this.xhr = new XMLHttpRequest();
 37+ this.xhr.addEventListener("load", function (evt) {
 38+ var response;
 39+ try {
 40+ response = JSON.parse(evt.target.responseText);
 41+ } catch(e) {
 42+ response = {
 43+ responseText: evt.target.responseText
 44+ };
 45+ }
 46+ //upload finished and can be unstashed later
 47+ _this.transportedCb(response);
 48+ }, false);
 49+ this.xhr.addEventListener("error", function (evt) {
 50+ var response;
 51+ try {
 52+ response = JSON.parse(evt.target.responseText);
 53+ } catch(e) {
 54+ response = {
 55+ responseText: evt.target.responseText
 56+ };
 57+ }
 58+ _this.transportedCb(response);
 59+ }, false);
 60+ this.xhr.upload.addEventListener("progress", function (evt) {
 61+ if (evt.lengthComputable) {
 62+ var progress = parseFloat(evt.loaded) / bytesAvailable;
 63+ _this.progressCb(progress);
 64+ }
 65+ }, false);
 66+ this.xhr.addEventListener("abort", function (evt) {
 67+ var response;
 68+ try {
 69+ response = JSON.parse(evt.target.responseText);
 70+ } catch(e) {
 71+ response = {
 72+ responseText: evt.target.responseText
 73+ };
 74+ }
 75+ _this.transportedCb(response);
 76+ }, false);
 77+
 78+ var formData = new FormData();
 79+
 80+ $j.each(this.formData, function(key, value) {
 81+ formData.append(key, value);
 82+ });
 83+ formData.append('filename', file.name);
 84+ formData.append('file', file);
 85+ this.xhr.open("POST", _this.postUrl, true);
 86+ this.xhr.send(formData);
 87+ }
 88+ },
 89+ uploadChunk: function(offset) {
 90+ var _this = this,
 91+ file = this.$form.find('input[name=file]')[0].files[0],
 92+ bytesAvailable = file.size,
 93+ chunk;
 94+
 95+ //Slice API was changed and has vendor prefix for now
 96+ //new version now require start/end and not start/length
 97+ if(file.mozSlice) {
 98+ chunk = file.mozSlice(offset, offset+_this.chunkSize, file.type);
 99+ } else if(file.webkitSlice) {
 100+ chunk = file.webkitSlice(offset, offset+_this.chunkSize, file.type);
 101+ } else {
 102+ chunk = file.slice(offset, offset+_this.chunkSize, file.type);
 103+ }
 104+
 105+ this.xhr = new XMLHttpRequest();
 106+ this.xhr.addEventListener("load", function (evt) {
 107+ var response;
 108+ _this.responseText = evt.target.responseText;
 109+ try {
 110+ response = JSON.parse(evt.target.responseText);
 111+ } catch(e) {
 112+ response = {
 113+ responseText: evt.target.responseText
 114+ };
 115+ }
 116+ if(response.upload && response.upload.filekey) {
 117+ _this.filekey = response.upload.filekey;
 118+ }
 119+ if (response.upload && response.upload.result == 'Success') {
 120+ //upload finished and can be unstashed later
 121+ _this.transportedCb(response);
 122+ }
 123+ else if (response.upload && response.upload.result == 'Continue') {
 124+ //reset retry counter
 125+ _this.retries = 0;
 126+ //start uploading next chunk
 127+ _this.uploadChunk(response.upload.offset);
 128+ } else {
 129+ //failed to upload, try again in 3 second
 130+ _this.retries++;
 131+ if (_this.maxRetries > 0 && _this.retries >= _this.maxRetries) {
 132+ //upload failed, raise response
 133+ _this.transportedCb(response);
 134+ } else {
 135+ setTimeout(function() {
 136+ _this.uploadChunk(offset);
 137+ }, 3000);
 138+ }
 139+ }
 140+ }, false);
 141+ this.xhr.addEventListener("error", function (evt) {
 142+ var response;
 143+ //failed to upload, try again in 3 second
 144+ _this.retries++;
 145+ if (_this.maxRetries > 0 && _this.retries >= _this.maxRetries) {
 146+ try {
 147+ response = JSON.parse(evt.target.responseText);
 148+ } catch(e) {
 149+ response = {
 150+ responseText: evt.target.responseText
 151+ };
 152+ }
 153+ _this.transportedCb(response);
 154+ } else {
 155+ setTimeout(function() {
 156+ _this.uploadChunk(offset);
 157+ }, 3000);
 158+ }
 159+ }, false);
 160+ this.xhr.upload.addEventListener("progress", function (evt) {
 161+ if (evt.lengthComputable) {
 162+ var progress = parseFloat(offset+evt.loaded)/bytesAvailable;
 163+ _this.progressCb(progress);
 164+ }
 165+ }, false);
 166+ this.xhr.addEventListener("abort", function (evt) {
 167+ var response;
 168+ try {
 169+ response = JSON.parse(evt.target.responseText);
 170+ } catch(e) {
 171+ response = {
 172+ responseText: evt.target.responseText
 173+ };
 174+ }
 175+ _this.transportedCb(response);
 176+ }, false);
 177+
 178+ var formData;
 179+ if(_this.gecko) {
 180+ formData = _this.geckoFormData();
 181+ } else {
 182+ formData = new FormData();
 183+ }
 184+ $j.each(this.formData, function(key, value) {
 185+ formData.append(key, value);
 186+ });
 187+ formData.append('offset', offset);
 188+ formData.append('filename', file.name);
 189+
 190+ if (_this.filekey) {
 191+ formData.append('filekey', _this.filekey);
 192+ }
 193+ formData.append('filesize', bytesAvailable);
 194+ if(_this.gecko) {
 195+ formData.appendBlob('chunk', chunk, 'chunk.bin');
 196+ } else {
 197+ formData.append('chunk', chunk);
 198+ }
 199+ this.xhr.open("POST", _this.postUrl, true);
 200+ if(_this.gecko) {
 201+ formData.send(this.xhr);
 202+ } else {
 203+ this.xhr.send(formData);
 204+ }
 205+ },
 206+ geckoFormData: function() {
 207+ var boundary = '------XX' + Math.random(),
 208+ dashdash = '--',
 209+ crlf = '\r\n',
 210+ builder = '', // Build RFC2388 string.
 211+ wait = 0;
 212+
 213+ builder += dashdash + boundary + crlf;
 214+
 215+ var formData = {
 216+ append: function(name, data) {
 217+ // Generate headers.
 218+ builder += 'Content-Disposition: form-data; name="'+ name +'"';
 219+ builder += crlf;
 220+ builder += crlf;
 221+
 222+ // Write data.
 223+ builder += data;
 224+ builder += crlf;
 225+
 226+ // Write boundary.
 227+ builder += dashdash + boundary + crlf;
 228+ },
 229+ appendFile: function(name, data, type, filename) {
 230+ builder += 'Content-Disposition: form-data; name="'+ name +'"';
 231+ builder += '; filename="' + filename + '"';
 232+ builder += crlf;
 233+ builder += 'Content-Type: ' + type;
 234+ builder += crlf;
 235+ builder += crlf;
 236+
 237+ // Write binary data.
 238+ builder += data;
 239+ builder += crlf;
 240+
 241+ // Write boundary.
 242+ builder += dashdash + boundary + crlf;
 243+ },
 244+ appendBlob: function(name, blob, filename) {
 245+ wait++;
 246+ var reader = new FileReader();
 247+ reader.onload = function(e) {
 248+ formData.appendFile(name, e.target.result,
 249+ blob.type, filename);
 250+ // Call onload after last Blob
 251+ wait--;
 252+ if(!wait && formData.xhr) {
 253+ onload();
 254+ }
 255+ };
 256+ reader.readAsBinaryString(blob);
 257+ },
 258+ send: function(xhr) {
 259+ formData.xhr = xhr;
 260+ if(!wait) {
 261+ onload();
 262+ }
 263+ }
 264+ };
 265+ var onload = function() {
 266+ // Mark end of the request.
 267+ builder += dashdash + boundary + dashdash + crlf;
 268+
 269+ // Send to server
 270+ formData.xhr.setRequestHeader("Content-type",
 271+ "multipart/form-data; boundary=" + boundary);
 272+ formData.xhr.sendAsBinary(builder);
 273+ };
 274+ return formData;
 275+ }
 276+};

Follow-up revisions

RevisionCommit summaryAuthorDate
r99013refine FormData upload from r94627 based on review...j17:25, 5 October 2011

Comments

#Comment by NeilK (talk | contribs)   18:18, 22 August 2011

Deferring the review because we'd like to deploy the trunk version of the extension. This change does nothing unless there is a configuration change to activate it, which we just won't do in production.

Will review this in more detail afterwards. Sorry for the delay, Jan.

#Comment by Raindrift (talk | contribs)   23:16, 21 September 2011 
+			} else if( mw.UploadWizard.config[ 'enableFormData' ] &&
+						(($j.browser.mozilla && $j.browser.version >= '5.0') ||
+						 ($j.browser.webkit && $j.browser.version >= '534.28'))

Is there a reason to not use mw.fileApi.isAvailable()? I'm concerned that this won't work with future versions of IE that support the File API. If the check that's there isn't sufficient, it might make sense to add a new method to mw.fileApi so at least all the checking happens in one place.

#Comment by NeilK (talk | contribs)   04:24, 22 September 2011

I think this *might* be an attempt to determine whether the browser supports a slice() API.

In UploadWizard's model, we determine what the uploadHandler is going to be at object creation time. But! At this point there isn't a file object around that's handy to test. So perhaps this is the reason to look at browser versions.

So, perhaps we need to allow a later decision of what the uploadHandler is. Or, we figure out some way to do a more browser-neutral test earlier (maybe we can create zero-length file with a data:// URL and see if it has a slice method?)

#Comment by NeilK (talk | contribs)   05:02, 22 September 2011
  • 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

#Comment by NeilK (talk | contribs)   05:04, 22 September 2011

guh, sorry for the poor formatting. I always forget this is in wikitext.

#Comment by BotInc (talk | contribs)   07:44, 22 September 2011
  • using $j.parseJSON might be better, but are there Browsers where FromData works but JSON does not?
  • geckoFormData was written by me before for another site. To make the code simpler and better maintainable in the future, geckoFormData can also be taken out since Firefox 7 will be released next week and its only required for older versions of Firefox.
  • wait could be renamed, not sure chunksRemaining is a better name right now, it is needed since FileReader is async in appendBlob and the xhr can only be send once all FileReaders are done.
#Comment by Raindrift (talk | contribs)   19:51, 4 October 2011

People are asking for this, and the multi-file select feature depends on it now as well. Also, 1.18 is coming out soon. It sounds like we can't deploy until at least the error handling is taken care of, so I'm going to switch this to a FIXME.

#Comment by BotInc (talk | contribs)   09:44, 6 October 2011

r99013 refines FormData upload

  • use $j.parseJSON and set response.error if it fails
  • move function to detect slice api support into mw.fileApi
  • rename wait to chunsRemaining
#Comment by NeilK (talk | contribs)   18:07, 14 October 2011

marking ok, since followup revision r99013

Status & tagging log