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

webkit/media/buffered_resource_loader.cc

Index: webkit/media/buffered_resource_loader.cc
diff --git a/webkit/media/buffered_resource_loader.cc b/webkit/media/buffered_resource_loader.cc
index 059904b42cd56170014c3e89b1650389a1457c7d..5d084113fd6e68871af89e7edcd38ae9b30a2b23 100644
--- a/webkit/media/buffered_resource_loader.cc
+++ b/webkit/media/buffered_resource_loader.cc
@@ -299,11 +299,9 @@ void BufferedResourceLoader::Read(
     }
 
     // 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;
   }
@@ -610,61 +608,31 @@ 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.
-  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.
+  NOTREACHED();
   return false;
 }