Commit 14c46bc4 by olly Committed by Toni

Remove contentTypePredicate from DataSource constructors

The only known use case for contentTypePredicate is to catch
the case when a paywall web page is returned via a DataSource,
rather than the data that was being requested. These days streaming
providers should be using HTTPS, where this problem does not exist.
Devices have also gotten a lot better at showing their own
notifications when paywalls are detected, which largely mitigates
the need for the app to show a more optimal error message or
redirect the user to a browser.

It therefore makes sense to deprioritize this feature. In
particular by removing the arg from constructors, where nearly
all applications are probably passing null.

PiperOrigin-RevId: 249634594
parent 33143919
......@@ -116,7 +116,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
private final CronetEngine cronetEngine;
private final Executor executor;
@Nullable private final Predicate<String> contentTypePredicate;
private final int connectTimeoutMs;
private final int readTimeoutMs;
private final boolean resetTimeoutOnRedirects;
......@@ -126,6 +125,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
private final ConditionVariable operation;
private final Clock clock;
@Nullable private Predicate<String> contentTypePredicate;
// Accessed by the calling thread only.
private boolean opened;
private long bytesToSkip;
......@@ -158,7 +159,78 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
* handling is a fast operation when using a direct executor.
*/
public CronetDataSource(CronetEngine cronetEngine, Executor executor) {
this(cronetEngine, executor, /* contentTypePredicate= */ null);
this(
cronetEngine,
executor,
DEFAULT_CONNECT_TIMEOUT_MILLIS,
DEFAULT_READ_TIMEOUT_MILLIS,
/* resetTimeoutOnRedirects= */ false,
/* defaultRequestProperties= */ null);
}
/**
* @param cronetEngine A CronetEngine.
* @param executor The {@link java.util.concurrent.Executor} that will handle responses. This may
* be a direct executor (i.e. executes tasks on the calling thread) in order to avoid a thread
* hop from Cronet's internal network thread to the response handling thread. However, to
* avoid slowing down overall network performance, care must be taken to make sure response
* handling is a fast operation when using a direct executor.
* @param connectTimeoutMs The connection timeout, in milliseconds.
* @param readTimeoutMs The read timeout, in milliseconds.
* @param resetTimeoutOnRedirects Whether the connect timeout is reset when a redirect occurs.
* @param defaultRequestProperties Optional default {@link RequestProperties} to be sent to the
* server as HTTP headers on every request.
*/
public CronetDataSource(
CronetEngine cronetEngine,
Executor executor,
int connectTimeoutMs,
int readTimeoutMs,
boolean resetTimeoutOnRedirects,
@Nullable RequestProperties defaultRequestProperties) {
this(
cronetEngine,
executor,
connectTimeoutMs,
readTimeoutMs,
resetTimeoutOnRedirects,
Clock.DEFAULT,
defaultRequestProperties,
/* handleSetCookieRequests= */ false);
}
/**
* @param cronetEngine A CronetEngine.
* @param executor The {@link java.util.concurrent.Executor} that will handle responses. This may
* be a direct executor (i.e. executes tasks on the calling thread) in order to avoid a thread
* hop from Cronet's internal network thread to the response handling thread. However, to
* avoid slowing down overall network performance, care must be taken to make sure response
* handling is a fast operation when using a direct executor.
* @param connectTimeoutMs The connection timeout, in milliseconds.
* @param readTimeoutMs The read timeout, in milliseconds.
* @param resetTimeoutOnRedirects Whether the connect timeout is reset when a redirect occurs.
* @param defaultRequestProperties Optional default {@link RequestProperties} to be sent to the
* server as HTTP headers on every request.
* @param handleSetCookieRequests Whether "Set-Cookie" requests on redirect should be forwarded to
* the redirect url in the "Cookie" header.
*/
public CronetDataSource(
CronetEngine cronetEngine,
Executor executor,
int connectTimeoutMs,
int readTimeoutMs,
boolean resetTimeoutOnRedirects,
@Nullable RequestProperties defaultRequestProperties,
boolean handleSetCookieRequests) {
this(
cronetEngine,
executor,
connectTimeoutMs,
readTimeoutMs,
resetTimeoutOnRedirects,
Clock.DEFAULT,
defaultRequestProperties,
handleSetCookieRequests);
}
/**
......@@ -171,7 +243,10 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
* @param contentTypePredicate An optional {@link Predicate}. If a content type is rejected by the
* predicate then an {@link InvalidContentTypeException} is thrown from {@link
* #open(DataSpec)}.
* @deprecated Use {@link #CronetDataSource(CronetEngine, Executor)} and {@link
* #setContentTypePredicate(Predicate)}.
*/
@Deprecated
public CronetDataSource(
CronetEngine cronetEngine,
Executor executor,
......@@ -182,9 +257,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
contentTypePredicate,
DEFAULT_CONNECT_TIMEOUT_MILLIS,
DEFAULT_READ_TIMEOUT_MILLIS,
false,
null,
false);
/* resetTimeoutOnRedirects= */ false,
/* defaultRequestProperties= */ null);
}
/**
......@@ -200,9 +274,12 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
* @param connectTimeoutMs The connection timeout, in milliseconds.
* @param readTimeoutMs The read timeout, in milliseconds.
* @param resetTimeoutOnRedirects Whether the connect timeout is reset when a redirect occurs.
* @param defaultRequestProperties The optional default {@link RequestProperties} to be sent to
* the server as HTTP headers on every request.
* @param defaultRequestProperties Optional default {@link RequestProperties} to be sent to the
* server as HTTP headers on every request.
* @deprecated Use {@link #CronetDataSource(CronetEngine, Executor, int, int, boolean,
* RequestProperties)} and {@link #setContentTypePredicate(Predicate)}.
*/
@Deprecated
public CronetDataSource(
CronetEngine cronetEngine,
Executor executor,
......@@ -218,9 +295,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
connectTimeoutMs,
readTimeoutMs,
resetTimeoutOnRedirects,
Clock.DEFAULT,
defaultRequestProperties,
false);
/* handleSetCookieRequests= */ false);
}
/**
......@@ -236,11 +312,14 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
* @param connectTimeoutMs The connection timeout, in milliseconds.
* @param readTimeoutMs The read timeout, in milliseconds.
* @param resetTimeoutOnRedirects Whether the connect timeout is reset when a redirect occurs.
* @param defaultRequestProperties The optional default {@link RequestProperties} to be sent to
* the server as HTTP headers on every request.
* @param defaultRequestProperties Optional default {@link RequestProperties} to be sent to the
* server as HTTP headers on every request.
* @param handleSetCookieRequests Whether "Set-Cookie" requests on redirect should be forwarded to
* the redirect url in the "Cookie" header.
* @deprecated Use {@link #CronetDataSource(CronetEngine, Executor, int, int, boolean,
* RequestProperties, boolean)} and {@link #setContentTypePredicate(Predicate)}.
*/
@Deprecated
public CronetDataSource(
CronetEngine cronetEngine,
Executor executor,
......@@ -253,19 +332,18 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
this(
cronetEngine,
executor,
contentTypePredicate,
connectTimeoutMs,
readTimeoutMs,
resetTimeoutOnRedirects,
Clock.DEFAULT,
defaultRequestProperties,
handleSetCookieRequests);
this.contentTypePredicate = contentTypePredicate;
}
/* package */ CronetDataSource(
CronetEngine cronetEngine,
Executor executor,
@Nullable Predicate<String> contentTypePredicate,
int connectTimeoutMs,
int readTimeoutMs,
boolean resetTimeoutOnRedirects,
......@@ -276,7 +354,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
this.urlRequestCallback = new UrlRequestCallback();
this.cronetEngine = Assertions.checkNotNull(cronetEngine);
this.executor = Assertions.checkNotNull(executor);
this.contentTypePredicate = contentTypePredicate;
this.connectTimeoutMs = connectTimeoutMs;
this.readTimeoutMs = readTimeoutMs;
this.resetTimeoutOnRedirects = resetTimeoutOnRedirects;
......@@ -287,6 +364,17 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
operation = new ConditionVariable();
}
/**
* Sets a content type {@link Predicate}. If a content type is rejected by the predicate then a
* {@link HttpDataSource.InvalidContentTypeException} is thrown from {@link #open(DataSpec)}.
*
* @param contentTypePredicate The content type {@link Predicate}, or {@code null} to clear a
* predicate that was previously set.
*/
public void setContentTypePredicate(@Nullable Predicate<String> contentTypePredicate) {
this.contentTypePredicate = contentTypePredicate;
}
// HttpDataSource implementation.
@Override
......@@ -363,6 +451,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
}
// Check for a valid content type.
Predicate<String> contentTypePredicate = this.contentTypePredicate;
if (contentTypePredicate != null) {
List<String> contentTypeHeaders = responseInfo.getAllHeaders().get(CONTENT_TYPE);
String contentType = isEmpty(contentTypeHeaders) ? null : contentTypeHeaders.get(0);
......
......@@ -38,7 +38,6 @@ import com.google.android.exoplayer2.upstream.HttpDataSource;
import com.google.android.exoplayer2.upstream.HttpDataSource.HttpDataSourceException;
import com.google.android.exoplayer2.upstream.TransferListener;
import com.google.android.exoplayer2.util.Clock;
import com.google.android.exoplayer2.util.Predicate;
import com.google.android.exoplayer2.util.Util;
import java.io.IOException;
import java.net.SocketTimeoutException;
......@@ -85,7 +84,6 @@ public final class CronetDataSourceTest {
@Mock private UrlRequest.Builder mockUrlRequestBuilder;
@Mock private UrlRequest mockUrlRequest;
@Mock private Predicate<String> mockContentTypePredicate;
@Mock private TransferListener mockTransferListener;
@Mock private Executor mockExecutor;
@Mock private NetworkException mockNetworkException;
......@@ -95,21 +93,19 @@ public final class CronetDataSourceTest {
private boolean redirectCalled;
@Before
public void setUp() throws Exception {
public void setUp() {
MockitoAnnotations.initMocks(this);
dataSourceUnderTest =
new CronetDataSource(
mockCronetEngine,
mockExecutor,
mockContentTypePredicate,
TEST_CONNECT_TIMEOUT_MS,
TEST_READ_TIMEOUT_MS,
true, // resetTimeoutOnRedirects
/* resetTimeoutOnRedirects= */ true,
Clock.DEFAULT,
null,
false);
/* defaultRequestProperties= */ null,
/* handleSetCookieRequests= */ false);
dataSourceUnderTest.addTransferListener(mockTransferListener);
when(mockContentTypePredicate.evaluate(anyString())).thenReturn(true);
when(mockCronetEngine.newUrlRequestBuilder(
anyString(), any(UrlRequest.Callback.class), any(Executor.class)))
.thenReturn(mockUrlRequestBuilder);
......@@ -283,7 +279,13 @@ public final class CronetDataSourceTest {
@Test
public void testRequestOpenValidatesContentTypePredicate() {
mockResponseStartSuccess();
when(mockContentTypePredicate.evaluate(anyString())).thenReturn(false);
ArrayList<String> testedContentTypes = new ArrayList<>();
dataSourceUnderTest.setContentTypePredicate(
(String input) -> {
testedContentTypes.add(input);
return false;
});
try {
dataSourceUnderTest.open(testDataSpec);
......@@ -292,7 +294,8 @@ public final class CronetDataSourceTest {
assertThat(e instanceof HttpDataSource.InvalidContentTypeException).isTrue();
// Check for connection not automatically closed.
verify(mockUrlRequest, never()).cancel();
verify(mockContentTypePredicate).evaluate(TEST_CONTENT_TYPE);
assertThat(testedContentTypes).hasSize(1);
assertThat(testedContentTypes.get(0)).isEqualTo(TEST_CONTENT_TYPE);
}
}
......@@ -734,7 +737,6 @@ public final class CronetDataSourceTest {
new CronetDataSource(
mockCronetEngine,
mockExecutor,
mockContentTypePredicate,
TEST_CONNECT_TIMEOUT_MS,
TEST_READ_TIMEOUT_MS,
true, // resetTimeoutOnRedirects
......@@ -765,13 +767,12 @@ public final class CronetDataSourceTest {
new CronetDataSource(
mockCronetEngine,
mockExecutor,
mockContentTypePredicate,
TEST_CONNECT_TIMEOUT_MS,
TEST_READ_TIMEOUT_MS,
true, // resetTimeoutOnRedirects
/* resetTimeoutOnRedirects= */ true,
Clock.DEFAULT,
null,
true);
/* defaultRequestProperties= */ null,
/* handleSetCookieRequests= */ true);
dataSourceUnderTest.addTransferListener(mockTransferListener);
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
......@@ -804,13 +805,12 @@ public final class CronetDataSourceTest {
new CronetDataSource(
mockCronetEngine,
mockExecutor,
mockContentTypePredicate,
TEST_CONNECT_TIMEOUT_MS,
TEST_READ_TIMEOUT_MS,
true, // resetTimeoutOnRedirects
/* resetTimeoutOnRedirects= */ true,
Clock.DEFAULT,
null,
true);
/* defaultRequestProperties= */ null,
/* handleSetCookieRequests= */ true);
dataSourceUnderTest.addTransferListener(mockTransferListener);
mockSingleRedirectSuccess();
mockFollowRedirectSuccess();
......
......@@ -57,14 +57,14 @@ public class OkHttpDataSource extends BaseDataSource implements HttpDataSource {
private final Call.Factory callFactory;
private final RequestProperties requestProperties;
private final @Nullable String userAgent;
private final @Nullable Predicate<String> contentTypePredicate;
private final @Nullable CacheControl cacheControl;
private final @Nullable RequestProperties defaultRequestProperties;
private @Nullable DataSpec dataSpec;
private @Nullable Response response;
private @Nullable InputStream responseByteStream;
@Nullable private final String userAgent;
@Nullable private final CacheControl cacheControl;
@Nullable private final RequestProperties defaultRequestProperties;
@Nullable private Predicate<String> contentTypePredicate;
@Nullable private DataSpec dataSpec;
@Nullable private Response response;
@Nullable private InputStream responseByteStream;
private boolean opened;
private long bytesToSkip;
......@@ -79,7 +79,28 @@ public class OkHttpDataSource extends BaseDataSource implements HttpDataSource {
* @param userAgent An optional User-Agent string.
*/
public OkHttpDataSource(Call.Factory callFactory, @Nullable String userAgent) {
this(callFactory, userAgent, /* contentTypePredicate= */ null);
this(callFactory, userAgent, /* cacheControl= */ null, /* defaultRequestProperties= */ null);
}
/**
* @param callFactory A {@link Call.Factory} (typically an {@link okhttp3.OkHttpClient}) for use
* by the source.
* @param userAgent An optional User-Agent string.
* @param cacheControl An optional {@link CacheControl} for setting the Cache-Control header.
* @param defaultRequestProperties Optional default {@link RequestProperties} to be sent to the
* server as HTTP headers on every request.
*/
public OkHttpDataSource(
Call.Factory callFactory,
@Nullable String userAgent,
@Nullable CacheControl cacheControl,
@Nullable RequestProperties defaultRequestProperties) {
super(/* isNetwork= */ true);
this.callFactory = Assertions.checkNotNull(callFactory);
this.userAgent = userAgent;
this.cacheControl = cacheControl;
this.defaultRequestProperties = defaultRequestProperties;
this.requestProperties = new RequestProperties();
}
/**
......@@ -89,7 +110,10 @@ public class OkHttpDataSource extends BaseDataSource implements HttpDataSource {
* @param contentTypePredicate An optional {@link Predicate}. If a content type is rejected by the
* predicate then a {@link InvalidContentTypeException} is thrown from {@link
* #open(DataSpec)}.
* @deprecated Use {@link #OkHttpDataSource(Call.Factory, String)} and {@link
* #setContentTypePredicate(Predicate)}.
*/
@Deprecated
public OkHttpDataSource(
Call.Factory callFactory,
@Nullable String userAgent,
......@@ -110,9 +134,12 @@ public class OkHttpDataSource extends BaseDataSource implements HttpDataSource {
* predicate then a {@link InvalidContentTypeException} is thrown from {@link
* #open(DataSpec)}.
* @param cacheControl An optional {@link CacheControl} for setting the Cache-Control header.
* @param defaultRequestProperties The optional default {@link RequestProperties} to be sent to
* the server as HTTP headers on every request.
* @param defaultRequestProperties Optional default {@link RequestProperties} to be sent to the
* server as HTTP headers on every request.
* @deprecated Use {@link #OkHttpDataSource(Call.Factory, String, CacheControl,
* RequestProperties)} and {@link #setContentTypePredicate(Predicate)}.
*/
@Deprecated
public OkHttpDataSource(
Call.Factory callFactory,
@Nullable String userAgent,
......@@ -128,6 +155,17 @@ public class OkHttpDataSource extends BaseDataSource implements HttpDataSource {
this.requestProperties = new RequestProperties();
}
/**
* Sets a content type {@link Predicate}. If a content type is rejected by the predicate then a
* {@link HttpDataSource.InvalidContentTypeException} is thrown from {@link #open(DataSpec)}.
*
* @param contentTypePredicate The content type {@link Predicate}, or {@code null} to clear a
* predicate that was previously set.
*/
public void setContentTypePredicate(@Nullable Predicate<String> contentTypePredicate) {
this.contentTypePredicate = contentTypePredicate;
}
@Override
public @Nullable Uri getUri() {
return response == null ? null : Uri.parse(response.request().url().toString());
......
......@@ -89,7 +89,6 @@ public final class OkHttpDataSourceFactory extends BaseFactory {
new OkHttpDataSource(
callFactory,
userAgent,
/* contentTypePredicate= */ null,
cacheControl,
defaultRequestProperties);
if (listener != null) {
......
......@@ -74,13 +74,13 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
private final int connectTimeoutMillis;
private final int readTimeoutMillis;
private final String userAgent;
private final @Nullable Predicate<String> contentTypePredicate;
private final @Nullable RequestProperties defaultRequestProperties;
@Nullable private final RequestProperties defaultRequestProperties;
private final RequestProperties requestProperties;
private @Nullable DataSpec dataSpec;
private @Nullable HttpURLConnection connection;
private @Nullable InputStream inputStream;
@Nullable private Predicate<String> contentTypePredicate;
@Nullable private DataSpec dataSpec;
@Nullable private HttpURLConnection connection;
@Nullable private InputStream inputStream;
private boolean opened;
private long bytesToSkip;
......@@ -91,7 +91,50 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
/** @param userAgent The User-Agent string that should be used. */
public DefaultHttpDataSource(String userAgent) {
this(userAgent, /* contentTypePredicate= */ null);
this(userAgent, DEFAULT_CONNECT_TIMEOUT_MILLIS, DEFAULT_READ_TIMEOUT_MILLIS);
}
/**
* @param userAgent The User-Agent string that should be used.
* @param connectTimeoutMillis The connection timeout, in milliseconds. A timeout of zero is
* interpreted as an infinite timeout.
* @param readTimeoutMillis The read timeout, in milliseconds. A timeout of zero is interpreted as
* an infinite timeout.
*/
public DefaultHttpDataSource(String userAgent, int connectTimeoutMillis, int readTimeoutMillis) {
this(
userAgent,
connectTimeoutMillis,
readTimeoutMillis,
/* allowCrossProtocolRedirects= */ false,
/* defaultRequestProperties= */ null);
}
/**
* @param userAgent The User-Agent string that should be used.
* @param connectTimeoutMillis The connection timeout, in milliseconds. A timeout of zero is
* interpreted as an infinite timeout. Pass {@link #DEFAULT_CONNECT_TIMEOUT_MILLIS} to use the
* default value.
* @param readTimeoutMillis The read timeout, in milliseconds. A timeout of zero is interpreted as
* an infinite timeout. Pass {@link #DEFAULT_READ_TIMEOUT_MILLIS} to use the default value.
* @param allowCrossProtocolRedirects Whether cross-protocol redirects (i.e. redirects from HTTP
* to HTTPS and vice versa) are enabled.
* @param defaultRequestProperties The default request properties to be sent to the server as HTTP
* headers or {@code null} if not required.
*/
public DefaultHttpDataSource(
String userAgent,
int connectTimeoutMillis,
int readTimeoutMillis,
boolean allowCrossProtocolRedirects,
@Nullable RequestProperties defaultRequestProperties) {
super(/* isNetwork= */ true);
this.userAgent = Assertions.checkNotEmpty(userAgent);
this.requestProperties = new RequestProperties();
this.connectTimeoutMillis = connectTimeoutMillis;
this.readTimeoutMillis = readTimeoutMillis;
this.allowCrossProtocolRedirects = allowCrossProtocolRedirects;
this.defaultRequestProperties = defaultRequestProperties;
}
/**
......@@ -99,7 +142,10 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
* @param contentTypePredicate An optional {@link Predicate}. If a content type is rejected by the
* predicate then a {@link HttpDataSource.InvalidContentTypeException} is thrown from {@link
* #open(DataSpec)}.
* @deprecated Use {@link #DefaultHttpDataSource(String)} and {@link
* #setContentTypePredicate(Predicate)}.
*/
@Deprecated
public DefaultHttpDataSource(String userAgent, @Nullable Predicate<String> contentTypePredicate) {
this(
userAgent,
......@@ -117,7 +163,10 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
* interpreted as an infinite timeout.
* @param readTimeoutMillis The read timeout, in milliseconds. A timeout of zero is interpreted as
* an infinite timeout.
* @deprecated Use {@link #DefaultHttpDataSource(String, int, int)} and {@link
* #setContentTypePredicate(Predicate)}.
*/
@Deprecated
public DefaultHttpDataSource(
String userAgent,
@Nullable Predicate<String> contentTypePredicate,
......@@ -146,7 +195,10 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
* to HTTPS and vice versa) are enabled.
* @param defaultRequestProperties The default request properties to be sent to the server as HTTP
* headers or {@code null} if not required.
* @deprecated Use {@link #DefaultHttpDataSource(String, int, int, boolean, RequestProperties)}
* and {@link #setContentTypePredicate(Predicate)}.
*/
@Deprecated
public DefaultHttpDataSource(
String userAgent,
@Nullable Predicate<String> contentTypePredicate,
......@@ -164,6 +216,17 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
this.defaultRequestProperties = defaultRequestProperties;
}
/**
* Sets a content type {@link Predicate}. If a content type is rejected by the predicate then a
* {@link HttpDataSource.InvalidContentTypeException} is thrown from {@link #open(DataSpec)}.
*
* @param contentTypePredicate The content type {@link Predicate}, or {@code null} to clear a
* predicate that was previously set.
*/
public void setContentTypePredicate(@Nullable Predicate<String> contentTypePredicate) {
this.contentTypePredicate = contentTypePredicate;
}
@Override
public @Nullable Uri getUri() {
return connection == null ? null : Uri.parse(connection.getURL().toString());
......
......@@ -107,7 +107,6 @@ public final class DefaultHttpDataSourceFactory extends BaseFactory {
DefaultHttpDataSource dataSource =
new DefaultHttpDataSource(
userAgent,
/* contentTypePredicate= */ null,
connectTimeoutMillis,
readTimeoutMillis,
allowCrossProtocolRedirects,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment