Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

[Backport] gn: Remove unnecessary v8 defaults #378

Merged
merged 1 commit into from
Aug 26, 2016

Conversation

rakuco
Copy link
Member

@rakuco rakuco commented Aug 25, 2016

This is necessary for us to be able to change the value of
v8_use_external_startup_data in Crosswalk.

IMPORTANT: In M52 we only get rid of v8_use_external_startup_data, as
https://codereview.chromium.org/2024833002/ does not apply cleanly on
M52's V8. This patch must be rewritten for M53 to contain the entire
upstream change.

See also: crosswalk-project/v8-crosswalk#164

Original commit message:

Remove chromium defaults for v8_optimized_debug and
v8_use_external_startup_data.

This is not needed after v8 provides these defaults:
https://codereview.chromium.org/2025803003/
https://codereview.chromium.org/2024833002/

It also interferes if somebody tries to override the gn args
with a different value.

BUG=chromium:616034
TBR=alokp@chromium.org, brettw@chromium.org

Committed: https://crrev.com/0fffeb2adaa3c284b760922c1aecce1516b998ce
Review-Url: https://codereview.chromium.org/2058033002

This is necessary for us to be able to change the value of
v8_use_external_startup_data in Crosswalk.

IMPORTANT: In M52 we only get rid of v8_use_external_startup_data, as
https://codereview.chromium.org/2024833002/ does not apply cleanly on
M52's V8. This patch must be rewritten for M53 to contain the entire
upstream change.

See also: crosswalk-project/v8-crosswalk#164

Original commit message:
> Remove chromium defaults for v8_optimized_debug and
> v8_use_external_startup_data.
>
> This is not needed after v8 provides these defaults:
> https://codereview.chromium.org/2025803003/
> https://codereview.chromium.org/2024833002/
>
> It also interferes if somebody tries to override the gn args
> with a different value.
>
> BUG=chromium:616034
> TBR=alokp@chromium.org, brettw@chromium.org
>
> Committed: https://crrev.com/0fffeb2adaa3c284b760922c1aecce1516b998ce
> Review-Url: https://codereview.chromium.org/2058033002
@crosswalk-trybot
Copy link

crosswalk-trybot commented Aug 25, 2016

Testing patch series with rakuco/chromium-crosswalk@0908bc7 as its head.

Bot Status
Content Shell Android-x86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Content Shell Android-x86/builds/331)
Content Shell Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Content Shell Linux/builds/329)

@rakuco
Copy link
Member Author

rakuco commented Aug 25, 2016

@darktears

@heke123: FYI.

@msisov @mrunalk: please see the part abou M52/M53 in the pull request message.

@pozdnyakov
Copy link
Contributor

lgtm

@heke123
Copy link
Contributor

heke123 commented Aug 29, 2016

@rakuco In BUILD.gn it imports this build_overrides/v8.gni, (not v8/BUILD.gn)
So I think we should set value to v8_use_external_startup_data here, and don't need to change in v8/BUILD.gn

In xwalk/build/common.gni, we removed the "defination" of v8_use_external_startup_data too, so "gn gen" currently complains:

  ERROR at //BUILD.gn:294:7: Undefined identifier
  if (v8_use_external_startup_data) {

@heke123
Copy link
Contributor

heke123 commented Aug 29, 2016

Or I can add the v8_use_external_startup_data back inxwalk/build/common.gni. which one do you prefer? thanks.

@rakuco
Copy link
Member Author

rakuco commented Aug 29, 2016

Ugh, good catch. GN support in M52's V8 is a pain in the ass to work with.

Setting v8_use_external_startup_data in src/xwalk is not enough, as the content shell builds would remain broken. For M52, I think the best course of action is to move the v8_use_external_startup_data from v8/BUILD.gn to v8/build_overrides/v8.gni and do the same thing in Chromium.

rakuco added a commit to rakuco/crosswalk that referenced this pull request Aug 29, 2016
This fixes a configuration issue with GN introduced by previous commits
to chromium-crosswalk.

See crosswalk-project/chromium-crosswalk#378.

chromium-crosswalk:
* 0a4d8d4f Merge pull request crosswalk-project#381 from rakuco/gn-external_snapshot-arg
* 44aed2e [Temp] Declare v8_use_external_startup_data in build_overrides/v8.gni

v8-crosswalk:
* 9e7fe2b Merge pull request crosswalk-project#166 from rakuco/gn-external_snapshot-arg
* 92973b7 [Temp] Declare v8_use_external_startup_data in build_overrides/v8.gni
imreotto pushed a commit to tenta-browser/chromium-crosswalk that referenced this pull request Nov 2, 2017
Remove beta string of Play Store and Google Play Store.

TBR=dpapad@chromium.org, cpu@chromium.org

Bug: 767212
Test: manul.
Change-Id: I99fbab7d985c4658da12b833867cdfd524438074
Reviewed-on: https://chromium-review.googlesource.com/676101
Commit-Queue: Long Cheng <lgcheng@google.com>
Reviewed-by: Long Cheng <lgcheng@google.com>
Cr-Original-Commit-Position: refs/heads/master@{#503286}(cherry picked from commit 680c073)
Reviewed-on: https://chromium-review.googlesource.com/676952
Cr-Commit-Position: refs/branch-heads/3202@{crosswalk-project#378}
Cr-Branched-From: fa6a5d8-refs/heads/master@{#499098}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants