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 283 matching lines @@
       playback_rate_, bitrate_, &backward_capacity, &forward_capacity);
 
   // 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);
 }
 
 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();
 
     // Defer if we've reached max capacity.
     case kCapacityDefer:
       return buffer_.forward_bytes() >= buffer_.forward_capacity();
   }
-  // Otherwise don't enable defer.
   return false;

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

NOTREACHED?

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

Done.

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