Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(211)

Issue 6308097: Handling ImmutableList in Data.clone

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 6 months ago by rmistry
Modified:
13 years, 5 months ago
Reviewers:
Base URL:
https://google-http-java-client.googlecode.com/hg/
Visibility:
Public.

Description

Created to fix http://code.google.com/p/google-http-java-client/issues/detail?id=71 : Data.clone cannot handle ImmutableList.

Patch Set 1 #

Total comments: 4

Patch Set 2 : Addressing code review comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -1 line) Patch
M google-http-client/src/main/java/com/google/api/client/util/Data.java View 1 2 chunks +7 lines, -0 lines 2 comments Download
M google-http-client/src/test/java/com/google/api/client/util/DataTest.java View 3 chunks +39 lines, -1 line 0 comments Download

Messages

Total messages: 6
rmistry
13 years, 6 months ago (2012-06-20 17:41:25 UTC) #1
yanivi
http://codereview.appspot.com/6308097/diff/1/google-http-client/src/main/java/com/google/api/client/util/Data.java File google-http-client/src/main/java/com/google/api/client/util/Data.java (right): http://codereview.appspot.com/6308097/diff/1/google-http-client/src/main/java/com/google/api/client/util/Data.java#newcode232 google-http-client/src/main/java/com/google/api/client/util/Data.java:232: } else if (data instanceof ImmutableList<?>) { I am ...
13 years, 6 months ago (2012-06-21 00:58:08 UTC) #2
rmistry
http://codereview.appspot.com/6308097/diff/1/google-http-client/src/main/java/com/google/api/client/util/Data.java File google-http-client/src/main/java/com/google/api/client/util/Data.java (right): http://codereview.appspot.com/6308097/diff/1/google-http-client/src/main/java/com/google/api/client/util/Data.java#newcode232 google-http-client/src/main/java/com/google/api/client/util/Data.java:232: } else if (data instanceof ImmutableList<?>) { On 2012/06/21 ...
13 years, 6 months ago (2012-06-21 12:13:37 UTC) #3
yanivi
http://codereview.appspot.com/6308097/diff/5001/google-http-client/src/main/java/com/google/api/client/util/Data.java File google-http-client/src/main/java/com/google/api/client/util/Data.java (right): http://codereview.appspot.com/6308097/diff/5001/google-http-client/src/main/java/com/google/api/client/util/Data.java#newcode232 google-http-client/src/main/java/com/google/api/client/util/Data.java:232: } else if (data instanceof ImmutableList<?>) { Some concrete ...
13 years, 5 months ago (2012-07-30 21:14:20 UTC) #4
rmistry
http://codereview.appspot.com/6308097/diff/5001/google-http-client/src/main/java/com/google/api/client/util/Data.java File google-http-client/src/main/java/com/google/api/client/util/Data.java (right): http://codereview.appspot.com/6308097/diff/5001/google-http-client/src/main/java/com/google/api/client/util/Data.java#newcode232 google-http-client/src/main/java/com/google/api/client/util/Data.java:232: } else if (data instanceof ImmutableList<?>) { On 2012/07/30 ...
13 years, 5 months ago (2012-07-31 15:30:51 UTC) #5
yanivi
13 years, 5 months ago (2012-07-31 16:53:52 UTC) #6
On 2012/07/31 15:30:51, rmistry wrote:
>
http://codereview.appspot.com/6308097/diff/5001/google-http-client/src/main/j...
> File google-http-client/src/main/java/com/google/api/client/util/Data.java
> (right):
> 
>
http://codereview.appspot.com/6308097/diff/5001/google-http-client/src/main/j...
> google-http-client/src/main/java/com/google/api/client/util/Data.java:232: }
> else if (data instanceof ImmutableList<?>) {
> On 2012/07/30 21:14:20, yanivi wrote:
> > Some concrete numbers: the installed size of calendar-preview-android-sample
> > goes from 328KB to 356KB, which is 28KB more.  I'm still undecided about
this
> > change.
> 
> 12.7% increase. Your call on whether we should do this or defer it till we
> absolutely need a com.google.collect dependency.

Defer.  Of course keep the diff in case we decide to do it later.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b