Adding drawable to CRD andorid and iOS gl rendering pipeline.

remoting/client/gl_drawable.h

+// 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.
+
+#ifndef REMOTING_CLIENT_GL_DRAWABLE_H_
+#define REMOTING_CLIENT_GL_DRAWABLE_H_
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+
+namespace remoting {
+
+class GlCanvas;
+
+enum DrawableZIndex {

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

Z index is fine, but would it make sense to refer to these as the layers
instead?  It is semantic difference so this is just a nit.

+  AUTO = -1,

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

DEFAULT instead of AUTO?

[nicholss, 2016/12/22 16:21:41]

The default is AUTO, which is a css construct. 

[joedow, 2016/12/22 19:18:09]

Neither Android nor iOS use CSS so I wouldn't have thought you were using a web
construct here.  If that is the intention, it would be good to document that
intention above.  Otherwise I could easily see a maintenance engineer coming in
and adding a new layer with a name which doesn't conform to the expectations of
the enum.

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

I don't really understand the issue leaning on a css construct here. I don't see
what a maintenance engineer could add that is wrong.

+  DESKTOP = 100,
+  CURSOR_FEEDBACK = 200,
+  CURSOR = 300,
+};
+
+class GlDrawable {
+ public:
+  GlDrawable() {}
+  virtual ~GlDrawable(){};

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

space between () and {}

+
+  // Sets the canvas on which the object will be drawn.
+  // If |canvas| is nullptr, nothing will happen when calling Draw().

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

What should Draw() return if |canvas| is nullptr?  That might be good to specify
here (or on that method).

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

It would depend on the implementation. Draw returns true if it has a next frame.
You could choose to continue returning true while canvas is null to make sure as
soon as you get a canvas you can draw, or you could wait until something signals
the drawable. I don't want to define that.

+  virtual void SetCanvas(GlCanvas* canvas) = 0;
+
+  // Draws the object on the canvas.
+  // Return true if you have a next frame.
+  virtual bool Draw() = 0;
+
+  // Used for the renderer to keep a stack of drawables.

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

This could be used for other reasons too (i.e. if a test wanted to make sure a
drawable was cleaned up after it finished rendering).  You might want to make
this comment more generic and remove the impl details about the renderer.

+  virtual base::WeakPtr<GlDrawable> GetWeakPtr() = 0;
+
+  int GetZIndex() { return z_index_; };
+
+  // Specify the Z Index for this drawable. A higher Z Index will draw after a

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

There is a lot of renderer impl details in the comment for this property.  I
think it would be simpler to remove the extra renderer info and move that to the
place where the GlDrawable is used (that way there is only one location to
update if the behavior ever changed).

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

Normally interfaces are where behaviors and side effects are described. This is
what I am trying to do. I prefer to look at the interface to see the
documentation rather than the implementation. 

+  // lower z index. Elements with the same Z Index will draw in order inserted.
+  void SetZIndex(int z_index) { z_index_ = z_index; };
+
+ private:
+  int z_index_ = DrawableZIndex::AUTO;
+
+  DISALLOW_COPY_AND_ASSIGN(GlDrawable);
+};
+
+namespace drawable {
+
+struct {

[Yuwei, 2016/12/21 23:41:59]

Why not put the name after `struct`?

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

old c habits.

+  bool operator()(base::WeakPtr<GlDrawable> a, base::WeakPtr<GlDrawable> b) {

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

I don't think this belongs here.  I feel like this should be defined by the
renderer (probably in an anonymous namespace in the renderer cc file

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

Can these be const ref instead of passed by value?

+    return a->GetZIndex() < b->GetZIndex();
+  }
+} SortDrawablesZOrder;

[Yuwei, 2016/12/21 23:41:59]

Since this is not a sorter, I think it makes more sense to call it something
like DrawablesZOrderComparator.

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

Good suggestion. 

+
+}  // namespace drawable
+
+}  // namespace remoting
+
+#endif  // REMOTING_CLIENT_GL_DRAWABLE_H_