[iOS][GN] Port ios_web_unittests to build with gn

[iOS][GN] Port ios_web_unittests to build with gn

Add template to run a java command as an "script", i.e. templates
java_action/java_action_foreach similar to action/action_foreach.

Add template to compile a bunch of JavaScript to a bundle and to
compile JavaScript files with closure compile (only enable checks
that are known to work).

Fix gcdwebserver public configuration to add the dependency on libz
to libs instead of using ldflags.

Add files missing from //ui/base:test_support when building for iOS
with gn (they are present in the gyp build).

BUG=459705
Committed: https://crrev.com/aa7c1cf96274a0a531629f934bd9091a55439270
Cr-Commit-Position: refs/heads/master@{#355460}

[sdefresne, 2015-10-08 15:53:58]

sdefresne@chromium.org changed reviewers:
+ dpranke@chromium.org, eugenebut@chromium.org

[sdefresne, 2015-10-08 15:53:58]

eugenebut: please review as OWNERS
dpranke: please review gn files

BTW, proof that this works!

$ gn gen out/Default-iphoneos
Done. Wrote 762 targets from 212 files in 1964ms
$ ninja -C out/Default-iphoneos ios_web_unittests
ninja: Entering directory `out/Default-iphoneos'
[1120/2730] LIBTOOL-STATIC obj/third_party/harfbuzz-ng/libharfbuzz-ng.a
/Applications/Xcode7.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool:
for architecture: armv7 file:
obj/third_party/harfbuzz-ng/libharfbuzz-ng/hb-warning.o has no symbols
[2730/2730] STAMP obj/ios/web/ios_web_unittests.stamp

I've checked the generated .js files and they are identical between gn and gyp
build.

[Eugene But (OOO till 7-30), 2015-10-08 16:10:41]

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode19
ios/web/BUILD.gn:19: # supports iOS 7.0 so need to use weak linking.
Do we need a bug and TODO to update this, once iOS 7 support is dropped?

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode28
ios/web/BUILD.gn:28: ":core",
Shouldn't "//" dependencies go before ":"?

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py
File ios/web/closure_compiler.py (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py...
ios/web/closure_compiler.py:6: def __main__():
Need 2 blank lines before top level def

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py...
ios/web/closure_compiler.py:6: def __main__():
Optional NIT: s/__main__/main ? Otherwise the name is not valid according to
Python Style Guide.

https://codereview.chromium.org/1393303003/diff/1/ios/web/js_compile.gni
File ios/web/js_compile.gni (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/js_compile.gni#newc...
ios/web/js_compile.gni:83: #     If false or undefined, the JavaScript files
will not be compiled but
I don't think that this comment is accurate. if invoker.compile, then source
code will be compiled (unless I misunderstood gn rules)

https://codereview.chromium.org/1393303003/diff/1/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/1393303003/diff/1/ui/base/BUILD.gn#newcode607
ui/base/BUILD.gn:607: } else {
Optional NIT: Maybe flip if-else blocks? Positive statements ("if (is_ios)")
tend to be more readable.

[Dirk Pranke, 2015-10-08 17:49:45]

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode28
ios/web/BUILD.gn:28: ":core",
On 2015/10/08 16:10:41, eugenebut wrote:
> Shouldn't "//" dependencies go before ":"?

Nope. List in-file dependencies first. `gn format` takes care of this for you.

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py
File ios/web/closure_compiler.py (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py...
ios/web/closure_compiler.py:6: def __main__():
On 2015/10/08 16:10:41, eugenebut wrote:
> Optional NIT: s/__main__/main ? Otherwise the name is not valid according to
> Python Style Guide.

I wouldn't call this change optional. Please change it :).

Do we really not already have some other script that does this? I would've
thought we had something for android, at least?

https://codereview.chromium.org/1393303003/diff/1/ios/web/js_compile.gni
File ios/web/js_compile.gni (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/js_compile.gni#newc...
ios/web/js_compile.gni:83: #     If false or undefined, the JavaScript files
will not be compiled but
On 2015/10/08 16:10:41, eugenebut wrote:
> I don't think that this comment is accurate. if invoker.compile, then source
> code will be compiled (unless I misunderstood gn rules)

Why even have the 'copy' part of this template? Why not just have a separate
copy target for that case?

[Eugene But (OOO till 7-30), 2015-10-08 18:04:11]

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode28
ios/web/BUILD.gn:28: ":core",
On 2015/10/08 17:49:45, Dirk Pranke wrote:
> On 2015/10/08 16:10:41, eugenebut wrote:
> > Shouldn't "//" dependencies go before ":"?
> 
> Nope. List in-file dependencies first. `gn format` takes care of this for you.
Then I guess it should be consistent with "test_support" deps list. Or not?

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py
File ios/web/closure_compiler.py (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py...
ios/web/closure_compiler.py:6: def __main__():
On 2015/10/08 17:49:45, Dirk Pranke wrote:
> On 2015/10/08 16:10:41, eugenebut wrote:
> > Optional NIT: s/__main__/main ? Otherwise the name is not valid according to
> > Python Style Guide.
> 
> I wouldn't call this change optional. Please change it :).
> 
> Do we really not already have some other script that does this? I would've
> thought we had something for android, at least?
I use gpylint for this.

[Dirk Pranke, 2015-10-08 18:22:29]

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode28
ios/web/BUILD.gn:28: ":core",
On 2015/10/08 18:04:10, eugenebut wrote:
> On 2015/10/08 17:49:45, Dirk Pranke wrote:
> > On 2015/10/08 16:10:41, eugenebut wrote:
> > > Shouldn't "//" dependencies go before ":"?
> > 
> > Nope. List in-file dependencies first. `gn format` takes care of this for
you.
> Then I guess it should be consistent with "test_support" deps list. Or not?

Yes, it should be. Did this get run through gn format?

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py
File ios/web/closure_compiler.py (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py...
ios/web/closure_compiler.py:6: def __main__():
On 2015/10/08 18:04:11, eugenebut wrote:
> On 2015/10/08 17:49:45, Dirk Pranke wrote:
> > On 2015/10/08 16:10:41, eugenebut wrote:
> > > Optional NIT: s/__main__/main ? Otherwise the name is not valid according
to
> > > Python Style Guide.
> > 
> > I wouldn't call this change optional. Please change it :).
> > 
> > Do we really not already have some other script that does this? I would've
> > thought we had something for android, at least?
> I use gpylint for this.

I meant, don't we have some other script that calls out to create a jar?

[sdefresne, 2015-10-09 13:54:32]

Patchset #2 (id:20001) has been deleted

[sdefresne, 2015-10-09 13:54:45]

Please take another look.

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode19
ios/web/BUILD.gn:19: # supports iOS 7.0 so need to use weak linking.
On 2015/10/08 at 16:10:41, eugenebut wrote:
> Do we need a bug and TODO to update this, once iOS 7 support is dropped?

Done.

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode28
ios/web/BUILD.gn:28: ":core",
On 2015/10/08 at 18:22:28, Dirk Pranke wrote:
> On 2015/10/08 18:04:10, eugenebut wrote:
> > On 2015/10/08 17:49:45, Dirk Pranke wrote:
> > > On 2015/10/08 16:10:41, eugenebut wrote:
> > > > Shouldn't "//" dependencies go before ":"?
> > > 
> > > Nope. List in-file dependencies first. `gn format` takes care of this for
you.
> > Then I guess it should be consistent with "test_support" deps list. Or not?
> 
> Yes, it should be. Did this get run through gn format?

Yes, and there is no changes:
$ gn format --in-place ios/web/BUILD.gn

Should I file a bug against "gn format"?

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py
File ios/web/closure_compiler.py (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py...
ios/web/closure_compiler.py:6: def __main__():
On 2015/10/08 at 18:22:29, Dirk Pranke wrote:
> On 2015/10/08 18:04:11, eugenebut wrote:
> > On 2015/10/08 17:49:45, Dirk Pranke wrote:
> > > On 2015/10/08 16:10:41, eugenebut wrote:
> > > > Optional NIT: s/__main__/main ? Otherwise the name is not valid
according to
> > > > Python Style Guide.
> > > 
> > > I wouldn't call this change optional. Please change it :).
> > > 
> > > Do we really not already have some other script that does this? I would've
> > > thought we had something for android, at least?
> > I use gpylint for this.
> 
> I meant, don't we have some other script that calls out to create a jar?

I've looked around but could find nothing. There is a script
third_party/closure_compiler/compile.py but it adds flags that we do not want on
iOS and we cannot use it directly. Instead I've factored out the code the run
"java_action" and a wrapper python script and moved this to
//build/util/java_action.{gni,py}.

WDYT?

https://codereview.chromium.org/1393303003/diff/1/ios/web/js_compile.gni
File ios/web/js_compile.gni (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/js_compile.gni#newc...
ios/web/js_compile.gni:83: #     If false or undefined, the JavaScript files
will not be compiled but
On 2015/10/08 at 17:49:45, Dirk Pranke wrote:
> On 2015/10/08 16:10:41, eugenebut wrote:
> > I don't think that this comment is accurate. if invoker.compile, then source
> > code will be compiled (unless I misunderstood gn rules)

Removed the comment as I've changed the value to a global configurable by the
user.

> Why even have the 'copy' part of this template? Why not just have a separate
copy target for that case?

My plan was to stay as close as the gyp rule as possible. The caller could just
pass the value of a global (as is done with gyp) and do not have to care how to
implement the "compile" flag (there's only one caller currently, so not really
necessary, but if/when we add more, it will be good to be able to have a central
place responsible for this flag instead of multiple location).

[Eugene But (OOO till 7-30), 2015-10-09 15:52:36]

lgtm

https://codereview.chromium.org/1393303003/diff/40001/build/util/java_action.py
File build/util/java_action.py (right):

https://codereview.chromium.org/1393303003/diff/40001/build/util/java_action....
build/util/java_action.py:28: """Lookup for |command| in PATH.
NIT: s/Lookup/Looks up

[Dirk Pranke, 2015-10-09 19:27:04]

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode28
ios/web/BUILD.gn:28: ":core",
On 2015/10/09 13:54:45, sdefresne (OOO till 19th Oct.) wrote:
> On 2015/10/08 at 18:22:28, Dirk Pranke wrote:
> > On 2015/10/08 18:04:10, eugenebut wrote:
> > > On 2015/10/08 17:49:45, Dirk Pranke wrote:
> > > > On 2015/10/08 16:10:41, eugenebut wrote:
> > > > > Shouldn't "//" dependencies go before ":"?
> > > > 
> > > > Nope. List in-file dependencies first. `gn format` takes care of this
for
> you.
> > > Then I guess it should be consistent with "test_support" deps list. Or
not?
> > 
> > Yes, it should be. Did this get run through gn format?
> 
> Yes, and there is no changes:
> $ gn format --in-place ios/web/BUILD.gn
> 
> Should I file a bug against "gn format"?

Yes, please do so.

[Dirk Pranke, 2015-10-09 20:02:59]

On 2015/10/09 13:54:45, sdefresne (OOO till 19th Oct.) wrote:
> Please take another look.
> 
> https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn
> File ios/web/BUILD.gn (right):
> 
> https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode19
> ios/web/BUILD.gn:19: # supports iOS 7.0 so need to use weak linking.
> On 2015/10/08 at 16:10:41, eugenebut wrote:
> > Do we need a bug and TODO to update this, once iOS 7 support is dropped?
> 
> Done.
> 
> https://codereview.chromium.org/1393303003/diff/1/ios/web/BUILD.gn#newcode28
> ios/web/BUILD.gn:28: ":core",
> On 2015/10/08 at 18:22:28, Dirk Pranke wrote:
> > On 2015/10/08 18:04:10, eugenebut wrote:
> > > On 2015/10/08 17:49:45, Dirk Pranke wrote:
> > > > On 2015/10/08 16:10:41, eugenebut wrote:
> > > > > Shouldn't "//" dependencies go before ":"?
> > > > 
> > > > Nope. List in-file dependencies first. `gn format` takes care of this
for
> you.
> > > Then I guess it should be consistent with "test_support" deps list. Or
not?
> > 
> > Yes, it should be. Did this get run through gn format?
> 
> Yes, and there is no changes:
> $ gn format --in-place ios/web/BUILD.gn
> 
> Should I file a bug against "gn format"?
> 
> https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py
> File ios/web/closure_compiler.py (right):
> 
>
https://codereview.chromium.org/1393303003/diff/1/ios/web/closure_compiler.py...
> ios/web/closure_compiler.py:6: def __main__():
> On 2015/10/08 at 18:22:29, Dirk Pranke wrote:
> > On 2015/10/08 18:04:11, eugenebut wrote:
> > > On 2015/10/08 17:49:45, Dirk Pranke wrote:
> > > > On 2015/10/08 16:10:41, eugenebut wrote:
> > > > > Optional NIT: s/__main__/main ? Otherwise the name is not valid
> according to
> > > > > Python Style Guide.
> > > > 
> > > > I wouldn't call this change optional. Please change it :).
> > > > 
> > > > Do we really not already have some other script that does this? I
would've
> > > > thought we had something for android, at least?
> > > I use gpylint for this.
> > 
> > I meant, don't we have some other script that calls out to create a jar?
> 
> I've looked around but could find nothing. There is a script
> third_party/closure_compiler/compile.py but it adds flags that we do not want
on
> iOS and we cannot use it directly. Instead I've factored out the code the run
> "java_action" and a wrapper python script and moved this to
> //build/util/java_action.{gni,py}.
> 
> WDYT?
> 
> https://codereview.chromium.org/1393303003/diff/1/ios/web/js_compile.gni
> File ios/web/js_compile.gni (right):
> 
>
https://codereview.chromium.org/1393303003/diff/1/ios/web/js_compile.gni#newc...
> ios/web/js_compile.gni:83: #     If false or undefined, the JavaScript files
> will not be compiled but
> On 2015/10/08 at 17:49:45, Dirk Pranke wrote:
> > On 2015/10/08 16:10:41, eugenebut wrote:
> > > I don't think that this comment is accurate. if invoker.compile, then
source
> > > code will be compiled (unless I misunderstood gn rules)
> 
> Removed the comment as I've changed the value to a global configurable by the
> user.
> 
> > Why even have the 'copy' part of this template? Why not just have a separate
> copy target for that case?
> 
> My plan was to stay as close as the gyp rule as possible. The caller could
just
> pass the value of a global (as is done with gyp) and do not have to care how
to
> implement the "compile" flag (there's only one caller currently, so not really
> necessary, but if/when we add more, it will be good to be able to have a
central
> place responsible for this flag instead of multiple location).

Having a global setting seems reasonable. If we were doing this per-target, I
would
recommend that use two different rules instead.

[Dirk Pranke, 2015-10-09 20:03:12]

lgtm.

https://codereview.chromium.org/1393303003/diff/40001/build/util/java_action.py
File build/util/java_action.py (right):

https://codereview.chromium.org/1393303003/diff/40001/build/util/java_action....
build/util/java_action.py:28: """Lookup for |command| in PATH.
On 2015/10/09 15:52:36, eugenebut wrote:
> NIT: s/Lookup/Looks up

Technically you want s/Lookup for/Looks up/ :)

https://codereview.chromium.org/1393303003/diff/40001/build/util/java_action....
build/util/java_action.py:52: java_path = FindCommand('java')
This won't work right on windows. Do you care? If you don't, you might want to 
add an assert somewhere just to be safe.

[sdefresne, 2015-10-21 01:29:05]

https://codereview.chromium.org/1393303003/diff/40001/build/util/java_action.py
File build/util/java_action.py (right):

https://codereview.chromium.org/1393303003/diff/40001/build/util/java_action....
build/util/java_action.py:28: """Lookup for |command| in PATH.
On 2015/10/09 at 20:03:12, Dirk Pranke wrote:
> On 2015/10/09 15:52:36, eugenebut wrote:
> > NIT: s/Lookup/Looks up
> 
> Technically you want s/Lookup for/Looks up/ :)

Fixed.

https://codereview.chromium.org/1393303003/diff/40001/build/util/java_action....
build/util/java_action.py:52: java_path = FindCommand('java')
On 2015/10/09 at 20:03:12, Dirk Pranke wrote:
> This won't work right on windows. Do you care? If you don't, you might want to

> add an assert somewhere just to be safe.

I've added support for PATHEXT to FindCommand and think it should work on
Windows now. I do not have access to a Windows machine to test. Is there any
other reason this would not work on Windows?

[Dirk Pranke, 2015-10-21 19:40:36]

still lgtm.

https://codereview.chromium.org/1393303003/diff/60001/build/util/java_action.py
File build/util/java_action.py (right):

https://codereview.chromium.org/1393303003/diff/60001/build/util/java_action....
build/util/java_action.py:39: On Windows, this respect the PATHEXT environment
variable when the
Nit: s/respect/respects/ .

https://codereview.chromium.org/1393303003/diff/60001/build/util/java_action....
build/util/java_action.py:47: if os.name == 'nt':
Nit: usually we check against sys.platform == 'win32' instead.

[Dirk Pranke, 2015-10-21 19:41:19]

Oh, I didn't answer the question. I would guess otherwise this might work on
windows. I wouldn't put any more effort into it for now, though; we can wait
until if/when we need it.

[sdefresne, 2015-10-21 23:23:08]

sdefresne@chromium.org changed reviewers:
+ droger@chromium.org

[sdefresne, 2015-10-21 23:23:08]

droger, can you review ios/third_party/gcdwebserver/BUILD.gn as OWNERS?

[droger, 2015-10-21 23:37:22]

lgtm

[sdefresne, 2015-10-21 23:39:02]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-10-21 23:39:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1393303003/#ps100001
(title: "TODO fix-it")

[commit-bot: I haz the power, 2015-10-21 23:39:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393303003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393303003/100001

[sdefresne, 2015-10-21 23:40:42]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2015-10-21 23:50:42]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-10-21 23:50:42]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
dpranke@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/1393303003/#ps120001
(title: "TODO fix-it")

[commit-bot: I haz the power, 2015-10-21 23:52:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393303003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393303003/120001

[commit-bot: I haz the power, 2015-10-22 00:41:18]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-10-22 00:42:13]

Patchset 6 (id:??) landed as
https://crrev.com/aa7c1cf96274a0a531629f934bd9091a55439270
Cr-Commit-Position: refs/heads/master@{#355460}