Add host-side rate-limiting to desktop resize events.

remoting/host/resizing_host_observer.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 "remoting/host/resizing_host_observer.h"
 
 #include <list>
 
+#include "base/bind.h"
 #include "base/logging.h"
+#include "base/message_loop.h"
 #include "remoting/host/desktop_resizer.h"
 #include "remoting/host/screen_resolution.h"
 
 namespace {
 
+const int kMinimumResizeIntervalMs = 1000;

[Jamie, 2013/06/03 23:26:28]

This value is taken directly from the previous implementation. The bug report
suggests making this configurable per-platform. I don't think that's worth doing
unless we have some data on what suitable resize times might be on each
platform.

[alexeypa (please no reviews), 2013/06/04 00:05:26]

nit #1: Add a comment explaining what this timeout is for.

[Jamie, 2013/06/04 01:27:49]

Done.

+
 class CandidateSize {
  public:
   CandidateSize(const SkISize& candidate, const SkISize& preferred)
       : size_(candidate) {
     // Protect against division by zero.
     CHECK(!candidate.isEmpty());
     DCHECK(!preferred.isEmpty());
 
     // The client scale factor is the smaller of the candidate:preferred ratios
     // for width and height.
@@ skipping 69 matching lines @@
   SkISize size_;
 };
 
 }  // namespace
 
 namespace remoting {
 
 ResizingHostObserver::ResizingHostObserver(
     scoped_ptr<DesktopResizer> desktop_resizer)
     : desktop_resizer_(desktop_resizer.Pass()),
-      original_size_(desktop_resizer_->GetCurrentSize()) {
+      original_size_(desktop_resizer_->GetCurrentSize()),
+      resize_pending_(false),
+      weak_factory_(this) {
 }
 
 ResizingHostObserver::~ResizingHostObserver() {
   if (!original_size_.isZero())
     desktop_resizer_->RestoreSize(original_size_);
 }
 
 void ResizingHostObserver::SetScreenResolution(
     const ScreenResolution& resolution) {
   if (resolution.IsEmpty())
     return;
 
+  // Resizing the desktop too often is probably not a good idea, so apply a
+  // simple rate-limiting scheme.
+  base::TimeDelta time_since_previous_resize =
+      base::Time::Now() - previous_resize_time_;

[alexeypa (please no reviews), 2013/06/04 00:05:26]

nit: This looks like potential integer overflow.

if (previous_resize_time_ +
base::TimeDelta::FromMilliseconds(kMinimumResizeIntervalMs) <=
base::Time::Now()) {
  ...
}

[Jamie, 2013/06/04 01:27:49]

I've rewritten it to something I think is a bit clearer than both my original
and your suggested replacement. LMKWYT.

+  base::TimeDelta time_until_resize_permitted =
+      base::TimeDelta::FromMilliseconds(kMinimumResizeIntervalMs) -
+      time_since_previous_resize;
+  if (time_until_resize_permitted > base::TimeDelta::FromMilliseconds(0)) {
+    pending_resolution_ = resolution;
+    if (!resize_pending_) {
+      resize_pending_ = true;
+      base::MessageLoop::current()->PostDelayedTask(

[alexeypa (please no reviews), 2013/06/04 00:05:26]

nit: Actually base::OneShotTimer expresses the intent much better. 

[Jamie, 2013/06/04 01:27:49]

Nice, I was wasn't familiar with that.

+          FROM_HERE,
+          base::Bind(&ResizingHostObserver::SetPendingScreenResolution,
+                     weak_factory_.GetWeakPtr()),
+          time_until_resize_permitted);
+    }
+    return;
+  }
+
   // If the implementation returns any sizes, pick the best one according to
   // the algorithm described in CandidateSize::IsBetterThen.
-  SkISize dimentions = SkISize::Make(
+  SkISize dimensions = SkISize::Make(
       resolution.dimensions().width(), resolution.dimensions().height());
-  std::list<SkISize> sizes = desktop_resizer_->GetSupportedSizes(dimentions);
+  std::list<SkISize> sizes = desktop_resizer_->GetSupportedSizes(dimensions);
   if (sizes.empty())
     return;
-  CandidateSize best_size(sizes.front(), dimentions);
+  CandidateSize best_size(sizes.front(), dimensions);
   for (std::list<SkISize>::const_iterator i = ++sizes.begin();
        i != sizes.end(); ++i) {
-    CandidateSize candidate_size(*i, dimentions);
+    CandidateSize candidate_size(*i, dimensions);
     if (candidate_size.IsBetterThan(best_size)) {
       best_size = candidate_size;
     }
   }
   SkISize current_size = desktop_resizer_->GetCurrentSize();
   if (best_size.size() != current_size)
     desktop_resizer_->SetSize(best_size.size());
+
+  // Update the time of last resize to allow it to be rate-limited.
+  previous_resize_time_ = base::Time::Now();
+}
+
+void ResizingHostObserver::SetPendingScreenResolution() {
+  resize_pending_ = false;

[alexeypa (please no reviews), 2013/06/04 00:05:26]

nit: This flag and the whole function is not needed when base::OneShotTimer is
used. SetScreenResolution() will simply rearm the timer every time a resize
message arrives too early.

[Jamie, 2013/06/04 01:27:49]

Done.

+  SetScreenResolution(pending_resolution_);
 }
 
 }  // namespace remoting