[Chromoting] Enable remoting webapp main.html GN build.

[Chromoting] Enable remoting webapp main.html GN build.

The previous remoting webapp GN build files were not enabled and at
least 6 months out of date. This cl fixes the problem by bringing
the build rules up to date and enables the webapp build.

It also starts to move all the remoting-specific build rules into
the remoting dir rather than having them in the main build.gn file.

BUG=

Committed: https://crrev.com/62ee8a9cfebbc8bdc037aaa3b37ebfbb45e02472
Cr-Commit-Position: refs/heads/master@{#325551}

[garykac, 2015-04-15 00:44:06]

garykac@chromium.org changed reviewers:
+ dpranke@chromium.org

[garykac, 2015-04-15 00:44:08]

https://codereview.chromium.org/1082083005/diff/1/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1082083005/diff/1/BUILD.gn#oldcode139
BUILD.gn:139: if (enable_remoting_host) {
Moving these into the remoting GN build files since it makes sense to keep all
that info in one place.

https://codereview.chromium.org/1082083005/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1082083005/diff/1/BUILD.gn#newcode104
BUILD.gn:104: "//remoting:all_r",
I attempted to use 'all' here (since duplicate names are supposed to be ok), but
ninja complained about 'all' being defined multiple times.

'all_r' is a placeholder, what naming conventions do you suggest for this?

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn
File remoting/BUILD.gn (right):

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn#newcode18
remoting/BUILD.gn:18: #"//remoting:remoting_base",
These comments parallel the GYP file. They will be removed as we convert
targets.

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn#newcode88
remoting/BUILD.gn:88: group("all_tests") {
Test targets needing to be in a separate testonly group differs from the way we
have our GYP files.

I'm probably not doing this the right way. Open to suggestions.

https://codereview.chromium.org/1082083005/diff/1/remoting/remoting_enable.gni
File remoting/remoting_enable.gni (right):

https://codereview.chromium.org/1082083005/diff/1/remoting/remoting_enable.gn...
remoting/remoting_enable.gni:12: if (is_win || is_mac) {
This is structured to parallel the GYPI code to make it easier for people to
keep in sync.

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/BUILD.gn
File remoting/webapp/BUILD.gn (left):

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/BUILD.gn#ol...
remoting/webapp/BUILD.gn:176: # generated file directory.
Fixed.

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni
File remoting/webapp/files.gni (right):

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni#n...
remoting/webapp/files.gni:50: "<@(remoting_webapp_browsertest_js_mock_files)",
This still has GYP strings in it where we haven't converted the targets to GN.

They are left here so that this file more closely parallels the GYP file. This
makes it easier for people to keep the 2 files in sync during the transition.

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni#n...
remoting/webapp/files.gni:197: "crd/js/gnubby_auth_handler.js"
The 'gn format' presubmit warning wants this to be:
remoting_webapp_js_gnubby_auth_files = [ "crd/js/gnubby_auth_handler.js" ]

but that's worse than having the filename here line up with all the other
filenames (since it makes it harder to quickly scan the filenames while looking
through this file).

I think this format warning should be removed since it's not always a
readability improvement.

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni#n...
remoting/webapp/files.gni:289: #+ remoting_webapp_browsertest_all_js_files
The gn formatter also gets confused by this comment block. It wants them all to
start at column 0.

[Dirk Pranke, 2015-04-15 01:08:40]

https://codereview.chromium.org/1082083005/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1082083005/diff/1/BUILD.gn#newcode104
BUILD.gn:104: "//remoting:all_r",
I'm not 100% convinced that we should move the files into a subdirectory. The
advantage to having everything defined in the top-level file is that you can
easily see what all is built in one place; you don't have to jump around to N
different files like you do in GYP.

However, remoting is complicated, and I can see the counterargument, so let's
take remoting as an experiment for now about creating roll-up targets in
subdirectories.

You should use '*_all' for a name, e.g. 'remoting_all'.

However, I would also like the structure of this target to mirror the structure
of the 'gn_all' target in build/gn_migration.gypi. Can you please update that as
well?

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn
File remoting/BUILD.gn (right):

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn#newcode18
remoting/BUILD.gn:18: #"//remoting:remoting_base",
On 2015/04/15 00:44:07, garykac wrote:
> These comments parallel the GYP file. They will be removed as we convert
> targets.

Generally I've tried to avoid having a roll-up target depend on intermediate
targets like libraries, and only depend on the "root" targets like executables.
(of course, if on some platforms you only build libraries, that's fine).

That said, I'm a bit confused; I thought that most of these targets have already
been ported?

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn#newcode88
remoting/BUILD.gn:88: group("all_tests") {
On 2015/04/15 00:44:07, garykac wrote:
> Test targets needing to be in a separate testonly group differs from the way
we
> have our GYP files.
> 
> I'm probably not doing this the right way. Open to suggestions.

Just mark the "all" target as a testonly target, and include these there (that's
what gn_all is). I would not have two separate group targets, myself.

https://codereview.chromium.org/1082083005/diff/1/remoting/remoting_enable.gni
File remoting/remoting_enable.gni (right):

https://codereview.chromium.org/1082083005/diff/1/remoting/remoting_enable.gn...
remoting/remoting_enable.gni:12: if (is_win || is_mac) {
On 2015/04/15 00:44:07, garykac wrote:
> This is structured to parallel the GYPI code to make it easier for people to
> keep in sync.

Yup, understood.

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni
File remoting/webapp/files.gni (right):

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni#n...
remoting/webapp/files.gni:197: "crd/js/gnubby_auth_handler.js"
On 2015/04/15 00:44:07, garykac wrote:
> The 'gn format' presubmit warning wants this to be:
> remoting_webapp_js_gnubby_auth_files = [ "crd/js/gnubby_auth_handler.js" ]
> 
> but that's worse than having the filename here line up with all the other
> filenames (since it makes it harder to quickly scan the filenames while
looking
> through this file).
> 
> I think this format warning should be removed since it's not always a
> readability improvement.

Please file a bug against 'gn format' for this, but don't bypass the check. It's
better to have all of the files formatted consistently (i.e., programmatically)
for now.

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni#n...
remoting/webapp/files.gni:289: #+ remoting_webapp_browsertest_all_js_files
On 2015/04/15 00:44:07, garykac wrote:
> The gn formatter also gets confused by this comment block. It wants them all
to
> start at column 0.

Odd. That definitely sounds like a bug.

[garykac, 2015-04-16 02:11:58]

https://codereview.chromium.org/1082083005/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1082083005/diff/1/BUILD.gn#newcode104
BUILD.gn:104: "//remoting:all_r",
On 2015/04/15 01:08:40, Dirk Pranke wrote:
> I'm not 100% convinced that we should move the files into a subdirectory. The
> advantage to having everything defined in the top-level file is that you can
> easily see what all is built in one place; you don't have to jump around to N
> different files like you do in GYP.
> 
> However, remoting is complicated, and I can see the counterargument, so let's
> take remoting as an experiment for now about creating roll-up targets in
> subdirectories.
> 
> You should use '*_all' for a name, e.g. 'remoting_all'.
> 
> However, I would also like the structure of this target to mirror the
structure
> of the 'gn_all' target in build/gn_migration.gypi. Can you please update that
as
> well?

Done.

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn
File remoting/BUILD.gn (right):

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn#newcode18
remoting/BUILD.gn:18: #"//remoting:remoting_base",
On 2015/04/15 01:08:40, Dirk Pranke wrote:
> On 2015/04/15 00:44:07, garykac wrote:
> > These comments parallel the GYP file. They will be removed as we convert
> > targets.
> 
> Generally I've tried to avoid having a roll-up target depend on intermediate
> targets like libraries, and only depend on the "root" targets like
executables.
> (of course, if on some platforms you only build libraries, that's fine).
> 
> That said, I'm a bit confused; I thought that most of these targets have
already
> been ported?

I was keeping it in sync with the .gyp file, where we have them explicitly
mentioned, but I went ahead and removed them here.

https://codereview.chromium.org/1082083005/diff/1/remoting/BUILD.gn#newcode88
remoting/BUILD.gn:88: group("all_tests") {
On 2015/04/15 01:08:40, Dirk Pranke wrote:
> On 2015/04/15 00:44:07, garykac wrote:
> > Test targets needing to be in a separate testonly group differs from the way
> we
> > have our GYP files.
> > 
> > I'm probably not doing this the right way. Open to suggestions.
> 
> Just mark the "all" target as a testonly target, and include these there
(that's
> what gn_all is). I would not have two separate group targets, myself.

Done.

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/BUILD.gn
File remoting/webapp/BUILD.gn (left):

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/BUILD.gn#ol...
remoting/webapp/BUILD.gn:176: # generated file directory.
On 2015/04/15 00:44:07, garykac wrote:
> Fixed.

These changes also fix a problem where main.html was not being generated (even
though the build was reporting success). I'll file a bug for that.

This bug is probably why remoting/resources:verify_resources was failing and
needed to be commented out.

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni
File remoting/webapp/files.gni (right):

https://codereview.chromium.org/1082083005/diff/1/remoting/webapp/files.gni#n...
remoting/webapp/files.gni:197: "crd/js/gnubby_auth_handler.js"
On 2015/04/15 01:08:40, Dirk Pranke wrote:
> On 2015/04/15 00:44:07, garykac wrote:
> > The 'gn format' presubmit warning wants this to be:
> > remoting_webapp_js_gnubby_auth_files = [ "crd/js/gnubby_auth_handler.js" ]
> > 
> > but that's worse than having the filename here line up with all the other
> > filenames (since it makes it harder to quickly scan the filenames while
> looking
> > through this file).
> > 
> > I think this format warning should be removed since it's not always a
> > readability improvement.
> 
> Please file a bug against 'gn format' for this, but don't bypass the check.
It's
> better to have all of the files formatted consistently (i.e.,
programmatically)
> for now. 

I assume in crbug? What are the proper tags to add to these bugs?

[Dirk Pranke, 2015-04-16 02:18:44]

On 2015/04/16 02:11:58, garykac wrote:
> > Please file a bug against 'gn format' for this, but don't bypass the check.
> It's
> > better to have all of the files formatted consistently (i.e.,
> programmatically)
> > for now. 
> 
> I assume in crbug? What are the proper tags to add to these bugs?

Yes, in crbug. Use the label 'GN'.

[garykac, 2015-04-16 20:47:49]

On 2015/04/16 02:18:44, Dirk Pranke wrote:
> On 2015/04/16 02:11:58, garykac wrote:
> > > Please file a bug against 'gn format' for this, but don't bypass the
check.
> > It's
> > > better to have all of the files formatted consistently (i.e.,
> > programmatically)
> > > for now. 
> > 
> > I assume in crbug? What are the proper tags to add to these bugs?
> 
> Yes, in crbug. Use the label 'GN'.

Any other comments on the cl?  I'm working on adding support for the webapp and
would like to build on this cl once I know it's structured in a reasonable way.

[Dirk Pranke, 2015-04-16 20:55:10]

sorry for the delay. this basically lgtm otherwise, though I'll have to take
your word for it as to whether the remoting part is actually right :). I can do
a closer look if you want/need me to.

https://codereview.chromium.org/1082083005/diff/20001/remoting/remoting_all.gyp
File remoting/remoting_all.gyp (right):

https://codereview.chromium.org/1082083005/diff/20001/remoting/remoting_all.g...
remoting/remoting_all.gyp:77: '../remoting/remoting.gyp:remoting_key_tester',
You might want to add a comment here on this depending on crbug.com/471924 to
work properly (we don't have PNaCl working yet).

https://codereview.chromium.org/1082083005/diff/20001/remoting/remoting_clien...
File remoting/remoting_client.gypi (right):

https://codereview.chromium.org/1082083005/diff/20001/remoting/remoting_clien...
remoting/remoting_client.gypi:52: # Keep in sync with remoting/webapp/BUILD.gn
By convention we list the actual target name, like so:

  "# GN: //remoting/webapp:html"

https://codereview.chromium.org/1082083005/diff/20001/remoting/webapp/BUILD.gn
File remoting/webapp/BUILD.gn (right):

https://codereview.chromium.org/1082083005/diff/20001/remoting/webapp/BUILD.g...
remoting/webapp/BUILD.gn:8: import("//remoting/webapp/files.gni")
You can avoid having to duplicate the list of files by using the
//build/gypi_to_gn.py script to read the gypi file and extract the various
variables as a dictionary.

See, for example //chrome/browser/BUILD.gn:24 (though we use this all over the
place).

But, you're free to duplicate the files if you'd prefer that.

[garykac, 2015-04-16 21:38:47]

https://codereview.chromium.org/1082083005/diff/20001/remoting/remoting_all.gyp
File remoting/remoting_all.gyp (right):

https://codereview.chromium.org/1082083005/diff/20001/remoting/remoting_all.g...
remoting/remoting_all.gyp:77: '../remoting/remoting.gyp:remoting_key_tester',
On 2015/04/16 20:55:09, Dirk Pranke wrote:
> You might want to add a comment here on this depending on crbug.com/471924 to
> work properly (we don't have PNaCl working yet).

Done.

https://codereview.chromium.org/1082083005/diff/20001/remoting/remoting_clien...
File remoting/remoting_client.gypi (right):

https://codereview.chromium.org/1082083005/diff/20001/remoting/remoting_clien...
remoting/remoting_client.gypi:52: # Keep in sync with remoting/webapp/BUILD.gn
On 2015/04/16 20:55:09, Dirk Pranke wrote:
> By convention we list the actual target name, like so:
> 
>   "# GN: //remoting/webapp:html"

Removed this for now since I'll be updating a bunch of these in the next cl.

https://codereview.chromium.org/1082083005/diff/20001/remoting/webapp/BUILD.gn
File remoting/webapp/BUILD.gn (right):

https://codereview.chromium.org/1082083005/diff/20001/remoting/webapp/BUILD.g...
remoting/webapp/BUILD.gn:8: import("//remoting/webapp/files.gni")
On 2015/04/16 20:55:09, Dirk Pranke wrote:
> You can avoid having to duplicate the list of files by using the
> //build/gypi_to_gn.py script to read the gypi file and extract the various
> variables as a dictionary.
> 
> See, for example //chrome/browser/BUILD.gn:24 (though we use this all over the
> place).
> 
> But, you're free to duplicate the files if you'd prefer that.

Acknowledged.

[garykac, 2015-04-16 22:19:13]

The CQ bit was checked by garykac@chromium.org

[garykac, 2015-04-16 22:19:13]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1082083005/#ps80001
(title: "Add PNaCl bug ref to GN file.")

[commit-bot: I haz the power, 2015-04-16 22:25:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082083005/80001

[commit-bot: I haz the power, 2015-04-16 23:30:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-16 23:32:12]

Patchset 5 (id:??) landed as
https://crrev.com/62ee8a9cfebbc8bdc037aaa3b37ebfbb45e02472
Cr-Commit-Position: refs/heads/master@{#325551}

[maniscalco, 2015-04-16 23:54:16]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1090023004/ by maniscalco@chromium.org.

The reason for reverting is: Suspected of breaking the build.  For details, see
https://code.google.com/p/chromium/issues/detail?id=477869.