Add ScopedSurfaceRequestManager

content/browser/android/scoped_surface_request_manager_unittest.cc

+// Copyright 2016 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 "content/browser/android/scoped_surface_request_manager.h"
+
+#include "base/bind.h"
+#include "base/callback_forward.h"
+#include "base/run_loop.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gl/android/scoped_java_surface.h"
+#include "ui/gl/android/surface_texture.h"
+
+namespace content {
+
+class ScopedSurfaceRequestManagerUnitTest : public testing::Test {
+ public:
+  ScopedSurfaceRequestManagerUnitTest() {
+    manager = ScopedSurfaceRequestManager::GetInstance();
+
+    // The need to reset the callbacks because the
+    // ScopedSurfaceRequestManager's lifetime outlive the tests.
+    manager->clear_callbacks_for_testing();

[watk, 2016/08/31 00:47:36]

Can you create a new manager instead?

[tguilbert, 2016/08/31 17:58:56]

Not really, the constructor is private. I would have to create a for_testing()
which would be equivalent.

+
+    surface_texture = gl::SurfaceTexture::Create(0);
+    dummy_request =
+        base::Bind(&ScopedSurfaceRequestManagerUnitTest::DummyCallback,
+                   base::Unretained(this));
+    specific_logging_request =
+        base::Bind(&ScopedSurfaceRequestManagerUnitTest::LoggingCallback,
+                   base::Unretained(this), kSpecificCallbackId);
+  }
+
+  // No-op callback.
+  void DummyCallback(gl::ScopedJavaSurface surface) {}
+
+  // Callback that updates |last_received_request| to allow differentiation
+  // between callback instances in tests.
+  void LoggingCallback(int request_id, gl::ScopedJavaSurface surface) {
+    last_received_request = request_id;
+  }
+
+  ScopedSurfaceRequestManager::ScopedSurfaceRequestCB dummy_request;
+  ScopedSurfaceRequestManager::ScopedSurfaceRequestCB specific_logging_request;
+  scoped_refptr<gl::SurfaceTexture> surface_texture;
+
+  uint64_t last_received_request;
+  const uint64_t kSpecificCallbackId = 1357;
+  const uint64_t kDummyToken = 0xABCDE;
+
+  ScopedSurfaceRequestManager* manager;

[watk, 2016/08/31 00:47:35]

We usually use the underscore suffix for members in tests too I think

[tguilbert, 2016/08/31 17:58:56]

Interesting. TIL! In my mind underscores were only for private vars, but I
double checked and you are right.

+
+  content::TestBrowserThreadBundle thread_bundle_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedSurfaceRequestManagerUnitTest);
+};
+
+// Makes sure we can successfully register a callback.
+TEST_F(ScopedSurfaceRequestManagerUnitTest, RegisterRequest_ShouldSucceed) {
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+
+  manager->RegisterScopedSurfaceRequest(dummy_request);
+
+  EXPECT_EQ(1, manager->callback_count_for_testing());
+}
+
+// Makes sure we can successfully register multiple callbacks, and that they
+// return distinct request tokens.
+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       RegisterMultipleRequests_ShouldSucceed) {
+  EXPECT_EQ(0, manager->callback_count_for_testing());

[watk, 2016/08/31 00:47:35]

This duplicates the assertion made in RegisterRequest_ShouldSucceed. We should
delete it to keep the tests small and focused on testing one thing. 

I think all the tests below have similarly redundant EXPECTS.

[tguilbert, 2016/08/31 17:58:56]

Done.

+
+  uint64_t token1 = manager->RegisterScopedSurfaceRequest(dummy_request);
+  uint64_t token2 = manager->RegisterScopedSurfaceRequest(dummy_request);
+
+  EXPECT_EQ(2, manager->callback_count_for_testing());
+  EXPECT_NE(token1, token2);
+}
+
+// Makes sure GetInstance() is idempotent/that the class is a proper singleton.
+TEST_F(ScopedSurfaceRequestManagerUnitTest, VerifySingleton_ShouldSucceed) {
+  manager->RegisterScopedSurfaceRequest(dummy_request);
+
+  ScopedSurfaceRequestManager* manager_other =
+      ScopedSurfaceRequestManager::GetInstance();
+
+  EXPECT_EQ(manager, manager_other);

[watk, 2016/08/31 00:47:36]

This test can be EXPECT_EQ(manager, ScopedSurfaceRequestManager::GetInstance())

RegisterRequest_ShouldSucceed covers the rest.

[tguilbert, 2016/08/31 17:58:56]

Done.

+  EXPECT_EQ(1, manager_other->callback_count_for_testing());
+}
+
+// Makes sure we can unregister a callback after registering it.
+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       GetRegisteredRequest_ShouldSucceed) {
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+
+  uint64_t token = manager->RegisterScopedSurfaceRequest(dummy_request);
+  EXPECT_EQ(1, manager->callback_count_for_testing());
+
+  manager->UnregisterScopedSurfaceRequest(token);
+
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+}
+
+// Makes sure that unregistering a callback only affects the specified callback.
+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       GetRegisteredRequestFromMultipleRequests_ShouldSucceed) {
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+
+  uint64_t token = manager->RegisterScopedSurfaceRequest(dummy_request);
+  manager->RegisterScopedSurfaceRequest(dummy_request);
+  EXPECT_EQ(2, manager->callback_count_for_testing());
+
+  manager->UnregisterScopedSurfaceRequest(token);
+
+  EXPECT_EQ(1, manager->callback_count_for_testing());
+}
+
+// Makes sure that unregistration is a noop permitted when there are no
+// registered requests.
+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       UnregisteredRequest_ShouldReturnNullCallback) {
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+
+  manager->UnregisterScopedSurfaceRequest(123);
+
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+}
+
+// Makes sure that unregistering and invalid |request_token| doesn't affect

[watk, 2016/08/31 00:47:36]

an

[tguilbert, 2016/08/31 17:58:56]

Done.

+// other registered callbacks.
+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       GetUnregisteredRequestFromMultipleRequests_ShouldReturnNullCallback) {
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+
+  manager->RegisterScopedSurfaceRequest(dummy_request);
+
+  manager->UnregisterScopedSurfaceRequest(kDummyToken);
+
+  EXPECT_EQ(1, manager->callback_count_for_testing());
+}
+
+// Makes sure that trying to fulfill a request for an invalid |request_token|
+// does nothing, and does not affect other callbacks.
+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       FulfillUnregisteredRequest_ShouldDoNothing) {
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+
+  manager->RegisterScopedSurfaceRequest(specific_logging_request);
+
+  last_received_request = 0;
+
+  manager->FulfillScopedSurfaceRequest(
+      kDummyToken, gl::ScopedJavaSurface(surface_texture.get()));
+
+  EXPECT_EQ(1, manager->callback_count_for_testing());
+  EXPECT_NE(kSpecificCallbackId, last_received_request);
+}
+
+// Makes sure that trying to fulfill a request fulfills the right request, and
+// does not affect other registered requests.
+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       FulfillRegisteredRequest_ShouldSucceed) {
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+
+  const uint64_t kOtherCallbackId = 5678;
+
+  uint64_t specific_token =
+      manager->RegisterScopedSurfaceRequest(specific_logging_request);
+
+  manager->RegisterScopedSurfaceRequest(
+      base::Bind(&ScopedSurfaceRequestManagerUnitTest::LoggingCallback,
+                 base::Unretained(this), kOtherCallbackId));
+
+  last_received_request = 0;
+
+  manager->FulfillScopedSurfaceRequest(
+      specific_token, gl::ScopedJavaSurface(surface_texture.get()));
+
+  EXPECT_EQ(1, manager->callback_count_for_testing());
+  EXPECT_EQ(kSpecificCallbackId, last_received_request);
+}
+
+// Makes sure that the code is resilient to empty callbacks.

[watk, 2016/08/31 00:47:35]

Easier to DCHECK the request is not null IMO

[tguilbert, 2016/08/31 17:58:56]

I added a DCHECK in the Register(), and replaced the "if(!request.is_null())" by
a DCHECK in Fulfill(). Let me know if this is not what you had in mind.

request should never be null, but I am just a bit worried that, if it is, we
will cause a crash in the browser process.

Should I keep the "if" instead of the DCHECK in Fulfill()?

[watk, 2016/08/31 18:12:14]

DCHECK in register SG. Yes, I'd say keep the if in Fulfill since if a request is
unregistered it will be null.

[tguilbert, 2016/08/31 20:44:44]

Oops! Forgot about unregistered CBs returning null, you are right.

+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       FulfillEmptyRequest_ShouldNotCrash) {
+  uint64_t token = manager->RegisterScopedSurfaceRequest(
+      base::Callback<void(gl::ScopedJavaSurface)>());
+
+  manager->FulfillScopedSurfaceRequest(
+      token, gl::ScopedJavaSurface(surface_texture.get()));
+
+  EXPECT_EQ(0, manager->callback_count_for_testing());
+}
+
+// Makes sure that the ScopedSurfaceRequestConduit implementation properly
+// fulfills requests.
+TEST_F(ScopedSurfaceRequestManagerUnitTest,
+       ForwardSurfaceTexture_ShouldFulfillRequest) {
+  uint64_t token =
+      manager->RegisterScopedSurfaceRequest(specific_logging_request);
+
+  last_received_request = 0;

[watk, 2016/08/31 00:47:35]

initialize in the constructor/SetUp

[tguilbert, 2016/08/31 17:58:56]

Done.

+
+  manager->ForwardSurfaceTextureForSurfaceRequest(token, surface_texture.get());
+
+  EXPECT_EQ(0, manager->callback_count_for_testing());

[watk, 2016/08/31 00:47:35]

delete

[tguilbert, 2016/08/31 17:58:56]

I feel like this one should stay, because, without looking at the .cc file,
there is no guarantee that the manager uses Fulfill() (and it could directly
access the request map, without unregistering the request).

+  EXPECT_EQ(kSpecificCallbackId, last_received_request);
+}
+
+}  // Content