Remove JsonHttpClient.builder and add enableGZipContent to JsonHttpRequest

[yanivi, 2012-06-05 19:47:22]

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java
(right):

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:249:
public JsonHttpClient(
can we make this protected?

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:457:
public static Builder builder(
why don't we just delete this one too?  developers should be using the builder
from the generated code

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:522:
public Builder(HttpTransport transport, JsonFactory jsonFactory, GenericUrl
baseUrl) {
keep it protected

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:540:
HttpTransport transport, JsonFactory jsonFactory, GenericUrl rootUrl, String
servicePath) {
why don't we just add HttpRequestInitializer to the builder?

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:599:
* Use this method only if {@link #Builder(HttpTransport, JsonFactory,
GenericUrl)} was used.
change this to use words instead of pointing to a specific constructor/method

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java
(right):

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java:86:
public JsonHttpRequest setEnableGZipContent(boolean enableGZipContent) {
need testEnableGZipContent()

may look at GoogleClientTest.testEnableGZipContent()

[rmistry, 2012-06-06 12:04:10]

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java
(right):

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:249:
public JsonHttpClient(
On 2012/06/05 19:47:22, yanivi wrote:
> can we make this protected?

Why make it protected? I thought the pattern was a public constructor with
required params and builder for optional params.

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:457:
public static Builder builder(
On 2012/06/05 19:47:22, yanivi wrote:
> why don't we just delete this one too?  developers should be using the builder
> from the generated code

We could. There are a couple of internal references using JsonHttpClient
directly and to make it easy we should either:
1) delete builder and make Builder constructor public.
2) leave builder with protected Builder constructors.

Going with (1) here.

If the goal of your comments is to get us to the point where we eventually
move/delete JsonHttpClient then we should be doing that in later CLs as part of
that effort.

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:522:
public Builder(HttpTransport transport, JsonFactory jsonFactory, GenericUrl
baseUrl) {
On 2012/06/05 19:47:22, yanivi wrote:
> keep it protected

As mentioned above, we should have either a builder or a public constructor.

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:540:
HttpTransport transport, JsonFactory jsonFactory, GenericUrl rootUrl, String
servicePath) {
On 2012/06/05 19:47:22, yanivi wrote:
> why don't we just add HttpRequestInitializer to the builder?

Done.

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:599:
* Use this method only if {@link #Builder(HttpTransport, JsonFactory,
GenericUrl)} was used.
On 2012/06/05 19:47:22, yanivi wrote:
> change this to use words instead of pointing to a specific constructor/method

Done.

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java
(right):

http://codereview.appspot.com/6297045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java:86:
public JsonHttpRequest setEnableGZipContent(boolean enableGZipContent) {
On 2012/06/05 19:47:22, yanivi wrote:
> need testEnableGZipContent()
> 
> may look at GoogleClientTest.testEnableGZipContent()

Done.

[rmistry, 2012-06-06 13:24:06]

hg pull -u

[yanivi, 2012-06-06 15:49:44]

http://codereview.appspot.com/6297045/diff/3003/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java
(right):

http://codereview.appspot.com/6297045/diff/3003/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:43:
public class JsonHttpClient {
please add an upgrade warning about the builder() being removed and instead now
you should do new JsonHttpClient.Builder()

http://codereview.appspot.com/6297045/diff/3003/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:579:
public Builder(HttpTransport transport, JsonFactory jsonFactory, GenericUrl
baseUrl) {
this deprecated constructor was protected in 1.9, so I disagree with changing it
to public in 1.10

http://codereview.appspot.com/6297045/diff/3003/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:597:
public Builder(
the recommended style is that the signature of the constructor of X should match
the signature of the constructor of X.Builder

my recommendation: JsonHttpClient.Builder constructor rootUrl parameter should
be a String, and the JsonHttpClient constructor should add an
httpRequestInitializer parameter

[rmistry, 2012-06-06 18:16:51]

http://codereview.appspot.com/6297045/diff/3003/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java
(right):

http://codereview.appspot.com/6297045/diff/3003/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:43:
public class JsonHttpClient {
On 2012/06/06 15:49:45, yanivi wrote:
> please add an upgrade warning about the builder() being removed and instead
now
> you should do new JsonHttpClient.Builder()

Done.

http://codereview.appspot.com/6297045/diff/3003/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:579:
public Builder(HttpTransport transport, JsonFactory jsonFactory, GenericUrl
baseUrl) {
On 2012/06/06 15:49:45, yanivi wrote:
> this deprecated constructor was protected in 1.9, so I disagree with changing
it
> to public in 1.10

The problem with not changing it to public in 1.10 is that we do not give
existing users time to upgrade and remove deprecated calls. But like you said
there may not be a lot of users, I noticed 1 or 2 internally and hopefully they
will not be that difficult to upgrade.

Done.

http://codereview.appspot.com/6297045/diff/3003/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:597:
public Builder(
On 2012/06/06 15:49:45, yanivi wrote:
> the recommended style is that the signature of the constructor of X should
match
> the signature of the constructor of X.Builder
> 
> my recommendation: JsonHttpClient.Builder constructor rootUrl parameter should
> be a String, and the JsonHttpClient constructor should add an
> httpRequestInitializer parameter

Makes sense. Done.

[yanivi, 2012-06-06 21:28:08]

LGTM