Enable external V8 snapshot on Android through GN.

Enable external V8 snapshot on Android through GN.

BUG=421063
Committed: https://crrev.com/1660022c515b379091f061eaffd7a6c310ca959b
Cr-Commit-Position: refs/heads/master@{#306875}

[baixo1, 2014-11-24 16:05:40]

baixo@google.com changed reviewers:
+ baixo@google.com, jochen@chromium.org, rmcilroy@chromium.org

[baixo1, 2014-11-24 16:05:40]

jochen and rmcilroy: could you please take a look?

This CL has to land before this:
https://codereview.chromium.org/749213004

[rmcilroy, 2014-11-24 17:05:26]

https://codereview.chromium.org/753103003/diff/1/build/config/android/config.gni
File build/config/android/config.gni (right):

https://codereview.chromium.org/753103003/diff/1/build/config/android/config....
build/config/android/config.gni:156: v8_use_external_startup_data = true
I think you want this to be false until
https://codereview.chromium.org/749213004/ lands otherwise this break the GN
build, right?

https://codereview.chromium.org/753103003/diff/1/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/753103003/diff/1/build/config/features.gni#ne...
build/config/features.gni:43: v8_use_external_startup_data = false
Could you move this to a non-declare_args variable (in the set of enable_xxx
below) and set it as follows (see comment above):

#enable_webrtcis_android && !is_android_webview_build TODO(baixo) enable this
once V8 CL lands.
v8_use_external_startup_data = false

[baixo1, 2014-11-24 21:27:10]

Addressed Ross' comments.

https://codereview.chromium.org/753103003/diff/1/build/config/android/config.gni
File build/config/android/config.gni (right):

https://codereview.chromium.org/753103003/diff/1/build/config/android/config....
build/config/android/config.gni:156: v8_use_external_startup_data = true
On 2014/11/24 17:05:25, rmcilroy wrote:
> I think you want this to be false until
> https://codereview.chromium.org/749213004/ lands otherwise this break the GN
> build, right?

Yes, thanks!

https://codereview.chromium.org/753103003/diff/1/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/753103003/diff/1/build/config/features.gni#ne...
build/config/features.gni:43: v8_use_external_startup_data = false
On 2014/11/24 17:05:25, rmcilroy wrote:
> Could you move this to a non-declare_args variable (in the set of enable_xxx
> below) and set it as follows (see comment above):
> 
> #enable_webrtcis_android && !is_android_webview_build TODO(baixo) enable this
> once V8 CL lands.
> v8_use_external_startup_data = false

Done.

[rmcilroy, 2014-11-25 10:49:24]

On 2014/11/24 21:27:10, baixo1 wrote:
> Addressed Ross' comments.
> 
>
https://codereview.chromium.org/753103003/diff/1/build/config/android/config.gni
> File build/config/android/config.gni (right):
> 
>
https://codereview.chromium.org/753103003/diff/1/build/config/android/config....
> build/config/android/config.gni:156: v8_use_external_startup_data = true
> On 2014/11/24 17:05:25, rmcilroy wrote:
> > I think you want this to be false until
> > https://codereview.chromium.org/749213004/ lands otherwise this break the GN
> > build, right?
> 
> Yes, thanks!
> 
> https://codereview.chromium.org/753103003/diff/1/build/config/features.gni
> File build/config/features.gni (right):
> 
>
https://codereview.chromium.org/753103003/diff/1/build/config/features.gni#ne...
> build/config/features.gni:43: v8_use_external_startup_data = false
> On 2014/11/24 17:05:25, rmcilroy wrote:
> > Could you move this to a non-declare_args variable (in the set of enable_xxx
> > below) and set it as follows (see comment above):
> > 
> > #enable_webrtcis_android && !is_android_webview_build TODO(baixo) enable
this
> > once V8 CL lands.
> > v8_use_external_startup_data = false
> 
> Done.

lgtm (but you will need owners lgtm from brettw)

[baixo1, 2014-11-25 11:08:17]

baixo@google.com changed reviewers:
+ brettw@chromium.org

[baixo1, 2014-11-25 11:08:17]

brettw: could you please take a look?

Thanks!

[brettw, 2014-11-25 18:50:27]

https://codereview.chromium.org/753103003/diff/20001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/753103003/diff/20001/build/config/features.gn...
build/config/features.gni:164: v8_use_external_startup_data = false
I don't think this should be in "features.gni". I tried to express this in some
thread, but there were several threads and maybe not everybody was on all of
them.

The .gni files in this directory are basically all hacks that should go away.
Chrome feature flags shouldn't be in the build directory since the build
directory should be shared. But the current build expects everything to be
global, so I isolated the chrome flags into a couple of files here. The right
thing long-term to do is to have the ui flags in src/ui, the crypto flags in
src/crypto, etc.

For these flags that need to be set "on" v8 by different modules, I want to make
a new place for them (this thread you weren't on, sorry). My current favorite
name is "//build/module_args/v8.gni".

The idea is that it should be outside of "//build/config" since build/config is
the thing that should be shared between projects. For this new directory, each
project would have their own files that declare certain arguments for each
module they DEPS in. I put the name "module" in there because I want to
discourage people from dumping all build flags in there. These would only be the
ones that are exposed externally by a module.

[baixo1, 2014-12-01 14:50:31]

On 2014/11/25 18:50:27, brettw wrote:
> https://codereview.chromium.org/753103003/diff/20001/build/config/features.gni
> File build/config/features.gni (right):
> 
>
https://codereview.chromium.org/753103003/diff/20001/build/config/features.gn...
> build/config/features.gni:164: v8_use_external_startup_data = false
> I don't think this should be in "features.gni". I tried to express this in
some
> thread, but there were several threads and maybe not everybody was on all of
> them.
> 
> The .gni files in this directory are basically all hacks that should go away.
> Chrome feature flags shouldn't be in the build directory since the build
> directory should be shared. But the current build expects everything to be
> global, so I isolated the chrome flags into a couple of files here. The right
> thing long-term to do is to have the ui flags in src/ui, the crypto flags in
> src/crypto, etc.
> 
> For these flags that need to be set "on" v8 by different modules, I want to
make
> a new place for them (this thread you weren't on, sorry). My current favorite
> name is "//build/module_args/v8.gni".
> 
> The idea is that it should be outside of "//build/config" since build/config
is
> the thing that should be shared between projects. For this new directory, each
> project would have their own files that declare certain arguments for each
> module they DEPS in. I put the name "module" in there because I want to
> discourage people from dumping all build flags in there. These would only be
the
> ones that are exposed externally by a module.

brettw: could you take another look, please?

I tried to address your comments, but I'm not sure if I correctly grasped all
your ideas on this :)

Thanks!

[baixo1, 2014-12-04 15:27:12]

brettw: could you please take a look at this?

Thanks!

[brettw, 2014-12-04 18:36:42]

Whoops, sorry I lost this. LGTM

https://codereview.chromium.org/753103003/diff/40001/build/module_args/v8.gni
File build/module_args/v8.gni (right):

https://codereview.chromium.org/753103003/diff/40001/build/module_args/v8.gni...
build/module_args/v8.gni:1: if (is_android) {
Need copyright.

[baixo1, 2014-12-04 19:07:23]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-12-04 19:08:09]

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

[commit-bot: I haz the power, 2014-12-04 20:08:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-04 20:09:33]

Patchset 4 (id:??) landed as
https://crrev.com/1660022c515b379091f061eaffd7a6c310ca959b
Cr-Commit-Position: refs/heads/master@{#306875}