Issue 2591363002: Adding drawable to CRD andorid and iOS gl rendering pipeline.

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

4 years ago (2016-12-21 22:37:39 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/1

4 years ago (2016-12-21 22:38:28 UTC) #2

Yuwei

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/BUILD.gn File remoting/client/BUILD.gn (left): https://codereview.chromium.org/2591363002/diff/20001/remoting/client/BUILD.gn#oldcode99 remoting/client/BUILD.gn:99: "//remoting/proto", Is this because iOS doesn't support some of ...

4 years ago (2016-12-21 23:41:59 UTC) #3

Yuwei

Also I'm not an owner of client/ so you will need to grab another guy ...

4 years ago (2016-12-21 23:56:04 UTC) #4

joedow

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc File remoting/client/gl_canvas.cc (right): https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc#newcode77 remoting/client/gl_canvas.cc:77: #endif Can you reuse the Clear() method after everything ...

4 years ago (2016-12-22 00:29:03 UTC) #6

nicholss

Working on some changes... https://codereview.chromium.org/2591363002/diff/20001/remoting/client/BUILD.gn File remoting/client/BUILD.gn (left): https://codereview.chromium.org/2591363002/diff/20001/remoting/client/BUILD.gn#oldcode99 remoting/client/BUILD.gn:99: "//remoting/proto", On 2016/12/21 23:41:58, Yuwei ...

4 years ago (2016-12-22 16:21:42 UTC) #7

Yuwei

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.h File remoting/client/gl_canvas.h (right): https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.h#newcode101 remoting/client/gl_canvas.h:101: class FakeGlCanvas : public GlCanvas { On 2016/12/22 16:21:41, ...

4 years ago (2016-12-22 19:01:10 UTC) #8

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
File remoting/client/gl_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.h:101: class FakeGlCanvas : public GlCanvas {
On 2016/12/22 16:21:41, nicholss wrote:
> On 2016/12/21 23:41:59, Yuwei wrote:
> > Is this just for testing? I think it will be clearer if you make this into
an
> > individual file and make a common interface for both the fake one and the
real
> > one.
> > 
> > Also you may still be able to verify the drawing order without introducing a
> > fake canvas. Currently if the canvas is null, no drawable will attempt to
> draw.
> 
> This was added to support testing that the draw methods get called. 
> 
> I was not sure if it was worth it to make a new interface, so I tried this
> method with CreateGlCanvas. Thoughts?

We do sometimes use interface to make things testable. See X11Keyboard and
X11KeyboarImpl.

I think it is more important to make sure the implementation code is isolated
from the test code so that later if we want to test against the real OpenGL
context, we can easily remove the interface/test code without needing to do
nontrivial changes to the implementation.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
File remoting/client/gl_renderer.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:132: return weak_factory_.GetWeakPtr();
On 2016/12/22 16:21:41, nicholss wrote:
> On 2016/12/21 23:41:59, Yuwei wrote:
> > Why removing |weak_ptr_|? We used |weak_ptr_| since GetWeakPtr() is
> > thread-unsafe so we just call it once in ctor and store the pointer.
> 
> Nothing in the documentation says this call is thread-unsafe. Are you sure?

Yep, this is an undocumented bug("feature"?). See https://crbug.com/627602

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:163: RequestRender();
On 2016/12/22 16:21:41, nicholss wrote:
> On 2016/12/22 00:29:03, joedow wrote:
> > Can you break out of the loop once a drawable indicates it should be drawn?
> 
> No, request render means: next draw loop, I have something to draw. Like an
> animation.

I think it worth it to add a comment about the return value to
GlDrawable::Draw().

>Otherwise draw will only happen when an event causes a render to be
> requested, like a new frame from the video decode is pushed in for example.

FYI we only draw/swap buffer on demand since swapping buffer will block the
display thread for 0~16ms to vsync. The display thread is also used to do other
heavy works like uploading textures so we don't want it to be block want nothing
needs to be drawn.

joedow

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc File remoting/client/gl_canvas.cc (right): https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc#newcode101 remoting/client/gl_canvas.cc:101: if (gl_constructed_) { On 2016/12/22 16:21:41, nicholss wrote: > ...

4 years ago (2016-12-22 19:18:10 UTC) #9

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
File remoting/client/gl_canvas.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.cc:101: if (gl_constructed_) {
On 2016/12/22 16:21:41, nicholss wrote:
> On 2016/12/22 00:29:02, joedow wrote:
> > On 2016/12/21 23:41:58, Yuwei wrote:
> > > If you have a separate GlCanvas interface then I don't think you need
> > > |gl_constructed_|...
> > 
> > This feels a bit odd.  Is there a way to guarantee the object is valid when
it
> > is constructed?  I think that would be cleaner.
> 
> It is odd because this is not dependency injection based code so I had to do
> funky stuff to test or rewrite it.

I don't think DI is required to prevent having test and production code in this
class.

A common approach is to use a factory, production uses a factory that provides a
fully initialized object, the test factory provides one specific to the task at
hand.  The class that uses the factory provides a "SetFactoryForTesting" method
which allows the test to pass in a factory that does what it wants.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
File remoting/client/gl_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.h:70: static GlCanvas* CreateGlCanvas(int gl_version);
If you want the caller to own the instance, the expected ownership would be more
transparent if you returned a std::unique_ptr here instead.  It will also
prevent leaks.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.h:101: class FakeGlCanvas : public GlCanvas {
On 2016/12/22 19:01:10, Yuwei wrote:
> On 2016/12/22 16:21:41, nicholss wrote:
> > On 2016/12/21 23:41:59, Yuwei wrote:
> > > Is this just for testing? I think it will be clearer if you make this into
> an
> > > individual file and make a common interface for both the fake one and the
> real
> > > one.
> > > 
> > > Also you may still be able to verify the drawing order without introducing
a
> > > fake canvas. Currently if the canvas is null, no drawable will attempt to
> > draw.
> > 
> > This was added to support testing that the draw methods get called. 
> > 
> > I was not sure if it was worth it to make a new interface, so I tried this
> > method with CreateGlCanvas. Thoughts?
> 
> We do sometimes use interface to make things testable. See X11Keyboard and
> X11KeyboarImpl.
> 
> I think it is more important to make sure the implementation code is isolated
> from the test code so that later if we want to test against the real OpenGL
> context, we can easily remove the interface/test code without needing to do
> nontrivial changes to the implementation.

We separate prod and test code routinely.  If you need an interface, you can do
that, otherwise the fake instance can inherit from the production class (as it
is doing here) and stub out the methods it cares about.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_draw...
File remoting/client/gl_drawable.h (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_draw...
remoting/client/gl_drawable.h:16: AUTO = -1,
On 2016/12/22 16:21:41, nicholss wrote:
> On 2016/12/22 00:29:02, joedow wrote:
> > DEFAULT instead of AUTO?
> 
> The default is AUTO, which is a css construct. 

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.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
File remoting/client/gl_renderer.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:132: return weak_factory_.GetWeakPtr();
On 2016/12/22 19:01:10, Yuwei wrote:
> On 2016/12/22 16:21:41, nicholss wrote:
> > On 2016/12/21 23:41:59, Yuwei wrote:
> > > Why removing |weak_ptr_|? We used |weak_ptr_| since GetWeakPtr() is
> > > thread-unsafe so we just call it once in ctor and store the pointer.
> > 
> > Nothing in the documentation says this call is thread-unsafe. Are you sure?
> 
> Yep, this is an undocumented bug("feature"?). See https://crbug.com/627602

Thanks for pointing that out Yuwei!  That is a good point.  You want to make
sure all of the WeakPtr's are bound to the render thread.  I'm pretty sure they
are today but that doesn't prevent problems in the future.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:163: RequestRender();
On 2016/12/22 19:01:10, Yuwei wrote:
> On 2016/12/22 16:21:41, nicholss wrote:
> > On 2016/12/22 00:29:03, joedow wrote:
> > > Can you break out of the loop once a drawable indicates it should be
drawn?
> > 
> > No, request render means: next draw loop, I have something to draw. Like an
> > animation.
> 
> I think it worth it to add a comment about the return value to
> GlDrawable::Draw().
> 
> >Otherwise draw will only happen when an event causes a render to be
> > requested, like a new frame from the video decode is pushed in for example.
> 
> FYI we only draw/swap buffer on demand since swapping buffer will block the
> display thread for 0~16ms to vsync. The display thread is also used to do
other
> heavy works like uploading textures so we don't want it to be block want
nothing
> needs to be drawn.

Ah yeah, I misread that.  As long as RequestRender is a cheap operation then
this is fine.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
File remoting/client/gl_renderer.h (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.h:100: static GlRenderer*
CreateGlRendererWithDesktop();
On 2016/12/22 00:29:03, joedow wrote:
> Add comment block please.  Who owns the renderer* passed out?  Details about
> expected ownership would be good.

Actually this would also be more clear if it returned a std::unique_ptr.  That
way it is clear that ownership of the object is transferred to the caller.

Sergey Ulanov

sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org

4 years ago (2016-12-22 22:38:45 UTC) #10

Sergey Ulanov

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc File remoting/client/gl_canvas.cc (right): https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc#newcode101 remoting/client/gl_canvas.cc:101: if (gl_constructed_) { On 2016/12/22 19:18:09, joedow wrote: > ...

4 years ago (2016-12-22 22:38:46 UTC) #11

nicholss

On 2016/12/22 22:38:46, Sergey Ulanov wrote: > https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc > File remoting/client/gl_canvas.cc (right): > > https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc#newcode101 ...

4 years ago (2016-12-22 22:56:24 UTC) #12

Yuwei

On 2016/12/22 22:56:24, nicholss wrote: > On 2016/12/22 22:38:46, Sergey Ulanov wrote: > > > ...

4 years ago (2016-12-22 23:07:44 UTC) #13

nicholss

On 2016/12/22 23:07:44, Yuwei wrote: > On 2016/12/22 22:56:24, nicholss wrote: > > On 2016/12/22 ...

4 years ago (2016-12-22 23:11:02 UTC) #14

Sergey Ulanov

On 2016/12/22 23:11:02, nicholss wrote: > On 2016/12/22 23:07:44, Yuwei wrote: > > The reason ...

3 years, 12 months ago (2016-12-27 21:00:10 UTC) #15

Sergey Ulanov

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_renderer.cc File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_renderer.cc#newcode132 remoting/client/gl_renderer.cc:132: return weak_factory_.GetWeakPtr(); On 2016/12/22 19:18:09, joedow wrote: > On ...

3 years, 12 months ago (2016-12-27 21:00:19 UTC) #16

Yuwei

On 2016/12/27 21:00:19, Sergey Ulanov wrote: > https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_renderer.cc > File remoting/client/gl_renderer.cc (right): > > https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_renderer.cc#newcode132 ...

3 years, 12 months ago (2016-12-27 21:39:17 UTC) #17

nicholss

Update CL, note that I moved files supported by https://codereview.chromium.org/2614443003/ PTAL, I think I have ...

3 years, 11 months ago (2017-01-09 18:50:25 UTC) #18

Update CL, note that I moved files supported by
https://codereview.chromium.org/2614443003/

PTAL, I think I have addressed all comments.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
File remoting/client/gl_canvas.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.cc:77: #endif
On 2016/12/22 00:29:02, joedow wrote:
> Can you reuse the Clear() method after everything is initialized?  Then you
> wouldn't need duplicated code.

removing the clear function

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.cc:101: if (gl_constructed_) {
On 2016/12/22 19:18:09, joedow wrote:
> On 2016/12/22 16:21:41, nicholss wrote:
> > On 2016/12/22 00:29:02, joedow wrote:
> > > On 2016/12/21 23:41:58, Yuwei wrote:
> > > > If you have a separate GlCanvas interface then I don't think you need
> > > > |gl_constructed_|...
> > > 
> > > This feels a bit odd.  Is there a way to guarantee the object is valid
when
> it
> > > is constructed?  I think that would be cleaner.
> > 
> > It is odd because this is not dependency injection based code so I had to do
> > funky stuff to test or rewrite it.
> 
> I don't think DI is required to prevent having test and production code in
this
> class.
> 
> A common approach is to use a factory, production uses a factory that provides
a
> fully initialized object, the test factory provides one specific to the task
at
> hand.  The class that uses the factory provides a "SetFactoryForTesting"
method
> which allows the test to pass in a factory that does what it wants.

I added an interface and moved to a more dependency injection model to allow for
testing.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.cc:101: if (gl_constructed_) {
On 2016/12/22 22:38:45, Sergey Ulanov wrote:
> On 2016/12/22 19:18:09, joedow wrote:
> > I don't think DI is required to prevent having test and production code in
> this
> > class.
> > 
> > A common approach is to use a factory, production uses a factory that
provides
> a
> > fully initialized object, the test factory provides one specific to the task
> at
> > hand.  The class that uses the factory provides a "SetFactoryForTesting"
> method
> > which allows the test to pass in a factory that does what it wants.
> 
> In this case I don't think we need any of it. It should be possible to use
real
> OpenGL in unittests. We already do that on Android.

I would prefer to not test OpenGL and the renderer loop at the same time. It is
OS dependent and my preference while adding renderer tests (making sure the
stack/drawables has been created correctly) to be a true unit test rather than
an integration test.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.cc:130: glClear(GL_COLOR_BUFFER_BIT);
On 2016/12/21 23:41:58, Yuwei wrote:
> glClearColor only specifies the color and glClear will actually clear the
buffer
> with the color so I think the order is wrong?

Fixed.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.cc:174: return new FakeGlCanvas();
On 2016/12/22 00:29:01, joedow wrote:
> I don't think this logic should be included in production code.  We should use
a
> factory or similar mechanism to provide fake instances for testing.

Removing.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
File remoting/client/gl_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.h:70: static GlCanvas* CreateGlCanvas(int gl_version);
On 2016/12/22 19:18:09, joedow wrote:
> If you want the caller to own the instance, the expected ownership would be
more
> transparent if you returned a std::unique_ptr here instead.  It will also
> prevent leaks.

good call. Changing to return a std::unique_prt.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.h:94: bool gl_constructed_;
On 2016/12/22 00:29:02, joedow wrote:
> You should use inline inits for primitives instead of using initializer lists
> (or leaving it uninitialized :P)

this was removed and replaced with the use of an interface and a fake impl.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.h:101: class FakeGlCanvas : public GlCanvas {
On 2016/12/22 19:18:09, joedow wrote:
> On 2016/12/22 19:01:10, Yuwei wrote:
> > On 2016/12/22 16:21:41, nicholss wrote:
> > > On 2016/12/21 23:41:59, Yuwei wrote:
> > > > Is this just for testing? I think it will be clearer if you make this
into
> > an
> > > > individual file and make a common interface for both the fake one and
the
> > real
> > > > one.
> > > > 
> > > > Also you may still be able to verify the drawing order without
introducing
> a
> > > > fake canvas. Currently if the canvas is null, no drawable will attempt
to
> > > draw.
> > > 
> > > This was added to support testing that the draw methods get called. 
> > > 
> > > I was not sure if it was worth it to make a new interface, so I tried this
> > > method with CreateGlCanvas. Thoughts?
> > 
> > We do sometimes use interface to make things testable. See X11Keyboard and
> > X11KeyboarImpl.
> > 
> > I think it is more important to make sure the implementation code is
isolated
> > from the test code so that later if we want to test against the real OpenGL
> > context, we can easily remove the interface/test code without needing to do
> > nontrivial changes to the implementation.
> 
> We separate prod and test code routinely.  If you need an interface, you can
do
> that, otherwise the fake instance can inherit from the production class (as it
> is doing here) and stub out the methods it cares about.

Went ahead and created the interface.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_draw...
File remoting/client/gl_drawable.h (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_draw...
remoting/client/gl_drawable.h:16: AUTO = -1,
On 2016/12/22 19:18:09, joedow wrote:
> On 2016/12/22 16:21:41, nicholss wrote:
> > On 2016/12/22 00:29:02, joedow wrote:
> > > DEFAULT instead of AUTO?
> > 
> > The default is AUTO, which is a css construct. 
> 
> 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.

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.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_draw...
remoting/client/gl_drawable.h:28: // If |canvas| is nullptr, nothing will happen
when calling Draw().
On 2016/12/22 00:29:02, joedow wrote:
> What should Draw() return if |canvas| is nullptr?  That might be good to
specify
> here (or on that method).

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.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_draw...
remoting/client/gl_drawable.h:40: // Specify the Z Index for this drawable. A
higher Z Index will draw after a
On 2016/12/22 00:29:02, joedow wrote:
> 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).

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.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_draw...
remoting/client/gl_drawable.h:52: struct {
On 2016/12/21 23:41:59, Yuwei wrote:
> Why not put the name after `struct`?

old c habits.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_draw...
remoting/client/gl_drawable.h:56: } SortDrawablesZOrder;
On 2016/12/21 23:41:59, Yuwei wrote:
> Since this is not a sorter, I think it makes more sense to call it something
> like DrawablesZOrderComparator.

Good suggestion.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
File remoting/client/gl_renderer.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:26: GlRenderer*
GlRenderer::CreateGlRendererWithDesktop() {
On 2016/12/21 23:41:59, Yuwei wrote:
> I think it makes more sense to create a new class that wraps GlRenderer and
> handles everything related desktop and cursor and make GlRenderer a class that
> simply renders a stack of drawables.

I think that makes sense but not required for this pass. Maybe next time.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:132: return weak_factory_.GetWeakPtr();
On 2016/12/27 21:00:19, Sergey Ulanov wrote:
> On 2016/12/22 19:18:09, joedow wrote:
> > On 2016/12/22 19:01:10, Yuwei wrote:
> > > On 2016/12/22 16:21:41, nicholss wrote:
> > > > On 2016/12/21 23:41:59, Yuwei wrote:
> > > > > Why removing |weak_ptr_|? We used |weak_ptr_| since GetWeakPtr() is
> > > > > thread-unsafe so we just call it once in ctor and store the pointer.
> > > > 
> > > > Nothing in the documentation says this call is thread-unsafe. Are you
> sure?
> > > 
> > > Yep, this is an undocumented bug("feature"?). See https://crbug.com/627602
> > 
> > Thanks for pointing that out Yuwei!  That is a good point.  You want to make
> > sure all of the WeakPtr's are bound to the render thread.  I'm pretty sure
> they
> > are today but that doesn't prevent problems in the future.
> 
> weak ptrs get bound to a thread when they are dereferenced, not when they are
> created. 
> AFAICT GetWeakPtr() call is probably safe here. It may only be unsafe when
it's
> being called on multiple threads in parallel, which shouldn't be happening
here.
> Still agree with Yuwei it's better to keep weak_ptr_.
> FWIW here is a related CL: https://codereview.chromium.org/1602443003 that
will
> clarify threading semantics for that method.

reverted;

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
File remoting/client/gl_renderer.h (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.h:100: static GlRenderer*
CreateGlRendererWithDesktop();
On 2016/12/22 19:18:10, joedow wrote:
> On 2016/12/22 00:29:03, joedow wrote:
> > Add comment block please.  Who owns the renderer* passed out?  Details about
> > expected ownership would be good.
> 
> Actually this would also be more clear if it returned a std::unique_ptr.  That
> way it is clear that ownership of the object is transferred to the caller.

Done.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
File remoting/client/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer_unittest.cc:86: FakeGlDrawable() : drawn_(0),
id_(-1), weak_factory_(this) {}
On 2016/12/22 00:29:03, joedow wrote:
> nit: init the primitives inline and clean up the initializer list.

Done.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer_unittest.cc:109:
base::WeakPtrFactory<FakeGlDrawable> weak_factory_;
On 2016/12/22 00:29:03, joedow wrote:
> DISALLOW_COPY_AND_ASSIGN macro?

Done.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer_unittest.cc:118:
std::vector<base::WeakPtr<GlDrawable>> GetDrawables();
On 2016/12/22 00:29:03, joedow wrote:
> Can this return a const ref so it doesn't need to make a copy of the vector
and
> contents?

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

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer_unittest.cc:268: EXPECT_EQ(1, GetDrawablesCount());
On 2016/12/22 00:29:03, joedow wrote:
> 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)?

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.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer_unittest.cc:282: for (auto& drawable : drawables) {
On 2016/12/22 00:29:03, joedow wrote:
> replace 'drawable' with GetDrawables() instead?

Done.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer_unittest.cc:283: FakeGlDrawable* fg =
(FakeGlDrawable*)drawable.get();
On 2016/12/22 00:29:03, joedow wrote:
> can you use a c++ style cast instead a c style? 
> reinterpret_cast<FakeGlDrawable*>(drawable)

Done.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_gl_display_handler.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.cc:65: GlRenderer* renderer_ =
GlRenderer::CreateGlRendererWithDesktop();
On 2016/12/22 00:29:03, joedow wrote:
> This member should be initialized with a nullptr and the heavy create/assign
> work should be done in the c'tor.

Done.

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-09 18:51:05 UTC) #19

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/40001

3 years, 11 months ago (2017-01-09 18:51:39 UTC) #20

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-09 19:11:09 UTC) #21

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/367099)

3 years, 11 months ago (2017-01-09 19:11:10 UTC) #22

Yuwei

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc File remoting/client/gl_canvas.cc (right): https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canvas.cc#newcode101 remoting/client/gl_canvas.cc:101: if (gl_constructed_) { On 2017/01/09 18:50:23, nicholss wrote: > ...

3 years, 11 months ago (2017-01-09 20:28:52 UTC) #23

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
File remoting/client/gl_canvas.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_canv...
remoting/client/gl_canvas.cc:101: if (gl_constructed_) {
On 2017/01/09 18:50:23, nicholss wrote:
> On 2016/12/22 22:38:45, Sergey Ulanov wrote:
> > On 2016/12/22 19:18:09, joedow wrote:
> > > I don't think DI is required to prevent having test and production code in
> > this
> > > class.
> > > 
> > > A common approach is to use a factory, production uses a factory that
> provides
> > a
> > > fully initialized object, the test factory provides one specific to the
task
> > at
> > > hand.  The class that uses the factory provides a "SetFactoryForTesting"
> > method
> > > which allows the test to pass in a factory that does what it wants.
> > 
> > In this case I don't think we need any of it. It should be possible to use
> real
> > OpenGL in unittests. We already do that on Android.
> 
> I would prefer to not test OpenGL and the renderer loop at the same time. It
is
> OS dependent and my preference while adding renderer tests (making sure the
> stack/drawables has been created correctly) to be a true unit test rather than
> an integration test.

The previous tests work based on these preconditions:
* Test binary can be successfully built with the corresponding
  OpenGL dependencies.
* Test code path doesn't call any gl* functions. For previous
  implementation that is OnSurfaceCreated never being called or
  canvas_ == nullptr

You can get it work as long as your changes/new tests fulfill these
preconditions. The extra interface/DI may or may not be required.

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
File remoting/client/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2591363002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer_unittest.cc:268: EXPECT_EQ(1, GetDrawablesCount());
On 2017/01/09 18:50:24, nicholss wrote:
> On 2016/12/22 00:29:03, joedow wrote:
> > 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)?
> 
> 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.

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.

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
File remoting/client/display/gl_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
remoting/client/display/gl_canvas.h:71: GlCanvas();
Where is this constructor used?

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
File remoting/client/display/gl_renderer.h (right):

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
remoting/client/display/gl_renderer.h:134: std::vector<base::WeakPtr<Drawable>>
drawables_;
Is it really more useful to store a WeakPtr instead of a unique_ptr? Since
GlRenderer controls the drawable after it is added to the vector, I think it
makes more sense to make the renderer own the drawables.

Also by doing so you can get rid of the GetWeakPtr() declaration in the drawable
interface and only define it for the fake drawable, and you can also remove
those three drawables above and create them inside
CreateGlRendererWithDesktop().

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/jni/jni...
File remoting/client/jni/jni_gl_display_handler.cc (right):

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.cc:134:
renderer_->OnSurfaceCreated(std::move(canvas));
Maybe use MakeUnique?

renderer_->OnSurfaceCreated(base::MakeUnique<GlCanvas>(static_cast<int>(...))

nicholss

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display/gl_canvas.h File remoting/client/display/gl_canvas.h (right): https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display/gl_canvas.h#newcode71 remoting/client/display/gl_canvas.h:71: GlCanvas(); On 2017/01/09 20:28:52, Yuwei wrote: > Where is ...

3 years, 11 months ago (2017-01-09 20:39:24 UTC) #24

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
File remoting/client/display/gl_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
remoting/client/display/gl_canvas.h:71: GlCanvas();
On 2017/01/09 20:28:52, Yuwei wrote:
> Where is this constructor used?

it is not, removed.

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
File remoting/client/display/gl_renderer.h (right):

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
remoting/client/display/gl_renderer.h:134: std::vector<base::WeakPtr<Drawable>>
drawables_;
On 2017/01/09 20:28:52, Yuwei wrote:
> Is it really more useful to store a WeakPtr instead of a unique_ptr? Since
> GlRenderer controls the drawable after it is added to the vector, I think it
> makes more sense to make the renderer own the drawables.
> 
> Also by doing so you can get rid of the GetWeakPtr() declaration in the
drawable
> interface and only define it for the fake drawable, and you can also remove
> those three drawables above and create them inside
> CreateGlRendererWithDesktop().

I absolutely don't want the renderer to own the drawable. I would like to push 
in and pop out drawable elements in the render stack and I want to be able 
to push messages into them from other places and have it rendered for me
automatically. Think of the MVC pattern. I don't want the V to also be the C.

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/jni/jni...
File remoting/client/jni/jni_gl_display_handler.cc (right):

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.cc:134:
renderer_->OnSurfaceCreated(std::move(canvas));
On 2017/01/09 20:28:52, Yuwei wrote:
> Maybe use MakeUnique?
> 
> renderer_->OnSurfaceCreated(base::MakeUnique<GlCanvas>(static_cast<int>(...))

does that make the code more clean? Seems like this change would just make it
confusing to me.

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-09 20:59:40 UTC) #25

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/60001

3 years, 11 months ago (2017-01-09 21:00:14 UTC) #26

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-09 21:40:56 UTC) #27

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/344620)

3 years, 11 months ago (2017-01-09 21:40:57 UTC) #28

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-09 22:17:30 UTC) #29

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/80001

3 years, 11 months ago (2017-01-09 22:18:17 UTC) #30

Yuwei

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display/gl_renderer.h File remoting/client/display/gl_renderer.h (right): https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display/gl_renderer.h#newcode134 remoting/client/display/gl_renderer.h:134: std::vector<base::WeakPtr<Drawable>> drawables_; On 2017/01/09 20:39:24, nicholss wrote: > On ...

3 years, 11 months ago (2017-01-09 22:41:54 UTC) #31

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
File remoting/client/display/gl_renderer.h (right):

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/display...
remoting/client/display/gl_renderer.h:134: std::vector<base::WeakPtr<Drawable>>
drawables_;
On 2017/01/09 20:39:24, nicholss wrote:
> On 2017/01/09 20:28:52, Yuwei wrote:
> > Is it really more useful to store a WeakPtr instead of a unique_ptr? Since
> > GlRenderer controls the drawable after it is added to the vector, I think it
> > makes more sense to make the renderer own the drawables.
> > 
> > Also by doing so you can get rid of the GetWeakPtr() declaration in the
> drawable
> > interface and only define it for the fake drawable, and you can also remove
> > those three drawables above and create them inside
> > CreateGlRendererWithDesktop().
> 
> I absolutely don't want the renderer to own the drawable. I would like to push

> in and pop out drawable elements in the render stack and I want to be able 
> to push messages into them from other places and have it rendered for me
> automatically.

This can still be done if you grab the WeakPtr of the Drawable before pushing it
to the renderer though.

> Think of the MVC pattern. I don't want the V to also be the C.

Currently GlRenderer is for both controlling the the render behavior and
actually rendering out the stack of Drawables so it's already like V+C...

I'm good with making the controller own the Drawables but only after the
controller part is separated from GlRenderer. If you will do that later at some
point then I'm okay with this change.

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/jni/jni...
File remoting/client/jni/jni_gl_display_handler.cc (right):

https://codereview.chromium.org/2591363002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.cc:134:
renderer_->OnSurfaceCreated(std::move(canvas));
On 2017/01/09 20:39:24, nicholss wrote:
> On 2017/01/09 20:28:52, Yuwei wrote:
> > Maybe use MakeUnique?
> > 
> >
renderer_->OnSurfaceCreated(base::MakeUnique<GlCanvas>(static_cast<int>(...))
> 
> does that make the code more clean? Seems like this change would just make it
> confusing to me.

This is just a helper function to avoid wrapping the raw pointer and
std::mov'ing it. It's up to you though.

https://codereview.chromium.org/2591363002/diff/80001/remoting/client/display...
File remoting/client/display/gl_renderer.cc (right):

https://codereview.chromium.org/2591363002/diff/80001/remoting/client/display...
remoting/client/display/gl_renderer.cc:21: namespace drawable {
Make it an anonymous namespace? This is not used outside this file.

https://codereview.chromium.org/2591363002/diff/80001/remoting/client/display...
File remoting/client/display/sys_opengl.h (right):

https://codereview.chromium.org/2591363002/diff/80001/remoting/client/display...
remoting/client/display/sys_opengl.h:21: #include <EGL/egl.h>
Why is EGL included here? I think it's only used the EglThreadContext... Also
OpenGL and EGL are different components.

nicholss

https://codereview.chromium.org/2591363002/diff/80001/remoting/client/display/sys_opengl.h File remoting/client/display/sys_opengl.h (right): https://codereview.chromium.org/2591363002/diff/80001/remoting/client/display/sys_opengl.h#newcode21 remoting/client/display/sys_opengl.h:21: #include <EGL/egl.h> On 2017/01/09 22:41:54, Yuwei wrote: > Why ...

3 years, 11 months ago (2017-01-09 22:51:13 UTC) #32

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-09 22:58:21 UTC) #33

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/100001

3 years, 11 months ago (2017-01-09 22:58:53 UTC) #34

Yuwei

https://codereview.chromium.org/2591363002/diff/80001/remoting/client/display/sys_opengl.h File remoting/client/display/sys_opengl.h (right): https://codereview.chromium.org/2591363002/diff/80001/remoting/client/display/sys_opengl.h#newcode21 remoting/client/display/sys_opengl.h:21: #include <EGL/egl.h> On 2017/01/09 22:51:12, nicholss wrote: > On ...

3 years, 11 months ago (2017-01-09 23:06:37 UTC) #35

Yuwei

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/jni/jni_gl_display_handler.cc File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2591363002/diff/100001/remoting/client/jni/jni_gl_display_handler.cc#newcode132 remoting/client/jni/jni_gl_display_handler.cc:132: renderer_->OnSurfaceCreated(std::make_unique<GlCanvas>( Use base::MakeUnique<Type>(arguments...) and no need to new. renderer_->OnSurfaceCreated(base::MakeUnique<GlCanvas>(static_cast<int>(egl_context_->client_version()))); ...

3 years, 11 months ago (2017-01-09 23:11:52 UTC) #36

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-09 23:35:58 UTC) #37

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/120001

3 years, 11 months ago (2017-01-09 23:36:59 UTC) #38

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-09 23:58:56 UTC) #39

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/110021

3 years, 11 months ago (2017-01-10 00:00:13 UTC) #40

joedow

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h#newcode11 remoting/client/display/canvas.h:11: #include "base/threading/thread_checker.h" Remote thread_checker header since it is no ...

3 years, 11 months ago (2017-01-10 00:19:44 UTC) #41

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-10 01:16:00 UTC) #42

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/190999)

3 years, 11 months ago (2017-01-10 01:16:01 UTC) #43

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-10 16:58:27 UTC) #44

nicholss

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h#newcode11 remoting/client/display/canvas.h:11: #include "base/threading/thread_checker.h" On 2017/01/10 00:19:43, joedow wrote: > Remote ...

3 years, 11 months ago (2017-01-10 16:58:34 UTC) #45

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/canvas.h:11: #include "base/threading/thread_checker.h"
On 2017/01/10 00:19:43, joedow wrote:
> Remote thread_checker header since it is no longer used in this file

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/canvas.h:20: Canvas(){};
On 2017/01/10 00:19:43, joedow wrote:
> please run clang format to fix some spacing nits (i.e. space between () and
{})

It does not work on mac.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/canvas.h:60: // Version if applicable to implementation.
On 2017/01/10 00:19:43, joedow wrote:
> I could see this returning the OpenGL version for that impl but what does
> version mean for other impls?  The caller would necessarily know what
> implementation they are calling into so how would Version be used in an
> impl-agnostic fashion?

It is up to the impl. It is not a real concern here because we just have OpenGL
but I am attempting to remove the GL references from the interface. So I guess
it is TBD when that bridge needs crossing.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/drawable.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:16: AUTO = -1,
On 2017/01/10 00:19:43, joedow wrote:
> 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.

That is an opinion.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:35: // Used for the renderer to keep a stack
of drawables.
On 2017/01/10 00:19:43, joedow wrote:
> 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).

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

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:41: // lower z index. Elements with the same
Z Index will draw in order inserted.
On 2017/01/10 00:19:43, joedow wrote:
> 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.

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

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/fake_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/fake_canvas.h:27: void SetViewSize(int width, int
height) override {}
On 2017/01/10 00:19:43, joedow wrote:
> Since you have a cc file, the impls for all of the methods should go there.  I
> haven't seen any cases were we have some empty impls in the header and other
> methods in the CC file.

Given this is a test file, does it matter?

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_canvas.h:42: // offset of the upper-left corner in
pixel.
On 2017/01/10 00:19:44, joedow wrote:
> Since this class now implements the Canvas interface and all of these comments
> exist in that file, you should remove these comments, snug all of the
overrides
> together and add a comment indicating the overrides are
implementing/overriding
> the base interface.
> 
> The problem I foresee is that if anything defined in these comments changes,
> then we would need to update comments in multiple places.

I will remove the override but I would think the override keyword would mean it
is overriding the base interface.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_cursor.cc (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_cursor.cc:105: return weak_factory_.GetWeakPtr();
On 2017/01/10 00:19:44, joedow wrote:
> Maybe I missed the conclusion of this discussion from the CL that was
superseded
> by this one, but I had thought this was dangerous as the affinity of the
WeakPtr
> would be bound to the first thread which called this method and that it was
> safer to store a WeakPtr member which was initialized on the correct thread
and
> had references passed out.

That was not for this class.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_renderer.cc (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_renderer.cc:21: namespace drawable {
On 2017/01/10 00:19:44, joedow wrote:
> Shouldn't this be anonymous instead of a name namespace?  If someone has a
> similarly name struct (or adds something which conflicts) you would start
> getting multi-definition linking errors that would have to be tracked down.

Done.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/70021

3 years, 11 months ago (2017-01-10 16:58:40 UTC) #46

joedow

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h#newcode20 remoting/client/display/canvas.h:20: Canvas(){}; On 2017/01/10 16:58:34, nicholss wrote: > On 2017/01/10 ...

3 years, 11 months ago (2017-01-10 18:49:27 UTC) #47

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/canvas.h:20: Canvas(){};
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:43, joedow wrote:
> > please run clang format to fix some spacing nits (i.e. space between () and
> {})
> 
> It does not work on mac.

Weird, ok then add a space manually between parens and curly braces.

Is there an infrastructure bug open to get git cl format (clang format) working
for mac?

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/canvas.h:60: // Version if applicable to implementation.
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:43, joedow wrote:
> > I could see this returning the OpenGL version for that impl but what does
> > version mean for other impls?  The caller would necessarily know what
> > implementation they are calling into so how would Version be used in an
> > impl-agnostic fashion?
> 
> It is up to the impl. It is not a real concern here because we just have
OpenGL
> but I am attempting to remove the GL references from the interface. So I guess
> it is TBD when that bridge needs crossing.

This still seem confusing to have on an interface for the canvas.  Can you
update the comment so you are explicit about what Version is used for?  I'm
trying to think of how this interface would be used and it isn't apparent what
Version means just by looking at the interface documentation.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/drawable.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:16: AUTO = -1,
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:43, joedow wrote:
> > 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.
> 
> That is an opinion.

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).

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:35: // Used for the renderer to keep a stack
of drawables.
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:43, joedow wrote:
> > 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).
> 
> This is a repeat comment already discussed in previous patch of this CL.

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.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:41: // lower z index. Elements with the same
Z Index will draw in order inserted.
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:43, joedow wrote:
> > 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.
> 
> This is a repeat comment already discussed in previous patch of this CL. 

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.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/fake_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/fake_canvas.h:27: void SetViewSize(int width, int
height) override {}
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:43, joedow wrote:
> > Since you have a cc file, the impls for all of the methods should go there. 
I
> > haven't seen any cases were we have some empty impls in the header and other
> > methods in the CC file.
> 
> Given this is a test file, does it matter?

Test code should beheld to the same (or very similar) standards as product code.
 It is also super easy to do.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_canvas.h:42: // offset of the upper-left corner in
pixel.
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:44, joedow wrote:
> > Since this class now implements the Canvas interface and all of these
comments
> > exist in that file, you should remove these comments, snug all of the
> overrides
> > together and add a comment indicating the overrides are
> implementing/overriding
> > the base interface.
> > 
> > The problem I foresee is that if anything defined in these comments changes,
> > then we would need to update comments in multiple places.
> 
> I will remove the override but I would think the override keyword would mean
it
> is overriding the base interface.

Thatcommenting style is used throughout the Chromoting project, it is common
practice to group the related interface methods together and add a comment above
them specifying which interface they are overriding/implementing.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_cursor.cc (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_cursor.cc:105: return weak_factory_.GetWeakPtr();
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:44, joedow wrote:
> > Maybe I missed the conclusion of this discussion from the CL that was
> superseded
> > by this one, but I had thought this was dangerous as the affinity of the
> WeakPtr
> > would be bound to the first thread which called this method and that it was
> > safer to store a WeakPtr member which was initialized on the correct thread
> and
> > had references passed out.
> 
> That was not for this class.

I thought it was for all classes which expose a WeakPtr.  The problem as I
understand it that the first time you call GetWeakPtr() the underlying thread
affinity for the factory and weakptr are set.  If someone doesn't call this
method as expected (i.e. on a different thread) then you will have a problem.

One approach would be to use a thread checker and DCHECK that each access to
this method occurs on the thread which created the factory.  Then you should be
ok.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-10 19:00:20 UTC) #48

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/290173)

3 years, 11 months ago (2017-01-10 19:00:21 UTC) #49

Yuwei

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h#newcode60 remoting/client/display/canvas.h:60: // Version if applicable to implementation. On 2017/01/10 16:58:34, ...

3 years, 11 months ago (2017-01-10 19:36:10 UTC) #50

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/canvas.h:60: // Version if applicable to implementation.
On 2017/01/10 16:58:34, nicholss wrote:
> On 2017/01/10 00:19:43, joedow wrote:
> > I could see this returning the OpenGL version for that impl but what does
> > version mean for other impls?  The caller would necessarily know what
> > implementation they are calling into so how would Version be used in an
> > impl-agnostic fashion?
> 
> It is up to the impl. It is not a real concern here because we just have
OpenGL
> but I am attempting to remove the GL references from the interface. So I guess
> it is TBD when that bridge needs crossing.

GetGlVersion is only used by GlRenderLayer to decide whether ES3 optimization
should be applied. GlRenderLayer seems to be an implementation detail of
GlCanvas so alternatively you can have a CreateRenderLayer() function and return
a constructed object with the right version set up. Doesn't need to do it in
this CL though.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_cursor.cc (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_cursor.cc:105: return weak_factory_.GetWeakPtr();
On 2017/01/10 18:49:27, joedow wrote:
> On 2017/01/10 16:58:34, nicholss wrote:
> > On 2017/01/10 00:19:44, joedow wrote:
> > > Maybe I missed the conclusion of this discussion from the CL that was
> > superseded
> > > by this one, but I had thought this was dangerous as the affinity of the
> > WeakPtr
> > > would be bound to the first thread which called this method and that it
was
> > > safer to store a WeakPtr member which was initialized on the correct
thread
> > and
> > > had references passed out.
> > 
> > That was not for this class.
> 
> I thought it was for all classes which expose a WeakPtr.  The problem as I
> understand it that the first time you call GetWeakPtr() the underlying thread
> affinity for the factory and weakptr are set.  If someone doesn't call this
> method as expected (i.e. on a different thread) then you will have a problem.
> 
> One approach would be to use a thread checker and DCHECK that each access to
> this method occurs on the thread which created the factory.  Then you should
be
> ok.

As discussed offline, these Drawables are supposed to be only used on the
display thread so I just didn't bother introducing the thread checker. I think
it's good to introduce a thread checker just to be safer, since none of these
functions will work on a non-display thread.

https://codereview.chromium.org/2591363002/diff/70021/remoting/client/display...
File remoting/client/display/BUILD.gn (right):

https://codereview.chromium.org/2591363002/diff/70021/remoting/client/display...
remoting/client/display/BUILD.gn:41: "OpenSLES",
OpenSLES shouldn't be included here. It's for Android audio.

https://codereview.chromium.org/2591363002/diff/70021/remoting/client/display...
remoting/client/display/BUILD.gn:66: group("unit_tests") {
Please add the comment about the ANGLE GLESv2 issue back here.

nicholss

PTAL https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h#newcode67 remoting/client/display/canvas.h:67: DISALLOW_COPY_AND_ASSIGN(Canvas); On 2017/01/10 00:19:43, joedow wrote: > Add ...

3 years, 11 months ago (2017-01-10 21:43:10 UTC) #51

PTAL

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/canvas.h:67: DISALLOW_COPY_AND_ASSIGN(Canvas);
On 2017/01/10 00:19:43, joedow wrote:
> Add 'private:' above this line.  It was removed in the code deletion.

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/drawable.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:16: AUTO = -1,
On 2017/01/10 18:49:27, joedow wrote:
> On 2017/01/10 16:58:34, nicholss wrote:
> > On 2017/01/10 00:19:43, joedow wrote:
> > > 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.
> > 
> > That is an opinion.
> 
> 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).

Acknowledged.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:28: // If |canvas| is nullptr, nothing will
happen when calling Draw().
On 2017/01/10 00:19:43, joedow wrote:
> Can you add a comment about lifetime scope of canvas.  I'm assuming canvas
needs
> to live longer than all drawables that reference it.

it is now a WeakPtr.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:32: // Return true if you have a next frame.
On 2017/01/10 00:19:43, joedow wrote:
> s/Return/Returns.

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:35: // Used for the renderer to keep a stack
of drawables.
On 2017/01/10 18:49:27, joedow wrote:
> On 2017/01/10 16:58:34, nicholss wrote:
> > On 2017/01/10 00:19:43, joedow wrote:
> > > 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).
> > 
> > This is a repeat comment already discussed in previous patch of this CL.
> 
> 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.

Acknowledged.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/drawable.h:41: // lower z index. Elements with the same
Z Index will draw in order inserted.
On 2017/01/10 18:49:27, joedow wrote:
> On 2017/01/10 16:58:34, nicholss wrote:
> > On 2017/01/10 00:19:43, joedow wrote:
> > > 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.
> > 
> > This is a repeat comment already discussed in previous patch of this CL. 
> 
> 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.

Acknowledged.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/fake_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/fake_canvas.h:20: 
On 2017/01/10 00:19:43, joedow wrote:
> nit: rmeove this newline to squeeze the c'tor and d'tor together

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/fake_canvas.h:24: 
On 2017/01/10 00:19:43, joedow wrote:
> common practice is to remove the newlines for the overrides and add a comment
> like: "// Canvas implementation."

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/fake_canvas.h:27: void SetViewSize(int width, int
height) override {}
On 2017/01/10 18:49:27, joedow wrote:
> On 2017/01/10 16:58:34, nicholss wrote:
> > On 2017/01/10 00:19:43, joedow wrote:
> > > Since you have a cc file, the impls for all of the methods should go
there. 
> I
> > > haven't seen any cases were we have some empty impls in the header and
other
> > > methods in the CC file.
> > 
> > Given this is a test file, does it matter?
> 
> Test code should beheld to the same (or very similar) standards as product
code.
>  It is also super easy to do.

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/fake_canvas.h:37: 
On 2017/01/10 00:19:43, joedow wrote:
> add private:

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_canvas.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_canvas.h:42: // offset of the upper-left corner in
pixel.
On 2017/01/10 18:49:27, joedow wrote:
> On 2017/01/10 16:58:34, nicholss wrote:
> > On 2017/01/10 00:19:44, joedow wrote:
> > > Since this class now implements the Canvas interface and all of these
> comments
> > > exist in that file, you should remove these comments, snug all of the
> > overrides
> > > together and add a comment indicating the overrides are
> > implementing/overriding
> > > the base interface.
> > > 
> > > The problem I foresee is that if anything defined in these comments
changes,
> > > then we would need to update comments in multiple places.
> > 
> > I will remove the override but I would think the override keyword would mean
> it
> > is overriding the base interface.
> 
> Thatcommenting style is used throughout the Chromoting project, it is common
> practice to group the related interface methods together and add a comment
above
> them specifying which interface they are overriding/implementing.

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_cursor.cc (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_cursor.cc:105: return weak_factory_.GetWeakPtr();
On 2017/01/10 19:36:10, Yuwei wrote:
> On 2017/01/10 18:49:27, joedow wrote:
> > On 2017/01/10 16:58:34, nicholss wrote:
> > > On 2017/01/10 00:19:44, joedow wrote:
> > > > Maybe I missed the conclusion of this discussion from the CL that was
> > > superseded
> > > > by this one, but I had thought this was dangerous as the affinity of the
> > > WeakPtr
> > > > would be bound to the first thread which called this method and that it
> was
> > > > safer to store a WeakPtr member which was initialized on the correct
> thread
> > > and
> > > > had references passed out.
> > > 
> > > That was not for this class.
> > 
> > I thought it was for all classes which expose a WeakPtr.  The problem as I
> > understand it that the first time you call GetWeakPtr() the underlying
thread
> > affinity for the factory and weakptr are set.  If someone doesn't call this
> > method as expected (i.e. on a different thread) then you will have a
problem.
> > 
> > One approach would be to use a thread checker and DCHECK that each access to
> > this method occurs on the thread which created the factory.  Then you should
> be
> > ok.
> 
> As discussed offline, these Drawables are supposed to be only used on the
> display thread so I just didn't bother introducing the thread checker. I think
> it's good to introduce a thread checker just to be safer, since none of these
> functions will work on a non-display thread.

Acknowledged.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_cursor.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_cursor.h:44: 
On 2017/01/10 00:19:44, joedow wrote:
> smoosh the overrides together, remove the duplicate comment and add interface
> impl comment.

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_cursor_feedback.h (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_cursor_feedback.h:8: #include <stdint.h>
On 2017/01/10 00:19:44, joedow wrote:
> nit: not your change but I've been removing the old school C headers and
> replaced them with c++ versions, this one can be replaced with cstdint if you
> are interested :)

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
File remoting/client/display/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_renderer_unittest.cc:72:
base::WeakPtrFactory<FakeGlRendererDelegate> weak_factory_;
On 2017/01/10 00:19:44, joedow wrote:
> nit: add newline

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_renderer_unittest.cc:99: Canvas* canvas_;
On 2017/01/10 00:19:44, joedow wrote:
> Canvas* canvas_ = nullptr;

Done.

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/displa...
remoting/client/display/gl_renderer_unittest.cc:101:
base::WeakPtrFactory<FakeDrawable> weak_factory_;
On 2017/01/10 00:19:44, joedow wrote:
> nit: add newline

Done.

https://codereview.chromium.org/2591363002/diff/70021/remoting/client/display...
File remoting/client/display/BUILD.gn (right):

https://codereview.chromium.org/2591363002/diff/70021/remoting/client/display...
remoting/client/display/BUILD.gn:41: "OpenSLES",
On 2017/01/10 19:36:10, Yuwei wrote:
> OpenSLES shouldn't be included here. It's for Android audio.

Done.

https://codereview.chromium.org/2591363002/diff/70021/remoting/client/display...
remoting/client/display/BUILD.gn:66: group("unit_tests") {
On 2017/01/10 19:36:10, Yuwei wrote:
> Please add the comment about the ANGLE GLESv2 issue back here.

Done.

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-10 21:43:26 UTC) #52

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/160001

3 years, 11 months ago (2017-01-10 21:44:00 UTC) #53

joedow

lgtm https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/canvas.h#newcode59 remoting/client/display/canvas.h:59: // Version if applicable to implementation. Can this ...

3 years, 11 months ago (2017-01-10 22:01:55 UTC) #54

Yuwei

LGTM https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/gl_cursor.cc File remoting/client/display/gl_cursor.cc (right): https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/gl_cursor.cc#newcode78 remoting/client/display/gl_cursor.cc:78: layer_.reset(new GlRenderLayer(kGlCursorTextureId, canvas->GetWeakPtr())); Just pass canvas. No need ...

3 years, 11 months ago (2017-01-10 22:09:53 UTC) #55

Sergey Ulanov

https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/100001/remoting/client/display/canvas.h#newcode20 remoting/client/display/canvas.h:20: Canvas(){}; On 2017/01/10 18:49:27, joedow wrote: > On 2017/01/10 ...

3 years, 11 months ago (2017-01-10 23:07:18 UTC) #56

Sergey Ulanov

https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/canvas.h#newcode21 remoting/client/display/canvas.h:21: virtual ~Canvas(){}; remove ; in these two lines https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/canvas.h#newcode56 ...

3 years, 11 months ago (2017-01-10 23:24:02 UTC) #57

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-10 23:45:46 UTC) #58

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/290357)

3 years, 11 months ago (2017-01-10 23:45:47 UTC) #59

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-11 18:52:00 UTC) #60

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/180001

3 years, 11 months ago (2017-01-11 18:52:43 UTC) #61

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-11 18:56:27 UTC) #62

nicholss

PTAL. Comments have been addressed. https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/canvas.h File remoting/client/display/canvas.h (right): https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/canvas.h#newcode56 remoting/client/display/canvas.h:56: GLuint vertex_buffer, On 2017/01/10 ...

3 years, 11 months ago (2017-01-11 18:56:44 UTC) #63

PTAL. Comments have been addressed.

https://codereview.chromium.org/2591363002/diff/160001/remoting/client/displa...
File remoting/client/display/canvas.h (right):

https://codereview.chromium.org/2591363002/diff/160001/remoting/client/displa...
remoting/client/display/canvas.h:56: GLuint vertex_buffer,
On 2017/01/10 23:24:01, Sergey Ulanov wrote:
> probably not for this CL: is this interface supposed to have dependency on
> OpenGL. Right now it looks like gl headers need to be pulled just for GLuint
> type, which is easy to avoid by using int instead of GLuint here.

Done.

https://codereview.chromium.org/2591363002/diff/160001/remoting/client/displa...
File remoting/client/display/drawable.h (right):

https://codereview.chromium.org/2591363002/diff/160001/remoting/client/displa...
remoting/client/display/drawable.h:15: enum DrawableZIndex {
On 2017/01/10 23:24:01, Sergey Ulanov wrote:
> please nest it in Drawable class. Otherwise the values from this enum pollute
> remoting namespace. Also do you need to set values for each enum value instead
> of using defaults? Will we ever need z-index values in 100 and 200?

The intent was that these are guide-lines and if we have something that wants to
draw between a desktop and a cursor, there will be room without needing to
update this enum. I have adjusted the comment and the name of the enum to say as
much.

https://codereview.chromium.org/2591363002/diff/160001/remoting/client/displa...
remoting/client/display/drawable.h:38: int GetZIndex() { return z_index_; };
On 2017/01/10 23:24:01, Sergey Ulanov wrote:
> get_z_index()?

I am not sure what this comment is asking for. GetZIndex to get_z_index? That is
not inline with coding styles in remoting unless. Changing to just ZIndex (get
and set was used to make it settable and was attempting to not change existing
classes...)

https://codereview.chromium.org/2591363002/diff/160001/remoting/client/displa...
remoting/client/display/drawable.h:45: int z_index_ = DrawableZIndex::AUTO;
On 2017/01/10 23:24:01, Sergey Ulanov wrote:
> I don't think we need SetZIndex or z_index_. Instead it would be cleaner for
> child classes to override GetZIndex() to return z-index they want.

Done.

https://codereview.chromium.org/2591363002/diff/160001/remoting/proto/BUILD.gn
File remoting/proto/BUILD.gn (right):

https://codereview.chromium.org/2591363002/diff/160001/remoting/proto/BUILD.g...
remoting/proto/BUILD.gn:7: group("proto_lite") {
On 2017/01/10 23:24:01, Sergey Ulanov wrote:
> Would it be cleaner to rename this to proto and rename proto to something
else?
> It seems strange that proto_library() doesn't add dependency on protobuf_lite.

My intent was to have a proto dep that brings along the protobuf_lite dep. Other
archs pull in the protobuf dep no problem but iOS this does not work and it
needs the protobuf_lite version to be able to build. 

I did not want to change the name of the proto_library because most things in
the system don't need the _lite lib. So I made the proto_lite name to mean
"proto" + "protobuf_lite".

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/200001

3 years, 11 months ago (2017-01-11 18:57:06 UTC) #64

Yuwei

https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/drawable.h File remoting/client/display/drawable.h (right): https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/drawable.h#newcode38 remoting/client/display/drawable.h:38: int GetZIndex() { return z_index_; }; On 2017/01/11 18:56:43, ...

3 years, 11 months ago (2017-01-11 19:16:58 UTC) #65

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-11 19:59:54 UTC) #66

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-11 19:59:55 UTC) #67

Sergey Ulanov

lgtm when my comments are addressed https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/drawable.h File remoting/client/display/drawable.h (right): https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/drawable.h#newcode38 remoting/client/display/drawable.h:38: int GetZIndex() { ...

3 years, 11 months ago (2017-01-11 22:47:31 UTC) #68

nicholss

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-11 23:39:36 UTC) #69

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/220001

3 years, 11 months ago (2017-01-11 23:41:02 UTC) #70

nicholss

Will commit after tests are green again. Thanks for reviewing. https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/gl_renderer.cc File remoting/client/display/gl_renderer.cc (right): https://codereview.chromium.org/2591363002/diff/160001/remoting/client/display/gl_renderer.cc#newcode27 ...

3 years, 11 months ago (2017-01-11 23:43:25 UTC) #71

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-12 01:41:35 UTC) #72

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-12 01:41:36 UTC) #73

nicholss

The patchset sent to the CQ was uploaded after l-g-t-m from yuweih@chromium.org, joedow@chromium.org, sergeyu@chromium.org Link ...

3 years, 11 months ago (2017-01-12 16:00:17 UTC) #75

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2591363002/220001

3 years, 11 months ago (2017-01-12 16:00:44 UTC) #76

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484236816787500, "parent_rev": "39f5b80c5358f69f8407557990beed449fa03e74", "commit_rev": "f62cc8a96c95682f25b90b9f5264b4ecd9c30613"}

3 years, 11 months ago (2017-01-12 16:08:48 UTC) #77

commit-bot: I haz the power

Description was changed from ========== Adding drawable to CRD andorid and iOS gl rendering pipeline. ...

3 years, 11 months ago (2017-01-12 16:09:20 UTC) #78

commit-bot: I haz the power

Committed patchset #13 (id:220001) as https://chromium.googlesource.com/chromium/src/+/f62cc8a96c95682f25b90b9f5264b4ecd9c30613

3 years, 11 months ago (2017-01-12 16:09:22 UTC) #79

Yuwei

https://codereview.chromium.org/2591363002/diff/220001/remoting/client/display/gl_renderer.cc File remoting/client/display/gl_renderer.cc (right): https://codereview.chromium.org/2591363002/diff/220001/remoting/client/display/gl_renderer.cc#newcode177 remoting/client/display/gl_renderer.cc:177: renderer->AddDrawable(renderer->desktop_.GetWeakPtr()); Sorry that I haven't spied the bug here. ...

3 years, 11 months ago (2017-01-12 23:23:17 UTC) #80

joedow

3 years, 11 months ago (2017-01-13 15:49:45 UTC) #81

Message was sent while issue was closed.

https://codereview.chromium.org/2591363002/diff/220001/remoting/client/displa...
File remoting/client/display/gl_renderer.cc (right):

https://codereview.chromium.org/2591363002/diff/220001/remoting/client/displa...
remoting/client/display/gl_renderer.cc:177:
renderer->AddDrawable(renderer->desktop_.GetWeakPtr());
On 2017/01/12 23:23:17, Yuwei wrote:
> Sorry that I haven't spied the bug here. The thread checkers of these
drawables
> will be bound to whatever thread CreateGlRendererWithDesktop is called on.
> 
> I'll send a CL to fix this.

The thread checkers of the drawables will be initialized to the thread they were
created on (affinity is set in the c'tor of ewach drawable unless Detach is
called).  The renderer will have affinity to the thread
CreateGlRendererWithDesktop() is called on.  Not sure if that affects the
scenario you are concerned about.

Issue 2591363002: Adding drawable to CRD andorid and iOS gl rendering pipeline. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Minor cleanup of an unused const. #

Patch Set 3 : Cleanup and adding interfaces based on review feedback.: #

Patch Set 4 : Fixing compile errors for android and linux. Plus addressing feedback. #

Patch Set 5 : More special casing for Android and windows. #

Patch Set 6 : Trying to fix android. #

Patch Set 7 : removing include of EGL. #

Patch Set 8 : Adding public_configs for khronos_headers. #

Patch Set 9 : Changing a bunch of nits. #

Patch Set 10 : Finished next round of feedback changes. Added more weakptrs for canvas passing. #

Patch Set 11 : Trimming down the Drawable interface, removing Get/Set ZIndex; it is just ZIndex now. Fixing Test. #

Patch Set 12 : Woops, wrong unique_ptr. Typo. #

Patch Set 13 : More like GetZIndex. #

Messages