Merge pull request square#1589 from square/jw/2017-02-23/downloader

Make Downloader a thin veneer over a Call.Factory.

picasso/src/main/java/com/squareup/picasso/BitmapHunter.java

@@ -174,8 +174,8 @@ static Bitmap decodeStream(InputStream stream, Request request) throws IOExcepti
       } else {
         dispatcher.dispatchComplete(this);
       }
-    } catch (Downloader.ResponseException e) {
-      if (!e.localCacheOnly || e.responseCode != 504) {
+    } catch (NetworkRequestHandler.ResponseException e) {
+      if (!NetworkPolicy.isOfflineOnly(e.networkPolicy) || e.code != 504) {
         exception = e;
       }
       dispatcher.dispatchFailed(this);
@@ -210,7 +210,7 @@ Bitmap hunt() throws IOException {
       }
     }
 
-    data.networkPolicy = retryCount == 0 ? NetworkPolicy.OFFLINE.index : networkPolicy;
+    networkPolicy = retryCount == 0 ? NetworkPolicy.OFFLINE.index : networkPolicy;
     RequestHandler.Result result = requestHandler.load(data, networkPolicy);
     if (result != null) {
       loadedFrom = result.getLoadedFrom();

picasso/src/main/java/com/squareup/picasso/Downloader.java

@@ -15,79 +15,22 @@
  */
 package com.squareup.picasso;
 
-import android.graphics.Bitmap;
-import android.net.Uri;
 import android.support.annotation.NonNull;
-import android.support.annotation.Nullable;
-
 import java.io.IOException;
-import java.io.InputStream;
+import okhttp3.Response;
 
 /** A mechanism to load images from external resources such as a disk cache and/or the internet. */
 public interface Downloader {
   /**
    * Download the specified image {@code url} from the internet.
    *
-   * @param uri Remote image URL.
-   * @param networkPolicy The {@link NetworkPolicy} used for this request.
-   * @return {@link Response} containing either a {@link Bitmap} representation of the request or an
-   * {@link InputStream} for the image data. {@code null} can be returned to indicate a problem
-   * loading the bitmap.
    * @throws IOException if the requested URL cannot successfully be loaded.
    */
-  @Nullable Response load(@NonNull Uri uri, int networkPolicy) throws IOException;
+  @NonNull Response load(@NonNull okhttp3.Request request) throws IOException;
 
   /**
    * Allows to perform a clean up for this {@link Downloader} including closing the disk cache and
    * other resources.
    */
   void shutdown();
-
-  /** Thrown for non-2XX responses. */
-  class ResponseException extends IOException {
-    final boolean localCacheOnly;
-    final int responseCode;
-
-    public ResponseException(String message, int networkPolicy, int responseCode) {
-      super(message);
-      this.localCacheOnly = NetworkPolicy.isOfflineOnly(networkPolicy);
-      this.responseCode = responseCode;
-    }
-  }
-
-  /** Response stream or bitmap and info. */
-  class Response {
-    final InputStream stream;
-    final boolean cached;
-    final long contentLength;
-
-    /**
-     * Response stream and info.
-     *
-     * @param stream Image data stream.
-     * @param loadedFromCache {@code true} if the source of the stream is from a local disk cache.
-     * @param contentLength The content length of the response, typically derived by the
-     * {@code Content-Length} HTTP header.
-     */
-    public Response(InputStream stream, boolean loadedFromCache, long contentLength) {
-      if (stream == null) {
-        throw new IllegalArgumentException("Stream may not be null.");
-      }
-      this.stream = stream;
-      this.cached = loadedFromCache;
-      this.contentLength = contentLength;
-    }
-
-    /**
-     * Input stream containing image data.
-     */
-    public InputStream getInputStream() {
-      return stream;
-    }
-
-    /** Content length of the response. Only valid when used with {@link #getInputStream()}. */
-    public long getContentLength() {
-      return contentLength;
-    }
-  }
 }

picasso/src/main/java/com/squareup/picasso/NetworkRequestHandler.java

@@ -16,17 +16,16 @@
 package com.squareup.picasso;
 
 import android.net.NetworkInfo;
-import android.support.annotation.Nullable;
 import java.io.IOException;
 import java.io.InputStream;
+import okhttp3.CacheControl;
+import okhttp3.Response;
+import okhttp3.ResponseBody;
 
-import static com.squareup.picasso.Downloader.Response;
 import static com.squareup.picasso.Picasso.LoadedFrom.DISK;
 import static com.squareup.picasso.Picasso.LoadedFrom.NETWORK;
 
 class NetworkRequestHandler extends RequestHandler {
-  static final int RETRY_COUNT = 2;
-
   private static final String SCHEME_HTTP = "http";
   private static final String SCHEME_HTTPS = "https";
 
@@ -43,29 +42,35 @@ public NetworkRequestHandler(Downloader downloader, Stats stats) {
     return (SCHEME_HTTP.equals(scheme) || SCHEME_HTTPS.equals(scheme));
   }
 
-  @Override @Nullable public Result load(Request request, int networkPolicy) throws IOException {
-    Response response = downloader.load(request.uri, request.networkPolicy);
-    if (response == null) {
-      return null;
+  @Override public Result load(Request request, int networkPolicy) throws IOException {
+    okhttp3.Request downloaderRequest = createRequest(request, networkPolicy);
+    Response response = downloader.load(downloaderRequest);
+    ResponseBody body = response.body();
+
+    if (!response.isSuccessful()) {
+      body.close();
+      throw new ResponseException(response.code(), request.networkPolicy);
     }
 
-    Picasso.LoadedFrom loadedFrom = response.cached ? DISK : NETWORK;
+    // Cache response is only null when the response comes fully from the network. Both completely
+    // cached and conditionally cached responses will have a non-null cache response.
+    Picasso.LoadedFrom loadedFrom = response.cacheResponse() == null ? NETWORK : DISK;
 
-    InputStream is = response.getInputStream();
     // Sometimes response content length is zero when requests are being replayed. Haven't found
     // root cause to this but retrying the request seems safe to do so.
-    if (loadedFrom == DISK && response.getContentLength() == 0) {
-      Utils.closeQuietly(is);
+    if (loadedFrom == DISK && body.contentLength() == 0) {
+      body.close();
       throw new ContentLengthException("Received response with 0 content-length header.");
     }
-    if (loadedFrom == NETWORK && response.getContentLength() > 0) {
-      stats.dispatchDownloadFinished(response.getContentLength());
+    if (loadedFrom == NETWORK && body.contentLength() > 0) {
+      stats.dispatchDownloadFinished(body.contentLength());
     }
+    InputStream is = body.byteStream();
     return new Result(is, loadedFrom);
   }
 
   @Override int getRetryCount() {
-    return RETRY_COUNT;
+    return 2;
   }
 
   @Override boolean shouldRetry(boolean airplaneMode, NetworkInfo info) {
@@ -76,9 +81,44 @@ public NetworkRequestHandler(Downloader downloader, Stats stats) {
     return true;
   }
 
+  private static okhttp3.Request createRequest(Request request, int networkPolicy) {
+    CacheControl cacheControl = null;
+    if (networkPolicy != 0) {
+      if (NetworkPolicy.isOfflineOnly(networkPolicy)) {
+        cacheControl = CacheControl.FORCE_CACHE;
+      } else {
+        CacheControl.Builder builder = new CacheControl.Builder();
+        if (!NetworkPolicy.shouldReadFromDiskCache(networkPolicy)) {
+          builder.noCache();
+        }
+        if (!NetworkPolicy.shouldWriteToDiskCache(networkPolicy)) {
+          builder.noStore();
+        }
+        cacheControl = builder.build();
+      }
+    }
+
+    okhttp3.Request.Builder builder = new okhttp3.Request.Builder().url(request.uri.toString());
+    if (cacheControl != null) {
+      builder.cacheControl(cacheControl);
+    }
+    return builder.build();
+  }
+
   static class ContentLengthException extends IOException {
-    public ContentLengthException(String message) {
+    ContentLengthException(String message) {
       super(message);
     }
   }
+
+  static final class ResponseException extends IOException {
+    final int code;
+    final int networkPolicy;
+
+    ResponseException(int code, int networkPolicy) {
+      super("HTTP " + code);
+      this.code = code;
+      this.networkPolicy = networkPolicy;
+    }
+  }
 }

picasso/src/main/java/com/squareup/picasso/OkHttp3Downloader.java

@@ -16,22 +16,19 @@
 package com.squareup.picasso;
 
 import android.content.Context;
-import android.net.Uri;
 import android.support.annotation.NonNull;
 import android.support.annotation.VisibleForTesting;
-
 import java.io.File;
 import java.io.IOException;
 import okhttp3.Cache;
-import okhttp3.CacheControl;
 import okhttp3.Call;
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
-import okhttp3.ResponseBody;
+import okhttp3.Response;
 
 /** A {@link Downloader} which uses OkHttp to download images. */
 public final class OkHttp3Downloader implements Downloader {
-  private final Call.Factory client;
+  @VisibleForTesting final Call.Factory client;
   private final Cache cache;
   private boolean sharedClient = true;
 
@@ -90,53 +87,15 @@ public OkHttp3Downloader(Call.Factory client) {
     this.cache = null;
   }
 
-  @VisibleForTesting Cache getCache() {
-    return ((OkHttpClient) client).cache();
-  }
-
-  @Override public Response load(@NonNull Uri uri, int networkPolicy) throws IOException {
-    CacheControl cacheControl = null;
-    if (networkPolicy != 0) {
-      if (NetworkPolicy.isOfflineOnly(networkPolicy)) {
-        cacheControl = CacheControl.FORCE_CACHE;
-      } else {
-        CacheControl.Builder builder = new CacheControl.Builder();
-        if (!NetworkPolicy.shouldReadFromDiskCache(networkPolicy)) {
-          builder.noCache();
-        }
-        if (!NetworkPolicy.shouldWriteToDiskCache(networkPolicy)) {
-          builder.noStore();
-        }
-        cacheControl = builder.build();
-      }
-    }
-
-    Request.Builder builder = new okhttp3.Request.Builder().url(uri.toString());
-    if (cacheControl != null) {
-      builder.cacheControl(cacheControl);
-    }
-
-    okhttp3.Response response = client.newCall(builder.build()).execute();
-    int responseCode = response.code();
-    if (responseCode >= 300) {
-      response.body().close();
-      throw new ResponseException(responseCode + " " + response.message(), networkPolicy,
-          responseCode);
-    }
-
-    boolean fromCache = response.cacheResponse() != null;
-
-    ResponseBody responseBody = response.body();
-    return new Response(responseBody.byteStream(), fromCache, responseBody.contentLength());
+  @NonNull @Override public Response load(@NonNull Request request) throws IOException {
+    return client.newCall(request).execute();
   }
 
   @Override public void shutdown() {
-    if (!sharedClient) {
-      if (cache != null) {
-        try {
-          cache.close();
-        } catch (IOException ignored) {
-        }
+    if (!sharedClient && cache != null) {
+      try {
+        cache.close();
+      } catch (IOException ignored) {
       }
     }
   }

picasso/src/test/java/com/squareup/picasso/BitmapHunterTest.java

@@ -123,7 +123,7 @@ public class BitmapHunterTest {
   @Test public void responseExceptionDispatchFailed() {
     Action action = mockAction(URI_KEY_1, URI_1);
     BitmapHunter hunter = new TestableBitmapHunter(picasso, dispatcher, cache, stats, action, null,
-        new Downloader.ResponseException("Test", 0, 504));
+        new NetworkRequestHandler.ResponseException(0, 504));
     hunter.run();
     verify(dispatcher).dispatchFailed(hunter);
   }
@@ -1128,6 +1128,10 @@ private static class TestableRequestHandler extends RequestHandler {
       }
       return new Result(bitmap, MEMORY);
     }
+
+    @Override int getRetryCount() {
+      return 1;
+    }
   }
 
   private static class OOMBitmapHunter extends BitmapHunter {