Fixes for [TC-1557] and [TC-1792], changes in HTTPClient.js. #3693
Conversation
|
The Jira issue: https://jira.appcelerator.org/browse/TC-1557 |
|
|
||
| //This variable shows that responseType of XMLHttpRequest is 'arraybuffer' | ||
| //This type is valid only for async mode | ||
| _isArrayBuffer: void 0, |
There was a problem hiding this comment.
There is no point in creating a property with a default value of undefined. It is undefined by default and this line just wastes bytes
|
This PR adds support for new features. Anytime there is support for a new feature, the Docs must also be updated. If you don't update the docs, this PR will be rejected. |
| f = Filesystem.getFile(f); | ||
| f.writable && f.write(xhr.responseText); | ||
| //create file by name | ||
| this.file && (file = Filesystem.getFile(Filesystem.applicationDataDirectory, this.file)); |
There was a problem hiding this comment.
This won't work. By prepending the applicationDataDirectory, you're disallowing someone to save the response to the tempDirectory.
Furthermore, you're forcing someone to enter a non-fully qualified path for the "file" property which conflicts with the default behavior for non-fully qualified paths. Non-fully qualified paths default to the Resources directory, which isn't writable anyways.
And if someone passes in a fully qualified path that is in the applicationDataDirectory, it will be set twice and error as an invalid path.
There was a problem hiding this comment.
Also, I wouldn't define this so far above where it's next used.
|
Latest change (committed 16.01.2013) was tested on iOS 5.0.1, iOS 4.1, Android 2.3, Android 4.0. |
|
|
||
| //prepare Base64-encoded string required by Blob | ||
| var i = c.responseText.length, | ||
| binaryString = new Array(i); |
There was a problem hiding this comment.
Just do:
binaryString = [];
It saves bytes, and you won't incur much of a performance penalty
There was a problem hiding this comment.
On second though, given the length of the buffer, the performance hit will add up over time.
There was a problem hiding this comment.
edgithub, please disregard Bryan's comment. Using new Array(i) is the preferred way.
|
Before we proceed further, I need you to attach a test case to the JIRA ticket highlighting the problem in the original implementation |
|
Please refer to the ticket https://jira.appcelerator.org/browse/TIMOB-12394 for the test case sample. |
|
I dug into this issue a little more, and, in short, the wrong thing was fixed. HTTPClient works just fine, and Blob mostly works just fine (there are some issues with toString, but that is unrelated to this issue). The bug is in ImageView. We don't base64 encode blobs anymore, so imageview needs to be updated to base64 encode the data from blobs. Request rejected |
This pull request contains fixes for the following issues: