Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2575973002:
Add a GN arg to include PartitionAlloc sources (Closed)
DescriptionAdd a GN arg to include PartitionAlloc sources
This CL adds a GN arg so that the PartitionAlloc sources
can be included or excluded. Since PartitionAlloc is still under
development and not used by all components that depend on base/, this
will give an option to binary size sensitive projects(e.g. Cronet) to
not bear the size increase in the meanwhile.
BUG=674570
Committed: https://crrev.com/7519f1f1bff997fade77a4dec1f4bcbe2bc858aa
Cr-Commit-Position: refs/heads/master@{#438842}
Patch Set 1 : self review #Patch Set 2 : change to include by default #
Total comments: 2
Patch Set 3 : address comment #Messages
Total messages: 36 (27 generated)
Description was changed from ========== Add a GN arg to include PartitionAlloc This CL adds a GN arg so that the PartitionAlloc sources are not included by default. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will make binary size sensitive projects not bear the size increase. ========== to ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources are not included by default. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will make binary size sensitive projects not bear the size increase. ==========
Description was changed from ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources are not included by default. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will make binary size sensitive projects not bear the size increase. ========== to ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources are not included by default. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will make binary size sensitive projects not bear the size increase in the meanwhile. ==========
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + palmer@chromium.org
palmer@: PTAL. Will something like this work for you?
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
You'll need to also set enable_partition_alloc = true for (currently) all build targets that use WebKit.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Description was changed from ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources are not included by default. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will make binary size sensitive projects not bear the size increase in the meanwhile. ========== to ========== Add a GN arg to exclude PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be exclude. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects to not bear the size increase in the meanwhile. ==========
Description was changed from ========== Add a GN arg to exclude PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be exclude. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects to not bear the size increase in the meanwhile. ========== to ========== Add a GN arg to exclude PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects to not bear the size increase in the meanwhile. ==========
On 2016/12/14 19:52:12, palmer wrote: > You'll need to also set enable_partition_alloc = true for (currently) all build > targets that use WebKit. I don't know a way to do that so I changed this to include the PA sources by default except on iOS. Could you take a look? (I will make Cronet builds use this GN arg in a follow-up.)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. https://codereview.chromium.org/2575973002/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2575973002/diff/40001/base/BUILD.gn#newcode46 base/BUILD.gn:46: exclude_partition_alloc = is_ios Nit: My instinct is to phrase this the other way: use_partition_alloc = !is_ios to avoid the double-negative in the calling code ("if not exclude..."). But, it's not a big deal either way; do whichever makes the most sense to you. :)
Description was changed from ========== Add a GN arg to exclude PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects to not bear the size increase in the meanwhile. ========== to ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects to not bear the size increase in the meanwhile. ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xunjieli@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@chromium.org: Please review changes in base/BUILD.gn Thanks! https://codereview.chromium.org/2575973002/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2575973002/diff/40001/base/BUILD.gn#newcode46 base/BUILD.gn:46: exclude_partition_alloc = is_ios On 2016/12/14 23:28:53, palmer wrote: > Nit: My instinct is to phrase this the other way: > > use_partition_alloc = !is_ios > > to avoid the double-negative in the calling code ("if not exclude..."). > > But, it's not a big deal either way; do whichever makes the most sense to you. > :) Done. Makes sense. Since I probably won't touch this code again, I will use your suggestion so it feels natural to you :)
Description was changed from ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects to not bear the size increase in the meanwhile. ========== to ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects(e.g. Cronet) to not bear the size increase in the meanwhile. ==========
Description was changed from ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects(e.g. Cronet) to not bear the size increase in the meanwhile. ========== to ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects(e.g. Cronet) to not bear the size increase in the meanwhile. BUG=674570 ==========
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org Link to the patchset: https://codereview.chromium.org/2575973002/#ps60001 (title: "address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481818225033490,
"parent_rev": "62fc89201e347f27ba18c24452075f1722229f5f", "commit_rev":
"2d73b19a4b211c17239aa8ec87bc2bb25f738b27"}
Message was sent while issue was closed.
Description was changed from ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects(e.g. Cronet) to not bear the size increase in the meanwhile. BUG=674570 ========== to ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects(e.g. Cronet) to not bear the size increase in the meanwhile. BUG=674570 Review-Url: https://codereview.chromium.org/2575973002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects(e.g. Cronet) to not bear the size increase in the meanwhile. BUG=674570 Review-Url: https://codereview.chromium.org/2575973002 ========== to ========== Add a GN arg to include PartitionAlloc sources This CL adds a GN arg so that the PartitionAlloc sources can be included or excluded. Since PartitionAlloc is still under development and not used by all components that depend on base/, this will give an option to binary size sensitive projects(e.g. Cronet) to not bear the size increase in the meanwhile. BUG=674570 Committed: https://crrev.com/7519f1f1bff997fade77a4dec1f4bcbe2bc858aa Cr-Commit-Position: refs/heads/master@{#438842} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7519f1f1bff997fade77a4dec1f4bcbe2bc858aa Cr-Commit-Position: refs/heads/master@{#438842} |
