timob-9205: use CharsetDecoder to decode data. #2259

Merged
merged 4 commits into from May 23, 2012

3 participants

@pingwang2011 pingwang2011 commented on an outdated diff May 23, 2012
...rc/java/ti/modules/titanium/network/TiHTTPClient.java
try {
- responseText = new String(responseData.getBytes(), charset);
- } catch (UnsupportedEncodingException e) {
- Log.e(LCAT, "Unable to convert to String using charset: " + charset);
- } catch (OutOfMemoryError e) {
- Log.e(LCAT, "Unable to get response text: out of memory");
+ ByteBuffer data = ByteBuffer.wrap(responseData.getBytes());
+ CharBuffer b = decoder.decode(data);
+ responseText = b.toString();
+ } catch (CharacterCodingException e) {
@pingwang2011
pingwang2011 added a line comment May 23, 2012

Maybe we should catch a general exception instead of CharacterCodingException.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vishalduggal vishalduggal commented on an outdated diff May 23, 2012
...rc/java/ti/modules/titanium/network/TiHTTPClient.java
if (responseData != null && responseText == null) {
if (charset == null) {
charset = HTTP.UTF_8;
}
+ CharsetDecoder decoder = Charset.forName(charset).newDecoder();
@vishalduggal
vishalduggal added a line comment May 23, 2012

This statement can throw
IllegalCharsetNameException - If the given charset name is illegal
UnsupportedCharsetException - If no support for the named charset is available in this instance of the Java virtual machine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vishalduggal

Code reviewed. REJECTED

@vishalduggal vishalduggal commented on the diff May 23, 2012
...rc/java/ti/modules/titanium/network/TiHTTPClient.java
if (responseData != null && responseText == null) {
if (charset == null) {
charset = HTTP.UTF_8;
}
try {
- responseText = new String(responseData.getBytes(), charset);
- } catch (UnsupportedEncodingException e) {
- Log.e(LCAT, "Unable to convert to String using charset: " + charset);
- } catch (OutOfMemoryError e) {
- Log.e(LCAT, "Unable to get response text: out of memory");
+ CharsetDecoder decoder = Charset.forName(charset).newDecoder();
+ ByteBuffer data = ByteBuffer.wrap(responseData.getBytes());
+ CharBuffer b = decoder.decode(data);
+ responseText = b.toString();
+ } catch (Exception e) {
@vishalduggal
vishalduggal added a line comment May 23, 2012

This statement will not catch outOfMemory errors. Either include an OOM catcher or catch Throwable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vishalduggal

Code Reviewed. ACCEPTED

@pingwang2011

Code reviewed and functionally tested. The test case in TIMOB-6922 and TIMOB-9205 passed. Accepted

@vishalduggal vishalduggal merged commit d37d0b9 into appcelerator:master May 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment