Port chromevox build to GN.

Port chromevox build to GN.

This ports the chromevox build: chromevox and chromevox_tests targets to
gn.  Liblouis is still missing since it requires native client which is
not enabled for Chrome OS.

BUG=None

Committed: https://crrev.com/70028e5c6f31604faa2a430c8afda19dad77ad46
Cr-Commit-Position: refs/heads/master@{#338002}

[Peter Lundblad, 2015-06-09 15:47:37]

plundblad@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2015-06-09 22:21:27]

https://codereview.chromium.org/1145243006/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox.gni (right):

https://codereview.chromium.org/1145243006/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox.gni:13: chromevox_out_dir
= "$root_out_dir/resources/chromeos/chromevox"
I believe this is the only thing from this .gni file that's used in more than
one BUILD file. When things don't need to be shared, it's best to just put them
in the file themselves.

I'm not actually sure how much sharing this var name gets you. I don't think
it's a requirement that this be shared. It may be better to share it, I'll leave
it up to your judgement.

[Peter Lundblad, 2015-06-10 10:00:10]

brettw@chromium.org writes:
> 
>
https://codereview.chromium.org/1145243006/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/chromeos/chromevox/chromevox.gni (right):
> 
>
https://codereview.chromium.org/1145243006/diff/40001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/chromevox.gni:13:
> chromevox_out_dir = "$root_out_dir/resources/chromeos/chromevox"
> I believe this is the only thing from this .gni file that's used in more
> than one BUILD file. When things don't need to be shared, it's best to
> just put them in the file themselves.

I added a todo explaining why these are put here (referring
to crbug.com/395883).  If there are better workarounds, I am happy to change;)

> I'm not actually sure how much sharing this var name gets you. I don't
> think it's a requirement that this be shared. It may be better to share
> it, I'll leave it up to your judgement.

OK, I don't feel strongly, unshared it.

Thanks,
//peter

> https://codereview.chromium.org/1145243006/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2015-06-13 01:36:43]

lgtm

https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/braille_ime/BUILD.gn (right):

https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/braille_ime/BUILD.gn:5: if (is_chromeos) {
Instead of this, I would just put at the top:
  assert(is_chromeos)
since if anoybody references something from this file it's a bug.

https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/BUILD.gn (right):

https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/BUILD.gn:5: if (is_chromeos) {
assert(is_chromeos) here too

https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/BUILD.gn:7: chromevox_compress_js =
!is_debug
This should have a comment above it describing what it does.

https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/BUILD.gn:10:
import("//build/config/features.gni")
I usually put these at the very top.

https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/BUILD.gn:27:
"strings:chromevox_strings",
We prefer full paths to relative dirs. Can you add //chrome/browser/... before
this one? (The ":foo" ones are OK.)

https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/BUILD.gn:42:
template("run_jsbundler") {
This is only called once. If you plan on using it in more places, the template
is fine. But otherwise this is just complicating things with all of the
defined() checks and default values, etc. that will always be the same.

[Peter Lundblad, 2015-07-07 16:37:41]

brettw@chromium.org writes:
> lgtm
> 
> 
> 
> 
>
https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/chromeos/braille_ime/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
> chrome/browser/resources/chromeos/braille_ime/BUILD.gn:5: if
> (is_chromeos) {
> Instead of this, I would just put at the top:
>    assert(is_chromeos)
> since if anoybody references something from this file it's a bug.

Done.

>
https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/chromeos/chromevox/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/BUILD.gn:5: if (is_chromeos)
> {
> assert(is_chromeos) here too

Done.

>
https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/BUILD.gn:7:
> chromevox_compress_js = !is_debug
> This should have a comment above it describing what it does.

Done.

>
https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/BUILD.gn:10:
> import("//build/config/features.gni")
> I usually put these at the very top.

Done.

>
https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/BUILD.gn:27:
> "strings:chromevox_strings",
> We prefer full paths to relative dirs. Can you add //chrome/browser/...
> before this one? (The ":foo" ones are OK.)

Done.

>
https://codereview.chromium.org/1145243006/diff/60001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/BUILD.gn:42:
> template("run_jsbundler") {
> This is only called once. If you plan on using it in more places, the
> template is fine. But otherwise this is just complicating things with
> all of the defined() checks and default values, etc. that will always be
> the same.

Actually called twice, once from within another template.  Left as-is.

> https://codereview.chromium.org/1145243006/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2015-07-07 16:40:31]

The CQ bit was checked by plundblad@chromium.org

[Peter Lundblad, 2015-07-07 16:40:31]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1145243006/#ps80001
(title: "Minor stylish fixes.")

[commit-bot: I haz the power, 2015-07-07 16:42:23]

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

[commit-bot: I haz the power, 2015-07-07 17:29:27]

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

[commit-bot: I haz the power, 2015-07-07 17:29:28]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Lundblad, 2015-07-09 08:08:24]

The CQ bit was checked by plundblad@chromium.org

[Peter Lundblad, 2015-07-09 08:08:25]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1145243006/#ps100001
(title: "Workaround hardlink related spuroius rebuild issue by using a stamp
file (not ideal).")

[commit-bot: I haz the power, 2015-07-09 08:08:36]

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

[commit-bot: I haz the power, 2015-07-09 09:01:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-09 09:02:12]

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