Adding the iOS app and integration example with GlRenderer.

Moving source from internal to chromium to provide an app for CRD iOS.

Integrating CRD iOS with the new OpenGL renderer. This will allow iOS and android to share a significant amount of render code.  It also could be used to build a smoke test of the CRD iOS app in Chromium with similar dependencies to the real app.

BUG=671692
Review-Url: https://codereview.chromium.org/2555803002
Cr-Commit-Position: refs/heads/master@{#446679}
Committed: https://chromium.googlesource.com/chromium/src/+/28f74dd4d3b78681833af7c4ac7fb6b2d8f64859

[nicholss, 2016-12-19 22:12:26]

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

[commit-bot: I haz the power, 2016-12-19 22:13:31]

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

[nicholss, 2016-12-19 22:24:30]

Description was changed from

==========
Integrating CRD iOS with the new OpenGL renderer. This will allow iOS and
android to share a significant amount of render code.

BUG=671692
==========

to

==========
Adjusting how gl_renderer draws layers and added a demo app for CRD iOS.

The intent of this CL is to begin introducing a demo app that represents the
open source components used by the CRD iOS app. Integrating CRD iOS with the new
OpenGL renderer. This will allow iOS and android to share a significant amount
of render code.  It also could be used to build a smoke test of the CRD iOS app
in Chromium with similar dependencies to the real app.

BUG=671692
==========

[nicholss, 2016-12-19 22:24:30]

nicholss@chromium.org changed reviewers:
+ yuweih@chromium.org

[Yuwei, 2016-12-19 23:26:09]

Thanks for looking at the OpenGL stuffs!

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

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.h:95: void AddDrawable(GlDrawable* drawable);
Could we just make the class own the drawable? Holding raw pointer may cause
burden to maintain the order of destruction. What about passing and storing
std::unique_ptr?

Currently the way this class works is the caller sets the position of the cursor
and the desktop image, etc., then the class will just render out the result, so
how the canvas is drawn/the GlDrawable stuff seem to be an implementation detail
to me. I feel that it's clearer to provide a function like DrawDemoScreen() and
make the demo screen a part of the renderer.

Anyway it's still up to you to decide whether the user is allowed to add extra
drawables to the renderer O_o

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/chr...
File remoting/client/ios/chromiumoting/example_view_controller.mm (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/chr...
remoting/client/ios/chromiumoting/example_view_controller.mm:33: _context =
[[EAGLContext alloc] initWithAPI:kEAGLRenderingAPIOpenGLES2];
The renderer has optimizations that are only for GLES3. Could you create the
GLES2 context only when it fails to create GLES3 context? Or you can leave a
TODO if you don't want to do that in this CL.

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_demo_screen.mm:16: const GLfloat square[] =
{-1.0, -1.0, 1.0, -1.0, -1.0, 1.0, 1.0, 1.0};
kSquare?

https://google.github.io/styleguide/cppguide.html#Constant_Names

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_demo_screen.mm:31: const GLchar* a_position =
"a_position";
Maybe |kAPositionAttribute|?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_demo_screen.mm:77: glUseProgram(program);
GlCanvas was supposed to be used to draw textures on the view. If you want to
use your own program to draw the demo then I think you should also move the
glUseProgram() call from GlCanvas' contrustor to the beginning of
GlCanvas::DrawTexture.

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_demo_screen.mm:90: GLuint aPosition =
glGetAttribLocation(program, a_position);
Nit: maybe getting it once and store it in SetCanvas? The position should not
change after the program is created.

s/aPosition/a_position/?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:56: GlDemoScreen demoScreen_;
s/demoScreen_/demo_screen_/ ?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:109: eagl_context_ =
[[EAGLContext alloc] initWithAPI:kEAGLRenderingAPIOpenGLES2];
How does this work? Looks like you have created another context in
ExampleViewController...

[Sergey Ulanov, 2016-12-19 23:50:21]

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

[Sergey Ulanov, 2016-12-19 23:50:22]

Exciting stuff!

This CL looks too big. Is it possible to split it into multiple CLs? At very
least I think it should be possible to land chromiumoting parts separately.

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

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_draw...
remoting/client/gl_drawable.h:5: #ifndef REMOTING_CLIENT_OPENGL_GL_DRAWABLE_H_
remove _OPENGL_ part

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_draw...
remoting/client/gl_drawable.h:12: #include <memory>
nit: Don't need any of the includes above.

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

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:107: for
(std::vector<GlDrawable*>::const_iterator dii = drawables.begin();
use range loop with auto:
  for (auto& drawable : drawables)

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:128: for
(std::vector<GlDrawable*>::const_iterator dii = drawables.begin();
for (auto& drawable : drawables)

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:149: void GlRenderer::AddDrawable(GlDrawable* d)
{
s/d/drawable

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.cc:175: for
(std::vector<GlDrawable*>::const_iterator dii = drawables.begin();
for (auto& drawable : drawables)

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/chr...
File remoting/client/ios/chromiumoting/README.md (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/chr...
remoting/client/ios/chromiumoting/README.md:1: ### Chromiumoting
Can this be split into a separate CL? This CL is too big.

Also I suggest renaming it, so the name is meaningful without reading the docs,
e.g. client/ios/simple_client

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/hos...
File remoting/client/ios/host_preferences.h (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/hos...
remoting/client/ios/host_preferences.h:15: }
 // namespace ios

[commit-bot: I haz the power, 2016-12-19 23:51:46]

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

[commit-bot: I haz the power, 2016-12-19 23:51:46]

Dry run: This issue passed the CQ dry run.

[Yuwei, 2016-12-19 23:54:19]

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

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.h:95: void AddDrawable(GlDrawable* drawable);
On 2016/12/19 23:26:09, Yuwei wrote:
> I feel that it's clearer to provide a function like DrawDemoScreen()

I meant something like ShouldDrawDemoScreen(boolean). Not a function similar to
Draw()...

[joedow, 2016-12-19 23:56:25]

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

[joedow, 2016-12-19 23:56:26]

Can this CL be broken up?  It seems to be doing a lot.

Also, there is a bunch of commented out code w/o TODO or explanation.  This is
likely an artifact of porting the code over and getting it working, but I'm
concerned about missing things later when it is time to get it working.  Using
child branches and breaking the changes up might make the process easier.

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

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.h:131: std::vector<GlDrawable*> drawables;
s/drawables/drawables_

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.h:131: std::vector<GlDrawable*> drawables;
Does order of the drawing matter?  Maybe add a comment on whether it does or
not.

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/gl_rend...
remoting/client/gl_renderer.h:131: std::vector<GlDrawable*> drawables;
Is a raw* the right thing?  Can ownership be transferred or perhaps use a
WeakPtr here?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/app...
File remoting/client/ios/app_runtime.cc (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/app...
remoting/client/ios/app_runtime.cc:44: //  need to stop things?
Was this meant as a TODO?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/app...
File remoting/client/ios/app_runtime.h (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/app...
remoting/client/ios/app_runtime.h:42: //  }
Delete commented out code?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/chr...
File remoting/client/ios/chromiumoting/app_delegate.h (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/chr...
remoting/client/ios/chromiumoting/app_delegate.h:5: #ifndef
REMOTING_CLIENT_IOS_CHROMIUMOTING_APP_DELEGATE_H_
CHROMIUMOTING is a confusing term, can we use something more intuitive?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/cli...
File remoting/client/ios/client_gestures.mm (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/cli...
remoting/client/ios/client_gestures.mm:169: //       // }
Why is this code commented out?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_demo_screen.h (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_demo_screen.h:5: #ifndef
REMOTING_CLIENT_OPENGL_GL_DEMO_SCREEN_H_
The macro guard should match the dir structure:
REMOTING_CLIENT_IOS_DISPLAY_GL_DEMO_SCREEN_H_

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_demo_screen.h:25: 
Interface overrides are typically grouped together and commented (e.g. "//
GlDrawable interface.")

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:32: class Core :  // public
protocol::CursorShapeStub,
Should CursorShapeStub be commented out?  Delete if so...

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:82: return eagl_context_ !=
NULL;  // && egl_context_->IsWindowBound();
Is the commented out condition intentional?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:86: //
DCHECK(owned_frame_consumer_) << "The frame consumer is already grabbed.";
Why are all the DCHECKS commented out?

https://codereview.chromium.org/2555803002/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:102: //                       
   width, height));
Use NOTIMPLEMENTED() or NOTREACHED() instead of commenting out?

[nicholss, 2016-12-20 00:00:57]

Thanks for the comments all!

I am going to split this up into a couple CLs. I was doing integration and got
excited... yes this is doing a couple of things that can be independent but are
related. On the way..

[nicholss, 2016-12-21 22:46:29]

On 2016/12/20 00:00:57, nicholss wrote:
> Thanks for the comments all!
> 
> I am going to split this up into a couple CLs. I was doing integration and got
> excited... yes this is doing a couple of things that can be independent but
are
> related. On the way..

Here is just the gl changes to add Drawable. I also added a z index to the
drawable so now it does not matter when you add a component into the stack, it
will draw as you expect.

I also ported the current three layers to use Drawable and the draw stack inside
the renderer.

https://codereview.chromium.org/2591363002/

[nicholss, 2017-01-13 18:46:38]

Patchset #3 (id:40001) has been deleted

[nicholss, 2017-01-13 18:48:15]

Description was changed from

==========
Adjusting how gl_renderer draws layers and added a demo app for CRD iOS.

The intent of this CL is to begin introducing a demo app that represents the
open source components used by the CRD iOS app. Integrating CRD iOS with the new
OpenGL renderer. This will allow iOS and android to share a significant amount
of render code.  It also could be used to build a smoke test of the CRD iOS app
in Chromium with similar dependencies to the real app.

BUG=671692
==========

to

==========
Moving source from internal to chromium to provide a demo app for CRD iOS.

Integrating CRD iOS with the new OpenGL renderer. This will allow iOS and
android to share a significant amount of render code.  It also could be used to
build a smoke test of the CRD iOS app in Chromium with similar dependencies to
the real app.

BUG=671692
==========

[nicholss, 2017-01-13 18:49:15]

On 2016/12/21 22:46:29, nicholss wrote:
> On 2016/12/20 00:00:57, nicholss wrote:
> > Thanks for the comments all!
> > 
> > I am going to split this up into a couple CLs. I was doing integration and
got
> > excited... yes this is doing a couple of things that can be independent but
> are
> > related. On the way..
> 
> Here is just the gl changes to add Drawable. I also added a z index to the
> drawable so now it does not matter when you add a component into the stack, it
> will draw as you expect.
> 
> I also ported the current three layers to use Drawable and the draw stack
inside
> the renderer.
> 
> https://codereview.chromium.org/2591363002/

Please hold off looking at this CL, I am going to break it apart one more time
to reduce the scope.

[nicholss, 2017-01-19 17:14:43]

Patchset #4 (id:80001) has been deleted

[nicholss, 2017-01-19 17:16:28]

On 2017/01/13 18:49:15, nicholss wrote:
> On 2016/12/21 22:46:29, nicholss wrote:
> > On 2016/12/20 00:00:57, nicholss wrote:
> > > Thanks for the comments all!
> > > 
> > > I am going to split this up into a couple CLs. I was doing integration and
> got
> > > excited... yes this is doing a couple of things that can be independent
but
> > are
> > > related. On the way..
> > 
> > Here is just the gl changes to add Drawable. I also added a z index to the
> > drawable so now it does not matter when you add a component into the stack,
it
> > will draw as you expect.
> > 
> > I also ported the current three layers to use Drawable and the draw stack
> inside
> > the renderer.
> > 
> > https://codereview.chromium.org/2591363002/
> 
> Please hold off looking at this CL, I am going to break it apart one more time
> to reduce the scope.

Ok this CL is now only adding the demo app. All other CL's have landed in
chromium.

[nicholss, 2017-01-19 17:57:06]

PTAL, please note that this is not intended to be a production ready app or
code. This is a stepping stone I am using to migrate the chromium iOS app to
chromium. I am using it as a target that is build-able in chromium without any
other deps, meaning you can checkout chromium on a mac and build and run this,
no problem. So there is a lot of code that is commented out that will be
re-integrated later, and would like it to remain there at the moment.

As for the name, Chromoting = Chrome + Remoting. Chromiumoting = Chromium +
Remoting. Yep it is ugly, but as I said, this is a stepping stone. Please do not
force me to delete the commented out code or add UNIMPLEMENTED() in this demo,
it is a waste of effort. 

As this gets more added to it I will refactor the Chromiumoting project out and
it will be replaced with the real iOS app. But I need a a lot more CLs to get to
that point if you want bite sized pieces.

Thanks.

[joedow, 2017-01-19 18:56:46]

I am not comfortable with signing off on the chromiumoting name or the commented
out code.

My concern about code quality is that the initial check-in is the only way to
guarantee something is up to our code quality standards.

I do believe you intend to fix it and bring it up to the standards of the rest
of the code base, but the problem is that work is not guaranteed to happen.  You
could win the lottery tomorrow or be invited to work on a new super-shiny thing
and the code would never be updated, regardless of TODOs/Bugs/best intentions.

If these classes are stepping stones, I would suggest breaking the CL up a bit
more and include one class (or smallish group of related classes) in each CL
which are a bit more polished.

With respect to the chromiumoting name, I find it confusing and unintuitive,
just call it app or test app or simple app.

I'm interested in the opinions of the other reviewers, but I'd prefer to see
this broken up and/or updated to meet chromium code standards up front.

[nicholss, 2017-01-23 21:38:06]

On 2017/01/19 18:56:46, joedow wrote:
> I am not comfortable with signing off on the chromiumoting name or the
commented
> out code.
> 
> My concern about code quality is that the initial check-in is the only way to
> guarantee something is up to our code quality standards.
> 
> I do believe you intend to fix it and bring it up to the standards of the rest
> of the code base, but the problem is that work is not guaranteed to happen. 
You
> could win the lottery tomorrow or be invited to work on a new super-shiny
thing
> and the code would never be updated, regardless of TODOs/Bugs/best intentions.
> 
> If these classes are stepping stones, I would suggest breaking the CL up a bit
> more and include one class (or smallish group of related classes) in each CL
> which are a bit more polished.
> 
> With respect to the chromiumoting name, I find it confusing and unintuitive,
> just call it app or test app or simple app.
> 
> I'm interested in the opinions of the other reviewers, but I'd prefer to see
> this broken up and/or updated to meet chromium code standards up front.

I have removed the demo app and am just using a placeholder view controller now.
The plan is to replace that view controller with the real app as we start
migrating the app over to chromium. So now the app that build in chromium will
be the official app when development is done, no need to switch out a fake app
placeholder. Also moved out the few parts that are required for an app and moved
them into the app directory. 

The main point of this app is the gl_display_handler class and the remoting-ios
app.

PTAL, thanks

[nicholss, 2017-01-23 21:38:43]

Description was changed from

==========
Moving source from internal to chromium to provide a demo app for CRD iOS.

Integrating CRD iOS with the new OpenGL renderer. This will allow iOS and
android to share a significant amount of render code.  It also could be used to
build a smoke test of the CRD iOS app in Chromium with similar dependencies to
the real app.

BUG=671692
==========

to

==========
Moving source from internal to chromium to provide an app for CRD iOS.

Integrating CRD iOS with the new OpenGL renderer. This will allow iOS and
android to share a significant amount of render code.  It also could be used to
build a smoke test of the CRD iOS app in Chromium with similar dependencies to
the real app.

BUG=671692
==========

[Yuwei, 2017-01-23 21:56:16]

Some questions... Haven't carefully looked at the code yet.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
File remoting/client/ios/app/example_view_controller.mm (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.mm:33: _context = [[EAGLContext
alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
Are you establishing the EGL context on the main thread?

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.mm:33: _context = [[EAGLContext
alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
Does this handle the case if ES3 context is not supported? Otherwise I think you
should put a TODO

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
Why is this implemented in a .mm file? Does it use anything from core
foundation?

[joedow, 2017-01-25 23:16:29]

https://codereview.chromium.org/2555803002/diff/180001/remoting/branding_Chro...
File remoting/branding_Chromium (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/branding_Chro...
remoting/branding_Chromium:6: IOS_EXEC_NAME=chromoting
Should these mimic the names in the Chrome branding file?

If so, exec name should be 'ChromotingiOS' and the product name should be
'Chromoting for iOS'.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
File remoting/client/ios/app/example_view_controller.h (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.h:20: // The GLES3 context being
drawn too.
nit: s/too/to

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.h:23: GlDisplayHandler*
displayHandler;
displayHandler needs a leading underscore (_displayHandler).

For consistency, can you update the members with trailing underscores to use
leading?

Style guide is odd as it says to prefer leading underscores but says trailing
underscores are ok to match existing code.  In this CL I've seen both.  I'm
happy with either as long as it is consistent :)

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.h (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:8: #include <memory>
I don't think this header is used so you can remove it.  I don't see unique_ptr
used in this class.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:9: #include <vector>
AFAICT vector isn't used so you can remove it.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:15: #include
"third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
Remove the webrtc header?  I don't think it is used.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:32: base::WeakPtr<Canvas> canvas_ =
nullptr;
You shouldn't need to assign nullptr here, WeakPtr already does that in its
c'tor.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:33: int square_size_;
Init this to a default value here?

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/01/23 21:56:16, Yuwei wrote:
> Why is this implemented in a .mm file? Does it use anything from core
> foundation?

There is Chromium guidance that says to use the mm extension for objective c.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:10: #include
"base/memory/ptr_util.h"
Remove ptr_util.h?  I don't see it used here.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:31: const GLchar* a_position =
"a_position";
Encapsulating constants in an anonymous namespace would be good here (avoid
polluting the remoting namespace).

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:40: // Create and compile vertex
shader
All comments should end in a period (chromium style guide).

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:67: int height = 1024;
you could make width and height const or a constexpr at the top of the file

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
File remoting/client/ios/display/gl_display_handler.h (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.h:7: 
#include <memory> for unique_ptr usage

[nicholss, 2017-01-26 18:00:16]

Thanks for the review. Feedback addressed.

https://codereview.chromium.org/2555803002/diff/180001/remoting/branding_Chro...
File remoting/branding_Chromium (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/branding_Chro...
remoting/branding_Chromium:6: IOS_EXEC_NAME=chromoting
On 2017/01/25 23:16:28, joedow wrote:
> Should these mimic the names in the Chrome branding file?
> 
> If so, exec name should be 'ChromotingiOS' and the product name should be
> 'Chromoting for iOS'.
> 

These values are not final. I am not sure What they will end up as. Product name
here means the description that ends up the plist file, and I am not 100% sure
but I think it is not suppose to have spaces. So all of these values will be
updated as we get closer to shipping.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
File remoting/client/ios/app/example_view_controller.h (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.h:20: // The GLES3 context being
drawn too.
On 2017/01/25 23:16:28, joedow wrote:
> nit: s/too/to

Done.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.h:23: GlDisplayHandler*
displayHandler;
On 2017/01/25 23:16:28, joedow wrote:
> displayHandler needs a leading underscore (_displayHandler).
> 
> For consistency, can you update the members with trailing underscores to use
> leading?
> 
> Style guide is odd as it says to prefer leading underscores but says trailing
> underscores are ok to match existing code.  In this CL I've seen both.  I'm
> happy with either as long as it is consistent :)

This is really confusing to know which to use, infact bling has the same
problem. The reason for the trailing _ is the auto property magic addes a _ to
the front so _foo becomes __foo and foo_ becomes _foo_ 

Will fix to be consistent

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
File remoting/client/ios/app/example_view_controller.mm (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.mm:33: _context = [[EAGLContext
alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
On 2017/01/23 21:56:15, Yuwei wrote:
> Does this handle the case if ES3 context is not supported? Otherwise I think
you
> should put a TODO

ES3 is not supported for <=iPhone5. Not sure how many people still use this.
That device is 5 years old.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.h (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:8: #include <memory>
On 2017/01/25 23:16:28, joedow wrote:
> I don't think this header is used so you can remove it.  I don't see
unique_ptr
> used in this class.

Done.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:9: #include <vector>
On 2017/01/25 23:16:28, joedow wrote:
> AFAICT vector isn't used so you can remove it.

Done.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:15: #include
"third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
On 2017/01/25 23:16:28, joedow wrote:
> Remove the webrtc header?  I don't think it is used.

Done.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:32: base::WeakPtr<Canvas> canvas_ =
nullptr;
On 2017/01/25 23:16:28, joedow wrote:
> You shouldn't need to assign nullptr here, WeakPtr already does that in its
> c'tor.

Done.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.h:33: int square_size_;
On 2017/01/25 23:16:28, joedow wrote:
> Init this to a default value here?

Done.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:10: #include
"base/memory/ptr_util.h"
On 2017/01/25 23:16:28, joedow wrote:
> Remove ptr_util.h?  I don't see it used here.

Done.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:31: const GLchar* a_position =
"a_position";
On 2017/01/25 23:16:28, joedow wrote:
> Encapsulating constants in an anonymous namespace would be good here (avoid
> polluting the remoting namespace).

Done.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:40: // Create and compile vertex
shader
On 2017/01/25 23:16:28, joedow wrote:
> All comments should end in a period (chromium style guide).

Done. And added a short description for the class.

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:67: int height = 1024;
On 2017/01/25 23:16:28, joedow wrote:
> you could make width and height const or a constexpr at the top of the file

well... I am not so sure about this. These values might need to be dynamic in
the future based on the requested desktop size... because this is a demo app
(really for me to test integration with GL) I think I will leave this here and
as I work on the gestures this will become dynamic. 

Added a TODO to work on that when gestures get worked on.

[Jamie, 2017-01-26 18:06:12]

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
File remoting/client/ios/app/example_view_controller.mm (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.mm:33: _context = [[EAGLContext
alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
On 2017/01/26 18:00:16, nicholss wrote:
> On 2017/01/23 21:56:15, Yuwei wrote:
> > Does this handle the case if ES3 context is not supported? Otherwise I think
> you
> > should put a TODO
> 
> ES3 is not supported for <=iPhone5. Not sure how many people still use this.
> That device is 5 years old.

Drive-by: We might be able to get this information from the app store.

[nicholss, 2017-01-26 18:28:40]

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
File remoting/client/ios/app/example_view_controller.mm (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/ap...
remoting/client/ios/app/example_view_controller.mm:33: _context = [[EAGLContext
alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
On 2017/01/26 18:06:12, Jamie wrote:
> On 2017/01/26 18:00:16, nicholss wrote:
> > On 2017/01/23 21:56:15, Yuwei wrote:
> > > Does this handle the case if ES3 context is not supported? Otherwise I
think
> > you
> > > should put a TODO
> > 
> > ES3 is not supported for <=iPhone5. Not sure how many people still use this.
> > That device is 5 years old.
> 
> Drive-by: We might be able to get this information from the app store.

Looks like the iphone5 just makes the cut at continuing to be supported, will
add a TODO to fall back to ES2 if ES3 is not supported for prod code. This is
just example code so I am not going to update it for this CL.

[Yuwei, 2017-01-26 20:18:26]

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/180001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/01/25 23:16:28, joedow wrote:
> On 2017/01/23 21:56:16, Yuwei wrote:
> > Why is this implemented in a .mm file? Does it use anything from core
> > foundation?
> 
> There is Chromium guidance that says to use the mm extension for objective c.

What I meant is this file looks the same as a normal cc file. It seems weird to
me to give it mm extension just because it's part of an objective-c target.
Anyway if this is what the style guide says then I'm good with that.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:7: #import
<Foundation/Foundation.h>
Is core foundation used in this file?

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:43: canvas_ = canvas;
On Android, SetCanvas(null) will be called once the surface is destroyed. Please
clean up in that case.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:100: GLuint aPosition =
glGetAttribLocation(program_, a_position);
|a_position| just stores the name of the variable and |aPosition| is the actual
position. The name is kind of confusing to me... Maybe changing a_position to
position_var_name and changing aPosition to just position? "a" is just a prefix
for attribute in GLSL.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:113:
DCHECK(thread_checker_.CalledOnValidThread());
Maybe rename thread_checker_ to get_weak_ptr_thread_checker_? The class is
mainly used on the display thread but GetWeakPtr() is called on the main
thread...

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:90: // Nothing to do.
Use NOTIMPLEMENTED()?

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:101: eagl_context_ =
[EAGLContext currentContext];
Looks like you established the context on another thread (-(void)viewDidLoad)
then post a task to run Core::Created on the display thread (-(void)created).
How does the context move between thread, or it is not bound to any thread?

[joedow, 2017-01-26 21:30:23]

lgtm.  Looks much better after the cleanup, thanks!

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:63: return Drawable::DESKTOP + 1;
nit: This would be better cleaner if it were a constant defined at the top of
this file than well-known value + magic number inline (i.e. DEMO_DESKTOP =
DESKTOP + 1).

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:113:
DCHECK(thread_checker_.CalledOnValidThread());
On 2017/01/26 20:18:25, Yuwei wrote:
> Maybe rename thread_checker_ to get_weak_ptr_thread_checker_? The class is
> mainly used on the display thread but GetWeakPtr() is called on the main
> thread...

thread_checker_ is pretty standard in our codebase, it implies that the method
which references it is to be called on the same thread the c'tor and d'tor are
called on (or the thread the checker is re-attached to).  I'd prefer that the
name remain the same.

[Yuwei, 2017-01-26 21:33:53]

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:113:
DCHECK(thread_checker_.CalledOnValidThread());
On 2017/01/26 21:30:23, joedow wrote:
> On 2017/01/26 20:18:25, Yuwei wrote:
> > Maybe rename thread_checker_ to get_weak_ptr_thread_checker_? The class is
> > mainly used on the display thread but GetWeakPtr() is called on the main
> > thread...
> 
> thread_checker_ is pretty standard in our codebase, it implies that the method
> which references it is to be called on the same thread the c'tor and d'tor are
> called on (or the thread the checker is re-attached to).  I'd prefer that the
> name remain the same.

It implies the thread being checked is the thread that the class is mainly used
on, which is not true in this case and it just made it think that GetWeakPtr()
is called on the display thread...

[joedow, 2017-01-26 21:40:51]

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:113:
DCHECK(thread_checker_.CalledOnValidThread());
On 2017/01/26 21:33:53, Yuwei wrote:
> On 2017/01/26 21:30:23, joedow wrote:
> > On 2017/01/26 20:18:25, Yuwei wrote:
> > > Maybe rename thread_checker_ to get_weak_ptr_thread_checker_? The class is
> > > mainly used on the display thread but GetWeakPtr() is called on the main
> > > thread...
> > 
> > thread_checker_ is pretty standard in our codebase, it implies that the
method
> > which references it is to be called on the same thread the c'tor and d'tor
are
> > called on (or the thread the checker is re-attached to).  I'd prefer that
the
> > name remain the same.
> 
> It implies the thread being checked is the thread that the class is mainly
used
> on, which is not true in this case and it just made it think that GetWeakPtr()
> is called on the display thread...

ThreadChecker binds to the thread it is created on unless Detach...() is called.
 In this instance, thread_checker_ is validating that the WeakPtr is created on
the same thread (and weak_ptr_ is bound to the same thread) that the instance it
refers to was created on and will be destroyed on.  This method may not be
called on a display thread in some cases, say for testing.

[nicholss, 2017-01-26 22:27:46]

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

[commit-bot: I haz the power, 2017-01-26 22:28:32]

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

[nicholss, 2017-01-26 23:46:44]

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:7: #import
<Foundation/Foundation.h>
On 2017/01/26 20:18:25, Yuwei wrote:
> Is core foundation used in this file?

Done.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:43: canvas_ = canvas;
On 2017/01/26 20:18:25, Yuwei wrote:
> On Android, SetCanvas(null) will be called once the surface is destroyed.
Please
> clean up in that case.

WeakPtr deals with this case.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:100: GLuint aPosition =
glGetAttribLocation(program_, a_position);
On 2017/01/26 20:18:25, Yuwei wrote:
> |a_position| just stores the name of the variable and |aPosition| is the
actual
> position. The name is kind of confusing to me... Maybe changing a_position to
> position_var_name and changing aPosition to just position? "a" is just a
prefix
> for attribute in GLSL.

this is just a demo.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:90: // Nothing to do.
On 2017/01/26 20:18:26, Yuwei wrote:
> Use NOTIMPLEMENTED()?

It is implemented, it just does nothing. Perhaps I do not understand the usage
of NOTIMPLEMENTED(), I take it to mean the call to this method was unexpected
and has yet to be implemented. I don't have any work and I don't really care be
log calls to it.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:101: eagl_context_ =
[EAGLContext currentContext];
On 2017/01/26 20:18:26, Yuwei wrote:
> Looks like you established the context on another thread (-(void)viewDidLoad)
> then post a task to run Core::Created on the display thread (-(void)created).
> How does the context move between thread, or it is not bound to any thread?

iOS handles the GL context and moves it between threads for you.

[nicholss, 2017-01-26 23:54:16]

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

[Yuwei, 2017-01-26 23:55:18]

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:43: canvas_ = canvas;
On 2017/01/26 23:46:44, nicholss wrote:
> On 2017/01/26 20:18:25, Yuwei wrote:
> > On Android, SetCanvas(null) will be called once the surface is destroyed.
> Please
> > clean up in that case.
> 
> WeakPtr deals with this case.

When will the shaders and program below get destroyed?

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:90: // Nothing to do.
On 2017/01/26 23:46:44, nicholss wrote:
> On 2017/01/26 20:18:26, Yuwei wrote:
> > Use NOTIMPLEMENTED()?
> 
> It is implemented, it just does nothing. Perhaps I do not understand the usage
> of NOTIMPLEMENTED(), I take it to mean the call to this method was unexpected
> and has yet to be implemented. I don't have any work and I don't really care
be
> log calls to it.

Sorry that I have misunderstood this... Looks like this function will be called
by the renderer so you should use NOTIMPLEMENTED() here, otherwise it will
crash.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:101: eagl_context_ =
[EAGLContext currentContext];
On 2017/01/26 23:46:44, nicholss wrote:
> On 2017/01/26 20:18:26, Yuwei wrote:
> > Looks like you established the context on another thread
(-(void)viewDidLoad)
> > then post a task to run Core::Created on the display thread
(-(void)created).
> > How does the context move between thread, or it is not bound to any thread?
> 
> iOS handles the GL context and moves it between threads for you.

Hmm so there will only be context for each app? Anyway as long as it's thread
safe then it's fine.

[commit-bot: I haz the power, 2017-01-26 23:55:50]

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

[nicholss, 2017-01-27 00:51:05]

All comments addressed.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:43: canvas_ = canvas;
On 2017/01/26 23:55:18, Yuwei wrote:
> On 2017/01/26 23:46:44, nicholss wrote:
> > On 2017/01/26 20:18:25, Yuwei wrote:
> > > On Android, SetCanvas(null) will be called once the surface is destroyed.
> > Please
> > > clean up in that case.
> > 
> > WeakPtr deals with this case.
> 
> When will the shaders and program below get destroyed?

Not bothering with cleanup on a demo.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:90: // Nothing to do.
On 2017/01/26 23:55:18, Yuwei wrote:
> On 2017/01/26 23:46:44, nicholss wrote:
> > On 2017/01/26 20:18:26, Yuwei wrote:
> > > Use NOTIMPLEMENTED()?
> > 
> > It is implemented, it just does nothing. Perhaps I do not understand the
usage
> > of NOTIMPLEMENTED(), I take it to mean the call to this method was
unexpected
> > and has yet to be implemented. I don't have any work and I don't really care
> be
> > log calls to it.
> 
> Sorry that I have misunderstood this... Looks like this function will be
called
> by the renderer so you should use NOTIMPLEMENTED() here, otherwise it will
> crash.

No. NOTIMPLEMENTED() just logs an error to the logs as a warning that this was
not intended to be called. Is is not appropriate to use it here.

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:101: eagl_context_ =
[EAGLContext currentContext];
On 2017/01/26 23:55:18, Yuwei wrote:
> On 2017/01/26 23:46:44, nicholss wrote:
> > On 2017/01/26 20:18:26, Yuwei wrote:
> > > Looks like you established the context on another thread
> (-(void)viewDidLoad)
> > > then post a task to run Core::Created on the display thread
> (-(void)created).
> > > How does the context move between thread, or it is not bound to any
thread?
> > 
> > iOS handles the GL context and moves it between threads for you.
> 
> Hmm so there will only be context for each app? Anyway as long as it's thread
> safe then it's fine.

iOS does not care, you can have one  context that gets pulled around and with
careful coding it works out or you can have one per thread and the lib will make
sure it attempts to render things in the gpu correctly.

[Yuwei, 2017-01-27 01:11:48]

LGTM

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:43: canvas_ = canvas;
On 2017/01/27 00:51:05, nicholss wrote:
> On 2017/01/26 23:55:18, Yuwei wrote:
> > On 2017/01/26 23:46:44, nicholss wrote:
> > > On 2017/01/26 20:18:25, Yuwei wrote:
> > > > On Android, SetCanvas(null) will be called once the surface is
destroyed.
> > > Please
> > > > clean up in that case.
> > > 
> > > WeakPtr deals with this case.
> > 
> > When will the shaders and program below get destroyed?
> 
> Not bothering with cleanup on a demo.

I agree that a demo may not need to cleanup, but I guess it's still better to
say that in a comment?

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:90: // Nothing to do.
On 2017/01/27 00:51:05, nicholss wrote:
> On 2017/01/26 23:55:18, Yuwei wrote:
> > On 2017/01/26 23:46:44, nicholss wrote:
> > > On 2017/01/26 20:18:26, Yuwei wrote:
> > > > Use NOTIMPLEMENTED()?
> > > 
> > > It is implemented, it just does nothing. Perhaps I do not understand the
> usage
> > > of NOTIMPLEMENTED(), I take it to mean the call to this method was
> unexpected
> > > and has yet to be implemented. I don't have any work and I don't really
care
> > be
> > > log calls to it.
> > 
> > Sorry that I have misunderstood this... Looks like this function will be
> called
> > by the renderer so you should use NOTIMPLEMENTED() here, otherwise it will
> > crash.
> 
> No. NOTIMPLEMENTED() just logs an error to the logs as a warning that this was
> not intended to be called.

Looks like it depends on NOTIMPLEMENTED_POLICY

> Is is not appropriate to use it here.

Yep

https://codereview.chromium.org/2555803002/diff/220001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:101: eagl_context_ =
[EAGLContext currentContext];
On 2017/01/27 00:51:05, nicholss wrote:
> On 2017/01/26 23:55:18, Yuwei wrote:
> > On 2017/01/26 23:46:44, nicholss wrote:
> > > On 2017/01/26 20:18:26, Yuwei wrote:
> > > > Looks like you established the context on another thread
> > (-(void)viewDidLoad)
> > > > then post a task to run Core::Created on the display thread
> > (-(void)created).
> > > > How does the context move between thread, or it is not bound to any
> thread?
> > > 
> > > iOS handles the GL context and moves it between threads for you.
> > 
> > Hmm so there will only be context for each app? Anyway as long as it's
thread
> > safe then it's fine.
> 
> iOS does not care, you can have one  context that gets pulled around and with
> careful coding it works out or you can have one per thread and the lib will
make
> sure it attempts to render things in the gpu correctly.

Acknowledged.

[commit-bot: I haz the power, 2017-01-27 02:28:56]

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

[commit-bot: I haz the power, 2017-01-27 02:28:58]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-01-27 16:07:39]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-01-27 16:07:40]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2555803002/#ps240001
(title: "cleaning up includes.")

[commit-bot: I haz the power, 2017-01-27 16:08:06]

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

[commit-bot: I haz the power, 2017-01-27 16:14:14]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1485533259943420,
"parent_rev": "4c8c6cfe37c424387e26278371125cccbd16d5f3", "commit_rev":
"28f74dd4d3b78681833af7c4ac7fb6b2d8f64859"}

[commit-bot: I haz the power, 2017-01-27 16:14:46]

Description was changed from

==========
Moving source from internal to chromium to provide an app for CRD iOS.

Integrating CRD iOS with the new OpenGL renderer. This will allow iOS and
android to share a significant amount of render code.  It also could be used to
build a smoke test of the CRD iOS app in Chromium with similar dependencies to
the real app.

BUG=671692
==========

to

==========
Moving source from internal to chromium to provide an app for CRD iOS.

Integrating CRD iOS with the new OpenGL renderer. This will allow iOS and
android to share a significant amount of render code.  It also could be used to
build a smoke test of the CRD iOS app in Chromium with similar dependencies to
the real app.

BUG=671692

Review-Url: https://codereview.chromium.org/2555803002
Cr-Commit-Position: refs/heads/master@{#446679}
Committed:
https://chromium.googlesource.com/chromium/src/+/28f74dd4d3b78681833af7c4ac7f...
==========

[commit-bot: I haz the power, 2017-01-27 16:14:50]

Committed patchset #11 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/28f74dd4d3b78681833af7c4ac7f...

[Sergey Ulanov, 2017-01-27 19:47:44]

Sorry about belated comments.

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
File remoting/client/ios/display/gl_demo_screen.mm (right):

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:15: const GLfloat square[] =
{-1.0, -1.0, 1.0, -1.0, -1.0, 1.0, 1.0, 1.0};
This should be kSquare (or kSquareCoordinates).
Same for the consts below.

See https://google.github.io/styleguide/cppguide.html#Constant_Names

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:30: const GLchar* a_position =
"a_position";
kPositionAttributeName

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
remoting/client/ios/display/gl_demo_screen.mm:89: if (skip == square_size_) {
This whole statement can be written as follows.
 skip = (i % 2) * square_size_;

and then skip can be moved inside the loop.

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:34:
Core(remoting::ios::AppRuntime* runtime);
runtime_ is used only to get display_task_runner(), so maybe pass just the task
runner here?

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:48: // Will be std::move'd
when GrabFrameConsumer() is called.
Is this comment about owned_frame_consumer_? If so it should be moved one line
below.

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:121: @property(nonatomic)
remoting::GlDisplayHandler::Core* core_;
does this need to be a property instead of an interface field?

I'm not an expert in Obj C, so sorry if this is a stupid question.

https://codereview.chromium.org/2555803002/diff/240001/remoting/client/ios/di...
remoting/client/ios/display/gl_display_handler.mm:140:
self.core_->GetWeakPtr()));
Which thread is the core_ destroyed on? I assume it's supposed to be destroyed
on display_thread_, otherwise weak-pointer usage isn't safe. I don't see
anything that ensures that it's destroyed on the right thread. I think you need
to call display_task_runner()->DeleteSoon(core_) when GlDisplayHandler is being
destroyed. That would also ensure that Core is destroyed after all tasks that
were posted for it, so you won't need to use weak pointer here.