Closed Bug 1169421 Opened 5 years ago Closed 4 years ago

Switch Fennec to use ch.boye instead of org.apache.http to allow for building with Android M SDK 23

Categories

(Firefox for Android :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: snorp, Assigned: amoghbl1)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor-mobile])

Attachments

(2 files, 4 obsolete files)

Attached file build-23.log
I get the attached build failure when trying to build against the Android M preview SDK (after fixing a couple new warnings). Looks like it can't find the apache stuff for some reason.
Looks like the removed the apache http client in the SDK entirely. It's available as a separate jar, so we could do that. We apparently already include ch.boye, though, so we could also just change all the call sites to use that.
Moving to ch.boye entirely ? I'll take this up.
Assignee: nobody → amoghbl1
Status: NEW → ASSIGNED
Summary: Build fails with Android M SDK 23 → Switch Fennec to use ch.boye instead of org.apache.http to allow for building with Android M SDK 23
Version: Firefox 34 → Trunk
This switch would involve replacing the android.net.http.AndroidHttpClient with ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient .
The current call for a httpClient is (from LoadFaviconTask.java)
> static AndroidHttpClient httpClient = AndroidHttpClient.newInstance(GeckoAppShell.getGeckoInterface().getDefaultUAString());

This would need to be replaced with (from DoCommand.java)
> HttpClient httpClient = new DefaultHttpClient();
Flags: needinfo?(snorp)
Attached patch bug_1169421.patch (obsolete) — Splinter Review
Changes the http client library used for two files
1) mobile/android/base/distribution/Distribution.java
2) mobile/android/base/favicons/LoadFaviconTask.java
Also changes the AndroidHttpClient to DefaultHttpClient for the second class.
Attachment #8623308 - Flags: review?(mark.finkle)
Comment on attachment 8623308 [details] [diff] [review]
bug_1169421.patch

>diff --git a/mobile/android/base/favicons/LoadFaviconTask.java b/mobile/android/base/favicons/LoadFaviconTask.java

>-    static AndroidHttpClient httpClient = AndroidHttpClient.newInstance(GeckoAppShell.getGeckoInterface().getDefaultUAString());
>+    static DefaultHttpClient httpClient = new DefaultHttpClient();

What about the UA?

I think we'll need to add a line here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/LoadFaviconTask.java#131

Something like:
request.setHeader("User-Agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());

Other global approaches are here:
http://stackoverflow.com/questions/5027309/how-to-set-custom-user-agent-with-apache-http-client-library-4-1
Attachment #8623308 - Flags: review?(mark.finkle) → review-
Attached patch bug_1169421.patch (obsolete) — Splinter Review
User Agent header added to request.
Attachment #8623308 - Attachment is obsolete: true
Attachment #8624445 - Flags: review?(mark.finkle)
Comment on attachment 8624445 [details] [diff] [review]
bug_1169421.patch

You have a bunch of file deletes in this patch which I don't think we want :)
Attachment #8624445 - Flags: review?(mark.finkle) → review-
Attached patch fix_1169421.patch (obsolete) — Splinter Review
Attachment #8624445 - Attachment is obsolete: true
Attachment #8625033 - Flags: review?(mark.finkle)
Comment on attachment 8625033 [details] [diff] [review]
fix_1169421.patch

Looks OK
Attachment #8625033 - Flags: review?(mark.finkle) → review+
Is anything stopping us from landing this? I'll push the patch to try.
Flags: needinfo?(amoghbl1)
Oops, sorry about that, I've made a mistake in the import.
Will fix now!
Flags: needinfo?(amoghbl1)
Attached patch fix_1169421.patch (obsolete) — Splinter Review
Attachment #8625033 - Attachment is obsolete: true
Thank you for the updated patch! I just pushed it to try. :)
amoghbl1: is there a reason you're not pushing to try yourself?  (Other than too busy :)  I see I vouched for you in Bug 1039210.
Flags: needinfo?(amoghbl1)
nalexander: Shall do it for the next patch! I still need to read up on how to use try.
Flags: needinfo?(amoghbl1)
(In reply to amoghbl1 from comment #18)
> nalexander: Shall do it for the next patch! I still need to read up on how
> to use try.

I'm not nalexander but yeah, whenever you think your patch is ready, push it to try to see if there are any regressions or other things you missed.

You can find more information about the try server here: https://wiki.mozilla.org/ReleaseEngineering/TryServer Note that this page is very "patch queue" (mq) heavy - so this can be confusing if it's not your primary work flow. This TryChooser syntax builder can be helpful when creating a commit to push to try: http://trychooser.pub.build.mozilla.org/ (For most Android stuff: Select "both" build types, all three Android platforms and check all robocop tests)

Seems like the last try run was sucessful. I've setup a build system using the M preview SDK locally and will try to build with your patch applied to see if we have missed anything.
Yep! All HttpClient related build errors are gone. Thank you! :)
amoghbl1: I guess this patch is ready to land. Just update your commit message to include the reviewer "r=mfinkle" and remove the "[mq]: " prefix. Then we should be ready to add the "checkin-needed" keyword. :)
Flags: needinfo?(amoghbl1)
(In reply to :Sebastian Kaspari from comment #21)
> amoghbl1: I guess this patch is ready to land. Just update your commit
> message to include the reviewer "r=mfinkle" and remove the "[mq]: " prefix.
> Then we should be ready to add the "checkin-needed" keyword. :)

Wow, I forgot about this one! Updating now.
Flags: needinfo?(amoghbl1)
Updated the commit message.
Attachment #8632874 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b48142f4120
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Flags: needinfo?(snorp)
See Also: → 765064
Blocks: 1357994
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.