Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-25663] Android: HttpClient.cache. ImageView.cache #9719

Closed
wants to merge 12 commits into from
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
package ti.modules.titanium.android.httpresponsecache;

import org.appcelerator.kroll.KrollModule;
import org.appcelerator.kroll.annotations.Kroll;
import org.appcelerator.kroll.common.Log;
import org.appcelerator.titanium.TiApplication;

import ti.modules.titanium.android.AndroidModule;
import android.app.Activity;
import android.app.Notification;
import android.net.http.HttpResponseCache;

import java.io.File;
import java.io.IOException;

@Kroll.module(parentModule = AndroidModule.class)
public class HttpResponseCacheModule extends KrollModule
{
private static final String TAG = "HttpResponseCache";
private static final String CACHE_SIZE_KEY = "ti.android.cache.size.max";
private static final int DEFAULT_CACHE_SIZE = 25 * 1024; // 25MB

private String httpCachePath;
private long httpCacheSize;
private File httpCacheDir;

public HttpResponseCacheModule()
{
super();
httpCachePath = "http";
httpCacheSize =
TiApplication.getInstance().getAppProperties().getInt(CACHE_SIZE_KEY, DEFAULT_CACHE_SIZE) * 1024;
}

@Kroll.method
public boolean install() throws IOException
{
HttpResponseCache cache = HttpResponseCache.getInstalled();
if (cache != null) {
cache.flush();
cache.close();
}
TiApplication tiApp = TiApplication.getInstance();
File dir = tiApp.getApplicationContext().getExternalCacheDir();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not have the path (or httpCacheDir) property value determine the directory and relative path to store the cache?

I'm worried here because the Android docs point out that if the requests contain private data, the external cache dir is a bad place to stick the cache due to access controls not being enforced on external storage.

I'm assuming it's because we don't actually expose the external cache dir anywhere in our APIs? cc @garymathews @jquick-axway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming it's because we don't actually expose the external cache dir anywhere in our APIs?

Yes. Also this is how currently cacheDir calculated for TiResponseCache.

if (dir == null) {
dir = tiApp.getApplicationContext().getCacheDir();
}
httpCacheDir = new File(dir, httpCachePath);
if (!httpCacheDir.exists()) {
if (!httpCacheDir.mkdir()) {
Log.e(TAG, "Failed to create cache directory");
return false;
}
} else if (!httpCacheDir.isDirectory()) {
Log.e(TAG, "Failed to create cache directory. \"" + httpCacheDir.getPath() + "\" exists.");
return false;
}
cache = HttpResponseCache.install(httpCacheDir, httpCacheSize);
return true;
}

@Kroll.method
public void flush()
{
HttpResponseCache cache = HttpResponseCache.getInstalled();
if (cache != null) {
cache.flush();
}
}

@Kroll.method
public void remove() throws IOException
{
HttpResponseCache cache = HttpResponseCache.getInstalled();
if (cache != null) {
cache.delete();
}
}

@Kroll.method
public void close() throws IOException
{
HttpResponseCache cache = HttpResponseCache.getInstalled();
if (cache != null) {
cache.close();
}
}

@Kroll.method
public int getHitCount()
{
HttpResponseCache cache = HttpResponseCache.getInstalled();
if (cache != null) {
return cache.getHitCount();
}
return 0;
}

@Kroll.method
public int getNetworkCount()
{
HttpResponseCache cache = HttpResponseCache.getInstalled();
if (cache != null) {
cache.getNetworkCount();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing return here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
return 0;
}

@Kroll.method
public int getRequestCount()
{
HttpResponseCache cache = HttpResponseCache.getInstalled();
if (cache != null) {
return cache.getRequestCount();
}
return 0;
}

@Kroll.method
public long size()
{
HttpResponseCache cache = HttpResponseCache.getInstalled();
if (cache != null) {
return cache.size();
}
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this return -1? Or do we want to have differing values for "no cache installed" versus "cache installed, but unable to determine size"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do we want to have differing values for "no cache installed" versus "cache installed, but unable to determine size"?

I think, we shouldn't mix it.

}

// clang-format off
@Kroll.method
@Kroll.getProperty
public String getHttpCachePath()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getPath()? Remove the @Kroll.method annotation (and then the clang-format comments are unnecessary too)?

// clang-format on
{
return httpCachePath;
}

// clang-format off
@Kroll.method
@Kroll.getProperty
public long getHttpCacheSize()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not real fond of the long name here. I assume it was used to not clash with the size() method?

Why not just mirror the underlying API's use of maxSize for the property/method name?

Additionally, please note that we're going to be moving away from getter/setter methods that just wrap properties in favor of using typical JS property accessors. So in this case, that'd be removing the @Kroll.method annotation and just assigning this method to be the backing impl for cache.httpCacheSize calls in JS. (Or hopefully cache.maxSize if the property is renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxSize it is

// clang-format on
{
return httpCacheSize;
}

// clang-format off
@Kroll.method
@Kroll.setProperty
public void setHttpCachePath(String value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not fond of the long name. Why not just use path as the property?

Also, the setter method would be deprecated in 8.0.0 immediately anyhow. So maybe just use this as the @Kroll.setProperty impl backing cache.path = 'whatever'; calls. (i.e. remove @Kroll.method)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

httpCachePath -> path, property only

// clang-format on
{
httpCachePath = value;
}

// clang-format off
@Kroll.method
@Kroll.setProperty
public void setHttpCacheSize(long value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setMaxSize(long value)?

// clang-format on
{
httpCacheSize = value;
}

@Override
public String getApiName()
{
return "Ti.Android.HttpResponseCache";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
// clang-format off
@Kroll.proxy(creatableInModule = NetworkModule.class,
propertyAccessors = {
TiC.PROPERTY_CACHE,
TiC.PROPERTY_FILE,
TiC.PROPERTY_ONSENDSTREAM,
TiC.PROPERTY_ONLOAD,
Expand Down Expand Up @@ -54,6 +55,7 @@ public class HTTPClientProxy extends KrollProxy
public HTTPClientProxy()
{
super();
defaultValues.put(TiC.PROPERTY_CACHE, false);
this.client = new TiHTTPClient(this);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1358,13 +1358,14 @@ protected void setUpClient(HttpURLConnection client, Boolean isPostOrPutOrPatch)
return;
}

client.setUseCaches(TiConvert.toBoolean(proxy.getProperty(TiC.PROPERTY_CACHE)));
client.setRequestMethod(method);
client.setDoInput(true);

if (isPostOrPutOrPatch) {
client.setDoOutput(true);
}
client.setUseCaches(false);

//Set Authorization value for Basic authentication
if (hasAuthentication) {
String domain = proxy.getDomain();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// clang-format off
@Kroll.proxy(creatableInModule = UIModule.class,
propertyAccessors = {
TiC.PROPERTY_CACHE,
TiC.PROPERTY_DECODE_RETRIES,
TiC.PROPERTY_AUTOROTATE,
TiC.PROPERTY_DEFAULT_IMAGE,
Expand All @@ -32,6 +33,7 @@ public class ImageViewProxy extends ViewProxy
public ImageViewProxy()
{
super();
defaultValues.put(TiC.PROPERTY_CACHE, true);
}

@Override
Expand Down
5 changes: 5 additions & 0 deletions android/titanium/src/java/org/appcelerator/titanium/TiC.java
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,11 @@ public class TiC
*/
public static final String PROPERTY_BYPASS_DND = "bypassDnd";

/**
* @module.api
*/
public static final String PROPERTY_CACHE = "cache";

/**
* @module.api
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.lang.ref.SoftReference;
import java.net.HttpURLConnection;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
Expand Down Expand Up @@ -45,6 +46,7 @@
import android.os.Handler;
import android.os.Message;
import android.webkit.URLUtil;
import org.appcelerator.titanium.TiC;

@SuppressWarnings("deprecation")
public class TiFileHelper implements Handler.Callback
Expand Down Expand Up @@ -148,6 +150,10 @@ public static TiFileHelper getInstance()
}

public InputStream openInputStream(String path, boolean report) throws IOException
{
return openInputStream(path, report, false);
}
public InputStream openInputStream(String path, boolean report, boolean useCaches) throws IOException
{
InputStream is = null;

Expand Down Expand Up @@ -180,10 +186,11 @@ public InputStream openInputStream(String path, boolean report) throws IOExcepti
}
} else if (URLUtil.isNetworkUrl(path)) {
if (TiApplication.isUIThread()) {
is = (InputStream) TiMessenger.sendBlockingRuntimeMessage(
getRuntimeHandler().obtainMessage(MSG_NETWORK_URL), path);
Message message = getRuntimeHandler().obtainMessage(MSG_NETWORK_URL);
message.getData().putBoolean(TiC.PROPERTY_CACHE, useCaches);
is = (InputStream) TiMessenger.sendBlockingRuntimeMessage(message, path);
} else {
is = handleNetworkURL(path);
is = handleNetworkURL(path, useCaches);
}
} else if (path.startsWith(RESOURCE_ROOT_ASSETS)) {
int len = "file:///android_asset/".length();
Expand Down Expand Up @@ -229,7 +236,7 @@ public InputStream openInputStream(String path, boolean report) throws IOExcepti
return is;
}

private InputStream handleNetworkURL(String path) throws IOException
private InputStream handleNetworkURL(String path, boolean useCaches) throws IOException
{
InputStream is = null;
try {
Expand All @@ -245,7 +252,9 @@ private InputStream handleNetworkURL(String path) throws IOException
}

URL u = new URL(path);
InputStream lis = u.openStream();
HttpURLConnection client = (HttpURLConnection) u.openConnection();
client.setUseCaches(useCaches);
InputStream lis = client.getInputStream();
ByteArrayOutputStream bos = null;
try {
bos = new ByteArrayOutputStream(8192);
Expand Down Expand Up @@ -303,8 +312,8 @@ public Drawable loadDrawable(String path, boolean report, boolean checkForNinePa
/**
* This method creates a Drawable given the bitmap's path, and converts it to a NinePatch Drawable
* if checkForNinePatch param is true.
* @param path the path/url of the Drawable
* @param report this is not being used.
* @param path the path/url of the Drawable
* @param report this is not being used.
* @param checkForNinePatch a boolean to determine whether the returning Drawable is a NinePatch Drawable.
* @param densityScaled a boolean to determine whether the returning Drawable is scaled based on device density.
* @return a Drawable instance.
Expand Down Expand Up @@ -780,7 +789,8 @@ public boolean handleMessage(Message msg)
case MSG_NETWORK_URL:
AsyncResult result = (AsyncResult) msg.obj;
try {
result.setResult(handleNetworkURL(TiConvert.toString(result.getArg())));
result.setResult(handleNetworkURL(TiConvert.toString(result.getArg()),
msg.getData().getBoolean(TiC.PROPERTY_CACHE)));
} catch (IOException e) {
e.printStackTrace();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.appcelerator.kroll.common.Log;
import org.appcelerator.titanium.TiApplication;
import org.appcelerator.titanium.TiBlob;
import org.appcelerator.titanium.TiC;
import org.appcelerator.titanium.TiDimension;
import org.appcelerator.titanium.TiFileProxy;
import org.appcelerator.titanium.io.TiBaseFile;
Expand Down Expand Up @@ -81,6 +82,7 @@ public static class Bounds
private boolean anyDensityFalse = false;
private boolean autoRotate;
private int orientation = -1;
public boolean useCaches = false;

// TIMOB-3599: A bug in Gingerbread forces us to retry decoding bitmaps when they initially fail
public static final int DEFAULT_DECODE_RETRIES = 5;
Expand Down Expand Up @@ -163,7 +165,14 @@ public static TiDrawableReference fromUrl(KrollProxy proxy, String url)
if (url == null || url.length() == 0 || url.trim().length() == 0) {
return new TiDrawableReference(proxy.getActivity(), DrawableReferenceType.NULL);
}
return fromUrl(proxy.getActivity(), proxy.resolveUrl(null, url));
boolean useCaches = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can shorten this to:

boolean useCaches = TiConvert.toBoolean(proxy.getProperty(TiC.PROPERTY_CACHE), false);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Object cacheProperty = proxy.getProperty(TiC.PROPERTY_CACHE);
if (cacheProperty != null) {
useCaches = TiConvert.toBoolean(cacheProperty);
}
TiDrawableReference ref = fromUrl(proxy.getActivity(), proxy.resolveUrl(null, url));
ref.useCaches = useCaches;
return ref;
}

/**
Expand Down Expand Up @@ -239,8 +248,15 @@ public static TiDrawableReference fromObject(KrollProxy proxy, Object object)
object = proxy.resolveUrl(null, (String) object);
}

boolean useCaches = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, you can shrink this down to a one-liner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Object cacheProperty = proxy.getProperty(TiC.PROPERTY_CACHE);
if (cacheProperty != null) {
useCaches = TiConvert.toBoolean(cacheProperty);
}
// Create a drawable reference from the given object.
return TiDrawableReference.fromObject(activity, object);
TiDrawableReference ref = TiDrawableReference.fromObject(activity, object);
ref.useCaches = useCaches;
return ref;
}

/**
Expand Down Expand Up @@ -411,6 +427,7 @@ public Bitmap getBitmap(boolean needRetry, boolean densityScaled)
URL mURL = new URL(url);
connection = (HttpURLConnection) mURL.openConnection();
connection.setInstanceFollowRedirects(true);
connection.setUseCaches(useCaches);
connection.setDoInput(true);
connection.connect();
int responseCode = connection.getResponseCode();
Expand Down Expand Up @@ -893,7 +910,7 @@ public InputStream getInputStream()

if (isTypeUrl() && url != null) {
try {
stream = TiFileHelper.getInstance().openInputStream(url, false);
stream = TiFileHelper.getInstance().openInputStream(url, false, useCaches);

} catch (IOException e) {
Log.e(TAG, "Problem opening stream with url " + url + ": " + e.getMessage());
Expand Down