http issue 184 & 185: Add StreamingContent & HttpEncoding interfaces

http issue 184: Add StreamingContent interface

http issue 185: Add HttpEncoding interface

[rmistry, 2012-12-26 14:31:11]

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:147:
* </P>
</p>

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
(right):

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:956:
public LowLevelHttpResponse execute() throws IOException {
I dont understand this change, the method signature looks identical to the below
method.

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:347:
public HttpRequest enableGZipEncoding() {
Do we need this method? Callers can always directly call request.setEncoding(new
GZipEncoding);
It may be better to not give any specific implementation of HttpEncoding special
treatment, this gives the impression that if any new implementation of
HttpEncoding is added then we need a corresponding method in HttpRequest.

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1009:
} else {
Do you think it would be useful to log that we are replacing the old content
encoding XYZEncoding with the new specified content encoding ABCEncoding? (only
if both of them exist).

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1012:
if (contentRetrySupported) {
collapse this to one line?
contentLength = contentRetrySupported ? ....

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1036:
// temporarily pass an HttpContent to LowLevelHttpRequest for backwards
compatibility
Add TODO to remove in 1.15?

[yanivi, 2012-12-26 15:04:02]

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:147:
* </P>
On 2012/12/26 14:31:11, rmistry wrote:
> </p>

Done.

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
(right):

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:956:
public LowLevelHttpResponse execute() throws IOException {
On 2012/12/26 14:31:11, rmistry wrote:
> I dont understand this change, the method signature looks identical to the
below
> method.

Unfortunately it is a bug with codereview.appspot.com.  I've seen this before. 
If you look at the actual diff you can see there is no dup:

Index:
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
===================================================================
---
a/google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
+++
b/google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
@@ -953,11 +953,6 @@
     }
 
     @Override
-    public void setContent(HttpContent content) throws IOException {
-      throw new UnsupportedOperationException();
-    }
-
-    @Override
     public LowLevelHttpResponse execute() throws IOException {
       throw new UnsupportedOperationException();
     }

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:347:
public HttpRequest enableGZipEncoding() {
On 2012/12/26 14:31:11, rmistry wrote:
> Do we need this method? Callers can always directly call
request.setEncoding(new
> GZipEncoding);
> It may be better to not give any specific implementation of HttpEncoding
special
> treatment, this gives the impression that if any new implementation of
> HttpEncoding is added then we need a corresponding method in HttpRequest.

I want to make super-easy to enable GZip encoding, and I think of setEncoding as
an advanced feature.  GZip encoding has the potential to be a major performance
enhancement.  I originally didn't have it in this changeset, but after using it
a few times in google-api-java-client I figured this seems too useful to leave
out.

But if you really object I can remove it.  Let me know what you think.

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1009:
} else {
On 2012/12/26 14:31:11, rmistry wrote:
> Do you think it would be useful to log that we are replacing the old content
> encoding XYZEncoding with the new specified content encoding ABCEncoding?
(only
> if both of them exist).

Added Preconditions.  Please take a look again.

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1012:
if (contentRetrySupported) {
On 2012/12/26 14:31:11, rmistry wrote:
> collapse this to one line?
> contentLength = contentRetrySupported ? ....

Done.

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1036:
// temporarily pass an HttpContent to LowLevelHttpRequest for backwards
compatibility
On 2012/12/26 14:31:11, rmistry wrote:
> Add TODO to remove in 1.15?

I don't think a TODO is needed because it is already deprecated.

https://codereview.appspot.com/6999046/diff/14001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/6999046/diff/14001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1025:
if (oldContentEncoding != null) {
note: I added this back since it is still needed.

[rmistry, 2012-12-27 15:11:27]

LGTM

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:347:
public HttpRequest enableGZipEncoding() {
On 2012/12/26 15:04:02, yanivi wrote:
> On 2012/12/26 14:31:11, rmistry wrote:
> > Do we need this method? Callers can always directly call
> request.setEncoding(new
> > GZipEncoding);
> > It may be better to not give any specific implementation of HttpEncoding
> special
> > treatment, this gives the impression that if any new implementation of
> > HttpEncoding is added then we need a corresponding method in HttpRequest.
> 
> I want to make super-easy to enable GZip encoding, and I think of setEncoding
as
> an advanced feature.  GZip encoding has the potential to be a major
performance
> enhancement.  I originally didn't have it in this changeset, but after using
it
> a few times in google-api-java-client I figured this seems too useful to leave
> out.
> 
> But if you really object I can remove it.  Let me know what you think.

As discussed by IM, I prefer removing it but leaving it upto you.

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1036:
// temporarily pass an HttpContent to LowLevelHttpRequest for backwards
compatibility
On 2012/12/26 15:04:02, yanivi wrote:
> On 2012/12/26 14:31:11, rmistry wrote:
> > Add TODO to remove in 1.15?
> 
> I don't think a TODO is needed because it is already deprecated.

Yes, right.

[yanivi, 2012-12-27 15:19:34]

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/6999046/diff/3045/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:347:
public HttpRequest enableGZipEncoding() {
On 2012/12/27 15:11:27, rmistry wrote:
> On 2012/12/26 15:04:02, yanivi wrote:
> > On 2012/12/26 14:31:11, rmistry wrote:
> > > Do we need this method? Callers can always directly call
> > request.setEncoding(new
> > > GZipEncoding);
> > > It may be better to not give any specific implementation of HttpEncoding
> > special
> > > treatment, this gives the impression that if any new implementation of
> > > HttpEncoding is added then we need a corresponding method in HttpRequest.
> > 
> > I want to make super-easy to enable GZip encoding, and I think of
setEncoding
> as
> > an advanced feature.  GZip encoding has the potential to be a major
> performance
> > enhancement.  I originally didn't have it in this changeset, but after using
> it
> > a few times in google-api-java-client I figured this seems too useful to
leave
> > out.
> > 
> > But if you really object I can remove it.  Let me know what you think.
> 
> As discussed by IM, I prefer removing it but leaving it upto you.

Done.

[rmistry, 2012-12-27 15:20:40]

LGTM