Conversation
This change makes it feasible for Volley clients to remove their dependency on the legacy Apache HTTP library; if HurlStack is used, or if their existing HttpStack implementation is refactored atop the new BaseHttpStack (which should be trivial), then Volley will never depend on classes in org.apache at runtime. However, legacy clients built atop HttpStack (including HttpClientStack) should continue to work without modifications for now. Fixes #2
-Block large (>MAX_INT) responses in AdaptedHttpStack -Fix copying headers in BaseHttpStack
joebowbeer
left a comment
There was a problem hiding this comment.
I scanned the changes and nothing popped out.
jjoslin
left a comment
There was a problem hiding this comment.
Minor comments, okay to submit as-is.
|
|
||
| public NetworkResponse(byte[] data) { | ||
| this(HttpStatus.SC_OK, data, Collections.<String, String>emptyMap(), false, 0); | ||
| this(HttpURLConnection.HTTP_OK, data, Collections.<String, String>emptyMap(), false, 0); |
There was a problem hiding this comment.
Do you want to consider a static import for the codes to cut down on the verbosity (here and elsewhere)?
There was a problem hiding this comment.
I always thought this was against the style guide, but it doesn't look like it. Well, I guess I personally prefer having the verbosity since I feel like imports are generally not noticed when looking at a file and the source of these constants is kind of relevant, though I wouldn't push back if a static import were used elsewhere. Going to just leave this as is to avoid churn.
| headers.add(new BasicHeader(entry.getKey(), value)); | ||
| } | ||
| } | ||
| apacheResponse.setHeaders(headers.toArray(new Header[0])); |
There was a problem hiding this comment.
If you allocate the correct size up front, headers.toArray(new Header[headers.size()]), then you can avoid an unnecessary allocation of an array.
| } catch (DateParseException e) { | ||
| return newRfc1123Formatter().parse(dateStr).getTime(); | ||
| } catch (ParseException e) { | ||
| // Date in invalid format, fallback to 0 |
There was a problem hiding this comment.
I realize this is out of scope for this review but it seems like this exception should be logged.
This change makes it feasible for Volley clients to remove their dependency on the legacy Apache HTTP library; if HurlStack is used, or if their existing HttpStack implementation is refactored atop the new BaseHttpStack (which should be trivial), then Volley will never depend on classes in org.apache at runtime. However, legacy clients built atop HttpStack (including HttpClientStack) should continue to work without modifications for now.
Fixes #2