Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Try OkHTTP on Android again #2856

Closed
ljbade opened this issue Oct 29, 2015 · 6 comments
Closed

Try OkHTTP on Android again #2856

ljbade opened this issue Oct 29, 2015 · 6 comments
Assignees
Labels
Android Mapbox Maps SDK for Android offline

Comments

@ljbade
Copy link
Contributor

ljbade commented Oct 29, 2015

Time to try #2033 again and use OkHTTP instead of curl for Android.

Previous blocker for this was #2400 which has not been looked at for a while.

Things that might have fixed that crash:

OkHTTP will be an enabler for offline support.

/cc @bleege @tobrun

@ljbade ljbade added Android Mapbox Maps SDK for Android P0 labels Oct 29, 2015
@ljbade ljbade added this to the android-v2.3.0 milestone Oct 29, 2015
@ljbade ljbade added the offline label Oct 29, 2015
@ljbade
Copy link
Contributor Author

ljbade commented Oct 29, 2015

We will need to perform extended testing over a period of time as #2400 was a rare and hard to reproduce reliably.

Perhaps a Sirius build with this change included will provide the confidence needed.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 29, 2015

Simply switching config.mk to the Android HTTP implementation, reveals bit rot in http_request_android.cpp due to refactors.

@mb12
Copy link

mb12 commented Oct 29, 2015

@ljbade The only advantage of this is reduced .apk size. Its no longer an issue on Android (since apk size limit has been doubled). Why do we want to change this if curl implementation does not have any bugs?

@ljbade
Copy link
Contributor Author

ljbade commented Oct 29, 2015

After fixing the compile errors, the OkHTTP stack is working again. So far I have not been able to reproduce #2400 but more testing is needed.

ljbade pushed a commit that referenced this issue Oct 29, 2015
@ljbade ljbade self-assigned this Oct 29, 2015
@bleege
Copy link
Contributor

bleege commented Oct 30, 2015

Why do we want to change this if curl implementation does not have any bugs?

@mb12 The big reason is as you mentioned, a reduced .aar and .apk sizes. Not having to compile it for N ABI really helps keep things svelte. Another reason is to use more platform specific implementations (in this case Java for Android) as it is more accessible to developers.

ljbade pushed a commit that referenced this issue Nov 4, 2015
@bleege
Copy link
Contributor

bleege commented Nov 9, 2015

For future reference, by replacing curl with OkHttp the Mapbox Android Library .aar file goes from ~11 MB to ~7.8 MB.

curl
Mapbox Android SDK 2.2.0 with curl

okhttp
Mapbox Android SDK 2.3.0-SNAPSHOT with OkHttp replacing curl

The size of OkHttp and it's dependency Okio combine to be about ~375 KB so that the net result is a ~2.825 MB overall decrease in size by replacing curl with OkHttp.

okhttp-library
OkHttp Library

okio-library
Okio Library

ljbade pushed a commit that referenced this issue Nov 9, 2015
Update http_request_android.cpp for changes in #2727
Fix crash caused by calling both onFailure and onReponse in the same request
Fixes #2856
Fixes #2400
ljbade pushed a commit that referenced this issue Nov 12, 2015
Update http_request_android.cpp for changes in #2727
Fix crash caused by calling both onFailure and onReponse in the same request
Fixes #2856
Fixes #2400
@ljbade ljbade closed this as completed in af2d034 Nov 16, 2015
@bleege bleege mentioned this issue Dec 4, 2015
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android offline
Projects
None yet
Development

No branches or pull requests

3 participants