When downloading a file from a channel that has a valid "Content-Encoding" header, the byte progress events, content length, and byte range requests all refer to the HTTP entity-body. If automatic decoding is enabled, those byte indications may not match what is received by the channel stream, and thus what is written into the target file. The new JavaScript API for downloads should include tests for those cases.

This patch adds one more test test_download_cancel_midway_restart_with_content_encoding. Hoever, it doesn't return any progress until it reaches 100 so the download can't be canceled midway. I can't figure out why. Is it something to do with content-encoding: gzip won't return any progress?

Comment on attachment 750355 [details] [diff] [review] v2 WIP (In reply to Raymond Lee [:raymondlee] from comment #2) > Hoever, it doesn't return any progress until it reaches 100 so the download > can't be canceled midway. I can't figure out why. Is it something to do > with content-encoding: gzip won't return any progress? The reason is that the entire body of the response is sent in one go, while the body should be divided in two parts, like the handlers registered in head.js do. You may add a registerInterruptibleHandler call to test_common_initialize, and use that handler in a similar way to other test cases that handle responses in two phases.

I have added some code to test_common_initialize. However, the writeByteArray is only written to the target file once, even it has been executed in both first and second phrase. The target file only contains TEST_DATA_SHORT instead of TEST_DATA_SHORT+TEST_DATA_SHORT. Is this expected?

Comment on attachment 752693 [details] [diff] [review] v3 There are a couple of things. Firstly, I'm not sure that you can just concatenate the encoded GZIP data twice, you should probably split the array of bytes in two parts and send each part once. Secondly, to avoid concurrency issues, you should create a different instance of nsIBinaryOutputStream for every function call, because the firstPart and secondPart functions can be called for multiple requests at the same time, in no particular order (for example, firstPart for request 1, firstPart for request 2, secondPart for request 1, and it's also possible that secondPart for request 2 is not called).

(In reply to Paolo Amadini [:paolo] from comment #5) > Comment on attachment 752693 [details] [diff] [review] > v3 > > There are a couple of things. Firstly, I'm not sure that you can just > concatenate the encoded GZIP data twice, you should probably split the array > of bytes in two parts and send each part once. Secondly, to avoid concurrency > issues, you should create a different instance of nsIBinaryOutputStream for > every function call, because the firstPart and secondPart functions can be > called for multiple requests at the same time, in no particular order (for > example, firstPart for request 1, firstPart for request 2, secondPart for > request 1, and it's also possible that secondPart for request 2 is not > called). OK, done

Comment on attachment 753192 [details] [diff] [review] v4 Review of attachment 753192 [details] [diff] [review]: ----------------------------------------------------------------- This is almost ready, I've just a few comments before the final review. ::: toolkit/components/jsdownloads/test/unit/head.js @@ +365,5 @@ > + function firstPart(aRequest, aResponse) { > + aResponse.setHeader("Content-Type", "text/plain", false); > + aResponse.setHeader("Content-Encoding", "gzip", false); > + aResponse.setHeader("Content-Length", "" + TEST_DATA_SHORT_GZIP_ENCODED.length); > + aResponse.processAsync(); We call processAsync and "finish" in our infrastructure already, no need to repeat. @@ +369,5 @@ > + aResponse.processAsync(); > + > + let bos = Cc["@mozilla.org/binaryoutputstream;1"]. > + createInstance(Ci.nsIBinaryOutputStream); > + bos.setOutputStream(aResponse.bodyOutputStream); You may add a Components.constructor declaration to the file header, with setOutputStream as the initialization call: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/head.js#39 ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js @@ +761,5 @@ > + > + function cleanup() { > + gHttpServer.registerPathHandler(source_path, null); > + } > + nit: whitespace at end of line. @@ +767,5 @@ > + > + gHttpServer.registerPathHandler(source_path, function (aRequest, aResponse) { > + aResponse.setHeader("Content-Type", "text/plain", false); > + aResponse.setHeader("Content-Encoding", "gzip", false); > + aResponse.setHeader("Content-Length", "" + TEST_DATA_SHORT_GZIP_ENCODED.length, false); indentation nit: aResponse.setHeader("Content-Length", "" + TEST_DATA_SHORT_GZIP_ENCODED.length, false); @@ +775,5 @@ > + bos.setOutputStream(aResponse.bodyOutputStream); > + aResponse.processAsync(); > + bos.writeByteArray(TEST_DATA_SHORT_GZIP_ENCODED, > + TEST_DATA_SHORT_GZIP_ENCODED.length); > + aResponse.finish(); You can avoid processAsync and "finish" also here. In this case, it's because we write the entire response before returning from the function. @@ +789,5 @@ > + do_check_true(download.stopped); > + do_check_true(download.succeeded); > + do_check_false(download.canceled); > + do_check_eq(download.progress, 100); > + do_check_eq(download.totalBytes, TEST_DATA_SHORT_GZIP_ENCODED.length); Please check only "progress" and totalBytes. In general, when we are pretty sure that an object behaves in a certain way, based on other test cases in the suite and our knowledge of the code, there is no need to verify that again. In this case, start() succeeding is enough for avoiding the state tests. We are checking progress and totalBytes because we want to assert their behavior, since we know there are different data lengths involved. @@ +810,5 @@ > + let deferCancel = Promise.defer(); > + download.onchange = function () { > + if (!download.stopped && !download.canceled && download.progress >= 50, > + download.progress < 100) { > + deferCancel.resolve(download.cancel()); There's something strange here... maybe you could use a condition based on download.currentBytes equal to the data length of the first part, as the progress percentage is less precise here. @@ +837,5 @@ > + do_check_eq(download.progress, 0); > + do_check_eq(download.totalBytes, 0); > + do_check_eq(download.currentBytes, 0); > + > + yield promiseAttempt; Also here, all this block from "let promiseAttempt = download.start();" to "yield promiseAttempt;" can just become "yield download.start()"; @@ +843,5 @@ > + do_check_true(download.stopped); > + do_check_true(download.succeeded); > + do_check_false(download.canceled); > + do_check_eq(download.progress, 100); > + do_check_eq(download.totalBytes, TEST_DATA_SHORT_GZIP_ENCODED.length); And check only "progress" and totalBytes here.

Comment on attachment 757853 [details] [diff] [review] v5 Review of attachment 757853 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! r+ with the change below. ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js @@ +851,5 @@ > + let deferCancel = Promise.defer(); > + download.onchange = function () { > + // The data length of first part is 25. > + if (!download.stopped && !download.canceled && > + download.currentBytes == 25) { Just use TEST_DATA_SHORT_GZIP_ENCODED_FIRST.length, no comment needed.

https://tbpl.mozilla.org/?tree=Try&rev=7c971dd76687 Passed try