Follow-up from bug 1197720. Implement proxy support for DownloadContentService.

Richard: I filed this bug after your comment here: bug 1197720 comment 15. Do I need to manually configure a proxy to be used by HttpClient in some situations? Is there some code in sync I can look at?

We have some code to help: java/org/mozilla/gecko/util/ProxySelector.java but if you're using HttpClient, you're basically implementing Bug 942652. Not too hard, but it hasn't been done yet.

Hey Krishna! This is a first simple bug. We are creating the HttpUrlConnection for the DownloadContentService here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java#146 And we have a helper method to add the proxies to an URLConnection here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/util/ProxySelector.java#31 You basically just need to connect the both parts. :)

Hey Sebastian, I will look into the code and come up with the patch real soon :)

Hey Sebastian, Please review the patch I have submitted. Please let me know if anything is not correct.

Comment on attachment 8747441 [details] Bug1209496.diff >diff --git a/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java b/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java >--- a/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java >+++ b/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java >@@ -11,25 +11,27 @@ import android.util.Log; > > import org.mozilla.gecko.AppConstants; > import org.mozilla.gecko.background.nativecode.NativeCrypto; > import org.mozilla.gecko.dlc.catalog.DownloadContent; > import org.mozilla.gecko.dlc.catalog.DownloadContentCatalog; > import org.mozilla.gecko.sync.Utils; > import org.mozilla.gecko.util.HardwareUtils; > import org.mozilla.gecko.util.IOUtils; >+import org.mozilla.gecko.util.ProxySelector; > > import java.io.BufferedInputStream; > import java.io.File; > import java.io.FileInputStream; > import java.io.IOException; > import java.io.InputStream; > import java.net.HttpURLConnection; > import java.net.MalformedURLException; >-import java.net.URL; >+import java.net.URI; >+import java.net.URISyntaxException; > > public abstract class BaseAction { > private static final String LOGTAG = "GeckoDLCBaseAction"; > > /** > * Exception indicating a recoverable error has happened. Download of the content will be retried later. > */ > /* package-private */ static class RecoverableDownloadContentException extends Exception { >@@ -140,24 +142,26 @@ public abstract class BaseAction { > throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO, e); > } finally { > IOUtils.safeStreamClose(inputStream); > } > } > > protected HttpURLConnection buildHttpURLConnection(String url) > throws UnrecoverableDownloadContentException, IOException { >- // TODO: Implement proxy support (Bug 1209496) >+ > try { > System.setProperty("http.keepAlive", "true"); > >- HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection(); >+ HttpURLConnection connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(new URI(url)); > connection.setRequestProperty("User-Agent", HardwareUtils.isTablet() ? > AppConstants.USER_AGENT_FENNEC_TABLET : > AppConstants.USER_AGENT_FENNEC_MOBILE); > connection.setRequestMethod("GET"); > connection.setInstanceFollowRedirects(true); > return connection; > } catch (MalformedURLException e) { > throw new UnrecoverableDownloadContentException(e); >+ } catch (URISyntaxException e) { >+ throw new UnrecoverableDownloadContentException(e); > } > } > }

Comment on attachment 8747441 [details] Bug1209496.diff Review of attachment 8747441 [details]: ----------------------------------------------------------------- This looks good. Can you upload a patch with the nit fixed? I'll push it to the try server afterwards. ::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java @@ +146,5 @@ > } > > protected HttpURLConnection buildHttpURLConnection(String url) > throws UnrecoverableDownloadContentException, IOException { > + nit: Remove this empty line. :)

Updated code. Removed the empty line. :)

Comment on attachment 8749947 [details] [diff] [review] Bug1209496 Review of attachment 8749947 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. For the next patches: Are you exporting your patches using Mercurial? They do not contain a header (No author, no commit message etc.).

https://hg.mozilla.org/integration/fx-team/rev/e9a379142b334234d148ea4c3b89edf259a1668b Bug 1209496 - DownloadContentService: Add proxy support. r=sebastian

(In reply to Sebastian Kaspari (:sebastian) from comment #11) > https://hg.mozilla.org/integration/fx-team/rev/ > e9a379142b334234d148ea4c3b89edf259a1668b > Bug 1209496 - DownloadContentService: Add proxy support. r=sebastian I added commit message and author and landed the patch.