Android: Upgrade HTTP Lib

[dosiecki]

The default HTTP client lib on older Android devices is pretty badly broken and has no upgrade path. In the lastest release of Android, GOOG have stated that they have no intent of maintaining it.

We should probably switch to github.com/square/okhttp or one of the other popular client libs so we have better control over how we call the server. This would not only let us run a more secure backend config, it would let us tweak a lot of new options for speed (like connection reuse and more timeout options).

[manderson23]

How much effort do you think this is and how critical is it?

Just wondering if this is the sort of non-critical background task I could pick up to leave you to focus on delivering user facing features?

[dosiecki]

In theory, it should be pretty easy to grab the okhttp and okio jars, swap out our half-dozen uses of the HTTP client, and translate all the configuration calls into their parlance. The bulk of the work will be the huge amount of testing required to fully adapt to their handling of errors.

[manderson23]

OK. I'm happy to pick this one up.

[manderson23]

I noticed the following comment in APIRepsonse

TODO: the existing code rejects redirects as errors. Is this correct?

Is this something we need to keep?

[dosiecki]

No idea, the code was like that when I found it years ago. Maybe @samuelclay can confirm whether redirects from the API shouldn't be followed or if that was a fluke of the original impl.

[samuelclay]

Yes, that should be fine. There are several redirects, but they are mostly around importing from Reader and other OAuth flows, none of which are used on Android. We'll test every endpoint to make sure it works.

[manderson23]

In NetworkUtils I noticed the following

// we explicitly requested redirects, so if we still get one, it is because of a protocol
// change. inform the caller by returning the new URL

By protocol change do you mean HTTP -> HTTPS (or HTTPS -> HTTP)? If so OkHttp supports this by default so we don't need this any more?

[dosiecki]

Yeah, lines 29-35 inNetworkUtils and 59-64 in ImageCache are all to handle the same, annoying "feature" of the inbuilt HTTP client. Even with setInstanceFollowRedirects set, it will not follow redirects that switch from HTTP to HTTPS or vice-versa. It just writes out the literal HTTP redirect response as if it were the requested object. Thus, we have to sense the situation and re-start a new request.

For whatever reason, many content sites like to vend image URLs in one protocol and use redirects to the other. If the new client is smart enough, all that code can be gutted. It should be pretty easy to test if the new client is handling things correctly - just turn on image prefetch and see if any images appear "broken" when cached.

[manderson23]

getCategories and addCategory are unused in APIManager. Can they (and related classes) be safely deleted? I don't see the URLs they use on http://www.newsblur.com/api

[dosiecki]

Yeah, definitely remove anything unused. I have been working on cruft for over a year now, but there is still plenty left to rm!

[samuelclay]

GetCategories are for the first-time user experience. Those are the sites that come recommended for a new user. Create a throwaway and take a look.

[manderson23]

I think this step was removed from Android in #548 since it was broken. Hence these operations were unused.

[dosiecki]

Implemented in #680.