New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTPS, cleaned up HttpClient use, bump Jackson #180

Merged
merged 10 commits into from Sep 24, 2011

Conversation

Projects
None yet
2 participants
@candrews
Copy link
Contributor

candrews commented Aug 26, 2011

I've implemented HTTPS support, cleaned up the HttpClient code a tiny bit, and bumped the Jackson version from 1.5.2 to 1.8.5. I tested it, and everything seems to still be working - and securely.

I used small, single topic commits, with minimal modifications. If there's anything that isn't acceptable, please let me know.

Thanks!

candrews added some commits Aug 24, 2011

Reusing existing ObjectMapper in RestJsonClient.
Don't create new ObjectMapper's - one is sufficient.
ObjectMapper.readValue can use an InputStream directly, no need to convert to String first.
Use the HttpClient interface throughout instead of the specific Defau…
…ltHttpClient implementation

Expose HttpClient's cookie store via Common.getCookieStore() because the HttpClient interface doesn't expose it
Override only createClientConnectionManager in DefaultHttpClient, pre…
…serving the default params.

Make the https scheme work.
If available, use SSL session caching to speed up SSL connections. If…
… not available, use a non-caching SSL socket factory.

This feature is available starting in Android 8.
@talklittle

This comment has been minimized.

Copy link

talklittle commented on 34d5930 Sep 7, 2011

good change

@talklittle

This comment has been minimized.

Copy link

talklittle commented on 5052e7c Sep 7, 2011

good change. I would also remove the now-unused convertStreamToString method.

@talklittle

This comment has been minimized.

Copy link

talklittle commented on e1a3f1a Sep 7, 2011

This change adds about 250kb to the lib size (significant). Can you tell me the relevant changes between 1.5.2 and 1.8.5 that prompted the version update?

This comment has been minimized.

Copy link
Owner

candrews replied Sep 7, 2011

I always try to keep libraries up to date for various reasons (usually to fix bugs or improve performance). I'm surprised this library grew so much in size - I never checked before. Perhaps I should look into using proguard which should eliminate this growth as my next effort. In the mean time, other than "for the heck of it" there was no reason I updated the library, so feel free to not pull this commit. :-)

This comment has been minimized.

Copy link

talklittle replied Sep 7, 2011

Figuring out how to get Proguard working on this project would be fantastic. For now I think I'll omit this change.

This comment has been minimized.

Copy link
Owner

candrews replied Sep 7, 2011

Sounds good to me!

@talklittle

This comment has been minimized.

Copy link

talklittle commented on aa55495 Sep 7, 2011

I like this change.

@talklittle

This comment has been minimized.

Copy link

talklittle commented on 9a9792c Sep 7, 2011

Not sure why this change was made. I like the other way better, since it makes it a bit cleaner when constructing URLs--don't need to add the ".json" everywhere. In the past I used the .json in URLs, but api.reddit.com was later introduced by the reddit admins, and I prefer using that.

Maybe we could add a second constant, Constants.REDDIT_API_BASE_URL, and revert the ".json" pieces?

This comment has been minimized.

Copy link
Owner

candrews replied Sep 7, 2011

I made this change because there is no HTTPS service for the api.reddit.com-style URLs. So if we want SSL, we have to use the .json style :-/

This comment has been minimized.

Copy link

talklittle replied Sep 9, 2011

@talklittle

This comment has been minimized.

Hmm, client is null when loading reddit preferences in BrowserActivity. I don't remember why I did that, though. I'll have to remember what the reason was.

@talklittle

This comment has been minimized.

Copy link

talklittle commented on 32b2f40 Sep 7, 2011

This does seem to be better coding practice. Not sure if it's necessary though--look at most Android code snippets online and you see they're using DefaultHttpClient instead of HttpClient.

This comment has been minimized.

Copy link
Owner

candrews replied Sep 7, 2011

Besides coding practice, there are good practical reasons to do this change. For example, Android SDK>=8's AndroidHttpClient implements HttpClient, but does not extend DefaultHttpClient. Therefore, you can't use DefaultHttpClient everywhere. I really need to write a blog post and complain about how many Android app's use DefaultHttpClient, and end up missing out on SSL session caching, multithreading, and other such things.

This comment has been minimized.

Copy link

talklittle replied Sep 7, 2011

Ah, that does sound like a good reason. Thanks for the heads up.

@talklittle

This comment has been minimized.

Copy link

talklittle commented on 7588cdd Sep 7, 2011

Interesting. :)

@talklittle

This comment has been minimized.

Copy link

talklittle commented on 9262284 Sep 7, 2011

Nice work!

@talklittle

This comment has been minimized.

Copy link

talklittle commented on ba5fcd4 Sep 7, 2011

Cool

@talklittle

This comment has been minimized.

Copy link
Owner

talklittle commented Sep 7, 2011

Thanks a lot for the work you did on this! The one other thing I'd want to add is the ability to toggle between HTTP and HTTPS via the Settings page.

Add Proguard support.
Note that Proguard 4.4 (which is currently included in the Android SDK) is to buggy and creates an apk that crashes at startup. Proguard 4.6 works. See https://code.google.com/p/android/issues/detail?id=18371
Removed Markdown jar as it is not used.
Included "library dependency" jars that aren't used at runtime by the application but are referred to by the libraries the application uses and therefore are required by Proguard.
Bump Android target to 9, as there are some reflection references to classes in Android version 9.
@candrews

This comment has been minimized.

Copy link
Contributor

candrews commented Sep 11, 2011

I've added proguard support in my last commit. Let me know what you think.

@talklittle

This comment has been minimized.

Copy link

talklittle commented on baa48ba Sep 24, 2011

Awesome! Thank you for doing this. Is there an improvement in apk size?

This comment has been minimized.

Copy link
Owner

candrews replied Sep 24, 2011

720K ThreadsListActivity-debug.apk
488K ThreadsListActivity-release.apk

(debug is non-proguarded, release is proguarded)

32% savings. Not bad :-)

This comment has been minimized.

Copy link

talklittle replied Sep 24, 2011

Wow! very nice!

This comment has been minimized.

Copy link
Owner

candrews replied Sep 24, 2011

I'd love to get this patch series (proguard, https) in at some point. What else can I do to make that happen?

talklittle added a commit that referenced this pull request Sep 24, 2011

Merge pull request #180 from candrews/improvements
HTTPS, cleaned up HttpClient use, bump Jackson

@talklittle talklittle merged commit f70f0c6 into talklittle:master Sep 24, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment