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

remoting/host/resizing_host_observer_unittest.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 <list>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
+#include "base/message_loop.h"
+#include "base/run_loop.h"
 #include "remoting/host/desktop_resizer.h"
 #include "remoting/host/resizing_host_observer.h"
 #include "remoting/host/screen_resolution.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkSize.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
 
 std::ostream& operator<<(std::ostream& os, const SkISize& size) {
   return os << size.width() << "x" << size.height();
 }
@@ skipping 54 matching lines @@
  public:
   ResizingHostObserverTest() : desktop_resizer_(NULL) {
   }
 
   void SetDesktopResizer(scoped_ptr<FakeDesktopResizer> desktop_resizer) {
     CHECK(!desktop_resizer_) << "Call SetDeskopResizer once per test";
     desktop_resizer_ = desktop_resizer.get();
 
     resizing_host_observer_.reset(
         new ResizingHostObserver(desktop_resizer.PassAs<DesktopResizer>()));
+    resizing_host_observer_->SetMinimumResizeIntervalForTesting(
+        base::TimeDelta::FromMilliseconds(0));
   }
 
   SkISize GetBestSize(const SkISize& client_size) {
     resizing_host_observer_->SetScreenResolution(ScreenResolution(
         webrtc::DesktopSize(client_size.width(), client_size.height()),
         webrtc::DesktopVector(kDefaultDPI, kDefaultDPI)));
     return desktop_resizer_->GetCurrentSize();
   }
 
   void VerifySizes(const SkISize* client_sizes, const SkISize* expected_sizes,
                    int number_of_sizes) {
     for (int i = 0; i < number_of_sizes; ++i) {
       SkISize best_size = GetBestSize(client_sizes[i]);
       EXPECT_EQ(expected_sizes[i], best_size)
           << "Input size = " << client_sizes[i];
     }
   }
 
- private:
+ protected:
   scoped_ptr<ResizingHostObserver> resizing_host_observer_;
   FakeDesktopResizer* desktop_resizer_;
 };
 
 // Check that the host is not resized if GetSupportedSizes returns an empty
 // list (even if GetCurrentSize is supported).
 TEST_F(ResizingHostObserverTest, EmptyGetSupportedSizes) {
   SkISize initial = { 640, 480 };
   scoped_ptr<FakeDesktopResizer> desktop_resizer(
       new FakeDesktopResizer(initial, false, NULL, 0));
@@ skipping 72 matching lines @@
       new FakeDesktopResizer(SkISize::Make(640, 480), false,
                              supported_sizes, arraysize(supported_sizes));
   SetDesktopResizer(scoped_ptr<FakeDesktopResizer>(desktop_resizer));
 
   SkISize client_sizes[] = { { 640, 640 }, { 1024, 768 }, { 640, 480 } };
   SkISize expected_sizes[] = { { 640, 480 }, { 640, 480 }, { 640, 480 } };
   VerifySizes(client_sizes, expected_sizes, arraysize(client_sizes));
   EXPECT_EQ(desktop_resizer->set_size_call_count(), 0);
 }
 
+// Check that desktop resizes are rate-limited, and that if multiple resize
+// requests are received in the time-out period, the most recent is respected.
+TEST_F(ResizingHostObserverTest, RateLimited) {
+  int interval_ms = 10;

[alexeypa (please no reviews), 2013/06/06 21:45:11]

1ms should be good enough. All we need is to cause a task to be posted to the
message loop. Any non zero interval will work.

Also, there is no need to make it a variable. Remove |interval_ms|.

[Jamie, 2013/06/06 22:08:04]

There is a small chance that it might take > 1ms between the first two calls to
GetBestSize, which would make the test flaky. It's very unlikely, but I think an
extra 9ms is worth it in this case. LMK if you still disagree and I'll change it
as you suggest.
It's also used as a parameter to PostDelayedTask, so I'd prefer to keep it.

[alexeypa (please no reviews), 2013/06/07 16:23:10]

Oh, I see. So the first SetScreenResoultion() records base::Time::Now() but by
the time of the seconds call base::Time::Now() can already return an updated
value. In this case 10ms is not enough. The precision of base::Time::Now() is
18ms or so on Windows. Also if the machine running the test is under heavy load
the thread could easily be suspended for longer time. I.e. this is flaky.

Another approach we can try is instead of setting the timeout via
SetMinimumResizeIntervalForTesting() the test should control whether the next
resize attempt should be allowed or result in a PostTask (Arming the timer for
0ms seems to be doing PostTask).

+  FakeDesktopResizer* desktop_resizer =
+      new FakeDesktopResizer(SkISize::Make(640, 480), true, NULL, 0);
+  SetDesktopResizer(scoped_ptr<FakeDesktopResizer>(desktop_resizer));
+  resizing_host_observer_->SetMinimumResizeIntervalForTesting(
+      base::TimeDelta::FromMilliseconds(interval_ms));
+
+  base::MessageLoop message_loop;
+  base::RunLoop run_loop;
+
+  EXPECT_EQ(GetBestSize(SkISize::Make(100, 100)), SkISize::Make(100, 100));

[alexeypa (please no reviews), 2013/06/06 21:45:11]

If I read this correctly, it does not check whether the size was eventually set.
Can you add an expectation that size will be changed twice? You will be able to
bind posting of QuitClosure() to this expectation.

[Jamie, 2013/06/06 22:08:04]

That expectation is at the end of the test, after the RunLoop exits.

+  EXPECT_EQ(GetBestSize(SkISize::Make(200, 200)), SkISize::Make(100, 100));
+  EXPECT_EQ(GetBestSize(SkISize::Make(300, 300)), SkISize::Make(100, 100));
+
+  message_loop.PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&base::RunLoop::Quit, base::Unretained(&run_loop)),

[alexeypa (please no reviews), 2013/06/06 21:45:11]

Use |run_loop.QuitClosure()| instead of base::Bind.

[Jamie, 2013/06/06 22:08:04]

Done.

+      base::TimeDelta::FromMilliseconds(interval_ms + 1));
+  run_loop.Run();
+
+  EXPECT_EQ(desktop_resizer_->GetCurrentSize(), SkISize::Make(300, 300));

[alexeypa (please no reviews), 2013/06/07 16:23:10]

This expectation is flaky. Posting QuitClosure() before the timeout elapses will
exit the loop before the delayed tasks have a chance to be executed.

+}
+
 }  // namespace remoting