GN: Enable embedder to decide whether or not the external V8 snapshot is enabled.

GN: Enable embedder to decide whether or not the external V8 snapshot is enabled.

BUG=421063

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

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

[baixo1, 2014-11-24 16:06:41]

jochen and rmcilroy: could you please take a look?

This CL depends on this landing first:
https://codereview.chromium.org/753103003/

[rmcilroy, 2014-11-24 17:21:06]

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

https://codereview.chromium.org/749213004/diff/1/BUILD.gn#newcode1418
BUILD.gn:1418: deps = []
Is there any reason you need to make these changes?  I would have thought the
if/else if/else structure before would have given you the same result?

https://codereview.chromium.org/749213004/diff/1/BUILD.gn#newcode1453
BUILD.gn:1453: deps = []
ditto

[baixo1, 2014-11-24 17:28:11]

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

https://codereview.chromium.org/749213004/diff/1/BUILD.gn#newcode1418
BUILD.gn:1418: deps = []
On 2014/11/24 17:21:06, rmcilroy wrote:
> Is there any reason you need to make these changes?  I would have thought the
> if/else if/else structure before would have given you the same result?

I thought the same :). But no :)
When you have the structure:

if (v8_use_external_startup_data)
  do something;
else if (v8_snapshot)
  do something else;
else
  do another thing;

if v8_use_external_startup_data is true, it won't consider the other branches
exist. And then it'll complain that v8_snapshot was defined but never used :)

[rmcilroy, 2014-11-24 18:01:34]

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

https://codereview.chromium.org/749213004/diff/1/BUILD.gn#newcode1418
BUILD.gn:1418: deps = []
On 2014/11/24 17:28:11, baixo1 wrote:
> On 2014/11/24 17:21:06, rmcilroy wrote:
> > Is there any reason you need to make these changes?  I would have thought
the
> > if/else if/else structure before would have given you the same result?
> 
> I thought the same :). But no :)
> When you have the structure:
> 
> if (v8_use_external_startup_data)
>   do something;
> else if (v8_snapshot)
>   do something else;
> else
>   do another thing;
> 
> if v8_use_external_startup_data is true, it won't consider the other branches
> exist. And then it'll complain that v8_snapshot was defined but never used :)

Eww, yuck... Ohh well, perhaps you could just make it 
  if (v8_use_snapshot && v8_use_external_startup_data) {
Since I think v8_use_snapshot has to be true for v8_use_external_startup_data to
be used (and you could assert !v8_use_external_startup_data in the final "else"
branch.

[baixo1, 2014-11-24 21:35:13]

Addressed Ross' comments.

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

https://codereview.chromium.org/749213004/diff/1/BUILD.gn#newcode1418
BUILD.gn:1418: deps = []
On 2014/11/24 18:01:34, rmcilroy wrote:
> On 2014/11/24 17:28:11, baixo1 wrote:
> > On 2014/11/24 17:21:06, rmcilroy wrote:
> > > Is there any reason you need to make these changes?  I would have thought
> the
> > > if/else if/else structure before would have given you the same result?
> > 
> > I thought the same :). But no :)
> > When you have the structure:
> > 
> > if (v8_use_external_startup_data)
> >   do something;
> > else if (v8_snapshot)
> >   do something else;
> > else
> >   do another thing;
> > 
> > if v8_use_external_startup_data is true, it won't consider the other
branches
> > exist. And then it'll complain that v8_snapshot was defined but never used
:)
> 
> Eww, yuck... Ohh well, perhaps you could just make it 
>   if (v8_use_snapshot && v8_use_external_startup_data) {
> Since I think v8_use_snapshot has to be true for v8_use_external_startup_data
to
> be used (and you could assert !v8_use_external_startup_data in the final
"else"
> branch.

Done.

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

On 2014/11/24 21:35:13, baixo1 wrote:
> Addressed Ross' comments.
> 
> https://codereview.chromium.org/749213004/diff/1/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/749213004/diff/1/BUILD.gn#newcode1418
> BUILD.gn:1418: deps = []
> On 2014/11/24 18:01:34, rmcilroy wrote:
> > On 2014/11/24 17:28:11, baixo1 wrote:
> > > On 2014/11/24 17:21:06, rmcilroy wrote:
> > > > Is there any reason you need to make these changes?  I would have
thought
> > the
> > > > if/else if/else structure before would have given you the same result?
> > > 
> > > I thought the same :). But no :)
> > > When you have the structure:
> > > 
> > > if (v8_use_external_startup_data)
> > >   do something;
> > > else if (v8_snapshot)
> > >   do something else;
> > > else
> > >   do another thing;
> > > 
> > > if v8_use_external_startup_data is true, it won't consider the other
> branches
> > > exist. And then it'll complain that v8_snapshot was defined but never used
> :)
> > 
> > Eww, yuck... Ohh well, perhaps you could just make it 
> >   if (v8_use_snapshot && v8_use_external_startup_data) {
> > Since I think v8_use_snapshot has to be true for
v8_use_external_startup_data
> to
> > be used (and you could assert !v8_use_external_startup_data in the final
> "else"
> > branch.
> 
> Done.

lgtm, but please change the title description to "GN: Enable embedder to decide
whether or not the external V8 snapshot is enabled."

[jochen (gone - plz use gerrit), 2014-11-25 15:50:13]

lgtm

[baixo1, 2014-12-05 16:55:23]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-12-05 16:56:04]

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

[commit-bot: I haz the power, 2014-12-05 17:22:48]

Committed patchset #3 (id:40001)