Adding drawable to CRD andorid and iOS gl rendering pipeline.

remoting/client/display/drawable.h

Index: remoting/client/display/drawable.h
diff --git a/remoting/client/display/drawable.h b/remoting/client/display/drawable.h
new file mode 100644
index 0000000000000000000000000000000000000000..f885bc227017b6c3569f66250ad5e36989fca071
--- /dev/null
+++ b/remoting/client/display/drawable.h
@@ -0,0 +1,52 @@
+// 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_DISPLAY_DRAWABLE_H_
+#define REMOTING_CLIENT_DISPLAY_DRAWABLE_H_
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+
+namespace remoting {
+
+class Canvas;
+
+enum DrawableZIndex {
+  AUTO = -1,

[joedow, 2017/01/10 00:19:43]

I still think it is odd to include a CSS construct here without commenting.  To
me DEFAULT is much clearer than AUTO with respect to location in the stack the
item will be rendered.

[nicholss, 2017/01/10 16:58:34]

That is an opinion.

[joedow, 2017/01/10 18:49:27]

It is my opinion as a reviewer of the code.  When I see AUTO, I assume that the
renderer is going to do something magical and automatically figure out how to
use this value.  It actually doesn't though, this is just a default init value
that gets drawn below the desktop layer.  It's not apparent to me what that
value means which is why I asked for a comment or clarification (or name change
if you agreed).

[nicholss, 2017/01/10 21:43:10]

Acknowledged.

+  DESKTOP = 100,
+  CURSOR_FEEDBACK = 200,
+  CURSOR = 300,
+};
+
+class Drawable {
+ public:
+  Drawable() {}
+  virtual ~Drawable(){};
+
+  // Sets the canvas on which the object will be drawn.
+  // If |canvas| is nullptr, nothing will happen when calling Draw().

[joedow, 2017/01/10 00:19:43]

Can you add a comment about lifetime scope of canvas.  I'm assuming canvas needs
to live longer than all drawables that reference it.

[nicholss, 2017/01/10 21:43:10]

it is now a WeakPtr.

+  virtual void SetCanvas(Canvas* canvas) = 0;
+
+  // Draws the object on the canvas.
+  // Return true if you have a next frame.

[joedow, 2017/01/10 00:19:43]

s/Return/Returns.

[nicholss, 2017/01/10 21:43:10]

Done.

+  virtual bool Draw() = 0;
+
+  // Used for the renderer to keep a stack of drawables.

[joedow, 2017/01/10 00:19:43]

This could be used for purposes other than the renderer (the renderer may also
use something other than a stack to track the drawables).  Can you make this
comment more generic (or remove it).

[nicholss, 2017/01/10 16:58:34]

This is a repeat comment already discussed in previous patch of this CL.

[joedow, 2017/01/10 18:49:27]

I mentioned it but it doesn't seem to have been addressed.  This comment defines
impl details for a different class.  It can easily get out of date.  The comment
should be generic or removed as it doesn't provide much value as-is.

[nicholss, 2017/01/10 21:43:10]

Acknowledged.

+  virtual base::WeakPtr<Drawable> GetWeakPtr() = 0;
+
+  int GetZIndex() { return z_index_; };
+
+  // Specify the Z Index for this drawable. A higher Z Index will draw after a
+  // lower z index. Elements with the same Z Index will draw in order inserted.

[joedow, 2017/01/10 00:19:43]

Why does the drawable define the ZIndex instead of it being defined by the
renderer?  The assumption in this comment could easily be broken if the renderer
were changed.

[nicholss, 2017/01/10 16:58:34]

This is a repeat comment already discussed in previous patch of this CL. 

[joedow, 2017/01/10 18:49:27]

Yes, it was not addressed so I brought it up again (I had figured it was lost
when the code was moved) .  This seems like an impl detail of the renderer, not
the drawable.

[nicholss, 2017/01/10 21:43:10]

Acknowledged.

+  void SetZIndex(int z_index) { z_index_ = z_index; };
+
+ private:
+  int z_index_ = DrawableZIndex::AUTO;
+
+  DISALLOW_COPY_AND_ASSIGN(Drawable);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_CLIENT_DISPLAY_DRAWABLE_H_