Adding drawable to CRD andorid and iOS gl rendering pipeline.

remoting/client/gl_renderer_unittest.cc

Index: remoting/client/gl_renderer_unittest.cc
diff --git a/remoting/client/gl_renderer_unittest.cc b/remoting/client/gl_renderer_unittest.cc
index 1a17e42613203011e1cc34a504be33ae4680d2bc..6b629ee2b0b15f6d91850726ecb3ea252a2506d7 100644
--- a/remoting/client/gl_renderer_unittest.cc
+++ b/remoting/client/gl_renderer_unittest.cc
@@ -81,11 +81,41 @@ class FakeGlRendererDelegate : public GlRendererDelegate {
   base::WeakPtrFactory<FakeGlRendererDelegate> weak_factory_;
 };
 
+class FakeGlDrawable : public GlDrawable {
+ public:
+  FakeGlDrawable() : drawn_(0), id_(-1), weak_factory_(this) {}

[joedow, 2016/12/22 00:29:03]

nit: init the primitives inline and clean up the initializer list.

[nicholss, 2017/01/09 18:50:24]

Done.

+
+  void SetId(int id) { id_ = id; }
+  int GetId() { return id_; }
+
+  base::WeakPtr<GlDrawable> GetWeakPtr() override {
+    return weak_factory_.GetWeakPtr();
+  }
+
+  void SetCanvas(GlCanvas* canvas) override { canvas_ = canvas; }
+
+  bool Draw() override {
+    drawn_++;
+    return false;
+  }
+
+  int DrawnCount() { return drawn_; }
+
+ private:
+  int drawn_;
+  int id_;
+  GlCanvas* canvas_;
+
+  base::WeakPtrFactory<FakeGlDrawable> weak_factory_;

[joedow, 2016/12/22 00:29:03]

DISALLOW_COPY_AND_ASSIGN macro?

[nicholss, 2017/01/09 18:50:24]

Done.

+};
+
 class GlRendererTest : public testing::Test {
  public:
   void SetUp() override;
   void SetDesktopFrameWithSize(const webrtc::DesktopSize& size);
   void PostSetDesktopFrameTasks(const webrtc::DesktopSize& size, int count);
+  int GetDrawablesCount();
+  std::vector<base::WeakPtr<GlDrawable>> GetDrawables();

[joedow, 2016/12/22 00:29:03]

Can this return a const ref so it doesn't need to make a copy of the vector and
contents?

[nicholss, 2017/01/09 18:50:24]

I don't think it matters for the test. It is 5 elements.

 
  protected:
   void RequestRender();
@@ -113,6 +143,14 @@ void GlRendererTest::RequestRender() {
   renderer_->RequestRender();
 }
 
+int GlRendererTest::GetDrawablesCount() {
+  return (int)renderer_->drawables_.size();
+}
+
+std::vector<base::WeakPtr<GlDrawable>> GlRendererTest::GetDrawables() {
+  return renderer_->drawables_;
+}
+
 void GlRendererTest::SetDesktopFrameWithSize(const webrtc::DesktopSize& size) {
   renderer_->OnFrameReceived(
       base::MakeUnique<webrtc::BasicDesktopFrame>(size),
@@ -216,4 +254,110 @@ TEST_F(GlRendererTest, TestOnFrameReceivedDoneCallbacks) {
 
 // TODO(yuweih): Add tests to validate the rendered output.
 
+TEST_F(GlRendererTest, TestAddDrawable) {
+  FakeGlDrawable* drawable0 = new FakeGlDrawable();
+  drawable0->SetId(0);
+  renderer_->AddDrawable(drawable0->GetWeakPtr());
+  EXPECT_EQ(1, GetDrawablesCount());
+}
+
+TEST_F(GlRendererTest, TestAddDrawableDefaultOrder) {
+  FakeGlDrawable* drawable0 = new FakeGlDrawable();
+  drawable0->SetId(0);
+  renderer_->AddDrawable(drawable0->GetWeakPtr());
+  EXPECT_EQ(1, GetDrawablesCount());

[joedow, 2016/12/22 00:29:03]

Why use EXPECT instead of ASSERT?  Do you want the test to continue even though
this line failed (same with the rest of the file)?

[nicholss, 2017/01/09 18:50:24]

I do not know how to unit test in chromium... Don't know what the magic macros
mean. I was unable to find any documentation about this on the chromium site and
the file where ASSERT is defined is HUGE and really not well documented either.
Seems like if you have something to teach, we should have you present this at a
chromoting tech talk, that would be really awesome.

[Yuwei, 2017/01/09 20:28:52]

I think ASSERT will fail the test case and make the function immediately return
while EXPECT will just print out the error and still allow the rest of the test
to be run. Use ASSERT if the error is critical to the rest of the test or if you
just want to speed up the test.

Just to mention IIRC ASSERT can't be used inside destructor (might be the other
way around :P) so you'll get stuck with EXPECT in that case.

+
+  FakeGlDrawable* drawable1 = new FakeGlDrawable();
+  drawable1->SetId(1);
+  renderer_->AddDrawable(drawable1->GetWeakPtr());
+  EXPECT_EQ(2, GetDrawablesCount());
+
+  FakeGlDrawable* drawable2 = new FakeGlDrawable();
+  drawable2->SetId(2);
+  renderer_->AddDrawable(drawable2->GetWeakPtr());
+  EXPECT_EQ(3, GetDrawablesCount());
+
+  std::vector<base::WeakPtr<GlDrawable>> drawables = GetDrawables();
+  int i = 0;
+  for (auto& drawable : drawables) {

[joedow, 2016/12/22 00:29:03]

replace 'drawable' with GetDrawables() instead?

[nicholss, 2017/01/09 18:50:24]

Done.

+    FakeGlDrawable* fg = (FakeGlDrawable*)drawable.get();

[joedow, 2016/12/22 00:29:03]

can you use a c++ style cast instead a c style? 
reinterpret_cast<FakeGlDrawable*>(drawable)

[nicholss, 2017/01/09 18:50:24]

Done.

+    EXPECT_EQ(i, fg->GetId());
+    i++;
+  }
+  EXPECT_EQ(3, i);
+}
+
+TEST_F(GlRendererTest, TestAddDrawableOrder) {
+  FakeGlDrawable* drawable2 = new FakeGlDrawable();
+  drawable2->SetId(2);
+  drawable2->SetZIndex(2);
+  renderer_->AddDrawable(drawable2->GetWeakPtr());
+  EXPECT_EQ(1, GetDrawablesCount());
+
+  FakeGlDrawable* drawable0 = new FakeGlDrawable();
+  drawable0->SetId(0);
+  renderer_->AddDrawable(drawable0->GetWeakPtr());
+  EXPECT_EQ(2, GetDrawablesCount());
+
+  FakeGlDrawable* drawable1 = new FakeGlDrawable();
+  drawable1->SetId(1);
+  drawable1->SetZIndex(1);
+  renderer_->AddDrawable(drawable1->GetWeakPtr());
+  EXPECT_EQ(3, GetDrawablesCount());
+
+  std::vector<base::WeakPtr<GlDrawable>> drawables = GetDrawables();
+  int i = 0;
+  for (auto& drawable : drawables) {
+    FakeGlDrawable* fg = (FakeGlDrawable*)drawable.get();
+    EXPECT_EQ(i, fg->GetId());
+    i++;
+  }
+  EXPECT_EQ(3, i);
+}
+
+TEST_F(GlRendererTest, TestAddDrawableDrawn) {
+  renderer_->OnSurfaceCreated(-1);
+
+  delegate_.can_render_frame_ = true;
+  PostSetDesktopFrameTasks(webrtc::DesktopSize(16, 16), 1);
+
+  FakeGlDrawable* drawable0 = new FakeGlDrawable();
+  drawable0->SetId(3);
+  renderer_->AddDrawable(drawable0->GetWeakPtr());
+
+  RequestRender();
+  RunTasksInCurrentQueue();
+
+  FakeGlDrawable* drawable1 = new FakeGlDrawable();
+  drawable1->SetId(2);
+  drawable1->SetZIndex(1);
+  renderer_->AddDrawable(drawable1->GetWeakPtr());
+
+  RequestRender();
+  RunTasksInCurrentQueue();
+
+  FakeGlDrawable* drawable2 = new FakeGlDrawable();
+  drawable2->SetId(1);
+  drawable2->SetZIndex(2);
+  renderer_->AddDrawable(drawable2->GetWeakPtr());
+  EXPECT_EQ(3, GetDrawablesCount());
+
+  RequestRender();
+  RunTasksInCurrentQueue();
+  std::vector<base::WeakPtr<GlDrawable>> drawables = GetDrawables();
+  for (auto& drawable : drawables) {
+    FakeGlDrawable* fg = (FakeGlDrawable*)drawable.get();
+    EXPECT_EQ(fg->GetId(), fg->DrawnCount());
+  }
+}
+
+TEST_F(GlRendererTest, TestCreateGlRendererWithDesktop) {
+  renderer_.reset(GlRenderer::CreateGlRendererWithDesktop());
+  renderer_->SetDelegate(delegate_.GetWeakPtr());
+  EXPECT_EQ(3, GetDrawablesCount());
+}
+
+// TODO(nicholss): Add a test where the drawable is destructed and the renderer
+// gets a dead weakptr.
+
 }  // namespace remoting