code review comments

library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java

@@ -93,7 +93,7 @@ public final class DownloaderConstructorHelper {
     } else {
     } else {
       DataSink cacheWriteDataSink = cacheWriteDataSinkFactory != null
       DataSink cacheWriteDataSink = cacheWriteDataSinkFactory != null
           ? cacheWriteDataSinkFactory.createDataSink()
           ? cacheWriteDataSinkFactory.createDataSink()
-          : new CacheDataSink(cache, CacheDataSource.DEFAULT_MAX_CACHE_FILE_SIZE, false);
+          : new CacheDataSink(cache, CacheDataSource.DEFAULT_MAX_CACHE_FILE_SIZE);
       DataSource upstream = upstreamDataSourceFactory.createDataSource();
       DataSource upstream = upstreamDataSourceFactory.createDataSource();
       upstream = priorityTaskManager == null ? upstream
       upstream = priorityTaskManager == null ? upstream
           : new PriorityDataSource(upstream, priorityTaskManager, C.PRIORITY_DOWNLOAD);
           : new PriorityDataSource(upstream, priorityTaskManager, C.PRIORITY_DOWNLOAD);

library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java

@@ -43,7 +43,7 @@ public final class CacheDataSink implements DataSink {
   private final Cache cache;
   private final Cache cache;
   private final long maxCacheFileSize;
   private final long maxCacheFileSize;
   private final int bufferSize;
   private final int bufferSize;
-  private final boolean skipFDSync;
+  private final boolean syncFileDescriptor;
 
 
   private DataSpec dataSpec;
   private DataSpec dataSpec;
   private File file;
   private File file;
@@ -71,10 +71,22 @@ public final class CacheDataSink implements DataSink {
    * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the sink is opened for
    * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the sink is opened for
    *    a {@link DataSpec} whose size exceeds this value, then the data will be fragmented into
    *    a {@link DataSpec} whose size exceeds this value, then the data will be fragmented into
    *    multiple cache files.
    *    multiple cache files.
-   * @param skipFDSync Skip file descriptor sync when closing current output stream.
    */
    */
-  public CacheDataSink(Cache cache, long maxCacheFileSize, boolean skipFDSync) {
-    this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE, skipFDSync);
+  public CacheDataSink(Cache cache, long maxCacheFileSize) {
+    this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE, true);
+  }
+
+  /**
+   * Constructs a CacheDataSink using the {@link #DEFAULT_BUFFER_SIZE}.
+   *
+   * @param cache The cache into which data should be written.
+   * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the sink is opened for
+   *    a {@link DataSpec} whose size exceeds this value, then the data will be fragmented into
+   *    multiple cache files.
+   * @param syncFileDescriptor Skip file descriptor sync when closing current output stream.
+   */
+  public CacheDataSink(Cache cache, long maxCacheFileSize, boolean syncFileDescriptor) {
+    this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE, syncFileDescriptor);
   }
   }
 
 
   /**
   /**
@@ -84,13 +96,13 @@ public final class CacheDataSink implements DataSink {
    *    multiple cache files.
    *    multiple cache files.
    * @param bufferSize The buffer size in bytes for writing to a cache file. A zero or negative
    * @param bufferSize The buffer size in bytes for writing to a cache file. A zero or negative
    *    value disables buffering.
    *    value disables buffering.
-   * @param skipFDSync Skip file descriptor sync when closing current output stream.
+   * @param syncFileDescriptor Sync file descriptor when closing current output stream.
    */
    */
-  public CacheDataSink(Cache cache, long maxCacheFileSize, int bufferSize, boolean skipFDSync) {
+  public CacheDataSink(Cache cache, long maxCacheFileSize, int bufferSize, boolean syncFileDescriptor) {
     this.cache = Assertions.checkNotNull(cache);
     this.cache = Assertions.checkNotNull(cache);
     this.maxCacheFileSize = maxCacheFileSize;
     this.maxCacheFileSize = maxCacheFileSize;
     this.bufferSize = bufferSize;
     this.bufferSize = bufferSize;
-    this.skipFDSync = skipFDSync;
+    this.syncFileDescriptor = syncFileDescriptor;
   }
   }
 
 
   @Override
   @Override
@@ -174,7 +186,7 @@ public final class CacheDataSink implements DataSink {
     boolean success = false;
     boolean success = false;
     try {
     try {
       outputStream.flush();
       outputStream.flush();
-      if (!skipFDSync) {
+      if (syncFileDescriptor) {
         underlyingFileOutputStream.getFD().sync();
         underlyingFileOutputStream.getFD().sync();
       }
       }
       success = true;
       success = true;

library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java

@@ -44,7 +44,7 @@ public final class CacheDataSinkFactory implements DataSink.Factory {
 
 
   @Override
   @Override
   public DataSink createDataSink() {
   public DataSink createDataSink() {
-    return new CacheDataSink(cache, maxCacheFileSize, bufferSize, false);
+    return new CacheDataSink(cache, maxCacheFileSize, bufferSize, true);
   }
   }
 
 
 }
 }

library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java

@@ -157,7 +157,7 @@ public final class CacheDataSource implements DataSource {
    */
    */
   public CacheDataSource(Cache cache, DataSource upstream, @Flags int flags,
   public CacheDataSource(Cache cache, DataSource upstream, @Flags int flags,
       long maxCacheFileSize) {
       long maxCacheFileSize) {
-    this(cache, upstream, new FileDataSource(), new CacheDataSink(cache, maxCacheFileSize, false),
+    this(cache, upstream, new FileDataSource(), new CacheDataSink(cache, maxCacheFileSize),
         flags, null);
         flags, null);
   }
   }
 
 

library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java

@@ -79,22 +79,22 @@ public final class CacheDataSourceTest {
 
 
   @Test
   @Test
   public void testCacheAndRead() throws Exception {
   public void testCacheAndRead() throws Exception {
-    assertCacheAndRead(false, false, false);
+    assertCacheAndRead(false, false, true);
   }
   }
 
 
   @Test
   @Test
   public void testCacheAndReadUnboundedRequest() throws Exception {
   public void testCacheAndReadUnboundedRequest() throws Exception {
-    assertCacheAndRead(true, false, false);
+    assertCacheAndRead(true, false, true);
   }
   }
 
 
   @Test
   @Test
   public void testCacheAndReadUnknownLength() throws Exception {
   public void testCacheAndReadUnknownLength() throws Exception {
-    assertCacheAndRead(false, true, false);
+    assertCacheAndRead(false, true, true);
   }
   }
 
 
   @Test
   @Test
   public void testCacheAndReadUnboundedRequestUnknownLength() throws Exception {
   public void testCacheAndReadUnboundedRequestUnknownLength() throws Exception {
-    assertCacheAndRead(true, true, false);
+    assertCacheAndRead(true, true, true);
   }
   }
 
 
   @Test
   @Test
@@ -121,7 +121,7 @@ public final class CacheDataSourceTest {
   public void testUnsatisfiableRange() throws Exception {
   public void testUnsatisfiableRange() throws Exception {
     // Bounded request but the content length is unknown. This forces all data to be cached but not
     // Bounded request but the content length is unknown. This forces all data to be cached but not
     // the length
     // the length
-    assertCacheAndRead(false, true, false);
+    assertCacheAndRead(false, true, true);
 
 
     // Now do an unbounded request. This will read all of the data from cache and then try to read
     // Now do an unbounded request. This will read all of the data from cache and then try to read
     // more from upstream which will cause to a 416 so CDS will store the length.
     // more from upstream which will cause to a 416 so CDS will store the length.
@@ -368,10 +368,10 @@ public final class CacheDataSourceTest {
   }
   }
 
 
   private void assertCacheAndRead(boolean unboundedRequest, boolean simulateUnknownLength,
   private void assertCacheAndRead(boolean unboundedRequest, boolean simulateUnknownLength,
-      boolean skipFDSync)
+      boolean syncFD)
       throws IOException {
       throws IOException {
     // Read all data from upstream and write to cache
     // Read all data from upstream and write to cache
-    CacheDataSource cacheDataSource = createCacheDataSource(false, simulateUnknownLength, skipFDSync);
+    CacheDataSource cacheDataSource = createCacheDataSource(false, simulateUnknownLength, syncFD);
     assertReadDataContentLength(cacheDataSource, unboundedRequest, simulateUnknownLength);
     assertReadDataContentLength(cacheDataSource, unboundedRequest, simulateUnknownLength);
 
 
     // Just read from cache
     // Just read from cache
@@ -412,19 +412,19 @@ public final class CacheDataSourceTest {
 
 
   private CacheDataSource createCacheDataSource(boolean setReadException,
   private CacheDataSource createCacheDataSource(boolean setReadException,
       boolean simulateUnknownLength) {
       boolean simulateUnknownLength) {
-    return createCacheDataSource(setReadException, simulateUnknownLength, false);
+    return createCacheDataSource(setReadException, simulateUnknownLength, true);
   }
   }
 
 
   private CacheDataSource createCacheDataSource(boolean setReadException,
   private CacheDataSource createCacheDataSource(boolean setReadException,
-      boolean simulateUnknownLength, boolean skipFDSync) {
+      boolean simulateUnknownLength, boolean syncFD) {
     return createCacheDataSource(setReadException, simulateUnknownLength,
     return createCacheDataSource(setReadException, simulateUnknownLength,
-        CacheDataSource.FLAG_BLOCK_ON_CACHE, skipFDSync);
+        CacheDataSource.FLAG_BLOCK_ON_CACHE, syncFD);
   }
   }
 
 
   private CacheDataSource createCacheDataSource(boolean setReadException,
   private CacheDataSource createCacheDataSource(boolean setReadException,
-      boolean simulateUnknownLength, @CacheDataSource.Flags int flags, boolean skipFDSync) {
+      boolean simulateUnknownLength, @CacheDataSource.Flags int flags, boolean syncFD) {
     return createCacheDataSource(setReadException, simulateUnknownLength, flags,
     return createCacheDataSource(setReadException, simulateUnknownLength, flags,
-        new CacheDataSink(cache, MAX_CACHE_FILE_SIZE, skipFDSync));
+        new CacheDataSink(cache, MAX_CACHE_FILE_SIZE, syncFD));
   }
   }
 
 
   private CacheDataSource createCacheDataSource(boolean setReadException,
   private CacheDataSource createCacheDataSource(boolean setReadException,

library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java

@@ -166,7 +166,7 @@ public final class CacheDataSourceTest2 {
         ? new AesCipherDataSource(Util.getUtf8Bytes(secretKey), file) : file;
         ? new AesCipherDataSource(Util.getUtf8Bytes(secretKey), file) : file;
 
 
     // Sink and cipher
     // Sink and cipher
-    CacheDataSink cacheSink = new CacheDataSink(cache, EXO_CACHE_MAX_FILESIZE, false);
+    CacheDataSink cacheSink = new CacheDataSink(cache, EXO_CACHE_MAX_FILESIZE);
     byte[] scratch = new byte[3897];
     byte[] scratch = new byte[3897];
     DataSink cacheWriteDataSink = useAesEncryption
     DataSink cacheWriteDataSink = useAesEncryption
         ? new AesCipherDataSink(Util.getUtf8Bytes(secretKey), cacheSink, scratch) : cacheSink;
         ? new AesCipherDataSink(Util.getUtf8Bytes(secretKey), cacheSink, scratch) : cacheSink;