Collapse Should{Enable,Disable}Defer() into ShouldDefer() + simplify some other code.

webkit/media/buffered_resource_loader.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "webkit/media/buffered_resource_loader.h"
 
 #include "base/bits.h"
 #include "base/callback_helpers.h"
 #include "base/format_macros.h"
 #include "base/metrics/histogram.h"
@@ skipping 281 matching lines @@
     // capacity.
     //
     // 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_);
     }
 
     // Make sure we stop deferring now that there's additional capacity.
-    if (active_loader_->deferred())
-      SetDeferred(false);
-
-    DCHECK(!ShouldEnableDefer())
+    DCHECK(!ShouldDefer())
         << "Capacity was not adjusted properly to prevent deferring.";
+    UpdateDeferBehavior();
 
     return;
   }
 
   // Make a callback to report failure.
   DoneRead(kCacheMiss, 0);
 }
 
 int64 BufferedResourceLoader::content_length() {
   return content_length_;
@@ skipping 286 matching lines @@
   // 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);
 }
 
 void BufferedResourceLoader::UpdateDeferBehavior() {
   if (!active_loader_.get())
     return;
 
-  // If necessary, toggle defer state and continue/pause downloading data
-  // accordingly.
-  if (ShouldEnableDefer() || ShouldDisableDefer())
-    SetDeferred(!active_loader_->deferred());
+  SetDeferred(ShouldDefer());
 }
 
 void BufferedResourceLoader::SetDeferred(bool deferred) {
+  if (active_loader_->deferred() == deferred)
+    return;
+
   active_loader_->SetDeferred(deferred);
   loading_cb_.Run(deferred ? kLoadingDeferred : kLoading);
 }
 
-bool BufferedResourceLoader::ShouldEnableDefer() const {
-  // If we're already deferring, then enabling makes no sense.
-  if (active_loader_->deferred())
-    return false;
-
+bool BufferedResourceLoader::ShouldDefer() const {
   switch(defer_strategy_) {
-    // Never defer at all, so never enable defer.
     case kNeverDefer:
       return false;
 
-    // Defer if nothing is being requested.
     case kReadThenDefer:
+      DCHECK(read_cb_.is_null() || last_offset_ > buffer_.forward_bytes())

[Ami GONE FROM CHROMIUM, 2012/07/12 00:24:37]

Did you truth-tabling overtake your test-writing??

[scherkus (not reviewing), 2012/07/12 21:02:17]

As discussed offline...

This is covered by BufferedResourceLoaderTest.ReadThenDeferStrategy. If we
somehow forget to fulfill a read when we're capable of doing so inside
didReceiveData() this DCHECK() fires (you can test  by commenting out that block
/ appending "&& false" so the condition never fires, etc...)

+          << "We shouldn't stop deferring if we can fulfill the read";
       return read_cb_.is_null();
 
-    // Defer if we've reached max capacity.
     case kCapacityDefer:
       return buffer_.forward_bytes() >= buffer_.forward_capacity();
   }
-  // Otherwise don't enable defer.
+  NOTREACHED();
   return false;
 }
 
-bool BufferedResourceLoader::ShouldDisableDefer() const {
-  // If we're not deferring, then disabling makes no sense.
-  if (!active_loader_->deferred())
-    return false;
-
-  switch(defer_strategy_) {
-    // Always disable deferring.
-    case kNeverDefer:
-      return true;
-
-    // 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();
-
-    // Disable deferring whenever our forward-buffered amount falls beneath our
-    // capacity.
-    case kCapacityDefer:
-      return buffer_.forward_bytes() < buffer_.forward_capacity();
-  }
-
-  // Otherwise keep deferring.
-  return false;
-}
-
 bool BufferedResourceLoader::CanFulfillRead() const {
   // If we are reading too far in the backward direction.
   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())
     return false;
 
   // At the point, we verified that first byte requested is within the buffer.
@@ skipping 154 matching lines @@
 
 void BufferedResourceLoader::Log() {
   media_log_->AddEvent(
       media_log_->CreateBufferedExtentsChangedEvent(
           offset_ - buffer_.backward_bytes(),
           offset_,
           offset_ + buffer_.forward_bytes()));
 }
 
 }  // namespace webkit_media