[http] HttpMethod -> String, deprecate JsonHttpClient

[rmistry, 2012-09-10 13:16:28]

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpMethods.java
(right):

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpMethods.java:2:
* Copyright (c) 2010 Google Inc.
Remove this line as per recent communication?

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:509:
if (getRequest().getRequestMethod().equals(HttpMethods.HEAD)) {
Change to HttpMethods.HEAD.equals(getRequest().getRequestMethod()) because the
requestMethod could be null?

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java
(right):

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java:72:
{HttpMethods.DELETE, HttpMethods.GET, HttpMethods.POST, HttpMethods.PUT};
[optional] What do you think about using a HashSet<String> instead, then we can
do a constant time SUPPORTED_METHOD.contains(method).

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java:164:
protected LowLevelHttpRequest buildRequest(String method, String url) throws
IOException {
Change to abstract? or we can add a note that it will be changed to abstract in
1.13

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/apache/HttpExtensionMethod.java
(right):

http://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/apache/HttpExtensionMethod.java:2:
* Copyright (c) 2010 Google Inc.
Remove Copyright since this is now a new file.

[yanivi, 2012-09-10 16:55:39]

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpMethods.java
(right):

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpMethods.java:2:
* Copyright (c) 2010 Google Inc.
On 2012/09/10 13:16:28, rmistry wrote:
> Remove this line as per recent communication?

Done.

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:509:
if (getRequest().getRequestMethod().equals(HttpMethods.HEAD)) {
On 2012/09/10 13:16:28, rmistry wrote:
> Change to HttpMethods.HEAD.equals(getRequest().getRequestMethod()) because the
> requestMethod could be null?

it cannot be null here.  there is a precondition check in execute()

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java
(right):

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java:72:
{HttpMethods.DELETE, HttpMethods.GET, HttpMethods.POST, HttpMethods.PUT};
On 2012/09/10 13:16:28, rmistry wrote:
> [optional] What do you think about using a HashSet<String> instead, then we
can
> do a constant time SUPPORTED_METHOD.contains(method).

performance argument is weak when N == 4 :)

also memory usage with an array is smaller than HashSet

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java:164:
protected LowLevelHttpRequest buildRequest(String method, String url) throws
IOException {
On 2012/09/10 13:16:28, rmistry wrote:
> Change to abstract? or we can add a note that it will be changed to abstract
in
> 1.13

I wasn't sure what the best thing to do here would be.  It is really convenient
to be able to call super.buildRequest() to form the nice exception.  I could do
a utility method, but then that may just be more confusing and less discoverable
for someone implementing this.  I guess for me making it abstract is unnecessary
because anyone trying to run this without implementing this method will quickly
realize it needs to be implemented.

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/apache/HttpExtensionMethod.java
(right):

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/apache/HttpExtensionMethod.java:2:
* Copyright (c) 2010 Google Inc.
On 2012/09/10 13:16:28, rmistry wrote:
> Remove Copyright since this is now a new file.

Done.

[rmistry, 2012-09-10 17:03:06]

LGTM

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java
(right):

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java:72:
{HttpMethods.DELETE, HttpMethods.GET, HttpMethods.POST, HttpMethods.PUT};
On 2012/09/10 16:55:39, yanivi wrote:
> On 2012/09/10 13:16:28, rmistry wrote:
> > [optional] What do you think about using a HashSet<String> instead, then we
> can
> > do a constant time SUPPORTED_METHOD.contains(method).
> 
> performance argument is weak when N == 4 :)
> 
> also memory usage with an array is smaller than HashSet

Yes, I was trying to get around the binary search. Lets add a comment here
saying that entries in this array has to be sorted.

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java:164:
protected LowLevelHttpRequest buildRequest(String method, String url) throws
IOException {
On 2012/09/10 16:55:39, yanivi wrote:
> On 2012/09/10 13:16:28, rmistry wrote:
> > Change to abstract? or we can add a note that it will be changed to abstract
> in
> > 1.13
> 
> I wasn't sure what the best thing to do here would be.  It is really
convenient
> to be able to call super.buildRequest() to form the nice exception.  I could
do
> a utility method, but then that may just be more confusing and less
discoverable
> for someone implementing this.  I guess for me making it abstract is
unnecessary
> because anyone trying to run this without implementing this method will
quickly
> realize it needs to be implemented.

We could also add a class level javadoc comment that subclasses must implement
buildRequest and can call super.buildRequest for unsupported methods.

[yanivi, 2012-09-10 17:47:23]

Also updated UrlFetchTransport.  Please take a quick look again.

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java
(right):

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java:72:
{HttpMethods.DELETE, HttpMethods.GET, HttpMethods.POST, HttpMethods.PUT};
On 2012/09/10 17:03:06, rmistry wrote:
> On 2012/09/10 16:55:39, yanivi wrote:
> > On 2012/09/10 13:16:28, rmistry wrote:
> > > [optional] What do you think about using a HashSet<String> instead, then
we
> > can
> > > do a constant time SUPPORTED_METHOD.contains(method).
> > 
> > performance argument is weak when N == 4 :)
> > 
> > also memory usage with an array is smaller than HashSet
> 
> Yes, I was trying to get around the binary search. Lets add a comment here
> saying that entries in this array has to be sorted.

Done.

https://codereview.appspot.com/6496096/diff/3001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java:164:
protected LowLevelHttpRequest buildRequest(String method, String url) throws
IOException {
On 2012/09/10 17:03:06, rmistry wrote:
> On 2012/09/10 16:55:39, yanivi wrote:
> > On 2012/09/10 13:16:28, rmistry wrote:
> > > Change to abstract? or we can add a note that it will be changed to
abstract
> > in
> > > 1.13
> > 
> > I wasn't sure what the best thing to do here would be.  It is really
> convenient
> > to be able to call super.buildRequest() to form the nice exception.  I could
> do
> > a utility method, but then that may just be more confusing and less
> discoverable
> > for someone implementing this.  I guess for me making it abstract is
> unnecessary
> > because anyone trying to run this without implementing this method will
> quickly
> > realize it needs to be implemented.
> 
> We could also add a class level javadoc comment that subclasses must implement
> buildRequest and can call super.buildRequest for unsupported methods.

Done.

[rmistry, 2012-09-10 18:08:52]

LGTM