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..80914a84ccea241cb76ae949ef45e83e8a6c4e87 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,28 +608,24 @@ void BufferedResourceLoader::UpdateDeferBehavior() {
   if (!active_loader_.get())

[scherkus (not reviewing), 2012/07/11 18:11:51]

I don't know how useful this method is or whether this should be a DCHECK or
similar (why are we attempting to update the defer behaviour when we don't have
a loader?!)

     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.
+    // Never defer at all.

[Ami GONE FROM CHROMIUM, 2012/07/11 18:36:09]

This (and the comments for the other two cases) doesn't seem to be adding value.

[scherkus (not reviewing), 2012/07/12 00:21:03]

what is this -- webkit!? :P

     case kNeverDefer:
       return false;
 
-    // Defer if nothing is being requested.
+    // Defer if there are no pending reads.
     case kReadThenDefer:
       return read_cb_.is_null();
 
@@ -639,32 +633,6 @@ bool BufferedResourceLoader::ShouldEnableDefer() const {
     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();

[scherkus (not reviewing), 2012/07/11 18:11:51]

Need some scrutiny here!!!

The last half of this condition isn't really covered any more but I'm trying to
figure out when it would be needed

inside Read() if we can immediately satisfy a read we do so, meaning by time we
UpdateDeferBehavior() read_cb_ is null.

if we hit the WillFulfillRead() codepath inside Read() then we adjust
buffers+capacity+DCHECK() that we'll un-defer

[Ami GONE FROM CHROMIUM, 2012/07/11 18:36:09]

ISTM the thing to do is simply not call UpdateDeferBehavior in cases where we
know it can't have changed.

-
-    // 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;

[Ami GONE FROM CHROMIUM, 2012/07/11 18:36:09]

NOTREACHED?

[scherkus (not reviewing), 2012/07/12 00:21:03]

Done.

 }