Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1418243003:
Add GN template for android_assets(). Use it in content_shell_apk. (Closed)
|
Created:
5 years, 2 months ago by agrieve Modified:
5 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, vmpstr+watch_chromium.org, jam, jbudorick+watch_chromium.org, darin-cc_chromium.org, yfriedman+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, klundberg+watch_chromium.org, jochen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd GN template for android_assets()
The new rule is meant to work the same way as android_resources, and
allow easy sharing of assets between targets.
Does not remove support for android_apk()'s asset_location parameter,
but the intention is to remove it eventually.
Note that this does cause assets to appear later in the zip file than they
would when added by aapt. ContentShell.apk ran fine though.
BUG=547162
Committed: https://crrev.com/6c2686ba7fa6d655306a0dc6e876ceae7dc3664b
Cr-Commit-Position: refs/heads/master@{#357266}
Patch Set 1 #
Total comments: 18
Patch Set 2 : review1 #
Total comments: 2
Patch Set 3 : add support for asset merging and renaming. Pass via @FileArg. #Patch Set 4 : remove usages of android_assets() #
Total comments: 3
Patch Set 5 : outputs -> renaming_destination #
Total comments: 17
Patch Set 6 : review comments & two-pass _AddAssets #
Depends on Patchset: Messages
Total messages: 23 (5 generated)
agrieve@chromium.org changed reviewers: + brettw@chromium.org, pkotwicz@chromium.org
Description was changed from ========== Add GN template for android_assets(). Use it in content_shell_apk. The new rule is meant to work the same way as android_resources, and allow easy sharing of assets between targets. Does not remove support for android_apk()'s asset_location parameter, but the intention is to remove it eventually. BUG=547162 ========== to ========== Add GN template for android_assets(). Use it in content_shell_apk. The new rule is meant to work the same way as android_resources, and allow easy sharing of assets between targets. Does not remove support for android_apk()'s asset_location parameter, but the intention is to remove it eventually. Note that this does cause assets to appear later in the zip file than they would when added by aapt. ContentShell.apk ran fine though. BUG=547162 ==========
It is going to take me a while to review the CL. All of the config writing is new to me (A chance to learn something new!) https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:696: # enable_compression: Whether to enable compressed for files that are not Nit: compressed -> compression
Looks good in general https://codereview.chromium.org/1418243003/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1418243003/diff/1/base/BUILD.gn#newcode1025 base/BUILD.gn:1025: if (is_android) { Shouldn't this ideally be in third_party/icu third_party/icu is in a separate repository so I understand why you don't want to put it here as part of this CL. Can you please add a TODO? https://codereview.chromium.org/1418243003/diff/1/build/android/gyp/write_bui... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/1418243003/diff/1/build/android/gyp/write_bui... build/android/gyp/write_build_config.py:273: } Nit: New line https://codereview.chromium.org/1418243003/diff/1/build/android/gyp/write_bui... build/android/gyp/write_build_config.py:371: config['merged_assets'] = [a['assets'] for a in all_assets if 'assets' in a] I think that you can make things slightly clearer by having: config['merged_assets']['compress'] and config['merged_assets']['no_compress'] This should decrease the number of places which need to pass the path the to the config file around which I think will make things clearer https://codereview.chromium.org/1418243003/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1418243003/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:765: if (defined(invoker.extensions_to_not_compress)) { This parameter will also be removed when asset_location is removed? https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:695: # outputs: List of asset paths to use for the assets (similar to copy()). Can you remove the variable given that it is unused? https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:696: # enable_compression: Whether to enable compressed for files that are not I think that disable_compression is a better name and better reflects the default value https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:722: "prefix_to_strip", I can't find a place where "prefix_to_strip" is used https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:732: if (invoker.sources != []) { Should we assert that invoker.sources is non-empty instead?
https://codereview.chromium.org/1418243003/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1418243003/diff/1/base/BUILD.gn#newcode1025 base/BUILD.gn:1025: if (is_android) { On 2015/10/24 03:46:03, pkotwicz wrote: > Shouldn't this ideally be in third_party/icu > third_party/icu is in a separate repository so I understand why you don't want > to put it here as part of this CL. > > Can you please add a TODO? My thinking was that it was fine here since base/i18n is a thing and what you use to initialize icu https://codereview.chromium.org/1418243003/diff/1/build/android/gyp/write_bui... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/1418243003/diff/1/build/android/gyp/write_bui... build/android/gyp/write_build_config.py:273: } On 2015/10/24 03:46:03, pkotwicz wrote: > Nit: New line Done. https://codereview.chromium.org/1418243003/diff/1/build/android/gyp/write_bui... build/android/gyp/write_build_config.py:371: config['merged_assets'] = [a['assets'] for a in all_assets if 'assets' in a] On 2015/10/24 03:46:03, pkotwicz wrote: > I think that you can make things slightly clearer by having: > config['merged_assets']['compress'] and config['merged_assets']['no_compress'] > > This should decrease the number of places which need to pass the path the to the > config file around which I think will make things clearer I like that this would make for a shallower tree, but I think for now I'd like to keep the order intact in case we need to implement asset merging. Asset merging could be implemented in this merge step, so it's a weak stance, but I don't think it's a big enough improvement to change in the other direction either. https://codereview.chromium.org/1418243003/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1418243003/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:765: if (defined(invoker.extensions_to_not_compress)) { On 2015/10/24 03:46:03, pkotwicz wrote: > This parameter will also be removed when asset_location is removed? It still applies to files in resources https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:695: # outputs: List of asset paths to use for the assets (similar to copy()). On 2015/10/24 03:46:03, pkotwicz wrote: > Can you remove the variable given that it is unused? I'd like to keep it in to document how I'd like the template to work in the case of needing to put assets under subdirectories or renaming them. Added to the comment that it's not yet implemented. https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:696: # enable_compression: Whether to enable compressed for files that are not On 2015/10/23 23:17:22, pkotwicz wrote: > Nit: compressed -> compression Done. https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:696: # enable_compression: Whether to enable compressed for files that are not On 2015/10/24 03:46:03, pkotwicz wrote: > I think that disable_compression is a better name and better reflects the > default value Done. https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:722: "prefix_to_strip", On 2015/10/24 03:46:03, pkotwicz wrote: > I can't find a place where "prefix_to_strip" is used Whoops, zapped. https://codereview.chromium.org/1418243003/diff/1/build/config/android/rules.... build/config/android/rules.gni:732: if (invoker.sources != []) { On 2015/10/24 03:46:03, pkotwicz wrote: > Should we assert that invoker.sources is non-empty instead? We'll have empty sources in the examples in the patch. E.g. when !icu_use_data_file.
https://codereview.chromium.org/1418243003/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1418243003/diff/20001/base/BUILD.gn#newcode1037 base/BUILD.gn:1037: android_assets("icu_assets") { Can this be in the ICU file instead of base?
https://codereview.chromium.org/1418243003/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1418243003/diff/20001/base/BUILD.gn#newcode1037 base/BUILD.gn:1037: android_assets("icu_assets") { On 2015/10/25 23:51:22, brettw wrote: > Can this be in the ICU file instead of base? pkotwicz suggested this as well, but I think it's better in base because: 1. icu is a separate repository, adding frictions to changes (not sure why it doesn't follow the pattern of putting its files in third_party/icu/src so that BUILD.gn can be in third_party/icu without being a separate repo) 2. The disable_compression part is relevant to how the file is loaded, which is done by the base/i18n module (perhaps it's worth adding a BUILD.gn for base/i18n?)
On 2015/10/26 15:43:48, agrieve wrote: > https://codereview.chromium.org/1418243003/diff/20001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/1418243003/diff/20001/base/BUILD.gn#newcode1037 > base/BUILD.gn:1037: android_assets("icu_assets") { > On 2015/10/25 23:51:22, brettw wrote: > > Can this be in the ICU file instead of base? > > pkotwicz suggested this as well, but I think it's better in base because: > 1. icu is a separate repository, adding frictions to changes (not sure why it > doesn't follow the pattern of putting its files in third_party/icu/src so that > BUILD.gn can be in third_party/icu without being a separate repo) > 2. The disable_compression part is relevant to how the file is loaded, which is > done by the base/i18n module (perhaps it's worth adding a BUILD.gn for > base/i18n?) wdyt?
On 2015/10/28 13:41:29, agrieve wrote: > On 2015/10/26 15:43:48, agrieve wrote: > > https://codereview.chromium.org/1418243003/diff/20001/base/BUILD.gn > > File base/BUILD.gn (right): > > > > > https://codereview.chromium.org/1418243003/diff/20001/base/BUILD.gn#newcode1037 > > base/BUILD.gn:1037: android_assets("icu_assets") { > > On 2015/10/25 23:51:22, brettw wrote: > > > Can this be in the ICU file instead of base? > > > > pkotwicz suggested this as well, but I think it's better in base because: > > 1. icu is a separate repository, adding frictions to changes (not sure why it > > doesn't follow the pattern of putting its files in third_party/icu/src so that > > BUILD.gn can be in third_party/icu without being a separate repo) > > 2. The disable_compression part is relevant to how the file is loaded, which > is > > done by the base/i18n module (perhaps it's worth adding a BUILD.gn for > > base/i18n?) > > wdyt? Removed usages of android_assets() to get things moving. Also implemented outputs= and added the default compression logic (which I intended to add before but forgot)
https://codereview.chromium.org/1418243003/diff/60001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1418243003/diff/60001/build/config/android/ru... build/config/android/rules.gni:696: # values than exist in sources, remaining sources will use the default path. In which cases would this parameter be useful? If the parameter is necessary, replacing the parameter with output_dir would make the android_assets template easier to understand in my opinion
https://codereview.chromium.org/1418243003/diff/60001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1418243003/diff/60001/build/config/android/ru... build/config/android/rules.gni:696: # values than exist in sources, remaining sources will use the default path. I think that the purpose of outputs parameter is to allow a file to be stored in the APK but under a different name. I understand how this can be useful. I think that the outputs parameter is an overkill and that at least for now we can live without it We can use the copy() template when renaming is desired.
Description was changed from ========== Add GN template for android_assets(). Use it in content_shell_apk. The new rule is meant to work the same way as android_resources, and allow easy sharing of assets between targets. Does not remove support for android_apk()'s asset_location parameter, but the intention is to remove it eventually. Note that this does cause assets to appear later in the zip file than they would when added by aapt. ContentShell.apk ran fine though. BUG=547162 ========== to ========== Add GN template for android_assets() The new rule is meant to work the same way as android_resources, and allow easy sharing of assets between targets. Does not remove support for android_apk()'s asset_location parameter, but the intention is to remove it eventually. Note that this does cause assets to appear later in the zip file than they would when added by aapt. ContentShell.apk ran fine though. BUG=547162 ==========
https://codereview.chromium.org/1418243003/diff/60001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1418243003/diff/60001/build/config/android/ru... build/config/android/rules.gni:696: # values than exist in sources, remaining sources will use the default path. On 2015/10/29 23:23:33, pkotwicz wrote: > I think that the purpose of outputs parameter is to allow a file to be stored in > the APK but under a different name. I understand how this can be useful. > > I think that the outputs parameter is an overkill and that at least for now we > can live without it > We can use the copy() template when renaming is desired. I've modelled this after the copy() rule, which allows a list of outputs. Only place I think it'll be used is by webview to store both the 32 bit and 64 bit v8 snapshots. I think it's a reasonable parameter to support anyways though.
On 2015/10/30 00:04:45, agrieve wrote: > https://codereview.chromium.org/1418243003/diff/60001/build/config/android/ru... > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/1418243003/diff/60001/build/config/android/ru... > build/config/android/rules.gni:696: # values than exist in sources, > remaining sources will use the default path. > On 2015/10/29 23:23:33, pkotwicz wrote: > > I think that the purpose of outputs parameter is to allow a file to be stored > in > > the APK but under a different name. I understand how this can be useful. > > > > I think that the outputs parameter is an overkill and that at least for now we > > can live without it > > We can use the copy() template when renaming is desired. > > I've modelled this after the copy() rule, which allows a list of outputs. > > Only place I think it'll be used is by webview to store both the 32 bit and 64 > bit v8 snapshots. I think it's a reasonable parameter to support anyways though. You're right that copy() doesn't work this way. Turns out it was copy_ex() I was thinking of. I've changed it around to work the way copy_ex() works (for real this time). In terms of functionality, I do still think it's important that the rule be able to handle renaming even if we don't use it right away.
LGTM with nits https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/apkbu... File build/android/gyp/apkbuilder.py (right): https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/apkbu... build/android/gyp/apkbuilder.py:79: Nit: Extra new line https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/apkbu... build/android/gyp/apkbuilder.py:85: paths: List of paths (with optional :zipPath suffix) to add. Nit: Make :zip suffix non-optional Document what the prefix and suffix mean https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:118: optional. Nit: For simplicity make zipPath non optional https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:138: for dest, src in asset_map.iteritems(): I think that this would be clearer: for dest, src in asset_map.iteritems(): ret.append('%s:%s' % (src, dest)) # Sort to ensure deterministic ordering. ... https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:312: assets = [] Nit: rename |assets| to |sources| https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:319: 'paths': assets Nit: Rename 'paths' key to 'sources' https://codereview.chromium.org/1418243003/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1418243003/diff/80001/build/config/android/ru... build/config/android/rules.gni:697: # disable_compression: Whether to diesable compression for files that are diesable -> disable https://codereview.chromium.org/1418243003/diff/80001/build/config/android/ru... build/config/android/rules.gni:717: # renaming_sources = [ "//path/asset1.png" ] Nit: Can you name this asset not "asset1.png"? Using "asset1.png" is confusing because it is unclear whether asset1.png will appear once or twice in the APK
https://codereview.chromium.org/1418243003/diff/80001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1418243003/diff/80001/build/config/android/in... build/config/android/internal_rules.gni:676: # android_asset() everywhere (http://crbug.com/547162). Nit: android_asset() -> android_assets()
https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/apkbu... File build/android/gyp/apkbuilder.py (right): https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/apkbu... build/android/gyp/apkbuilder.py:79: On 2015/10/30 20:35:53, pkotwicz wrote: > Nit: Extra new line Yeah, I think it's a bit much too, but it's our python style: https://google.github.io/styleguide/pyguide.html#Blank_Lines https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:118: optional. On 2015/10/30 20:35:53, pkotwicz wrote: > Nit: For simplicity make zipPath non optional Done. https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:138: for dest, src in asset_map.iteritems(): On 2015/10/30 20:35:54, pkotwicz wrote: > I think that this would be clearer: > > for dest, src in asset_map.iteritems(): > ret.append('%s:%s' % (src, dest)) > # Sort to ensure deterministic ordering. > ... Done. https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:312: assets = [] On 2015/10/30 20:35:54, pkotwicz wrote: > Nit: rename |assets| to |sources| Done. https://codereview.chromium.org/1418243003/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:319: 'paths': assets On 2015/10/30 20:35:53, pkotwicz wrote: > Nit: Rename 'paths' key to 'sources' Done. https://codereview.chromium.org/1418243003/diff/80001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1418243003/diff/80001/build/config/android/in... build/config/android/internal_rules.gni:676: # android_asset() everywhere (http://crbug.com/547162). On 2015/10/30 20:37:42, pkotwicz wrote: > Nit: android_asset() -> android_assets() Done. https://codereview.chromium.org/1418243003/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1418243003/diff/80001/build/config/android/ru... build/config/android/rules.gni:697: # disable_compression: Whether to diesable compression for files that are On 2015/10/30 20:35:54, pkotwicz wrote: > diesable -> disable Done. https://codereview.chromium.org/1418243003/diff/80001/build/config/android/ru... build/config/android/rules.gni:717: # renaming_sources = [ "//path/asset1.png" ] On 2015/10/30 20:35:54, pkotwicz wrote: > Nit: Can you name this asset not "asset1.png"? Using "asset1.png" is confusing > because it is unclear whether asset1.png will appear once or twice in the APK Done.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1418243003/#ps100001 (title: "review comments & two-pass _AddAssets")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418243003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418243003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6c2686ba7fa6d655306a0dc6e876ceae7dc3664b Cr-Commit-Position: refs/heads/master@{#357266} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
