Don't reset BufferedResourceLoader::buffer_ inside Stop().

This addresses a TODO where we were using buffer_ as a signal to detect we're stopped. In short, we don't need to detect we're stopped since our code already checks for active_loader_.

BUG=none
TEST=none

Review URL: https://chromiumcodereview.appspot.com/10381067

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@136108 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 56 insertions, 81 deletions

diff --git a/webkit/media/buffered_resource_loader.cc b/webkit/media/buffered_resource_loader.cc
index 1a78aac..da2cf90 100644
--- a/webkit/media/buffered_resource_loader.cc
+++ b/webkit/media/buffered_resource_loader.cc
@@ -10,7 +10,6 @@
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "media/base/media_log.h"
-#include "media/base/seekable_buffer.h"
 #include "net/http/http_request_headers.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebKit.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLLoaderOptions.h"
@@ -109,7 +108,8 @@ BufferedResourceLoader::BufferedResourceLoader(
     int bitrate,
     float playback_rate,
     media::MediaLog* media_log)
-    : loader_failed_(false),
+    : buffer_(kMinBufferCapacity, kMinBufferCapacity),
+      loader_failed_(false),
       defer_strategy_(strategy),
       range_supported_(false),
       saved_forward_capacity_(0),
@@ -129,11 +129,9 @@ BufferedResourceLoader::BufferedResourceLoader(
       playback_rate_(playback_rate),
       media_log_(media_log) {
 
-  int backward_capacity;
-  int forward_capacity;
-  ComputeTargetBufferWindow(
-      playback_rate_, bitrate_, &backward_capacity, &forward_capacity);
-  buffer_.reset(new media::SeekableBuffer(backward_capacity, forward_capacity));
+  // Set the initial capacity of |buffer_| based on |bitrate_| and
+  // |playback_rate_|.
+  UpdateBufferWindow();
 }
 
 BufferedResourceLoader::~BufferedResourceLoader() {}
@@ -199,14 +197,6 @@ void BufferedResourceLoader::Stop() {
   event_cb_.Reset();
   read_cb_.Reset();
 
-  // Use the internal buffer to signal that we have been stopped.
-  // TODO(hclam): Not so pretty to do this.
-  if (!buffer_.get())
-    return;
-
-  // Destroy internal buffer.
-  buffer_.reset();
-
   // Cancel and reset any active loaders.
   active_loader_.reset();
 }
@@ -219,7 +209,6 @@ void BufferedResourceLoader::Read(
   DCHECK(start_cb_.is_null());
   DCHECK(read_cb_.is_null());
   DCHECK(!read_cb.is_null());
-  DCHECK(buffer_.get());
   DCHECK(buffer);
   DCHECK_GT(read_size, 0);
 
@@ -278,8 +267,8 @@ void BufferedResourceLoader::Read(
   // necessary and disable deferring.
   if (WillFulfillRead()) {
     // Advance offset as much as possible to create additional capacity.
-    int advance = std::min(first_offset_, buffer_->forward_bytes());
-    bool ret = buffer_->Seek(advance);
+    int advance = std::min(first_offset_, buffer_.forward_bytes());
+    bool ret = buffer_.Seek(advance);
     DCHECK(ret);
 
     offset_ += advance;
@@ -291,9 +280,9 @@ void BufferedResourceLoader::Read(
     //
     // This can happen when reading in a large seek index or when the
     // first byte of a read request falls within kForwardWaitThreshold.
-    if (last_offset_ > buffer_->forward_capacity()) {
-      saved_forward_capacity_ = buffer_->forward_capacity();
-      buffer_->set_forward_capacity(last_offset_);
+    if (last_offset_ > buffer_.forward_capacity()) {
+      saved_forward_capacity_ = buffer_.forward_capacity();
+      buffer_.set_forward_capacity(last_offset_);
     }
 
     // Make sure we stop deferring now that there's additional capacity.
@@ -311,9 +300,7 @@ void BufferedResourceLoader::Read(
 }
 
 int64 BufferedResourceLoader::GetBufferedPosition() {
-  if (buffer_.get())
-    return offset_ + buffer_->forward_bytes() - 1;
-  return kPositionNotSpecified;
+  return offset_ + buffer_.forward_bytes() - 1;
 }
 
 int64 BufferedResourceLoader::content_length() {
@@ -441,13 +428,7 @@ void BufferedResourceLoader::didReceiveData(
   DCHECK(active_loader_.get());
   DCHECK_GT(data_length, 0);
 
-  // If this loader has been stopped, |buffer_| would be destroyed.
-  // In this case we shouldn't do anything.
-  if (!buffer_.get())
-    return;
-
-  // Writes more data to |buffer_|.
-  buffer_->Append(reinterpret_cast<const uint8*>(data), data_length);
+  buffer_.Append(reinterpret_cast<const uint8*>(data), data_length);
 
   // If there is an active read request, try to fulfill the request.
   if (HasPendingRead() && CanFulfillRead())
@@ -457,9 +438,9 @@ void BufferedResourceLoader::didReceiveData(
   UpdateDeferBehavior();
 
   // Consume excess bytes from our in-memory buffer if necessary.
-  if (buffer_->forward_bytes() > buffer_->forward_capacity()) {
-    int excess = buffer_->forward_bytes() - buffer_->forward_capacity();
-    bool success = buffer_->Seek(excess);
+  if (buffer_.forward_bytes() > buffer_.forward_capacity()) {
+    int excess = buffer_.forward_bytes() - buffer_.forward_capacity();
+    bool success = buffer_.Seek(excess);
     DCHECK(success);
     offset_ += first_offset_ + excess;
   }
@@ -494,7 +475,7 @@ void BufferedResourceLoader::didFinishLoading(
 
   // If we didn't know the |instance_size_| we do now.
   if (instance_size_ == kPositionNotSpecified) {
-    instance_size_ = offset_ + buffer_->forward_bytes();
+    instance_size_ = offset_ + buffer_.forward_bytes();
   }
 
   // If there is a start callback, run it.
@@ -508,9 +489,6 @@ void BufferedResourceLoader::didFinishLoading(
   // If there is a pending read but the request has ended, return with what
   // we have.
   if (HasPendingRead()) {
-    // Make sure we have a valid buffer before we satisfy a read request.
-    DCHECK(buffer_.get());
-
     // Try to fulfill with what is in the buffer.
     if (CanFulfillRead())
       ReadInternal();
@@ -582,9 +560,6 @@ void BufferedResourceLoader::SetBitrate(int bitrate) {
 // Helper methods.
 
 void BufferedResourceLoader::UpdateBufferWindow() {
-  if (!buffer_.get())
-    return;
-
   int backward_capacity;
   int forward_capacity;
   ComputeTargetBufferWindow(
@@ -593,12 +568,12 @@ void BufferedResourceLoader::UpdateBufferWindow() {
   // This does not evict data from the buffer if the new capacities are less
   // than the current capacities; the new limits will be enforced after the
   // existing excess buffered data is consumed.
-  buffer_->set_backward_capacity(backward_capacity);
-  buffer_->set_forward_capacity(forward_capacity);
+  buffer_.set_backward_capacity(backward_capacity);
+  buffer_.set_forward_capacity(forward_capacity);
 }
 
 void BufferedResourceLoader::UpdateDeferBehavior() {
-  if (!active_loader_.get() || !buffer_.get())
+  if (!active_loader_.get())
     return;
 
   // If necessary, toggle defer state and continue/pause downloading data
@@ -628,7 +603,7 @@ bool BufferedResourceLoader::ShouldEnableDefer() const {
 
     // Defer if we've reached the max capacity of the threshold.
     case kThresholdDefer:
-      return buffer_->forward_bytes() >= buffer_->forward_capacity();
+      return buffer_.forward_bytes() >= buffer_.forward_capacity();
   }
   // Otherwise don't enable defer.
   return false;
@@ -647,7 +622,7 @@ bool BufferedResourceLoader::ShouldDisableDefer() const {
     // We have an outstanding read request, and we have not buffered enough
     // yet to fulfill the request; disable defer to get more data.
     case kReadThenDefer:
-      return !read_cb_.is_null() && last_offset_ > buffer_->forward_bytes();
+      return !read_cb_.is_null() && last_offset_ > buffer_.forward_bytes();
 
     // Disable deferring whenever our forward-buffered amount falls beneath our
     // threshold.
@@ -655,8 +630,8 @@ bool BufferedResourceLoader::ShouldDisableDefer() const {
     // TODO(scherkus): refer to http://crbug.com/124719 for more discussion on
     // how we could improve our buffering logic.
     case kThresholdDefer: {
-      int buffered = buffer_->forward_bytes();
-      int threshold = buffer_->forward_capacity() * kDisableDeferThreshold;
+      int buffered = buffer_.forward_bytes();
+      int threshold = buffer_.forward_capacity() * kDisableDeferThreshold;
       return buffered < threshold;
     }
   }
@@ -667,11 +642,11 @@ bool BufferedResourceLoader::ShouldDisableDefer() const {
 
 bool BufferedResourceLoader::CanFulfillRead() const {
   // If we are reading too far in the backward direction.
-  if (first_offset_ < 0 && (first_offset_ + buffer_->backward_bytes()) < 0)
+  if (first_offset_ < 0 && (first_offset_ + buffer_.backward_bytes()) < 0)
     return false;
 
   // If the start offset is too far ahead.
-  if (first_offset_ >= buffer_->forward_bytes())
+  if (first_offset_ >= buffer_.forward_bytes())
     return false;
 
   // At the point, we verified that first byte requested is within the buffer.
@@ -681,7 +656,7 @@ bool BufferedResourceLoader::CanFulfillRead() const {
 
   // If the resource request is still active, make sure the whole requested
   // range is covered.
-  if (last_offset_ > buffer_->forward_bytes())
+  if (last_offset_ > buffer_.forward_bytes())
     return false;
 
   return true;
@@ -689,11 +664,11 @@ bool BufferedResourceLoader::CanFulfillRead() const {
 
 bool BufferedResourceLoader::WillFulfillRead() const {
   // Trying to read too far behind.
-  if (first_offset_ < 0 && (first_offset_ + buffer_->backward_bytes()) < 0)
+  if (first_offset_ < 0 && (first_offset_ + buffer_.backward_bytes()) < 0)
     return false;
 
   // Trying to read too far ahead.
-  if ((first_offset_ - buffer_->forward_bytes()) >= kForwardWaitThreshold)
+  if ((first_offset_ - buffer_.forward_bytes()) >= kForwardWaitThreshold)
     return false;
 
   // The resource request has completed, there's no way we can fulfill the
@@ -706,11 +681,11 @@ bool BufferedResourceLoader::WillFulfillRead() const {
 
 void BufferedResourceLoader::ReadInternal() {
   // Seek to the first byte requested.
-  bool ret = buffer_->Seek(first_offset_);
+  bool ret = buffer_.Seek(first_offset_);
   DCHECK(ret);
 
   // Then do the read.
-  int read = buffer_->Read(read_buffer_, read_size_);
+  int read = buffer_.Read(read_buffer_, read_size_);
   offset_ += first_offset_ + read;
 
   // And report with what we have read.
@@ -803,8 +778,8 @@ std::string BufferedResourceLoader::GenerateHeaders(
 }
 
 void BufferedResourceLoader::DoneRead(Status status, int bytes_read) {
-  if (buffer_.get() && saved_forward_capacity_) {
-    buffer_->set_forward_capacity(saved_forward_capacity_);
+  if (saved_forward_capacity_) {
+    buffer_.set_forward_capacity(saved_forward_capacity_);
     saved_forward_capacity_ = 0;
   }
   read_position_ = 0;
@@ -832,13 +807,11 @@ bool BufferedResourceLoader::IsRangeRequest() const {
 }
 
 void BufferedResourceLoader::Log() {
-  if (buffer_.get()) {
-    media_log_->AddEvent(
-        media_log_->CreateBufferedExtentsChangedEvent(
-            offset_ - buffer_->backward_bytes(),
-            offset_,
-            offset_ + buffer_->forward_bytes()));
-  }
+  media_log_->AddEvent(
+      media_log_->CreateBufferedExtentsChangedEvent(
+          offset_ - buffer_.backward_bytes(),
+          offset_,
+          offset_ + buffer_.forward_bytes()));
 }
 
 }  // namespace webkit_media
diff --git a/webkit/media/buffered_resource_loader.h b/webkit/media/buffered_resource_loader.h
index 9186514..bcf0979 100644
--- a/webkit/media/buffered_resource_loader.h
+++ b/webkit/media/buffered_resource_loader.h
@@ -11,6 +11,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/timer.h"
 #include "googleurl/src/gurl.h"
+#include "media/base/seekable_buffer.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLLoader.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLLoaderClient.h"
@@ -254,7 +255,7 @@ class BufferedResourceLoader : public WebKit::WebURLLoaderClient {
   void Log();
 
   // A sliding window of buffer.
-  scoped_ptr<media::SeekableBuffer> buffer_;
+  media::SeekableBuffer buffer_;
 
   // Keeps track of an active WebURLLoader and associated state.
   scoped_ptr<ActiveLoader> active_loader_;
diff --git a/webkit/media/buffered_resource_loader_unittest.cc b/webkit/media/buffered_resource_loader_unittest.cc
index 7769ca5..35fc24d 100644
--- a/webkit/media/buffered_resource_loader_unittest.cc
+++ b/webkit/media/buffered_resource_loader_unittest.cc
@@ -94,8 +94,9 @@ class BufferedResourceLoaderTest : public testing::Test {
   }
 
   void SetLoaderBuffer(int forward_capacity, int backward_capacity) {
-    loader_->buffer_.reset(
-        new media::SeekableBuffer(backward_capacity, forward_capacity));
+    loader_->buffer_.set_forward_capacity(forward_capacity);
+    loader_->buffer_.set_backward_capacity(backward_capacity);
+    loader_->buffer_.Clear();
   }
 
   void Start() {
@@ -218,8 +219,8 @@ class BufferedResourceLoaderTest : public testing::Test {
   }
 
   void WriteUntilThreshold() {
-    int buffered = loader_->buffer_->forward_bytes();
-    int capacity = loader_->buffer_->forward_capacity();
+    int buffered = loader_->buffer_.forward_bytes();
+    int capacity = loader_->buffer_.forward_capacity();
     CHECK_LT(buffered, capacity);
 
     EXPECT_CALL(*this, NetworkCallback());
@@ -251,19 +252,19 @@ class BufferedResourceLoaderTest : public testing::Test {
                           int backward_capacity,
                           int forward_bytes,
                           int forward_capacity) {
-    EXPECT_EQ(backward_bytes, loader_->buffer_->backward_bytes());
-    EXPECT_EQ(backward_capacity, loader_->buffer_->backward_capacity());
-    EXPECT_EQ(forward_bytes, loader_->buffer_->forward_bytes());
-    EXPECT_EQ(forward_capacity, loader_->buffer_->forward_capacity());
+    EXPECT_EQ(backward_bytes, loader_->buffer_.backward_bytes());
+    EXPECT_EQ(backward_capacity, loader_->buffer_.backward_capacity());
+    EXPECT_EQ(forward_bytes, loader_->buffer_.forward_bytes());
+    EXPECT_EQ(forward_capacity, loader_->buffer_.forward_capacity());
   }
 
   void ConfirmLoaderBufferBackwardCapacity(int expected_backward_capacity) {
-    EXPECT_EQ(loader_->buffer_->backward_capacity(),
+    EXPECT_EQ(loader_->buffer_.backward_capacity(),
               expected_backward_capacity);
   }
 
   void ConfirmLoaderBufferForwardCapacity(int expected_forward_capacity) {
-    EXPECT_EQ(loader_->buffer_->forward_capacity(), expected_forward_capacity);
+    EXPECT_EQ(loader_->buffer_.forward_capacity(), expected_forward_capacity);
   }
 
   void ConfirmLoaderDeferredState(bool expectedVal) {
@@ -274,13 +275,13 @@ class BufferedResourceLoaderTest : public testing::Test {
   void CheckBufferWindowBounds() {
     // Corresponds to value defined in buffered_resource_loader.cc.
     static const int kMinBufferCapacity = 2 * 1024 * 1024;
-    EXPECT_GE(loader_->buffer_->forward_capacity(), kMinBufferCapacity);
-    EXPECT_GE(loader_->buffer_->backward_capacity(), kMinBufferCapacity);
+    EXPECT_GE(loader_->buffer_.forward_capacity(), kMinBufferCapacity);
+    EXPECT_GE(loader_->buffer_.backward_capacity(), kMinBufferCapacity);
 
     // Corresponds to value defined in buffered_resource_loader.cc.
     static const int kMaxBufferCapacity = 20 * 1024 * 1024;
-    EXPECT_LE(loader_->buffer_->forward_capacity(), kMaxBufferCapacity);
-    EXPECT_LE(loader_->buffer_->backward_capacity(), kMaxBufferCapacity);
+    EXPECT_LE(loader_->buffer_.forward_capacity(), kMaxBufferCapacity);
+    EXPECT_LE(loader_->buffer_.backward_capacity(), kMaxBufferCapacity);
   }
 
   MOCK_METHOD1(StartCallback, void(BufferedResourceLoader::Status));
@@ -288,8 +289,8 @@ class BufferedResourceLoaderTest : public testing::Test {
   MOCK_METHOD0(NetworkCallback, void());
 
   // Accessors for private variables on |loader_|.
-  int forward_bytes() { return loader_->buffer_->forward_bytes(); }
-  int forward_capacity() { return loader_->buffer_->forward_capacity(); }
+  int forward_bytes() { return loader_->buffer_.forward_bytes(); }
+  int forward_capacity() { return loader_->buffer_.forward_capacity(); }
 
  protected:
   GURL gurl_;