Moving the GL implementation details into a sub folder for client display.

Moving the GL implementation details into a sub folder for client display.

This change with allow me to cleanly add interfaces to the GL implementations.
The intent is to add interfaces to the different components, but these get lost
in the flat client/ directory. Moving display related files to their own location.

R=yuweih@chromium.org
BUG=671692
Review-Url: https://codereview.chromium.org/2614443003
Cr-Commit-Position: refs/heads/master@{#441727}
Committed: https://chromium.googlesource.com/chromium/src/+/9949f5bc318ed45aeadb657d2be743785fe830c7

[nicholss, 2017-01-03 18:22:58]

This does not change any code. Only file moving and path updating for the gl
files and the places they are used.

[Yuwei, 2017-01-03 20:08:47]

On 2017/01/03 18:22:58, nicholss wrote:
> This does not change any code. Only file moving and path updating for the gl
> files and the places they are used.

I think it was Sergey's suggestion to keep these gl* files directly in /client
(https://codereview.chromium.org/2045963004/). Please double check this change
with him O_o

[nicholss, 2017-01-03 20:24:11]

Description was changed from

==========
Moving the GL implementation details into a sub folder for client display.

This change with allow me to cleanly add interfaces to the GL implementations.
The intent is to add interfaces to the different components, but these get lost
in the flat client/ directory. Moving display related files to their own
location.

R=yuweih@chromium.org
BUG=671692
==========

to

==========
Moving the GL implementation details into a sub folder for client display.

This change with allow me to cleanly add interfaces to the GL implementations.
The intent is to add interfaces to the different components, but these get lost
in the flat client/ directory. Moving display related files to their own
location.

R=yuweih@chromium.org
BUG=671692
==========

[nicholss, 2017-01-03 20:24:11]

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

[nicholss, 2017-01-03 20:26:06]

On 2017/01/03 20:08:47, Yuwei wrote:
> On 2017/01/03 18:22:58, nicholss wrote:
> > This does not change any code. Only file moving and path updating for the gl
> > files and the places they are used.
> 
> I think it was Sergey's suggestion to keep these gl* files directly in /client
> (https://codereview.chromium.org/2045963004/). Please double check this change
> with him O_o

At the time there were fewer files and no interfaces. I think it is now (or will
be after https://codereview.chromium.org/2591363002) complex enough to warrant a
subdirectory for clarity.

Do you agree Sergey?

[nicholss, 2017-01-03 23:14:35]

Note the format changes via `git cl format`

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

https://codereview.chromium.org/2614443003/diff/20001/remoting/client/display...
remoting/client/display/gl_renderer.cc:22: GlRenderer::~GlRenderer() {}
This change leaked into this CL, this was not intended.

[Sergey Ulanov, 2017-01-04 00:58:46]

On 2017/01/03 20:26:06, nicholss wrote:
> On 2017/01/03 20:08:47, Yuwei wrote:
> > On 2017/01/03 18:22:58, nicholss wrote:
> > > This does not change any code. Only file moving and path updating for the
gl
> > > files and the places they are used.
> > 
> > I think it was Sergey's suggestion to keep these gl* files directly in
/client
> > (https://codereview.chromium.org/2045963004/). Please double check this
change
> > with him O_o
> 
> At the time there were fewer files and no interfaces. I think it is now (or
will
> be after https://codereview.chromium.org/2591363002) complex enough to warrant
a
> subdirectory for clarity.
> 
> Do you agree Sergey?

In the past we were normally creating directories in two cases:
 1. Platform-specific code (e.g. host/linux, host/windows).
 2. Logical components that are compiled separately (e.g. remoting/host and
remoting/client are compiled into separate targets, remoting/protocol is shared
between them).

Also, rendering is an essential part of the client and there is not a lot of
code beside the renderer in the client. The OpenGL renderer is not used in the
NaCl plugin, but we won't need the plugin long-term. Once the plugin is gone all
clients will use the same rendering code, i.e. it will be possible to merge
opengl_renderer build target with the client target. So I don't see much value
in reorganizing it now. 
Having said that, I don't feel very strongly about it - you can sway me in the
other direction if you think we really need a separate dir for the renderer.

[nicholss, 2017-01-04 16:12:11]

On 2017/01/04 00:58:46, Sergey Ulanov wrote:
> On 2017/01/03 20:26:06, nicholss wrote:
> > On 2017/01/03 20:08:47, Yuwei wrote:
> > > On 2017/01/03 18:22:58, nicholss wrote:
> > > > This does not change any code. Only file moving and path updating for
the
> gl
> > > > files and the places they are used.
> > > 
> > > I think it was Sergey's suggestion to keep these gl* files directly in
> /client
> > > (https://codereview.chromium.org/2045963004/). Please double check this
> change
> > > with him O_o
> > 
> > At the time there were fewer files and no interfaces. I think it is now (or
> will
> > be after https://codereview.chromium.org/2591363002) complex enough to
warrant
> a
> > subdirectory for clarity.
> > 
> > Do you agree Sergey?
> 
> In the past we were normally creating directories in two cases:
>  1. Platform-specific code (e.g. host/linux, host/windows).
>  2. Logical components that are compiled separately (e.g. remoting/host and
> remoting/client are compiled into separate targets, remoting/protocol is
shared
> between them).
> 
> Also, rendering is an essential part of the client and there is not a lot of
> code beside the renderer in the client. The OpenGL renderer is not used in the
> NaCl plugin, but we won't need the plugin long-term. Once the plugin is gone
all
> clients will use the same rendering code, i.e. it will be possible to merge
> opengl_renderer build target with the client target. So I don't see much value
> in reorganizing it now. 
> Having said that, I don't feel very strongly about it - you can sway me in the
> other direction if you think we really need a separate dir for the renderer.

My aim is to make the rendering layer a library that has easily understood edges
of
responsibility to allow me to easily add it to the iOS app without bringing in
dependencies
I do not want, and more importantly to add abstract classes to the rendering
code to 
able to test them and use a dependency style of coding to achieve that.

The flat organization style used in this codebase has given me a large amount of
grief while working on the build system, there not enough separation of concern
and
too building blocks that make up our components. Too many things are bundled 
together forcing me to remove sources that I do not want or adding deps that are

not appropriate for the project I am working on. Yes a client is the only user
of 
the rendering layer but as it is written, it is difficult to pull it out and
make a quick app
or test without shimming all of the trickle down object creation.

The way I like to think about coding is the connection of fundamental blocks
connected
to make a larger component. The client is the [protocol] + [input
interpretation] + 
[display rendering] + [UI Wrapper] + [connection state management].

Finally, Because I am adding interfaces, I am calling them very generic things
as interfaces
are intended. But they need the context of their location (or namespace if we
want to go that
way) to make complete sense. Personally I like the context of a location to help
me
understand the connection between the classes. But I am coming from a heavy java

background and this type of isolation is common and encouraged to keep code
clean.

[Yuwei, 2017-01-04 20:37:33]

LGTM if no one disagrees with the decision of moving these files.

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

https://codereview.chromium.org/2614443003/diff/40001/remoting/client/display...
remoting/client/display/BUILD.gn:76: if (is_win) {
It seems kind of weird to have a target with no source files if it's on Windows.
Alternatively you can try only depend on this in client:unit_tests if it is not
Windows.

[nicholss, 2017-01-04 20:51:25]

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

https://codereview.chromium.org/2614443003/diff/40001/remoting/client/display...
remoting/client/display/BUILD.gn:76: if (is_win) {
On 2017/01/04 20:37:33, Yuwei wrote:
> It seems kind of weird to have a target with no source files if it's on
Windows.
> Alternatively you can try only depend on this in client:unit_tests if it is
not
> Windows.

It really does not matter. The easiest solution is to have windows dep on an
empty target without logic on the deps side and if that changes then no changes
are required to undo that special logic.

[nicholss, 2017-01-04 23:02:05]

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

[commit-bot: I haz the power, 2017-01-04 23:02:45]

Dry run: 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-04 23:34:02]

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

[commit-bot: I haz the power, 2017-01-04 23:34:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nicholss, 2017-01-04 23:44:27]

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

[commit-bot: I haz the power, 2017-01-04 23:44:52]

Dry run: 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-05 00:42:28]

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

[commit-bot: I haz the power, 2017-01-05 00:42:29]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-01-05 01:33:23]

Can you also add DEPS file in the new directory with "-remoting/client" to
ensure that the code in client/display doesn't have unexpected dependencies?
That would the new component is easier to reuse.
otherwise LGTM.

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

https://codereview.chromium.org/2614443003/diff/60001/remoting/client/display...
remoting/client/display/BUILD.gn:69: "//remoting/client",
It looks wrong for remoting/client/display to depend on remoting/client. Can we
avoid this dependency? I would expect client to depend on client/display.

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

https://codereview.chromium.org/2614443003/diff/60001/remoting/client/display...
remoting/client/display/gl_renderer.cc:13: #include
"remoting/client/sys_opengl.h"
move sys_opengl.h to display directory too?

[nicholss, 2017-01-05 17:41:04]

On 2017/01/05 01:33:23, Sergey Ulanov wrote:
> Can you also add DEPS file in the new directory with "-remoting/client" to
> ensure that the code in client/display doesn't have unexpected dependencies?
> That would the new component is easier to reuse.
> otherwise LGTM.
> 
>
https://codereview.chromium.org/2614443003/diff/60001/remoting/client/display...
> File remoting/client/display/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2614443003/diff/60001/remoting/client/display...
> remoting/client/display/BUILD.gn:69: "//remoting/client",
> It looks wrong for remoting/client/display to depend on remoting/client. Can
we
> avoid this dependency? I would expect client to depend on client/display.
> 
>
https://codereview.chromium.org/2614443003/diff/60001/remoting/client/display...
> File remoting/client/display/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/2614443003/diff/60001/remoting/client/display...
> remoting/client/display/gl_renderer.cc:13: #include
> "remoting/client/sys_opengl.h"
> move sys_opengl.h to display directory too?

Good points. All addressed in next patch. Thanks for the review. I will run the
try bots one
more time and if that is green I will commit

[nicholss, 2017-01-05 17:45:55]

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

[commit-bot: I haz the power, 2017-01-05 17:46:26]

Dry run: 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-05 19:01:32]

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

[commit-bot: I haz the power, 2017-01-05 19:01:33]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-01-05 19:01:42]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-01-05 19:01:42]

The patchset sent to the CQ was uploaded after l-g-t-m from yuweih@chromium.org,
sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2614443003/#ps80001
(title: "Adding deps restriction on r/c/display; Moving sys_opengl.h")

[commit-bot: I haz the power, 2017-01-05 19:02:41]

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-05 19:29:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483642902112120,
"parent_rev": "5726c9905a4ae79e5d2f56a6254477266ddd9d82", "commit_rev":
"9949f5bc318ed45aeadb657d2be743785fe830c7"}

[commit-bot: I haz the power, 2017-01-05 19:29:51]

Description was changed from

==========
Moving the GL implementation details into a sub folder for client display.

This change with allow me to cleanly add interfaces to the GL implementations.
The intent is to add interfaces to the different components, but these get lost
in the flat client/ directory. Moving display related files to their own
location.

R=yuweih@chromium.org
BUG=671692
==========

to

==========
Moving the GL implementation details into a sub folder for client display.

This change with allow me to cleanly add interfaces to the GL implementations.
The intent is to add interfaces to the different components, but these get lost
in the flat client/ directory. Moving display related files to their own
location.

R=yuweih@chromium.org
BUG=671692

Review-Url: https://codereview.chromium.org/2614443003
Cr-Commit-Position: refs/heads/master@{#441727}
Committed:
https://chromium.googlesource.com/chromium/src/+/9949f5bc318ed45aeadb657d2be7...
==========

[commit-bot: I haz the power, 2017-01-05 19:29:52]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9949f5bc318ed45aeadb657d2be7...